diff mbox series

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

Message ID 20200710191327.GB16321@sunshine.barsnick.net
State Superseded
Headers show
Series [FFmpeg-devel,v4] avdevice/xcbgrab: check return values of xcb query functions
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Moritz Barsnick July 10, 2020, 7:13 p.m. UTC
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@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@gmx.net>
---
 libavdevice/xcbgrab.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

--
2.26.2

Comments

Andriy Gelman July 12, 2020, 2:54 p.m. UTC | #1
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@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@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,
diff mbox series

Patch

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");
+            free(p);
+            return AVERROR_EXTERNAL;
+        }
     }

     if (c->follow_mouse && p->same_screen)