[Spice-devel] [client 1/2 v2] streaming: Check the stream id in display_update_stream_report() too

[Victor Toso]

Hi,

On Mon, Aug 08, 2016 at 10:43:43AM -0400, Frediano Ziglio wrote:
> >
> > It's safer and more consistent than assuming the caller has done the
> > checks already.
> > 
> > Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> > ---
> >  src/channel-display.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index cf9c583..b4c9ec0 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1222,9 +1222,16 @@ void stream_display_frame(display_stream *st,
> > SpiceMsgIn *frame_msg,
> >  static void display_update_stream_report(SpiceDisplayChannel *channel,
> >  uint32_t stream_id,
> >                                           uint32_t frame_time, int32_t
> >                                           latency)
> >  {
> > -    display_stream *st = channel->priv->streams[stream_id];
> > +    SpiceDisplayChannelPrivate *c = channel->priv;
> > +    display_stream *st;
> >      guint64 now;
> >
> > +    g_return_if_fail(c != NULL);
> > +    g_return_if_fail(c->streams != NULL);
>
> I think these means a bug in the client, I would abort.

I think we use that rationale on the server but not in the client.
I think it make sense (to abort) but it was not discussed till now so
the default has been logging the critical message and keep going.

>
> > +    g_return_if_fail(c->nstreams > stream_id);
> 
> This could just mean a bad server or a MITM attack, yes
> a warning is enough.
> 
> > +
> > +    st = channel->priv->streams[stream_id];
> > +
> >      if (!st->report_is_active) {
> >          return;
> >      }
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel