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

[Andriy Gelman]

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.
> 
> Cheers,
> Moritz

> From 3bbf40dd08bb67e993cca97880aec032644fd02b Mon Sep 17 00:00:00 2001
> From: Moritz Barsnick <barsnick at gmx.net>
> Date: Fri, 10 Jul 2020 13:26:55 +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 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..be4e0d14f9 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;
> +    }
> 
>      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(c, 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(c, AV_LOG_ERROR, "Failed to get xcb geometry\n");

The rest of the av_log calls use AVFormatContext*.
You may want to update to be consistent.

> +            free(p);
> +            return AVERROR_EXTERNAL;
> +        }
>      }

Thanks,
-- 
Andriy