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

[Uri Lublin]

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 ?