diff mbox series

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

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

Checks

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

Commit Message

Moritz Barsnick Aug. 5, 2020, 12:37 p.m. UTC
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(+)

--
2.26.2

Comments

Andriy Gelman Aug. 8, 2020, 1:55 p.m. UTC | #1
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 Gelman Aug. 14, 2020, 3:43 a.m. UTC | #2
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
Andriy Gelman Aug. 15, 2020, 7:42 p.m. UTC | #3
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 mbox series

Patch

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)