Bug 21706

Summary: zombie ft_font_face / ft_unscaled_font mutual referencing problems
Product: cairo Reporter: Karl Tomlinson <bugs.freedesktop>
Component: freetype font backendAssignee: David Turner <david>
Status: RESOLVED MOVED QA Contact: cairo-bugs mailing list <cairo-bugs>
Severity: normal    
Priority: medium CC: freedesktop
Version: 1.9.1   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: patch

Description Karl Tomlinson 2009-05-12 13:55:00 UTC
There can be more than one zombie font_face belonging to an unscaled_font, but
only the first is destroyed:

http://cgit.freedesktop.org/cairo/tree/src/cairo-ft-font.c#n544

This leaks the client's FT_Face (and associated font data) as release of the
FT_Face depends on release of the font_face.

(The reason why Firefox ends up with two different font_faces for one
unscaled_font is that load_flags for faces with artificial oblique have
FT_LOAD_NO_BITMAP set.  https://bugzilla.mozilla.org/show_bug.cgi?id=486974)

Also it's possible for _cairo_ft_font_face_create to pull out a zombie
font_face from the unscaled_font, which would crash _cairo_ft_font_face_scaled_font_create, as that expects non-null font_face->unscaled (if !font-face->pattern).
Comment 1 Karl Tomlinson 2009-05-12 13:56:01 UTC
Created attachment 25802 [details] [review]
patch

The unscaled_font only needs to keep one zombie font_face.
Comment 2 Behdad Esfahbod 2009-05-12 14:06:24 UTC
I can't fully review the patch before studying the caching logic there from scratch again...  If you're confident that this is fully completely correct, please go ahead and push it in :).

Thanks
Comment 3 Chris Wilson 2009-05-15 13:39:38 UTC
I extended test/ft-font-create-for-face and was able to reproduce the leak but not the potential crash. As the patch caused no other side effects, I've pushed it to master:

commit 0238fe2cafea2e1ed19bb222117bd73ee6898d4d
Author: Karl Tomlinson <karlt+@karlt.net>
Date:   Thu May 14 11:46:29 2009 +0100

    [ft] Resolve mutual referencing problems with zombie faces
    
I tried to find a test case to exercise:
_cairo_ft_font_face_create()
    if (font_face->unscaled == NULL) {
		/* Resurrect this "zombie" font_face (from
		 * _cairo_ft_font_face_destroy), switching its unscaled_font
		 * from owner to ownee. */
		font_face->unscaled = unscaled;
		_cairo_unscaled_font_reference (&unscaled->base);
		return &font_face->base;
    }
and
  if (unscaled->faces && unscaled->faces->unscaled == NULL) {
	/* This "zombie" font_face (from _cairo_ft_font_face_destroy)
	 * is no longer needed. */
	assert (unscaled->from_face && unscaled->faces->next == NULL);
	cairo_font_face_destroy (&unscaled->faces->base);
	unscaled->faces = NULL;
    }
to no avail. Also I tried the test.html in the firefox bug report but saw no evidence that it even tried to use cairo_ft_font_create_for_face() - which is probably just user error on my part.

If you can identify a simple test case, or even trace, to reproduce the crash that would be very useful. Thanks.
Comment 4 Karl Tomlinson 2009-05-16 05:25:20 UTC
(In reply to comment #3)
> I extended test/ft-font-create-for-face and was able to reproduce the leak
> but not the potential crash. As the patch caused no other side effects, I've
> pushed it to master:

Thanks very much, Chris.

> If you can identify a simple test case, or even trace, to reproduce the crash
> that would be very useful. Thanks.

I don't have a testcase for the crash.  I haven't seen such a crash, but it
looked theoretically possible.  (The test in the Mozilla bug is run under the
"crashtest" infrastructure, which also tests leaks.  Also the test uses a
downloaded font from another part of the Mozilla source tree and so the
Mozilla tree needs to be accessible through a web server to make the test use
cairo_ft_font_create_for_face.)

Producing the crash would require:

1) Create two font_faces that would share a single unscaled_font.

2) Release (only) one font_face.  (It becomes a zombie.)

3) Create a font_face exactly the same as the one just released.
   (It pulls out the zombie face.)

4) Create a scaled_font from the font_face in 3. (crash)

> I tried to find a test case to exercise:
> _cairo_ft_font_face_create()
>     if (font_face->unscaled == NULL) {
>                 /* Resurrect this "zombie" font_face (from
>                  * _cairo_ft_font_face_destroy), switching its unscaled_font
>                  * from owner to ownee. */
>                 font_face->unscaled = unscaled;
>                 _cairo_unscaled_font_reference (&unscaled->base);
>                 return &font_face->base;
>     }
> and
>   if (unscaled->faces && unscaled->faces->unscaled == NULL) {
>         /* This "zombie" font_face (from _cairo_ft_font_face_destroy)
>          * is no longer needed. */
>         assert (unscaled->from_face && unscaled->faces->next == NULL);
>         cairo_font_face_destroy (&unscaled->faces->base);
>         unscaled->faces = NULL;
>     }
> to no avail.

I'm not sure that cairo could currently execute this.  It may be possible
through the unscaled_font reference that _cairo_type1_font_subset_init takes,
but I don't know that code well and whether that reference can remain alive
beyond the reference to the font_face (long enough to create another font_face
using the same unscaled_font).

The change in _cairo_ft_font_face_destroy means that it will now only make a
zombie font_face on the unscaled_font when there are no other font_faces on
the unscaled_font.  And scaled_fonts seem to release their unscaled_fonts
soon after the reference to the font_face.  So something else needs to keep a
reference to the unscaled_font to execute this code.
Comment 5 GitLab Migration User 2018-08-25 13:58:11 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/294.

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.