From a8c1858cf2bb6efb35c0678d135fd522ece9e2a4 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 27 May 2022 21:36:31 +0930 Subject: [PATCH] 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 --- src/cairo-mutex-impl-private.h | 6 ++++++ src/cairo-mutex-type-private.h | 7 +++++++ src/cairo-scaled-font.c | 9 ++++++--- 3 files changed, 19 insertions(+), 3 deletions(-) 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. + * * * 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