[cairo] Evince crash in FT_Get_PS_Font_Info

[Behdad Esfahbod]

On Tue, 2008-04-29 at 10:05 +0100, Chris Wilson wrote:
> On Mon, 2008-04-28 at 12:03 -0400, Behdad Esfahbod wrote:
> > On Mon, 2008-04-28 at 09:08 +0100, Chris Wilson wrote:
> > > On Sun, 2008-04-27 at 02:07 -0400, Behdad Esfahbod wrote:
> > > > Hi,
> > > > 
> > > > I came across this bug:
> > > > 
> > > >   https://bugs.launchpad.net/ubuntu/+source/evince/+bug/157797
> > > > 
> > > > Looks to me like an evince bug freeing FT_Face before cairo is done with
> > > > it, but thought I post here so people better at this can check it out.
> > > 
> > > Been there, done that, wrote the patch. ;-)
> > > http://cgit.freedesktop.org/poppler/poppler/commit/?id=42db4890e8295aaec5a1be12d1414fc0a9048550
> > 
> > Ah right.  I saw that going by.  Somehow I thought it was a mozilla
> > patch.  Doesn't make sense now that I think about it ;).
> > 
> > Two notes:
> > 
> >   - cairo doesn't use the face behind poppler's back.  It looks like
> > there should be a poppler lifecycle issue involved as well, and your
> > patch actually hiding the bug.  Just a thought.  And what about the
> > actual font file data, is that kept alive anyway?
> 
> AIUI, the cairo_font_face_t is kept alive via references to the
> scaled_font_t in the meta-surface and, potentially, the pdf-surface.

I see.

> Skimming through the poppler code paths, it looked like they create
> short-lived CairoFonts for immediate use, combined with a simple caching
> scheme to mitigate the number of
> FT_New_Face()/cairo_ft_font_face_create_for_ft_face() calls. (This will
> actually generate sub-par PDFs, as cairo will embed the same font
> multiple times - so a better patch would duplicate the caching scheme
> used within cairo for FcPattern font_faces.)

Stuart's patch may improve this if they pass in the same file/index pair
for the font.  Though now that gets me worried about the case that the
filename/index is the same but the actual file contents differ (create
temp file, create face for it, unlink, repeat).

> As for the original font file - it appears that poppler already uses a
> temporary file that only exists for the call to FT_New_Face() (it is
> immediately unlinked afterwards), so I presume the FT_Face either keeps
> the fd open, or sucks the entire font into memory for its lifetime.
> However, now you've got me worried!

That's definitely an interesting way to simplify memory management for
the font data and leave it to the OS to cleanup.  Otherwise they can
create a FT_Face out of in-memory data too.

> >   - Can update the documentation with the fact that the face should be
> > kept alive as long as the cairo_font_face_t is alive, and add sample
> > code using user_data for handling it.
> 
> The documentation for cairo_ft_font_face_create_for_ft_face() already
> mentions that the FT_Face object must be kept around until the
> cairo_font_face_t reference count drops to zero, and to use the
> user_data callback to determine when it safe to call FT_Done_Face. An
> example, will of course, be helpful.
> 
> Even worse though, it appears that not even
> test/ft-font-create-for-ft-face.c gets the lifetime management of the
> FT_Face correct (as the FT_Face leaks out of scope through a reference
> held by the context)!
> 
> I've added a bit of blurb to the documentation to provide an example of
> how to use cairo_font_face_set_user_data() in combination with an
> FT_Face and released the errant reference in the test code.

Thanks.

-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759