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

[Uri Lublin <uril@xxxxxxxxxx>]

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 ?

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel