[PATCH] avdevice/xcbgrab: check return values of xcb query functions
xcb_query_pointer_reply() and xcb_get_geometry_reply() return NULL when the display drops. This needs to be checked in order to cleanly exit, because the returned pointers are dereferenced later. Signed-off-by: Moritz Barsnick <barsnick@gmx.net> --- libavdevice/xcbgrab.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c index 6d142abd4f..609445918b 100644 --- a/libavdevice/xcbgrab.c +++ b/libavdevice/xcbgrab.c @@ -404,7 +404,17 @@ 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, "Could not get xcb pointer\n"); + ret = AVERROR(EIO); + goto fail; + } geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(c, AV_LOG_ERROR, "Could not get xcb geometry\n"); + ret = AVERROR_EOF; + goto fail; + } } if (c->follow_mouse && p->same_screen) @@ -425,8 +435,9 @@ static int xcbgrab_read_packet(AVFormatContext *s, AVPacket *pkt) xcbgrab_draw_mouse(s, pkt, p, geo); #endif - free(p); - free(geo); +fail: + av_free(p); + av_free(geo); return ret; } @@ -537,6 +548,10 @@ static int create_stream(AVFormatContext *s) gc = xcb_get_geometry(c->conn, c->screen->root); geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(c, AV_LOG_ERROR, "Could not get xcb geometry\n"); + return AVERROR(EIO); + } if (c->x + c->width > geo->width || c->y + c->height > geo->height) { -- 2.14.4
2018-06-26 17:42 GMT+02:00, Moritz Barsnick <barsnick@gmx.net>:
@@ -425,8 +435,9 @@ static int xcbgrab_read_packet xcbgrab_draw_mouse(s, pkt, p, geo); #endif
- free(p); - free(geo); +fail: + av_free(p); + av_free(geo);
I suspect this is incorrect, if it is correct, it should be a separate patch. Thank you, Carl Eugen
On Tue, Jun 26, 2018 at 23:01:16 +0200, Carl Eugen Hoyos wrote:
2018-06-26 17:42 GMT+02:00, Moritz Barsnick <barsnick@gmx.net>:
+fail: + av_free(p); + av_free(geo);
I suspect this is incorrect, if it is correct, it should be a separate patch.
I agree. There were two misconceptions of mine in there, and a further buglet. Patch drop, V2 to follow. Thanks, Moritz
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. Signed-off-by: Moritz Barsnick <barsnick@gmx.net> --- libavdevice/xcbgrab.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c index 6d142abd4f..ccab777c6e 100644 --- a/libavdevice/xcbgrab.c +++ b/libavdevice/xcbgrab.c @@ -404,7 +404,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, "Cannot get xcb pointer\n"); + return AVERROR(EIO); + } geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(c, AV_LOG_ERROR, "Cannot get xcb geometry\n"); + free(p); + return AVERROR(EIO); + } } if (c->follow_mouse && p->same_screen) @@ -537,6 +546,10 @@ static int create_stream(AVFormatContext *s) gc = xcb_get_geometry(c->conn, c->screen->root); geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(c, AV_LOG_ERROR, "Cannot get xcb geometry\n"); + return AVERROR(EIO); + } if (c->x + c->width > geo->width || c->y + c->height > geo->height) { -- 2.14.4
On Wed, Jun 27, 2018 at 10:21:51 +0200, Moritz Barsnick wrote:
Subject: [FFmpeg-devel] [PATCH] avdevice/xcbgrab: check return values of xcb query functions
Sorry, I had told git send-email to use [PATCH v2]. *shrug*
+ if (!p) { + av_log(c, AV_LOG_ERROR, "Cannot get xcb pointer\n");
The value of the messages is debatable. Feel free to discuss. This gives such an error using ffmpeg command line with this patch:
[xcbgrab indev @ 0xa156740] Cannot get xcb pointer :1: Input/output error
The last line may suffice. Thanks, Moritz
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> --- 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. libavdevice/xcbgrab.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c index b7e689343e..3fb3c56285 100644 --- a/libavdevice/xcbgrab.c +++ b/libavdevice/xcbgrab.c @@ -326,8 +326,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; @@ -404,7 +406,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(EIO); + } geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(c, AV_LOG_ERROR, "Failed get xcb geometry\n"); + free(p); + return AVERROR(EIO); + } } if (c->follow_mouse && p->same_screen) @@ -537,6 +548,10 @@ static int create_stream(AVFormatContext *s) gc = xcb_get_geometry(c->conn, c->screen->root); geo = xcb_get_geometry_reply(c->conn, gc, NULL); + if (!geo) { + av_log(c, AV_LOG_ERROR, "Failed to get xcb geometry\n"); + return AVERROR(EIO); + } if (c->x + c->width > geo->width || c->y + c->height > geo->height) { -- 2.20.1
On Thu, Sep 19, 2019 at 17:42:30 +0200, Moritz Barsnick wrote:
+ av_log(c, AV_LOG_ERROR, "Failed get xcb geometry\n");
D'uh, missing a "to " in there. Will fix, or pusher please do so. Moritz
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
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, -- Andriy
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
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
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. [...] Alexander
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
Hi Andriy! On 2020-07-19 19:47 -0400, Andriy Gelman wrote:
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: [...]
--- 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.
My argument is, that ci could well have been allocated, but maybe it doesn't contain cursor data and then xcb_xfixes_get_cursor_image_cursor_image checks for that and does return NULL instead of an offset. Or xcb_xfixes_get_cursor_image_cursor_image is changed to allocate an internally managed copy of the cursor at access time, which could fail. It's purely hypothetical, but it could happen. Being able to do things differently at some point in the future is often part of the argumentation for having accessor functions in the first place, but it only works if the callers don't assume details about the implementation.
But anyway, this part of the patch doesn't really have anything to do with ticket #7312, and should be in a separate patch.
Yes, it's definitely something that was changed in this patch at all. So it's better not to touch it in this patch. If you really like to change it in the future, that is acceptable for me though I don't really see the point. Maybe there is even stronger guarantees around, that I fail to see documented in the API docs I read so far. Alexander
On Mon, Jul 20, 2020 at 09:18:55 +0200, Alexander Strasser wrote:
On 2020-07-19 19:47 -0400, Andriy Gelman wrote:
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.
But anyway, this part of the patch doesn't really have anything to do with ticket #7312, and should be in a separate patch.
Yes, it's definitely something that was changed in this patch at all. So it's better not to touch it in this patch.
Okay, so I "fixed" dead code. You guys can remove the dead code yourselves then, if you like. ;-) New patch for the original issue attached, not touching the dead code. Thanks, Moritz
On Wed, 05. Aug 14:37, Moritz Barsnick wrote:
On Mon, Jul 20, 2020 at 09:18:55 +0200, Alexander Strasser wrote:
On 2020-07-19 19:47 -0400, Andriy Gelman wrote:
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.
But anyway, this part of the patch doesn't really have anything to do with ticket #7312, and should be in a separate patch.
Yes, it's definitely something that was changed in this patch at all. So it's better not to touch it in this patch.
Okay, so I "fixed" dead code. You guys can remove the dead code yourselves then, if you like. ;-)
New patch for the original issue attached, not touching the dead code.
Thanks, Moritz
From e44b7f03354add2272a2739e04aafb38b7ce027f Mon Sep 17 00:00:00 2001 From: Moritz Barsnick <barsnick@gmx.net> Date: Wed, 5 Aug 2020 14:06:53 +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.
Signed-off-by: Moritz Barsnick <barsnick@gmx.net> --- libavdevice/xcbgrab.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c index 6f6b2dbf15..8ef2a30d02 100644 --- a/libavdevice/xcbgrab.c +++ b/libavdevice/xcbgrab.c @@ -425,7 +425,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; + } }
if (c->follow_mouse && p->same_screen) -- 2.26.2
lgtm Thanks, -- Andriy
On Sat, 08. Aug 09:55, Andriy Gelman wrote:
On Wed, 05. Aug 14:37, Moritz Barsnick wrote:
On Mon, Jul 20, 2020 at 09:18:55 +0200, Alexander Strasser wrote:
On 2020-07-19 19:47 -0400, Andriy Gelman wrote:
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.
But anyway, this part of the patch doesn't really have anything to do with ticket #7312, and should be in a separate patch.
Yes, it's definitely something that was changed in this patch at all. So it's better not to touch it in this patch.
Okay, so I "fixed" dead code. You guys can remove the dead code yourselves then, if you like. ;-)
New patch for the original issue attached, not touching the dead code.
Thanks, Moritz
From e44b7f03354add2272a2739e04aafb38b7ce027f Mon Sep 17 00:00:00 2001 From: Moritz Barsnick <barsnick@gmx.net> Date: Wed, 5 Aug 2020 14:06:53 +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.
Signed-off-by: Moritz Barsnick <barsnick@gmx.net> --- libavdevice/xcbgrab.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c index 6f6b2dbf15..8ef2a30d02 100644 --- a/libavdevice/xcbgrab.c +++ b/libavdevice/xcbgrab.c @@ -425,7 +425,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; + } }
if (c->follow_mouse && p->same_screen) -- 2.26.2
lgtm
Will apply this tomorrow if no one objects. -- Andriy
On Thu, 13. Aug 23:43, Andriy Gelman wrote:
On Sat, 08. Aug 09:55, Andriy Gelman wrote:
On Wed, 05. Aug 14:37, Moritz Barsnick wrote:
On Mon, Jul 20, 2020 at 09:18:55 +0200, Alexander Strasser wrote:
On 2020-07-19 19:47 -0400, Andriy Gelman wrote:
> 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.
But anyway, this part of the patch doesn't really have anything to do with ticket #7312, and should be in a separate patch.
Yes, it's definitely something that was changed in this patch at all. So it's better not to touch it in this patch.
Okay, so I "fixed" dead code. You guys can remove the dead code yourselves then, if you like. ;-)
New patch for the original issue attached, not touching the dead code.
Thanks, Moritz
From e44b7f03354add2272a2739e04aafb38b7ce027f Mon Sep 17 00:00:00 2001 From: Moritz Barsnick <barsnick@gmx.net> Date: Wed, 5 Aug 2020 14:06:53 +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.
Signed-off-by: Moritz Barsnick <barsnick@gmx.net> --- libavdevice/xcbgrab.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/libavdevice/xcbgrab.c b/libavdevice/xcbgrab.c index 6f6b2dbf15..8ef2a30d02 100644 --- a/libavdevice/xcbgrab.c +++ b/libavdevice/xcbgrab.c @@ -425,7 +425,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; + } }
if (c->follow_mouse && p->same_screen) -- 2.26.2
lgtm
Will apply this tomorrow if no one objects.
Applied. Thanks, -- Andriy
participants (4)
-
Alexander Strasser -
Andriy Gelman -
Carl Eugen Hoyos -
Moritz Barsnick