On Tue, 14. Jul 14:14, Moritz Barsnick wrote:
On Sun, Jul 12, 2020 at 10:54:45 -0400, Andriy Gelman wrote:
On Fri, 10. Jul 21:13, Moritz Barsnick wrote:
Since xcbgrab is getting some attention recently...
Fixes a segfault, as reported in #7312.
To reproduce: Terminal 1: $ Xvfb :1 -nolisten tcp -screen 0 800x600x24 Terminal 2: $ ffmpeg -f x11grab -i :1 -f null - or rather $ gdb -ex r --args ffmpeg_g -f x11grab -i :1 -f null - Then terminate Xvfb while ffmpeg is running.
The rest of the av_log calls use AVFormatContext*. You may want to update to be consistent.
Good point. I didn't mind the "xcbgrab indev" vs. "x11grab", but it's indeed inconsistent. Updated patch attached.
Moritz
From 269b43209394c0eceb83f5ae384792c32305333a Mon Sep 17 00:00:00 2001 From: Moritz Barsnick <barsnick@gmx.net> Date: Tue, 14 Jul 2020 14:07:33 +0200 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.
cx = ci->x - ci->xhot; cy = ci->y - ci->yhot; @@ -425,7 +427,16 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt) pc = xcb_query_pointer(c->conn, c->screen->root); gc = xcb_get_geometry(c->conn, c->screen->root); p = xcb_query_pointer_reply(c->conn, pc, NULL);
+ if (!p) { + av_log(s, AV_LOG_ERROR, "Failed to query xcb pointer\n"); + return AVERROR_EXTERNAL; + } geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(s, AV_LOG_ERROR, "Failed to get xcb geometry\n"); + free(p); + return AVERROR_EXTERNAL; + }
This part lgtm. Btw, when I was testing with -draw_mouse 0, there were no error messages when the X server was killed. We should probably also test if img==NULL after: img = xcb_get_image_reply(c->conn, iq, &e); But this is different patch imo. Thanks, -- Andriy