[cairo-commit] 4 commits - src/cairo-mutex-impl-private.h src/cairo-mutex-type-private.h src/cairo-scaled-font.c src/cairo-scaled-font-private.h
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Sat May 28 23:13:35 UTC 2022
src/cairo-mutex-impl-private.h | 6
src/cairo-mutex-type-private.h | 7 +
src/cairo-scaled-font-private.h | 1
src/cairo-scaled-font.c | 241 ++++++----------------------------------
4 files changed, 55 insertions(+), 200 deletions(-)
New commits:
commit ccb306b8dd14e0f8462f6028dc2a90a211b72fea
Merge: 451dcd314 f823f4626
Author: Adrian Johnson <ajohnson at redneon.com>
Date: Sat May 28 23:13:33 2022 +0000
Merge branch 'scaled-font-deadlock' into 'master'
Fix deadlock in cairo-scaled-font.c
See merge request cairo/cairo!329
commit f823f46267894b6f79afd4482296e9f280cdeb84
Author: Adrian Johnson <ajohnson at redneon.com>
Date: Sat May 28 08:45:26 2022 +0930
Remove unused code
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index 372f85498..5fe81110f 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -2375,201 +2375,6 @@ _cairo_scaled_font_glyph_approximate_extents (cairo_scaled_font_t *scaled_font,
return TRUE;
}
-#if 0
-/* XXX win32 */
-cairo_status_t
-_cairo_scaled_font_show_glyphs (cairo_scaled_font_t *scaled_font,
- cairo_operator_t op,
- const cairo_pattern_t *pattern,
- cairo_surface_t *surface,
- int source_x,
- int source_y,
- int dest_x,
- int dest_y,
- unsigned int width,
- unsigned int height,
- cairo_glyph_t *glyphs,
- int num_glyphs,
- cairo_region_t *clip_region)
-{
- cairo_int_status_t status;
- cairo_surface_t *mask = NULL;
- cairo_format_t mask_format = CAIRO_FORMAT_A1; /* shut gcc up */
- cairo_surface_pattern_t mask_pattern;
- int i;
-
- /* These operators aren't interpreted the same way by the backends;
- * they are implemented in terms of other operators in cairo-gstate.c
- */
- assert (op != CAIRO_OPERATOR_SOURCE && op != CAIRO_OPERATOR_CLEAR);
-
- if (scaled_font->status)
- return scaled_font->status;
-
- if (!num_glyphs)
- return CAIRO_STATUS_SUCCESS;
-
- if (scaled_font->backend->show_glyphs != NULL) {
- int remaining_glyphs = num_glyphs;
- status = scaled_font->backend->show_glyphs (scaled_font,
- op, pattern,
- surface,
- source_x, source_y,
- dest_x, dest_y,
- width, height,
- glyphs, num_glyphs,
- clip_region,
- &remaining_glyphs);
- glyphs += num_glyphs - remaining_glyphs;
- num_glyphs = remaining_glyphs;
- if (remaining_glyphs == 0)
- status = CAIRO_INT_STATUS_SUCCESS;
- if (status != CAIRO_INT_STATUS_UNSUPPORTED)
- return _cairo_scaled_font_set_error (scaled_font, status);
- }
-
- /* Font display routine either does not exist or failed. */
-
- _cairo_scaled_font_freeze_cache (scaled_font);
-
- for (i = 0; i < num_glyphs; i++) {
- int x, y;
- cairo_image_surface_t *glyph_surface;
- cairo_scaled_glyph_t *scaled_glyph;
-
- status = _cairo_scaled_glyph_lookup (scaled_font,
- glyphs[i].index,
- CAIRO_SCALED_GLYPH_INFO_SURFACE,
- NULL, /* foreground color */
- &scaled_glyph);
-
- if (unlikely (status))
- goto CLEANUP_MASK;
-
- glyph_surface = scaled_glyph->surface;
-
- /* To start, create the mask using the format from the first
- * glyph. Later we'll deal with different formats. */
- if (mask == NULL) {
- mask_format = glyph_surface->format;
- mask = cairo_image_surface_create (mask_format, width, height);
- status = mask->status;
- if (unlikely (status))
- goto CLEANUP_MASK;
- }
-
- /* If we have glyphs of different formats, we "upgrade" the mask
- * to the wider of the formats. */
- if (glyph_surface->format != mask_format &&
- _cairo_format_bits_per_pixel (mask_format) <
- _cairo_format_bits_per_pixel (glyph_surface->format) )
- {
- cairo_surface_t *new_mask;
-
- switch (glyph_surface->format) {
- case CAIRO_FORMAT_ARGB32:
- case CAIRO_FORMAT_A8:
- case CAIRO_FORMAT_A1:
- mask_format = glyph_surface->format;
- break;
- case CAIRO_FORMAT_RGB16_565:
- case CAIRO_FORMAT_RGB24:
- case CAIRO_FORMAT_RGB30:
- case CAIRO_FORMAT_INVALID:
- default:
- ASSERT_NOT_REACHED;
- mask_format = CAIRO_FORMAT_ARGB32;
- break;
- }
-
- new_mask = cairo_image_surface_create (mask_format, width, height);
- status = new_mask->status;
- if (unlikely (status)) {
- cairo_surface_destroy (new_mask);
- goto CLEANUP_MASK;
- }
-
- _cairo_pattern_init_for_surface (&mask_pattern, mask);
- /* Note that we only upgrade masks, i.e. A1 -> A8 -> ARGB32, so there is
- * never any component alpha here.
- */
- status = _cairo_surface_composite (CAIRO_OPERATOR_ADD,
- &_cairo_pattern_white.base,
- &mask_pattern.base,
- new_mask,
- 0, 0,
- 0, 0,
- 0, 0,
- width, height,
- NULL);
-
- _cairo_pattern_fini (&mask_pattern.base);
-
- if (unlikely (status)) {
- cairo_surface_destroy (new_mask);
- goto CLEANUP_MASK;
- }
-
- cairo_surface_destroy (mask);
- mask = new_mask;
- }
-
- if (glyph_surface->width && glyph_surface->height) {
- cairo_surface_pattern_t glyph_pattern;
-
- /* round glyph locations to the nearest pixel */
- /* XXX: FRAGILE: We're ignoring device_transform scaling here. A bug? */
- x = _cairo_lround (glyphs[i].x -
- glyph_surface->base.device_transform.x0);
- y = _cairo_lround (glyphs[i].y -
- glyph_surface->base.device_transform.y0);
-
- _cairo_pattern_init_for_surface (&glyph_pattern,
- &glyph_surface->base);
- if (mask_format == CAIRO_FORMAT_ARGB32)
- glyph_pattern.base.has_component_alpha = TRUE;
-
- status = _cairo_surface_composite (CAIRO_OPERATOR_ADD,
- &_cairo_pattern_white.base,
- &glyph_pattern.base,
- mask,
- 0, 0,
- 0, 0,
- x - dest_x, y - dest_y,
- glyph_surface->width,
- glyph_surface->height,
- NULL);
-
- _cairo_pattern_fini (&glyph_pattern.base);
-
- if (unlikely (status))
- goto CLEANUP_MASK;
- }
- }
-
- _cairo_pattern_init_for_surface (&mask_pattern, mask);
- if (mask_format == CAIRO_FORMAT_ARGB32)
- mask_pattern.base.has_component_alpha = TRUE;
-
- status = _cairo_surface_composite (op, pattern, &mask_pattern.base,
- surface,
- source_x, source_y,
- 0, 0,
- dest_x, dest_y,
- width, height,
- clip_region);
-
- _cairo_pattern_fini (&mask_pattern.base);
-
-CLEANUP_MASK:
- _cairo_scaled_font_thaw_cache (scaled_font);
-
- if (mask != NULL)
- cairo_surface_destroy (mask);
- return _cairo_scaled_font_set_error (scaled_font, status);
-}
-#endif
-
/* Add a single-device-unit rectangle to a path. */
static cairo_status_t
_add_unit_rectangle_to_path (cairo_path_fixed_t *path,
commit 76e0df566595a77e271de523b3d06f86f3e85813
Author: Adrian Johnson <ajohnson at redneon.com>
Date: Sat May 28 07:14:52 2022 +0930
Fix deadlock in cairo-scaled-font.c
A user font glyph containing a font can cause deadlock in
_cairo_scaled_glyph_fini due to the destroy recording surface while
holding _cairo_scaled_glyph_page_cache_mutex. When the font in the
recording surface is removed from the page cache it will attempt to
also acquire the _cairo_scaled_glyph_page_cache_mutex resulting in
deadlock.
Instead of destroying the recording surface in
_cairo_scaled_glyph_page_cache_mutex, move it to an array in the
scaled font and destroy it after the
_cairo_scaled_glyph_page_cache_mutex is released.
Fixes the font in user font case in #440
diff --git a/src/cairo-scaled-font-private.h b/src/cairo-scaled-font-private.h
index 7a19f2539..6fd772bdb 100644
--- a/src/cairo-scaled-font-private.h
+++ b/src/cairo-scaled-font-private.h
@@ -113,6 +113,7 @@ struct _cairo_scaled_font {
cairo_list_t glyph_pages;
cairo_bool_t cache_frozen;
cairo_bool_t global_cache_frozen;
+ cairo_array_t recording_surfaces_to_free; /* array of cairo_surface_t* */
cairo_list_t dev_privates;
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index 4000a7082..372f85498 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -39,6 +39,7 @@
*/
#include "cairoint.h"
+#include "cairo-array-private.h"
#include "cairo-error-private.h"
#include "cairo-image-surface-private.h"
#include "cairo-list-inline.h"
@@ -215,8 +216,17 @@ _cairo_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
_cairo_path_fixed_destroy (scaled_glyph->path);
if (scaled_glyph->recording_surface != NULL) {
- cairo_surface_finish (scaled_glyph->recording_surface);
- cairo_surface_destroy (scaled_glyph->recording_surface);
+ cairo_status_t status;
+
+ /* If the recording surface contains other fonts, destroying
+ * it while holding _cairo_scaled_glyph_page_cache_mutex will
+ * result in deadlock when the recording surface font is
+ * destroyed. Instead, move the recording surface to a list of
+ * surfaces to free and free it in
+ * _cairo_scaled_font_thaw_cache() after
+ * _cairo_scaled_glyph_page_cache_mutex is unlocked. */
+ status = _cairo_array_append (&scaled_font->recording_surfaces_to_free, &scaled_glyph->recording_surface);
+ assert (status == CAIRO_STATUS_SUCCESS);
}
if (scaled_glyph->color_surface != NULL)
@@ -250,6 +260,7 @@ static const cairo_scaled_font_t _cairo_scaled_font_nil = {
{ NULL, NULL }, /* pages */
FALSE, /* cache_frozen */
FALSE, /* global_cache_frozen */
+ { 0, 0, sizeof(cairo_surface_t*), NULL }, /* recording_surfaces_to_free */
{ NULL, NULL }, /* privates */
NULL /* backend */
};
@@ -765,6 +776,7 @@ _cairo_scaled_font_init (cairo_scaled_font_t *scaled_font,
cairo_list_init (&scaled_font->glyph_pages);
scaled_font->cache_frozen = FALSE;
scaled_font->global_cache_frozen = FALSE;
+ _cairo_array_init (&scaled_font->recording_surfaces_to_free, sizeof (cairo_surface_t *));
scaled_font->holdover = FALSE;
scaled_font->finished = FALSE;
@@ -786,6 +798,22 @@ _cairo_scaled_font_init (cairo_scaled_font_t *scaled_font,
return CAIRO_STATUS_SUCCESS;
}
+static void _cairo_scaled_font_free_recording_surfaces (cairo_scaled_font_t *scaled_font)
+{
+ int num_recording_surfaces;
+ cairo_surface_t *surface;
+
+ num_recording_surfaces = _cairo_array_num_elements (&scaled_font->recording_surfaces_to_free);
+ if (num_recording_surfaces > 0) {
+ for (int i = 0; i < num_recording_surfaces; i++) {
+ _cairo_array_copy_element (&scaled_font->recording_surfaces_to_free, i, &surface);
+ cairo_surface_finish (surface);
+ cairo_surface_destroy (surface);
+ }
+ _cairo_array_truncate (&scaled_font->recording_surfaces_to_free, 0);
+ }
+}
+
void
_cairo_scaled_font_freeze_cache (cairo_scaled_font_t *scaled_font)
{
@@ -808,6 +836,8 @@ _cairo_scaled_font_thaw_cache (cairo_scaled_font_t *scaled_font)
scaled_font->global_cache_frozen = FALSE;
}
+ _cairo_scaled_font_free_recording_surfaces (scaled_font);
+
scaled_font->cache_frozen = FALSE;
CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
}
@@ -889,6 +919,9 @@ _cairo_scaled_font_fini_internal (cairo_scaled_font_t *scaled_font)
cairo_font_face_destroy (scaled_font->font_face);
cairo_font_face_destroy (scaled_font->original_font_face);
+ _cairo_scaled_font_free_recording_surfaces (scaled_font);
+ _cairo_array_fini (&scaled_font->recording_surfaces_to_free);
+
CAIRO_MUTEX_FINI (scaled_font->mutex);
while (! cairo_list_is_empty (&scaled_font->dev_privates)) {
commit a8c1858cf2bb6efb35c0678d135fd522ece9e2a4
Author: Adrian Johnson <ajohnson at redneon.com>
Date: Fri May 27 21:36:31 2022 +0930
Fix deadlock in cairo-scaled-font.c
When cairo_scaled_glyph_page_cache needs to remove entries,
cairo-cache calls _cairo_hash_table_random_entry() with the predicate
_cairo_scaled_glyph_page_can_remove(). This function checks that the
glyph_page scaled_font is not locked by testing
scaled_font->cache_frozen. The scaled font is locked in the
cache-cache destroy entry callback: _cairo_scaled_glyph_page_pluck().
There is a race condition here between testing
scaled_font->cache_frozen and locking the font. Fix this by adding a
new CAIRO_MUTEX_TRY_LOCK mutex operation, and using it to test and
lock the scaled font in _cairo_scaled_glyph_page_can_remove().
Fixes the multithreaded case in #440
diff --git a/src/cairo-mutex-impl-private.h b/src/cairo-mutex-impl-private.h
index 9f208aaa9..a8599d47e 100644
--- a/src/cairo-mutex-impl-private.h
+++ b/src/cairo-mutex-impl-private.h
@@ -87,6 +87,9 @@
* No trailing semicolons are needed (in any macro you define here).
* You should be able to compile the following snippet:
*
+ * - #define CAIRO_MUTEX_IMPL_TRY_LOCK(mutex) to try locking the mutex object,
+ * returning TRUE if the lock is acquired, FALSE if the mutex could not be locked.
+ *
* <programlisting>
* cairo_mutex_impl_t _cairo_some_mutex;
*
@@ -163,6 +166,7 @@
# define CAIRO_MUTEX_IMPL_NO 1
# define CAIRO_MUTEX_IMPL_INITIALIZE() CAIRO_MUTEX_IMPL_NOOP
# define CAIRO_MUTEX_IMPL_LOCK(mutex) CAIRO_MUTEX_IMPL_NOOP1(mutex)
+# define CAIRO_MUTEX_IMPL_TRY_LOCK(mutex) (CAIRO_MUTEX_IMPL_NOOP1(mutex), TRUE)
# define CAIRO_MUTEX_IMPL_UNLOCK(mutex) CAIRO_MUTEX_IMPL_NOOP1(mutex)
# define CAIRO_MUTEX_IMPL_NIL_INITIALIZER 0
@@ -190,6 +194,7 @@
# define CAIRO_MUTEX_IMPL_WIN32 1
# define CAIRO_MUTEX_IMPL_LOCK(mutex) EnterCriticalSection (&(mutex))
+# define CAIRO_MUTEX_IMPL_TRY_LOCK(mutex) TryEnterCriticalSection (&(mutex))
# define CAIRO_MUTEX_IMPL_UNLOCK(mutex) LeaveCriticalSection (&(mutex))
# define CAIRO_MUTEX_IMPL_INIT(mutex) InitializeCriticalSection (&(mutex))
# define CAIRO_MUTEX_IMPL_FINI(mutex) DeleteCriticalSection (&(mutex))
@@ -208,6 +213,7 @@
# define CAIRO_MUTEX_IMPL_INIT(mutex) pthread_mutex_init (&(mutex), NULL)
#endif
# define CAIRO_MUTEX_IMPL_LOCK(mutex) pthread_mutex_lock (&(mutex))
+# define CAIRO_MUTEX_IMPL_TRY_LOCK(mutex) (pthread_mutex_trylock (&(mutex)) == 0)
# define CAIRO_MUTEX_IMPL_UNLOCK(mutex) pthread_mutex_unlock (&(mutex))
#if HAVE_LOCKDEP
# define CAIRO_MUTEX_IS_LOCKED(mutex) LOCKDEP_IS_LOCKED (&(mutex))
diff --git a/src/cairo-mutex-type-private.h b/src/cairo-mutex-type-private.h
index e8c493985..ef8fc5e46 100644
--- a/src/cairo-mutex-type-private.h
+++ b/src/cairo-mutex-type-private.h
@@ -48,6 +48,9 @@
#ifndef CAIRO_MUTEX_IMPL_LOCK
# error "CAIRO_MUTEX_IMPL_LOCK not defined. Check cairo-mutex-impl-private.h."
#endif
+#ifndef CAIRO_MUTEX_IMPL_TRY_LOCK
+# error "CAIRO_MUTEX_IMPL_TRY_LOCK not defined. Check cairo-mutex-impl-private.h."
+#endif
#ifndef CAIRO_MUTEX_IMPL_UNLOCK
# error "CAIRO_MUTEX_IMPL_UNLOCK not defined. Check cairo-mutex-impl-private.h."
#endif
@@ -138,6 +141,9 @@
#ifndef CAIRO_MUTEX_IMPL_LOCK
# error "CAIRO_MUTEX_IMPL_LOCK not defined"
#endif
+#ifndef CAIRO_MUTEX_IMPL_TRY_LOCK
+# error "CAIRO_MUTEX_IMPL_TRY_LOCK not defined"
+#endif
#ifndef CAIRO_MUTEX_IMPL_UNLOCK
# error "CAIRO_MUTEX_IMPL_UNLOCK not defined"
#endif
@@ -167,6 +173,7 @@ typedef cairo_recursive_mutex_impl_t cairo_recursive_mutex_t;
#define CAIRO_MUTEX_INITIALIZE CAIRO_MUTEX_IMPL_INITIALIZE
#define CAIRO_MUTEX_FINALIZE CAIRO_MUTEX_IMPL_FINALIZE
#define CAIRO_MUTEX_LOCK CAIRO_MUTEX_IMPL_LOCK
+#define CAIRO_MUTEX_TRY_LOCK CAIRO_MUTEX_IMPL_TRY_LOCK
#define CAIRO_MUTEX_UNLOCK CAIRO_MUTEX_IMPL_UNLOCK
#define CAIRO_MUTEX_INIT CAIRO_MUTEX_IMPL_INIT
#define CAIRO_MUTEX_FINI CAIRO_MUTEX_IMPL_FINI
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index 203b6a10c..4000a7082 100755
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -476,7 +476,7 @@ _cairo_scaled_glyph_page_pluck (void *closure)
scaled_font = page->scaled_font;
- CAIRO_MUTEX_LOCK (scaled_font->mutex);
+ /* The font is locked in _cairo_scaled_glyph_page_can_remove () */
_cairo_scaled_glyph_page_destroy (scaled_font, page);
CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
}
@@ -2855,14 +2855,17 @@ _cairo_scaled_glyph_set_color_surface (cairo_scaled_glyph_t *scaled_glyph,
scaled_glyph->has_info &= ~CAIRO_SCALED_GLYPH_INFO_COLOR_SURFACE;
}
+/* _cairo_hash_table_random_entry () predicate. To avoid race conditions,
+ * the font is locked when tested. The font is unlocked in
+ * _cairo_scaled_glyph_page_pluck. */
static cairo_bool_t
_cairo_scaled_glyph_page_can_remove (const void *closure)
{
const cairo_scaled_glyph_page_t *page = closure;
- const cairo_scaled_font_t *scaled_font;
+ cairo_scaled_font_t *scaled_font;
scaled_font = page->scaled_font;
- return scaled_font->cache_frozen == 0;
+ return CAIRO_MUTEX_TRY_LOCK (scaled_font->mutex);
}
static cairo_status_t
More information about the cairo-commit
mailing list