Message ID | 20200805123736.GA8049@sunshine.barsnick.net |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel,v6] avdevice/xcbgrab: check return values of xcb query functions | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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,
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,
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)