[vlc-devel] [PATCH] vout: xcb: Prefer using MIT-SHM FD-passing when possible
Rémi Denis-Courmont
remi at remlab.net
Fri Feb 9 19:05:44 CET 2018
Le perjantaina 9. helmikuuta 2018, 13.36.59 EET Alexander Volkov a écrit :
> This makes the shared memory visible only to vlc
> and the X server to which it is connected.
There was indeed a security issue that other users could read your pictures.
But it was already fixed over 8 years ago by allocating segments as 0600
instead of 0666.
The only outstanding problem, for now, is a short race condition whereby the
segment can be leaked (until reboot) if VLC dies between shmget() and
shmctl(RMID).
> Signed-off-by: Alexander Volkov <a.volkov at rusbitech.ru>
> ---
> modules/video_output/xcb/pictures.c | 122
> +++++++++++++++++++++++++++++++++--- modules/video_output/xcb/pictures.h |
> 9 +--
> modules/video_output/xcb/x11.c | 6 +-
> modules/video_output/xcb/xvideo.c | 6 +-
> 4 files changed, 126 insertions(+), 17 deletions(-)
>
> diff --git a/modules/video_output/xcb/pictures.c
> b/modules/video_output/xcb/pictures.c index 547e41e15e..6503df92a2 100644
> --- a/modules/video_output/xcb/pictures.c
> +++ b/modules/video_output/xcb/pictures.c
> @@ -45,10 +45,21 @@
> #include "pictures.h"
> #include "events.h"
>
> +#ifdef HAVE_MMAP
> +#if (XCB_SHM_MAJOR_VERSION == 1 && XCB_SHM_MINOR_VERSION >= 2) ||
> XCB_SHM_MAJOR_VERSION > 1 +
> # define HAVE_XCB_SHM_FD
> +#endif
> +#endif
> +
> +#ifdef HAVE_XCB_SHM_FD
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#endif
> +
> /** Check MIT-SHM shared memory support */
> -bool XCB_shm_Check (vlc_object_t *obj, xcb_connection_t *conn)
> +bool XCB_shm_Check (vlc_object_t *obj, xcb_connection_t *conn, bool
> *shm_fd)
Missing restrict.
> {
> -#ifdef HAVE_SYS_SHM_H
> +#if defined(HAVE_SYS_SHM_H) || defined(HAVE_XCB_SHM_FD)
> xcb_shm_query_version_cookie_t ck;
> xcb_shm_query_version_reply_t *r;
>
> @@ -56,6 +67,10 @@ bool XCB_shm_Check (vlc_object_t *obj, xcb_connection_t
> *conn) r = xcb_shm_query_version_reply (conn, ck, NULL);
> if (r != NULL)
> {
> +# ifdef HAVE_XCB_SHM_FD
> + *shm_fd = (r->major_version == 1 && r->minor_version >= 2) ||
> + r->major_version > 1;
> +# endif
> free (r);
> return true;
> }
> @@ -68,12 +83,27 @@ bool XCB_shm_Check (vlc_object_t *obj, xcb_connection_t
> *conn) return false;
> }
>
> +typedef struct
> +{
> + xcb_shm_seg_t segment;
> + size_t size;
> + bool shm_fd;
There are no needs for a boolean here. Just define separate deletion callbacks
for each cases.
> +} vout_xcb_shm_segment_info_t;
> +
> /**
> * Release picture private data: detach the shared memory segment.
> */
> static void XCB_picture_Destroy (picture_t *pic)
> {
> +#ifdef HAVE_XCB_SHM_FD
> + vout_xcb_shm_segment_info_t *shm_info = pic->p_sys;
> + if (shm_info->shm_fd)
> + munmap (pic->p[0].p_pixels, shm_info->size);
> + else
> +#else
> shmdt (pic->p[0].p_pixels);
> +#endif
> + free (shm_info);
> free (pic);
> }
>
> @@ -84,9 +114,68 @@ static void XCB_picture_Destroy (picture_t *pic)
> */
> int XCB_picture_Alloc (vout_display_t *vd, picture_resource_t *res,
> size_t size, xcb_connection_t *conn,
> - xcb_shm_seg_t segment)
> + xcb_shm_seg_t segment, bool shm_fd)
> {
> -#ifdef HAVE_SYS_SHM_H
> +#if defined(HAVE_XCB_SHM_FD) || defined(HAVE_SYS_SHM_H)
> + void *shm = NULL;
> +# ifdef HAVE_XCB_SHM_FD
> + if (shm_fd)
> + {
> + xcb_shm_create_segment_cookie_t ck;
> + xcb_shm_create_segment_reply_t *r;
> + xcb_generic_error_t *error = NULL;
> + int *fds;
> +
> + if (size > UINT32_MAX)
> + {
> + msg_Err (vd, "xcb_shm_create_segment() can't be called for
> size: %zu," + " maximum allowed size is %u", size,
> UINT32_MAX);
> + return -1;
This looks too late. And it will cause a compilation warning on 32-bits.
> + }
> +
> + if (segment == 0)
> + {
> + msg_Err (vd, "xcb_shm_create_segment() can't be called with
> shmseg 0");
> + return -1;
> + }
This looks like logically dead code.
> +
> + ck = xcb_shm_create_segment (conn, segment, size, false);
Why create the segment on the server? That seems to create more problem than
it solves. Also it won't work with the future picture buffer push model.
> + r = xcb_shm_create_segment_reply (conn, ck, &error);
> + if (error)
> + {
> + msg_Err (vd, "xcb_shm_create_segment() failed for size %zu,
> error code: %d",
> + size, error->error_code);
Please use consistent error message style as existing surroundings.
> + free (error);
> + free (r);
> + return -1;
> + }
> +
> + if (r->nfd != 1)
> + {
> + msg_Err (vd, "xcb_shm_create_segment() returned incorrect
> number of file descriptors: %d",
> + r->nfd);
> + free(r);
> + return -1;
> + }
> +
> + fds = xcb_shm_create_segment_reply_fds (conn, r);
> + free (r);
> + shm = mmap (NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fds[0],
> 0); + close (fds[0]);
> + if (shm == MAP_FAILED)
> + {
> + msg_Err (vd, "failed to mmap segment created by
> xcb_shm_create_segment() for size %zu (%s)", + size,
> vlc_strerror_c(errno));
> + xcb_shm_detach (conn, segment);
> + return -1;
> + }
> + }
> +# ifdef HAVE_SYS_SHM_H
> + else
> +# endif
> +# endif
> +# ifdef HAVE_SYS_SHM_H
> + {
> /* Allocate shared memory segment */
> int id = shmget (IPC_PRIVATE, size, IPC_CREAT | S_IRWXU);
> if (id == -1)
> @@ -97,7 +186,7 @@ int XCB_picture_Alloc (vout_display_t *vd,
> picture_resource_t *res, }
>
> /* Attach the segment to VLC */
> - void *shm = shmat (id, NULL, 0 /* read/write */);
> + shm = shmat (id, NULL, 0 /* read/write */);
> if (-1 == (intptr_t)shm)
> {
> msg_Err (vd, "shared memory attachment error: %s",
> @@ -135,6 +224,8 @@ int XCB_picture_Alloc (vout_display_t *vd,
> picture_resource_t *res, }
>
> shmctl (id, IPC_RMID, NULL);
> + }
> +# endif
> #else
> assert (segment == 0);
>
> @@ -143,7 +234,12 @@ int XCB_picture_Alloc (vout_display_t *vd,
> picture_resource_t *res, if (unlikely(shm == NULL))
> return -1;
> #endif
> - res->p_sys = (void *)(uintptr_t)segment;
> + vout_xcb_shm_segment_info_t *shm_info = malloc
> (sizeof(vout_xcb_shm_segment_info_t)); + shm_info->segment = segment;
> + shm_info->size = size;
> + shm_info->shm_fd = shm_fd;
> +
> + res->p_sys = shm_info;
> res->pf_destroy = XCB_picture_Destroy;
> res->p[0].p_pixels = shm;
> return 0;
> @@ -156,11 +252,23 @@ picture_t *XCB_picture_NewFromResource (const
> video_format_t *restrict fmt, picture_t *pic = picture_NewFromResource
> (fmt, res);
> if (unlikely(pic == NULL))
> {
> - xcb_shm_seg_t seg = (uintptr_t)res->p_sys;
> + vout_xcb_shm_segment_info_t *shm_info = res->p_sys;
> + xcb_shm_seg_t seg = shm_info->segment;
>
> if (seg != 0)
> xcb_shm_detach (conn, seg);
> +#ifdef HAVE_XCB_SHM_FD
> + if (shm_info->shm_fd)
> + munmap (res->p[0].p_pixels, shm_info->size);
> + else
> +#endif
> shmdt (res->p[0].p_pixels);
> }
> return pic;
> }
> +
> +xcb_shm_seg_t XCB_picture_GetSegment(const picture_t *pic)
> +{
> + vout_xcb_shm_segment_info_t *shm_info = pic->p_sys;
> + return shm_info->segment;
> +}
> diff --git a/modules/video_output/xcb/pictures.h
> b/modules/video_output/xcb/pictures.h index 9cb2deae24..585aa2bb83 100644
> --- a/modules/video_output/xcb/pictures.h
> +++ b/modules/video_output/xcb/pictures.h
> @@ -30,14 +30,11 @@
> #include <vlc_vout_display.h>
> #include <xcb/shm.h>
>
> -bool XCB_shm_Check (vlc_object_t *obj, xcb_connection_t *conn);
> +bool XCB_shm_Check (vlc_object_t *obj, xcb_connection_t *conn, bool
> *shm_fd); int XCB_picture_Alloc (vout_display_t *, picture_resource_t *,
> size_t size, - xcb_connection_t *, xcb_shm_seg_t);
> + xcb_connection_t *, xcb_shm_seg_t, bool shm_fd);
> picture_t *XCB_picture_NewFromResource (const video_format_t *,
> const picture_resource_t *,
> xcb_connection_t *);
>
> -static inline xcb_shm_seg_t XCB_picture_GetSegment(const picture_t *pic)
> -{
> - return (uintptr_t)pic->p_sys;
> -}
> +xcb_shm_seg_t XCB_picture_GetSegment(const picture_t *pic);
> diff --git a/modules/video_output/xcb/x11.c b/modules/video_output/xcb/x11.c
> index ef671c84f5..101c139b1e 100644
> --- a/modules/video_output/xcb/x11.c
> +++ b/modules/video_output/xcb/x11.c
> @@ -69,6 +69,7 @@ struct vout_display_sys_t
> xcb_window_t window; /* drawable X window */
> xcb_gcontext_t gc; /* context to put images */
> xcb_shm_seg_t seg_base; /**< shared memory segment XID base */
> + bool shm_fd; /* whether to use MIT-SHM FD-passing */
> bool visible; /* whether to draw */
> uint8_t depth; /* useful bits per pixel */
>
> @@ -287,7 +288,8 @@ found_format:;
> msg_Dbg (vd, "using X11 graphic context %08"PRIx32, sys->gc);
>
> sys->visible = false;
> - if (XCB_shm_Check (obj, conn))
> + sys->shm_fd = false;
> + if (XCB_shm_Check (obj, conn, &sys->shm_fd))
> {
> sys->seg_base = xcb_generate_id (conn);
> for (unsigned i = 1; i < MAX_PICTURES; i++)
> @@ -374,7 +376,7 @@ static picture_pool_t *Pool (vout_display_t *vd,
> unsigned requested_count) {
> xcb_shm_seg_t seg = (sys->seg_base != 0) ? (sys->seg_base + count)
> : 0;
>
> - if (XCB_picture_Alloc (vd, &res, size, sys->conn, seg))
> + if (XCB_picture_Alloc (vd, &res, size, sys->conn, seg,
> sys->shm_fd)) break;
> pic_array[count] = XCB_picture_NewFromResource (&vd->fmt, &res,
> sys->conn);
> diff --git a/modules/video_output/xcb/xvideo.c
> b/modules/video_output/xcb/xvideo.c index 46d4fb2b29..cf3aa30e2f 100644
> --- a/modules/video_output/xcb/xvideo.c
> +++ b/modules/video_output/xcb/xvideo.c
> @@ -91,6 +91,7 @@ struct vout_display_sys_t
> uint32_t data_size; /* picture byte size (for non-SHM) */
> bool swap_uv; /* U/V pointer must be swapped in a picture */
> bool shm; /* whether to use MIT-SHM */
> + bool shm_fd; /* whether to use MIT-SHM FD-passing */
> bool visible; /* whether it makes sense to draw at all */
>
> xcb_xv_query_image_attributes_reply_t *att;
> @@ -547,7 +548,8 @@ static int Open (vlc_object_t *obj)
> free(r);
> }
>
> - p_sys->shm = XCB_shm_Check (obj, conn);
> + p_sys->shm_fd = false;
> + p_sys->shm = XCB_shm_Check (obj, conn, &p_sys->shm_fd);
> p_sys->visible = false;
>
> /* */
> @@ -619,7 +621,7 @@ static void PoolAlloc (vout_display_t *vd, unsigned
> requested_count) {
> xcb_shm_seg_t seg = p_sys->shm ? xcb_generate_id (p_sys->conn) : 0;
>
> - if (XCB_picture_Alloc (vd, &res, p_sys->data_size, p_sys->conn,
> seg))
> + if (XCB_picture_Alloc (vd, &res, p_sys->data_size,
> p_sys->conn, seg, p_sys->shm_fd)) break;
>
> /* Allocate further planes as specified by XVideo */
--
雷米‧德尼-库尔蒙
https://www.remlab.net/
More information about the vlc-devel
mailing list