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

[Alexander Strasser]

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.


[...]

  Alexander