[Spice-devel] [PATCH spice-server] fixup! dcc-send: remove useless pipe_item assignment pipe_item

[Frediano Ziglio]

> 
> On 7/4/19 12:33 PM, Frediano Ziglio wrote:
> > Remove use-after-free introduced by
> > a78a7d251042892182b158650291d19a85bbd6b1
> > ---
> >   server/dcc-send.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > index 565a79f33..4582e3545 100644
> > --- a/server/dcc-send.c
> > +++ b/server/dcc-send.c
> > @@ -725,7 +725,6 @@ static void
> > red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
> >           RedPipeItem *pipe_item = l->data;
> >           Drawable *drawable;
> >           RedDrawablePipeItem *dpi;
> > -        RedImageItem *image;
> >   
> >           if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
> >               continue;
> > @@ -745,14 +744,16 @@ static void
> > red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
> >               continue;
> >           }
> >   
> > -        image = dcc_add_surface_area_image(dcc,
> > drawable->red_drawable->surface_id,
> > -                                           &drawable->red_drawable->bbox,
> > l, TRUE);
> > +        dcc_add_surface_area_image(dcc,
> > drawable->red_drawable->surface_id,
> > +                                   &drawable->red_drawable->bbox, l,
> > TRUE);
> >           resent_surface_ids[num_resent] =
> >           drawable->red_drawable->surface_id;
> >           resent_areas[num_resent] = drawable->red_drawable->bbox;
> >           num_resent++;
> >   
> > -        spice_assert(image);
> > +        GList *image_pos = l->next;
> 
> l may be the queue tail, and in that case l->next would be
> NULL, wouldn't it ?
> 

No, dcc_add_surface_area_image add an element after l so l->next can't be NULL.

> > +        spice_assert(image_pos);
> >           red_channel_client_pipe_remove_and_release_pos(RED_CHANNEL_CLIENT(dcc),
> >           l);
> > +        l = image_pos;
> >       }
> >   }
> >   
> > 
> 
> I solved it differently:
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 84fa1be72..255e893f7 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -713,7 +713,7 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
>       int resent_surface_ids[MAX_PIPE_SIZE];
>       SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since
> drawables may be released
>       int num_resent;
> -    GList *l;
> +    GList *l, *lprev;

I would use just "prev" or "l_prev".

>       GQueue *pipe;
> 
>       resent_surface_ids[0] = first_surface_id;
> @@ -723,12 +723,13 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
>       pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));
> 
>       // going from the oldest to the newest
> -    for (l = pipe->tail; l != NULL; l = l->prev) {
> +    for (l = pipe->tail; l != NULL; l = lprev) {
>           RedPipeItem *pipe_item = l->data;
>           Drawable *drawable;
>           RedDrawablePipeItem *dpi;
>           RedImageItem *image;
> 
> +        lprev = l->prev;
>           if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
>               continue;
>           dpi = SPICE_UPCAST(RedDrawablePipeItem, pipe_item);
> 
> 
> 
> 
> Uri.
> 

Fine with it.
Or maybe you can update "l" directly and use a "curr" instead with
limited scope (inside the "for") and always initialized?

Frediano