[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