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 <barsn...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
