Hang when recursing in _cairo_scaled_font_reset_cache()

Submitted by Hans Petter Jansson on July 21, 2016, 11:48 a.m.

Details

Message ID 1469101727.1515.65.camel@copyleft.no
State New
Headers show
Series "Hang when recursing in _cairo_scaled_font_reset_cache()" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Hans Petter Jansson July 21, 2016, 11:48 a.m.
Hi! I submitted a patch in bug 93891 [1] some time ago, and I wonder if
someone on this list had a few spare moments to review it.

The patch fixes a potential hang affecting client applications (observed
with Evince).

I'm attaching the same patch here for convenience.

[1] https://bugs.freedesktop.org/show_bug.cgi?id=93891

Patch hide | download patch | download mbox

From 8a6de4f53fb0cf8b8af654c18eadc31ed3a9525e Mon Sep 17 00:00:00 2001
From: Hans Petter Jansson <hpj@cl.no>
Date: Wed, 27 Jan 2016 12:55:01 -0600
Subject: [PATCH] scaled-font: Fix deadlock when recursing in
 _cairo_scaled_font_reset_cache()

The destruction of a scaled font could indirectly trigger the destruction
of a second scaled font, causing the global cache to be locked twice in
the same thread.

This is solved by unlinking the font's glyph pages while holding the global
lock, then releasing the lock before destruction takes place.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=93891
---
 src/cairo-scaled-font.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index ac80c97..a22b36e 100644
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -818,23 +818,35 @@  _cairo_scaled_font_thaw_cache (cairo_scaled_font_t *scaled_font)
 void
 _cairo_scaled_font_reset_cache (cairo_scaled_font_t *scaled_font)
 {
+    cairo_scaled_glyph_page_t *page;
+
     CAIRO_MUTEX_LOCK (scaled_font->mutex);
     assert (! scaled_font->cache_frozen);
     assert (! scaled_font->global_cache_frozen);
     CAIRO_MUTEX_LOCK (_cairo_scaled_glyph_page_cache_mutex);
-    while (! cairo_list_is_empty (&scaled_font->glyph_pages)) {
-	cairo_scaled_glyph_page_t *page =
-	    cairo_list_first_entry (&scaled_font->glyph_pages,
-				    cairo_scaled_glyph_page_t,
-				    link);
 
+    cairo_list_foreach_entry (page,
+			      cairo_scaled_glyph_page_t,
+			      &scaled_font->glyph_pages,
+			      link) {
 	cairo_scaled_glyph_page_cache.size -= page->cache_entry.size;
 	_cairo_hash_table_remove (cairo_scaled_glyph_page_cache.hash_table,
 				  (cairo_hash_entry_t *) &page->cache_entry);
+    }
 
+    CAIRO_MUTEX_UNLOCK (_cairo_scaled_glyph_page_cache_mutex);
+
+    /* Destroy scaled_font's pages while holding its lock only, and not the
+     * global page cache lock. The destructor can cause us to recurse and
+     * end up back here for a different scaled_font. */
+
+    while (! cairo_list_is_empty (&scaled_font->glyph_pages)) {
+	page = cairo_list_first_entry (&scaled_font->glyph_pages,
+				       cairo_scaled_glyph_page_t,
+				       link);
 	_cairo_scaled_glyph_page_destroy (scaled_font, page);
     }
-    CAIRO_MUTEX_UNLOCK (_cairo_scaled_glyph_page_cache_mutex);
+
     CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
 }
 
-- 
1.8.4.5