From c16094fc443c076475c4633d83d6e67539cf423e Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sun, 2 May 2021 17:49:32 +0200 Subject: [PATCH 1/2] xcb: Remove free XID cache cairo-xcb kept a cache of free xid to avoid calling xcb_generate_id() later. However, this is unsafe: When libxcb runs out of ids, it asks the X11 server for an empty range of ids to use. The X11 server of course does not know about cairo's cache and could hand out an id that cairo will use again later. This would then result in BadIdChoice errors later. Fix this by simply removing the whole cache. Fixes: https://gitlab.freedesktop.org/cairo/cairo/-/issues/434 Signed-off-by: Uli Schlachter --- src/cairo-xcb-connection-core.c | 6 ++-- src/cairo-xcb-connection-render.c | 2 -- src/cairo-xcb-connection-shm.c | 3 +- src/cairo-xcb-connection.c | 50 ++----------------------------- src/cairo-xcb-private.h | 10 ------- src/cairo-xcb-surface-render.c | 6 ++-- 6 files changed, 8 insertions(+), 69 deletions(-) diff --git a/src/cairo-xcb-connection-core.c b/src/cairo-xcb-connection-core.c index e01dc1a83..df6e2febd 100644 --- a/src/cairo-xcb-connection-core.c +++ b/src/cairo-xcb-connection-core.c @@ -42,7 +42,7 @@ _cairo_xcb_connection_create_pixmap (cairo_xcb_connection_t *connection, uint16_t width, uint16_t height) { - xcb_pixmap_t pixmap = _cairo_xcb_connection_get_xid (connection); + xcb_pixmap_t pixmap = xcb_generate_id (connection->xcb_connection); assert (width > 0); assert (height > 0); @@ -57,7 +57,6 @@ _cairo_xcb_connection_free_pixmap (cairo_xcb_connection_t *connection, xcb_pixmap_t pixmap) { xcb_free_pixmap (connection->xcb_connection, pixmap); - _cairo_xcb_connection_put_xid (connection, pixmap); } xcb_gcontext_t @@ -66,7 +65,7 @@ _cairo_xcb_connection_create_gc (cairo_xcb_connection_t *connection, uint32_t value_mask, uint32_t *values) { - xcb_gcontext_t gc = _cairo_xcb_connection_get_xid (connection); + xcb_gcontext_t gc = xcb_generate_id (connection->xcb_connection); xcb_create_gc (connection->xcb_connection, gc, drawable, value_mask, values); return gc; @@ -77,7 +76,6 @@ _cairo_xcb_connection_free_gc (cairo_xcb_connection_t *connection, xcb_gcontext_t gc) { xcb_free_gc (connection->xcb_connection, gc); - _cairo_xcb_connection_put_xid (connection, gc); } void diff --git a/src/cairo-xcb-connection-render.c b/src/cairo-xcb-connection-render.c index 61119653e..e27b2b3b2 100644 --- a/src/cairo-xcb-connection-render.c +++ b/src/cairo-xcb-connection-render.c @@ -79,7 +79,6 @@ _cairo_xcb_connection_render_free_picture (cairo_xcb_connection_t *connection, { assert (connection->flags & CAIRO_XCB_HAS_RENDER); xcb_render_free_picture (connection->xcb_connection, picture); - _cairo_xcb_connection_put_xid (connection, picture); } void @@ -133,7 +132,6 @@ _cairo_xcb_connection_render_free_glyph_set (cairo_xcb_connection_t *connec { assert (connection->flags & CAIRO_XCB_HAS_RENDER); xcb_render_free_glyph_set (connection->xcb_connection, glyphset); - _cairo_xcb_connection_put_xid (connection, glyphset); } void diff --git a/src/cairo-xcb-connection-shm.c b/src/cairo-xcb-connection-shm.c index 7720bbbd2..140a73cd0 100644 --- a/src/cairo-xcb-connection-shm.c +++ b/src/cairo-xcb-connection-shm.c @@ -43,7 +43,7 @@ _cairo_xcb_connection_shm_attach (cairo_xcb_connection_t *connection, uint32_t id, cairo_bool_t readonly) { - uint32_t segment = _cairo_xcb_connection_get_xid (connection); + uint32_t segment = xcb_generate_id (connection->xcb_connection); assert (connection->flags & CAIRO_XCB_HAS_SHM); xcb_shm_attach (connection->xcb_connection, segment, id, readonly); return segment; @@ -109,7 +109,6 @@ _cairo_xcb_connection_shm_detach (cairo_xcb_connection_t *connection, { assert (connection->flags & CAIRO_XCB_HAS_SHM); xcb_shm_detach (connection->xcb_connection, segment); - _cairo_xcb_connection_put_xid (connection, segment); } #endif /* CAIRO_HAS_XCB_SHM_FUNCTIONS */ diff --git a/src/cairo-xcb-connection.c b/src/cairo-xcb-connection.c index 51f5ee323..2c58cca70 100644 --- a/src/cairo-xcb-connection.c +++ b/src/cairo-xcb-connection.c @@ -255,7 +255,7 @@ pixmap_depths_usable (cairo_xcb_connection_t *connection, cairo_bool_t success = TRUE; int depth, i, j; - pixmap = _cairo_xcb_connection_get_xid (connection); + pixmap = xcb_generate_id (connection->xcb_connection); for (depth = 1, i = 0; depth <= 32; depth++) { if (missing & DEPTH_MASK(depth)) { @@ -275,8 +275,6 @@ pixmap_depths_usable (cairo_xcb_connection_t *connection, free (create_error); } - _cairo_xcb_connection_put_xid (connection, pixmap); - return success; } @@ -462,10 +460,9 @@ can_use_shm (cairo_xcb_connection_t *connection) return FALSE; } - shmseg = _cairo_xcb_connection_get_xid (connection); + shmseg = xcb_generate_id (connection->xcb_connection); cookie[0] = xcb_shm_attach_checked (c, shmseg, shmid, FALSE); cookie[1] = xcb_shm_detach_checked (c, shmseg); - _cairo_xcb_connection_put_xid (connection, shmseg); error = xcb_request_check (c, cookie[0]); if (error != NULL) @@ -586,8 +583,6 @@ _device_destroy (void *device) #endif _cairo_freepool_fini (&connection->shm_info_freelist); - _cairo_freepool_fini (&connection->xid_pool); - CAIRO_MUTEX_FINI (connection->shm_mutex); CAIRO_MUTEX_FINI (connection->screens_mutex); @@ -662,10 +657,6 @@ _cairo_xcb_connection_get (xcb_connection_t *xcb_connection) goto unlock; } - cairo_list_init (&connection->free_xids); - _cairo_freepool_init (&connection->xid_pool, - sizeof (cairo_xcb_xid_t)); - cairo_list_init (&connection->shm_pools); cairo_list_init (&connection->shm_pending); _cairo_freepool_init (&connection->shm_info_freelist, @@ -766,43 +757,6 @@ _cairo_xcb_connection_get_xrender_format_for_visual (cairo_xcb_connection_t *con return format ? format->xrender_format : XCB_NONE; } -void -_cairo_xcb_connection_put_xid (cairo_xcb_connection_t *connection, - uint32_t xid) -{ - cairo_xcb_xid_t *cache; - - assert (CAIRO_MUTEX_IS_LOCKED (connection->device.mutex)); - cache = _cairo_freepool_alloc (&connection->xid_pool); - if (likely (cache != NULL)) { - cache->xid = xid; - cairo_list_add (&cache->link, &connection->free_xids); - } -} - -uint32_t -_cairo_xcb_connection_get_xid (cairo_xcb_connection_t *connection) -{ - uint32_t xid; - - assert (CAIRO_MUTEX_IS_LOCKED (connection->device.mutex)); - if (! cairo_list_is_empty (&connection->free_xids)) { - cairo_xcb_xid_t *cache; - - cache = cairo_list_first_entry (&connection->free_xids, - cairo_xcb_xid_t, - link); - xid = cache->xid; - - cairo_list_del (&cache->link); - _cairo_freepool_free (&connection->xid_pool, cache); - } else { - xid = xcb_generate_id (connection->xcb_connection); - } - - return xid; -} - /** * cairo_xcb_device_get_connection: * @device: a #cairo_device_t for the XCB backend diff --git a/src/cairo-xcb-private.h b/src/cairo-xcb-private.h index f5d5a4c81..46b6e515d 100644 --- a/src/cairo-xcb-private.h +++ b/src/cairo-xcb-private.h @@ -226,9 +226,6 @@ struct _cairo_xcb_connection { const xcb_query_extension_reply_t *shm; xcb_render_sub_pixel_t *subpixel_orders; - cairo_list_t free_xids; - cairo_freepool_t xid_pool; - cairo_mutex_t shm_mutex; cairo_list_t shm_pools; cairo_list_t shm_pending; @@ -323,13 +320,6 @@ _cairo_xcb_connection_acquire (cairo_xcb_connection_t *connection) return cairo_device_acquire (&connection->device); } -cairo_private uint32_t -_cairo_xcb_connection_get_xid (cairo_xcb_connection_t *connection); - -cairo_private void -_cairo_xcb_connection_put_xid (cairo_xcb_connection_t *connection, - uint32_t xid); - static inline void _cairo_xcb_connection_release (cairo_xcb_connection_t *connection) { diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c index 6cb56cb78..c485524ce 100644 --- a/src/cairo-xcb-surface-render.c +++ b/src/cairo-xcb-surface-render.c @@ -128,7 +128,7 @@ _cairo_xcb_picture_create (cairo_xcb_screen_t *screen, cairo_list_add (&surface->link, &screen->pictures); surface->screen = screen; - surface->picture = _cairo_xcb_connection_get_xid (screen->connection); + surface->picture = xcb_generate_id (screen->connection->xcb_connection); surface->pixman_format = pixman_format; surface->xrender_format = xrender_format; @@ -308,7 +308,7 @@ _cairo_xcb_surface_ensure_picture (cairo_xcb_surface_t *surface) values[0] = surface->precision; } - surface->picture = _cairo_xcb_connection_get_xid (surface->connection); + surface->picture = xcb_generate_id (surface->connection->xcb_connection); _cairo_xcb_connection_render_create_picture (surface->connection, surface->picture, surface->drawable, @@ -4232,7 +4232,7 @@ _cairo_xcb_scaled_font_get_glyphset_info_for_format (cairo_xcb_connection_t *c, info = &priv->glyphset_info[glyphset_index]; if (info->glyphset == XCB_NONE) { - info->glyphset = _cairo_xcb_connection_get_xid (c); + info->glyphset = xcb_generate_id (c->xcb_connection); info->xrender_format = c->standard_formats[info->format]; _cairo_xcb_connection_render_create_glyph_set (c, From 009d75b8db33a8e5ae42eddcb63d9c5a5da9b60e Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sun, 2 May 2021 19:30:28 +0200 Subject: [PATCH 2/2] xcb: remove free pixmap/gc wrappers With the last commit, these became pointless. Just switch the code to call the underlying function directly. Signed-off-by: Uli Schlachter --- src/cairo-xcb-connection-core.c | 14 -------------- src/cairo-xcb-private.h | 8 -------- src/cairo-xcb-screen.c | 4 ++-- src/cairo-xcb-surface-core.c | 2 +- src/cairo-xcb-surface-render.c | 4 ++-- src/cairo-xcb-surface.c | 6 +++--- 6 files changed, 8 insertions(+), 30 deletions(-) diff --git a/src/cairo-xcb-connection-core.c b/src/cairo-xcb-connection-core.c index df6e2febd..71d2e515b 100644 --- a/src/cairo-xcb-connection-core.c +++ b/src/cairo-xcb-connection-core.c @@ -52,13 +52,6 @@ _cairo_xcb_connection_create_pixmap (cairo_xcb_connection_t *connection, return pixmap; } -void -_cairo_xcb_connection_free_pixmap (cairo_xcb_connection_t *connection, - xcb_pixmap_t pixmap) -{ - xcb_free_pixmap (connection->xcb_connection, pixmap); -} - xcb_gcontext_t _cairo_xcb_connection_create_gc (cairo_xcb_connection_t *connection, xcb_drawable_t drawable, @@ -71,13 +64,6 @@ _cairo_xcb_connection_create_gc (cairo_xcb_connection_t *connection, return gc; } -void -_cairo_xcb_connection_free_gc (cairo_xcb_connection_t *connection, - xcb_gcontext_t gc) -{ - xcb_free_gc (connection->xcb_connection, gc); -} - void _cairo_xcb_connection_change_gc (cairo_xcb_connection_t *connection, xcb_gcontext_t gc, diff --git a/src/cairo-xcb-private.h b/src/cairo-xcb-private.h index 46b6e515d..009f0820a 100644 --- a/src/cairo-xcb-private.h +++ b/src/cairo-xcb-private.h @@ -472,20 +472,12 @@ _cairo_xcb_connection_create_pixmap (cairo_xcb_connection_t *connection, uint16_t width, uint16_t height); -cairo_private void -_cairo_xcb_connection_free_pixmap (cairo_xcb_connection_t *connection, - xcb_pixmap_t pixmap); - cairo_private xcb_gcontext_t _cairo_xcb_connection_create_gc (cairo_xcb_connection_t *connection, xcb_drawable_t drawable, uint32_t value_mask, uint32_t *values); -cairo_private void -_cairo_xcb_connection_free_gc (cairo_xcb_connection_t *connection, - xcb_gcontext_t gc); - cairo_private void _cairo_xcb_connection_change_gc (cairo_xcb_connection_t *connection, xcb_gcontext_t gc, diff --git a/src/cairo-xcb-screen.c b/src/cairo-xcb-screen.c index 0d23ad3c3..f443eec29 100644 --- a/src/cairo-xcb-screen.c +++ b/src/cairo-xcb-screen.c @@ -169,7 +169,7 @@ _cairo_xcb_screen_finish (cairo_xcb_screen_t *screen) for (i = 0; i < ARRAY_LENGTH (screen->gc); i++) { if (screen->gc_depths[i] != 0) - _cairo_xcb_connection_free_gc (screen->connection, screen->gc[i]); + xcb_free_gc (screen->connection->xcb_connection, screen->gc[i]); } _cairo_cache_fini (&screen->linear_pattern_cache); @@ -350,7 +350,7 @@ _cairo_xcb_screen_put_gc (cairo_xcb_screen_t *screen, int depth, xcb_gcontext_t if (i == ARRAY_LENGTH (screen->gc)) { /* perform random substitution to ensure fair caching over depths */ i = rand () % ARRAY_LENGTH (screen->gc); - _cairo_xcb_connection_free_gc (screen->connection, screen->gc[i]); + xcb_free_gc (screen->connection->xcb_connection, screen->gc[i]); } screen->gc[i] = gc; diff --git a/src/cairo-xcb-surface-core.c b/src/cairo-xcb-surface-core.c index 91c0ff995..f9f12f04b 100644 --- a/src/cairo-xcb-surface-core.c +++ b/src/cairo-xcb-surface-core.c @@ -66,7 +66,7 @@ _cairo_xcb_pixmap_finish (void *abstract_surface) if (unlikely (status)) return status; - _cairo_xcb_connection_free_pixmap (surface->connection, + xcb_free_pixmap (surface->connection->xcb_connection, surface->pixmap); _cairo_xcb_connection_release (surface->connection); } diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c index c485524ce..a1eaad9a5 100644 --- a/src/cairo-xcb-surface-render.c +++ b/src/cairo-xcb-surface-render.c @@ -382,7 +382,7 @@ _picture_from_image (cairo_xcb_surface_t *target, 0, 0); } - _cairo_xcb_connection_free_pixmap (target->connection, pixmap); + xcb_free_pixmap (target->connection->xcb_connection, pixmap); return picture; } @@ -640,7 +640,7 @@ _solid_picture (cairo_xcb_surface_t *target, _cairo_xcb_screen_put_gc (target->screen, 32, gc); } - _cairo_xcb_connection_free_pixmap (target->connection, pixmap); + xcb_free_pixmap (target->connection->xcb_connection, pixmap); } return picture; diff --git a/src/cairo-xcb-surface.c b/src/cairo-xcb-surface.c index 7f601bb71..de020d8a0 100644 --- a/src/cairo-xcb-surface.c +++ b/src/cairo-xcb-surface.c @@ -155,7 +155,7 @@ _cairo_xcb_surface_create_similar (void *abstract_other, } if (unlikely (surface->base.status)) - _cairo_xcb_connection_free_pixmap (connection, pixmap); + xcb_free_pixmap (connection->xcb_connection, pixmap); _cairo_xcb_connection_release (connection); @@ -220,7 +220,7 @@ _cairo_xcb_surface_finish (void *abstract_surface) } if (surface->owns_pixmap) - _cairo_xcb_connection_free_pixmap (surface->connection, surface->drawable); + xcb_free_pixmap (surface->connection->xcb_connection, surface->drawable); _cairo_xcb_connection_release (surface->connection); } @@ -423,7 +423,7 @@ _get_image (cairo_xcb_surface_t *surface, pixmap, 0, 0, width, height); - _cairo_xcb_connection_free_pixmap (connection, pixmap); + xcb_free_pixmap (connection->xcb_connection, pixmap); } if (unlikely (reply == NULL)) {