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

[Frediano Ziglio]

> 
> On 7/4/19 4:31 PM, Frediano Ziglio wrote:
> >>>
> >>> 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;
> >>>> +        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?
> >>
> > 
> > Why not using GLIST_FOREACH_REVERSED ?
> 
> At first I thought it works, but how do you get
> the location to add the image when calling
> dcc_add_surface_area_image ?
> 
> 

Yes, you are right, in this case better the old "manual" way.

Frediano