diff mbox series

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

Message ID 20200714121407.GA24772@sunshine.barsnick.net
State Superseded
Headers show
Series [FFmpeg-devel,v5] 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 14, 2020, 12:14 p.m. UTC
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(-)

--
2.26.2

Comments

Andriy Gelman July 17, 2020, 3:54 a.m. UTC | #1
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,
Alexander Strasser July 19, 2020, 9:19 p.m. UTC | #2
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
Andriy Gelman July 19, 2020, 11:47 p.m. UTC | #3
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
Alexander Strasser July 20, 2020, 7:18 a.m. UTC | #4
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
diff mbox series

Patch

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;
+    }

     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;
+        }
     }

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