[cairo] Toy font face race condition / heap corruption?

[Paul Messmer]

Oops, no.  There's additional subtlety here.  It seems at least necessary (but perhaps not sufficient - I need to think more) for the destroy/no-destroy decision to be made just once, in _cairo_toy_font_face_destroy, while the hash table is locked, based on the reference count being zero or not at the time of the removal attempt.  That boolean decision made at that point needs to be used for all of the destruction logic (even for the base font object in the cairo_font_face_destroy function).

If we make a simple change to _cairo_toy_font_face_destroy to check the reference count AFTER the hash table is locked, and if it's non-zero to skip removal, unlock the table, and return, we've solved one problem but there's still another race due to the additional, separate check of the reference count in cairo_font_face_destroy after font_face->backend->destroy() returns.  Consider:

As before, in Thread 1:

1. We call cairo_font_face_destory on Helvetica with reference count 1
2. Decrement reference count to zero
3. Call font_face->backend->destroy, which ends up in _cairo_toy_font_face_destroy

Switch to Thread 2:

4.  Try to use Helvetica in a new context.
5.  Lock table.   Find Helvetica in table with 0 reference count.  Increment to 1.  Unlock table.  Return font to use.

Switch to Thread 1:

6.  _cairo_toy_font_face_destroy locks the table
7.  It checks the reference count.  Finds it 1 now.  Oops, shouldn't remove from the table or destroy.  So unlock the table and return.
8.  Now we're back in cairo_font_face_destroy as font_face->backend->destroy has just returned

Switch to Thread 2:

9.  The other context finishes with Helvetica (it's really quick!).  It calls call cairo_font_face_destroy on Helvetica (with reference count 1) and can go most or all of the way through destroying it.

Switch to Thread 1:

10.  The reference count check immediately following font_face->backend->destroy could be on an already deleted object now, or one that's also just about to be deleted by Thread 2 as well, resuling in double frees, etc...


--Paul

From: paulmessmer at hotmail.com
To: chris at chris-wilson.co.uk
Date: Mon, 26 Jan 2009 18:26:11 +0000
CC: cairo at cairographics.org
Subject: Re: [cairo] Toy font face race condition / heap corruption?









> From: chris at chris-wilson.co.uk
> 
> As I read it, the check on the reference count needs to be performed
> before we remove the entry from the hash table - but otherwise the
> analysis is spot on.

As long as the check on the reference count is performed AFTER
the hash table is locked and before the entry is removed, then it seems good (and this is almost certainly what you meant, but I wanted to clarify) and won't leave any orphaned objects like my suggestion.  In the existing code there's already a check (in the calling function) before the entry is removed and before the table is locked, yet there's a race because the reaching a reference count of 0 isn't atomic with entry removal.

Thanks,
-- Paul

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.cairographics.org/archives/cairo/attachments/20090126/f64c49d5/attachment.htm