[2/2] xcb: Use SHM FD-passing if possible

Submitted by Alexander Volkov on April 6, 2018, 12:33 p.m.

Details

Message ID 20180406123337.6606-2-a.volkov@rusbitech.ru
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Alexander Volkov April 6, 2018, 12:33 p.m.
Shared memory created this way is visible only to the X server
and an application.

- it can used if the X server is running as a non-root user other
  than a user an application is running as
- other applications of the same user can't access it

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=90512
Signed-off-by: Alexander Volkov <a.volkov@rusbitech.ru>
---
 boilerplate/Makefile.win32.features | 12 ++++
 build/Makefile.win32.features       |  1 +
 build/Makefile.win32.features-h     |  3 +
 build/configure.ac.features         |  1 +
 configure.ac                        | 11 ++++
 src/Makefile.win32.features         | 16 +++++
 src/cairo-xcb-connection-shm.c      | 98 ++++++++++++++++++++++-------
 src/cairo-xcb-connection.c          |  7 ++-
 src/cairo-xcb-private.h             |  4 +-
 9 files changed, 130 insertions(+), 23 deletions(-)

Patch hide | download patch | download mbox

diff --git a/boilerplate/Makefile.win32.features b/boilerplate/Makefile.win32.features
index 178d5b650..84df408d1 100644
--- a/boilerplate/Makefile.win32.features
+++ b/boilerplate/Makefile.win32.features
@@ -79,6 +79,18 @@  enabled_cairo_boilerplate_cxx_sources += $(cairo_boilerplate_xcb_shm_cxx_sources
 enabled_cairo_boilerplate_sources += $(cairo_boilerplate_xcb_shm_sources)
 endif
 
+supported_cairo_boilerplate_headers += $(cairo_boilerplate_xcb_shm_fd_headers)
+all_cairo_boilerplate_headers += $(cairo_boilerplate_xcb_shm_fd_headers)
+all_cairo_boilerplate_private += $(cairo_boilerplate_xcb_shm_fd_private)
+all_cairo_boilerplate_cxx_sources += $(cairo_boilerplate_xcb_shm_fd_cxx_sources)
+all_cairo_boilerplate_sources += $(cairo_boilerplate_xcb_shm_fd_sources)
+ifeq ($(CAIRO_HAS_XCB_SHM_FD_FUNCTIONS),1)
+enabled_cairo_boilerplate_headers += $(cairo_boilerplate_xcb_shm_fd_headers)
+enabled_cairo_boilerplate_private += $(cairo_boilerplate_xcb_shm_fd_private)
+enabled_cairo_boilerplate_cxx_sources += $(cairo_boilerplate_xcb_shm_fd_cxx_sources)
+enabled_cairo_boilerplate_sources += $(cairo_boilerplate_xcb_shm_fd_sources)
+endif
+
 unsupported_cairo_boilerplate_headers += $(cairo_boilerplate_qt_headers)
 all_cairo_boilerplate_headers += $(cairo_boilerplate_qt_headers)
 all_cairo_boilerplate_private += $(cairo_boilerplate_qt_private)
diff --git a/build/Makefile.win32.features b/build/Makefile.win32.features
index b15c4488f..f8a1e441d 100644
--- a/build/Makefile.win32.features
+++ b/build/Makefile.win32.features
@@ -5,6 +5,7 @@  CAIRO_HAS_XLIB_XRENDER_SURFACE=0
 CAIRO_HAS_XCB_SURFACE=0
 CAIRO_HAS_XLIB_XCB_FUNCTIONS=0
 CAIRO_HAS_XCB_SHM_FUNCTIONS=0
+CAIRO_HAS_XCB_SHM_FD_FUNCTIONS=0
 CAIRO_HAS_QT_SURFACE=0
 CAIRO_HAS_QUARTZ_SURFACE=0
 CAIRO_HAS_QUARTZ_FONT=0
diff --git a/build/Makefile.win32.features-h b/build/Makefile.win32.features-h
index 5759b48a3..53984bb4c 100644
--- a/build/Makefile.win32.features-h
+++ b/build/Makefile.win32.features-h
@@ -20,6 +20,9 @@  endif
 ifeq ($(CAIRO_HAS_XCB_SHM_FUNCTIONS),1)
 	@echo "#define CAIRO_HAS_XCB_SHM_FUNCTIONS 1" >> $(top_srcdir)/src/cairo-features.h
 endif
+ifeq ($(CAIRO_HAS_XCB_SHM_FD_FUNCTIONS),1)
+	@echo "#define CAIRO_HAS_XCB_SHM_FD_FUNCTIONS 1" >> $(top_srcdir)/src/cairo-features.h
+endif
 ifeq ($(CAIRO_HAS_QT_SURFACE),1)
 	@echo "#define CAIRO_HAS_QT_SURFACE 1" >> $(top_srcdir)/src/cairo-features.h
 endif
diff --git a/build/configure.ac.features b/build/configure.ac.features
index e0a46069c..6c8a8f071 100644
--- a/build/configure.ac.features
+++ b/build/configure.ac.features
@@ -405,6 +405,7 @@  AC_DEFUN([CAIRO_REPORT],
 	echo "  EGL functions:   $use_egl"
 	echo "  X11-xcb functions: $use_xlib_xcb"
 	echo "  XCB-shm functions: $use_xcb_shm"
+	echo "  XCB-shm FD-passing functions: $use_xcb_shm_fd"
 	echo ""
 	echo "The following features and utilities:"
 	echo "  cairo-trace:                $use_trace"
diff --git a/configure.ac b/configure.ac
index d78b2ed41..8c7fa4267 100644
--- a/configure.ac
+++ b/configure.ac
@@ -182,11 +182,22 @@  CAIRO_ENABLE_FUNCTIONS(xcb_shm, XCB/SHM, auto, [
       xcb_shm_REQUIRES="xcb-shm"
       PKG_CHECK_MODULES(xcb_shm, $xcb_shm_REQUIRES, ,
 			[use_xcb_shm="no (requires $xcb_shm http://xcb.freedesktop.org)"])
+
   else
     use_xcb_shm="no (requires --enable-xcb)"
   fi
 ])
 
+CAIRO_ENABLE_FUNCTIONS(xcb_shm_fd, XCB/SHM FD-passing, auto, [
+  if test "x$use_xcb_shm" = "xyes"; then
+      xcb_shm_fd_REQUIRES="xcb-shm >= 1.9.3"
+      PKG_CHECK_MODULES(xcb_shm_fd, $xcb_shm_fd_REQUIRES, ,
+                        [use_xcb_shm_fd="no (requires $xcb_shm_fd_REQUIRES http://xcb.freedesktop.org)"])
+  else
+      use_xcb_shm_fd="no (requires --enable-xcb-shm)"
+  fi
+])
+
 dnl ===========================================================================
 
 CAIRO_ENABLE_SURFACE_BACKEND(qt, Qt, no, [
diff --git a/src/Makefile.win32.features b/src/Makefile.win32.features
index 377d6dd12..163e2507c 100644
--- a/src/Makefile.win32.features
+++ b/src/Makefile.win32.features
@@ -101,6 +101,22 @@  ifeq ($(CAIRO_HAS_XCB_SHM_FUNCTIONS),1)
 enabled_cairo_pkgconf += cairo-xcb-shm.pc
 endif
 
+supported_cairo_headers += $(cairo_xcb_shm_fd_headers)
+all_cairo_headers += $(cairo_xcb_shm_fd_headers)
+all_cairo_private += $(cairo_xcb_shm_fd_private)
+all_cairo_cxx_sources += $(cairo_xcb_shm_fd_cxx_sources)
+all_cairo_sources += $(cairo_xcb_shm_fd_sources)
+ifeq ($(CAIRO_HAS_XCB_SHM_FD_FUNCTIONS),1)
+enabled_cairo_headers += $(cairo_xcb_shm_fd_headers)
+enabled_cairo_private += $(cairo_xcb_shm_fd_private)
+enabled_cairo_cxx_sources += $(cairo_xcb_shm_fd_cxx_sources)
+enabled_cairo_sources += $(cairo_xcb_shm_fd_sources)
+endif
+all_cairo_pkgconf += cairo-xcb-shm-fd.pc
+ifeq ($(CAIRO_HAS_XCB_SHM_FD_FUNCTIONS),1)
+enabled_cairo_pkgconf += cairo-xcb-shm-fd.pc
+endif
+
 unsupported_cairo_headers += $(cairo_qt_headers)
 all_cairo_headers += $(cairo_qt_headers)
 all_cairo_private += $(cairo_qt_private)
diff --git a/src/cairo-xcb-connection-shm.c b/src/cairo-xcb-connection-shm.c
index e00377cff..bb73ca9b1 100644
--- a/src/cairo-xcb-connection-shm.c
+++ b/src/cairo-xcb-connection-shm.c
@@ -41,35 +41,86 @@ 
 #include <sys/shm.h>
 #include <errno.h>
 
+#if CAIRO_HAS_XCB_SHM_FD_FUNCTIONS
+#include <sys/mman.h>
+#include <unistd.h>
+#endif
+
 cairo_int_status_t
 _cairo_xcb_connection_shm_create_segment (cairo_xcb_connection_t *connection,
 				  size_t size,
 				  cairo_bool_t readonly,
 				  cairo_xcb_shm_segment_info_t *shm)
 {
-    xcb_void_cookie_t cookie;
-    xcb_generic_error_t *error;
-
-    shm->shmid = shmget (IPC_PRIVATE, size, IPC_CREAT | 0600);
-    if (shm->shmid == -1) {
-	return errno == ENOMEM ? CAIRO_STATUS_NO_MEMORY : CAIRO_INT_STATUS_UNSUPPORTED;
-    }
-
-    shm->shm = shmat (shm->shmid, NULL, 0);
-    shmctl (shm->shmid, IPC_RMID, NULL);
-    if (unlikely (shm->shm == (char *) -1))
-	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
-
-    shm->shmseg = _cairo_xcb_connection_get_xid (connection);
-    cookie = xcb_shm_attach_checked (connection->xcb_connection, shm->shmseg, shm->shmid, readonly);
-    error = xcb_request_check (connection->xcb_connection, cookie);
-    if (error != NULL) {
-	_cairo_xcb_connection_put_xid (connection, shm->shmseg);
-	shmdt (shm->shm);
-	free (error);
-	return CAIRO_INT_STATUS_UNSUPPORTED;
+#if CAIRO_HAS_XCB_SHM_FD_FUNCTIONS
+    if (connection->flags & CAIRO_XCB_HAS_SHM_FD) {
+	xcb_shm_create_segment_cookie_t cookie;
+	xcb_shm_create_segment_reply_t *reply;
+	xcb_generic_error_t *error;
+	int *fds;
+	int err;
+
+	if (unlikely (size > UINT32_MAX))
+	    return CAIRO_STATUS_NO_MEMORY;
+
+	shm->shmseg = _cairo_xcb_connection_get_xid (connection);
+	cookie = xcb_shm_create_segment (connection->xcb_connection, shm->shmseg, size, FALSE);
+	reply = xcb_shm_create_segment_reply (connection->xcb_connection, cookie, &error);
+	if (error) {
+	    free (error);
+	    _cairo_xcb_connection_put_xid (connection, shm->shmseg);
+	    return CAIRO_INT_STATUS_UNSUPPORTED;
+	}
+
+	fds = xcb_shm_create_segment_reply_fds (connection->xcb_connection, reply);
+	if (reply->nfd != 1) {
+	    for (int i = 0; i < reply->nfd; i++)
+		close (fds[i]);
+
+	    free (reply);
+	    xcb_shm_detach (connection->xcb_connection, shm->shmseg);
+	    _cairo_xcb_connection_put_xid (connection, shm->shmseg);
+	    return CAIRO_INT_STATUS_UNSUPPORTED;
+	}
+
+	shm->shm = mmap (NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fds[0], 0);
+	err = errno;
+	close (fds[0]);
+	free (reply);
+
+	if (shm->shm == MAP_FAILED) {
+	    xcb_shm_detach (connection->xcb_connection, shm->shmseg);
+	    _cairo_xcb_connection_put_xid (connection, shm->shmseg);
+	    return err == ENOMEM ? CAIRO_STATUS_NO_MEMORY : CAIRO_INT_STATUS_UNSUPPORTED;
+	}
+    } else
+#endif
+    {
+	xcb_void_cookie_t cookie;
+	xcb_generic_error_t *error;
+
+	shm->shmid = shmget (IPC_PRIVATE, size, IPC_CREAT | 0600);
+	if (shm->shmid == -1) {
+	    return errno == ENOMEM ? CAIRO_STATUS_NO_MEMORY : CAIRO_INT_STATUS_UNSUPPORTED;
+	}
+
+	shm->shm = shmat (shm->shmid, NULL, 0);
+	shmctl (shm->shmid, IPC_RMID, NULL);
+	if (unlikely (shm->shm == (char *) -1))
+	    return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+
+	shm->shmseg = _cairo_xcb_connection_get_xid (connection);
+	cookie = xcb_shm_attach_checked (connection->xcb_connection, shm->shmseg, shm->shmid, readonly);
+	error = xcb_request_check (connection->xcb_connection, cookie);
+	if (error != NULL) {
+	    _cairo_xcb_connection_put_xid (connection, shm->shmseg);
+	    shmdt (shm->shm);
+	    free (error);
+	    return CAIRO_INT_STATUS_UNSUPPORTED;
+	}
     }
 
+    shm->size = size;
     return CAIRO_INT_STATUS_SUCCESS;
 }
 
@@ -79,6 +130,11 @@  _cairo_xcb_connection_shm_destroy_segment (cairo_xcb_connection_t *connection,
 {
     xcb_shm_detach (connection->xcb_connection, shm->shmseg);
     _cairo_xcb_connection_put_xid (connection, shm->shmseg);
+#if CAIRO_HAS_XCB_SHM_FD_FUNCTIONS
+    if (connection->flags & CAIRO_XCB_HAS_SHM_FD)
+	munmap (shm->shm, shm->size);
+    else
+#endif
     shmdt (shm->shm);
 }
 
diff --git a/src/cairo-xcb-connection.c b/src/cairo-xcb-connection.c
index 67e2b11a0..2f7645cbe 100644
--- a/src/cairo-xcb-connection.c
+++ b/src/cairo-xcb-connection.c
@@ -453,12 +453,17 @@  _cairo_xcb_connection_query_shm (cairo_xcb_connection_t *connection)
     if (version == NULL)
 	return;
 
+    connection->flags |= CAIRO_XCB_HAS_SHM;
+    if ((version->major_version == 1 && version->minor_version >= 2) ||
+	version->major_version > 1)
+	connection->flags |= CAIRO_XCB_HAS_SHM_FD;
+
     free (version);
 
-    connection->flags |= CAIRO_XCB_HAS_SHM;
     status = _cairo_xcb_connection_shm_create_segment (connection, 0x1000, FALSE, &seg_info);
     if (status != CAIRO_INT_STATUS_SUCCESS) {
 	connection->flags &= ~CAIRO_XCB_HAS_SHM;
+	connection->flags &= ~CAIRO_XCB_HAS_SHM_FD;
 	return;
     }
 
diff --git a/src/cairo-xcb-private.h b/src/cairo-xcb-private.h
index 4e417ddff..8a3b9715a 100644
--- a/src/cairo-xcb-private.h
+++ b/src/cairo-xcb-private.h
@@ -82,6 +82,7 @@  struct _cairo_xcb_shm_segment_info {
     int shmid;
     uint32_t shmseg;
     void *shm;
+    size_t size;
 };
 
 struct _cairo_xcb_shm_info {
@@ -271,7 +272,8 @@  enum {
     CAIRO_XCB_RENDER_HAS_FILTER_GOOD		= 0x0400,
     CAIRO_XCB_RENDER_HAS_FILTER_BEST		= 0x0800,
 
-    CAIRO_XCB_HAS_SHM				= 0x80000000,
+    CAIRO_XCB_HAS_SHM				= 0x40000000,
+    CAIRO_XCB_HAS_SHM_FD			= 0x80000000,
 
     CAIRO_XCB_RENDER_MASK = CAIRO_XCB_HAS_RENDER |
 			    CAIRO_XCB_RENDER_HAS_FILL_RECTANGLES |

Comments

Quoting Alexander Volkov (2018-04-06 13:33:37)
> +    if (connection->flags & CAIRO_XCB_HAS_SHM_FD) {
> +       xcb_shm_create_segment_cookie_t cookie;
> +       xcb_shm_create_segment_reply_t *reply;
> +       xcb_generic_error_t *error;
> +       int *fds;
> +       int err;
> +
> +       if (unlikely (size > UINT32_MAX))
> +           return CAIRO_STATUS_NO_MEMORY;

That's not the protocol limit, surely? It may well be, just a bit
surprising if it was.

> +       shm->shmseg = _cairo_xcb_connection_get_xid (connection);
> +       cookie = xcb_shm_create_segment (connection->xcb_connection, shm->shmseg, size, FALSE);
> +       reply = xcb_shm_create_segment_reply (connection->xcb_connection, cookie, &error);
> +       if (error) {
> +           free (error);
> +           _cairo_xcb_connection_put_xid (connection, shm->shmseg);
> +           return CAIRO_INT_STATUS_UNSUPPORTED;
> +       }
> +
> +       fds = xcb_shm_create_segment_reply_fds (connection->xcb_connection, reply);
> +       if (reply->nfd != 1) {
> +           for (int i = 0; i < reply->nfd; i++)
> +               close (fds[i]);
> +
> +           free (reply);
> +           xcb_shm_detach (connection->xcb_connection, shm->shmseg);
> +           _cairo_xcb_connection_put_xid (connection, shm->shmseg);
> +           return CAIRO_INT_STATUS_UNSUPPORTED;
> +       }
> +
> +       shm->shm = mmap (NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fds[0], 0);

A roundtrip on allocation. That doesn't seem like something we would want
in a frequent path? And creating temporary surfaces is frequent enough.
That would suggest to me that we want an allocation cache... And packing
into a larger object?
-Chris
06.04.2018 16:38, Chris Wilson пишет:
> Quoting Alexander Volkov (2018-04-06 13:33:37)
>> +    if (connection->flags & CAIRO_XCB_HAS_SHM_FD) {
>> +       xcb_shm_create_segment_cookie_t cookie;
>> +       xcb_shm_create_segment_reply_t *reply;
>> +       xcb_generic_error_t *error;
>> +       int *fds;
>> +       int err;
>> +
>> +       if (unlikely (size > UINT32_MAX))
>> +           return CAIRO_STATUS_NO_MEMORY;
> That's not the protocol limit, surely? It may well be, just a bit
> surprising if it was.
Unfortunately it is. The size argument of xcb_shm_create_segment() is of 
type uint32_t.

>> +       shm->shmseg = _cairo_xcb_connection_get_xid (connection);
>> +       cookie = xcb_shm_create_segment (connection->xcb_connection, shm->shmseg, size, FALSE);
>> +       reply = xcb_shm_create_segment_reply (connection->xcb_connection, cookie, &error);
>> +       if (error) {
>> +           free (error);
>> +           _cairo_xcb_connection_put_xid (connection, shm->shmseg);
>> +           return CAIRO_INT_STATUS_UNSUPPORTED;
>> +       }
>> +
>> +       fds = xcb_shm_create_segment_reply_fds (connection->xcb_connection, reply);
>> +       if (reply->nfd != 1) {
>> +           for (int i = 0; i < reply->nfd; i++)
>> +               close (fds[i]);
>> +
>> +           free (reply);
>> +           xcb_shm_detach (connection->xcb_connection, shm->shmseg);
>> +           _cairo_xcb_connection_put_xid (connection, shm->shmseg);
>> +           return CAIRO_INT_STATUS_UNSUPPORTED;
>> +       }
>> +
>> +       shm->shm = mmap (NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fds[0], 0);
> A roundtrip on allocation. That doesn't seem like something we would want
> in a frequent path? And creating temporary surfaces is frequent enough.
> That would suggest to me that we want an allocation cache... And packing
> into a larger object?
Well, it seems that it would be better to use xcb_shm_attach_fd() 
instead of xcb_shm_create_segment().
It will also allow to avoid UINT32_MAX restriction.
Maybe I'll try it next week.
06.04.2018 17:17, Alexander Volkov пишет:
> Well, it seems that it would be better to use xcb_shm_attach_fd() 
> instead of xcb_shm_create_segment().
> It will also allow to avoid UINT32_MAX restriction.
> Maybe I'll try it next week.
First we need a function that could create memory files correctly:
https://lists.freedesktop.org/archives/xcb/2018-April/011095.html