Bug 69470 - Race in _cairo_toy_font_face_destroy
Summary: Race in _cairo_toy_font_face_destroy
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: 1.12.14
Hardware: Other All
: medium normal
Assignee: Chris Wilson
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-17 14:13 UTC by Weeble
Modified: 2013-09-17 15:53 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Weeble 2013-09-17 14:13:12 UTC
When creating and destroying toy font faces concurrently on multiple threads, sometimes an assertion is raised:

/build/buildd/cairo-1.12.14/src/cairo-hash.c:506: _cairo_hash_table_lookup_exact_key: Assertion `!"reached"' failed.

I first observed this in Mono:

https://bugzilla.xamarin.com/show_bug.cgi?id=2426


I asked about it on the mailing list and someone confirmed that it occurs even in a trivial C program:

http://lists.cairographics.org/archives/cairo/2013-September/024667.html


I think this is the sequence of events:

Thread A runs cairo_toy_font_face_create to create a font. It completes normally and is added to cairo_toy_font_face_hash_table.

Thread A decrefs the font face. Its refcount drops to 0, and _cairo_toy_font_face_destroy will run, except--

Thread B runs cairo_toy_font_face_create to create the same font. It is found in the hash table, a reference is added, and it is returned.

Thread B decrefs the font face. Its refcount drops to 0.

Thread B runs _cairo_toy_font_face_destroy.

Thread B acquires the lock on the hash table, confirms that the refcount is still 0, then removes the hash table entry, releases the lock and goes on to deallocate the font.

Thead A wakes up and continues running _cairo_toy_font_face_destroy. It acquires the lock on the hash table, and - uh-oh! - it confirms that the refcount is still 0. It is! (I think this was potentially an access-after-free, too.) Thread A tries to remove the font from the hash table, and asserts.
Comment 1 Chris Wilson 2013-09-17 15:53:11 UTC
commit 337ab1f8d9e29086bfb4001508b28835b41c6390
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Sep 17 16:28:19 2013 +0100

    font: Push the last reference dec into the backend->destroy() callback
    
    In order to close a race between locking the backend and resurrecting a
    font via the cache, we need to keep the font face alive until after we
    take the backend lock. Once we have that lock, we can drop our reference
    and test if that was the last. Otherwise we must abort the destroy().
    
    This fixes the double-free exposed by multithreaded applications trying
    to create and destroy the same font concurrently.
    
    Reported-by: Weeble <clockworksaint@gmail.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69470
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.