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 <barsn...@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 <barsn...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
