[FFmpeg-devel] [PATCH v5] avdevice/xcbgrab: check return values of xcb query functions

[Andriy Gelman]

On Sun, 19. Jul 23:19, Alexander Strasser wrote:
> On 2020-07-16 23:54 -0400, Andriy Gelman wrote:
> > On Tue, 14. Jul 14:14, Moritz Barsnick wrote:
> [...]
> > > Subject: [PATCH] avdevice/xcbgrab: check return values of xcb query functions
> > >
> > > Fixes #7312, segmentation fault on close of X11 server
> > >
> > > xcb_query_pointer_reply() and xcb_get_geometry_reply() can return NULL
> > > if e.g. the X server closes or the connection is lost. This needs to
> > > be checked in order to cleanly exit, because the returned pointers are
> > > dereferenced later.
> > >
> > > Furthermore, their return values need to be free()d, also in error
> > > code paths.
> > >
> > > Signed-off-by: Moritz Barsnick <barsnick at gmx.net>
> > > ---
> > >  libavdevice/xcbgrab.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c
> > > index 6f6b2dbf15..8bc320d055 100644
> > > --- a/libavdevice/xcbgrab.c
> > > +++ b/libavdevice/xcbgrab.c
> > > @@ -346,8 +346,10 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, AVPacket *pkt,
> > >          return;
> > >
> >
> > >      cursor = xcb_xfixes_get_cursor_image_cursor_image(ci);
> > > -    if (!cursor)
> > > +    if (!cursor) {
> > > +        free(ci);
> > >          return;
> > > +    }
> >
> > This check seems dead code. Looking at xcb sources, cursor is just an offset in
> > memory from ci so I don't think it can be null here.
> 
> It's great to look at the sources, but I don't think we should turn
> an implementation snapshot into a guarantee.
> 
> I guess it's safer to keep the check, if there is no documentation
> about this being always non-NULL.
> 
> I'm not entirely sure how well this is documented. Surely some of
> functions definitely return NULL sometimes, which was the reason to
> submit this patch, I would probably only assume non-NULL returns for
> functions where that is explicitly documented.

xcb_xfixes_get_cursor_image_cursor_image(ci) is just an accessor function to the 
reply:

uint32_t *
xcb_xfixes_get_cursor_image_cursor_image (const xcb_xfixes_get_cursor_image_reply_t *R)
{
    return (uint32_t *) (R + 1);
}

All the error handling should be done after the call to:
ci = xcb_xfixes_get_cursor_image_reply(gr->conn, cc, NULL); 

We check whether ci == NULL, but you can also pass xcb_generic_error_t** and
check the result.  

But anyway, this part of the patch doesn't really have anything to do with
ticket #7312, and should be in a separate patch.

--
Andriy