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@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