Created attachment 114946 [details] PDF with duplicate TrueType fonts Attached PDF has a duplicated font which causes a memory leak. Output of valgrind --tool=memcheck --leak-check=full --show-leak-kinds=definite pdftocairo -png pdf/CairoDoubleTrueType.pdf a : ==10368== 4,096 bytes in 1 blocks are definitely lost in loss record 129 of 137 ==10368== at 0x4C28C20: malloc (vg_replace_malloc.c:296) ==10368== by 0x50553B8: gmalloc(unsigned long, bool) (gmem.cc:110) ==10368== by 0x5055425: gmalloc (gmem.cc:120) ==10368== by 0x4F45C04: Stream::toUnsignedChars(int*, int, int) (Stream.h:147) ==10368== by 0x4FA5036: GfxFont::readEmbFontFile(XRef*, int*) (GfxFont.cc:908) ==10368== by 0x41A3F1: CairoFreeTypeFont::create(GfxFont*, XRef*, FT_LibraryRec_*, bool) (CairoFontEngine.cc:433) ==10368== by 0x41B5CE: CairoFontEngine::getFont(GfxFont*, PDFDoc*, bool, XRef*) (CairoFontEngine.cc:818) ==10368== by 0x40E2F3: CairoOutputDev::updateFont(GfxState*) (CairoOutputDev.cc:631) ==10368== by 0x4F99E20: Gfx::opShowText(Object*, int) (Gfx.cc:3803) ==10368== by 0x4F89F6B: Gfx::execOp(Object*, Object*, int) (Gfx.cc:904) ==10368== by 0x4F8989C: Gfx::go(bool) (Gfx.cc:763) ==10368== by 0x4F896D0: Gfx::display(Object*, bool) (Gfx.cc:729)
Created attachment 114947 [details] [review] Free font data when we find duplicate Attached patch frees the font data when _ft_new_face detects a duplicate.
Comment on attachment 114947 [details] [review] Free font data when we find duplicate Review of attachment 114947 [details] [review]: ----------------------------------------------------------------- The commit message is a bit confusing, this only happens when _ft_new_face is called with font_data + font_data_len (embedded fonts). I wonder if we should release that data in more cases, because _ft_done_face is assuming the data is mmaped. So, for example, when FT_New_Memory_Face fails, we should also free the data? Should we check in _ft_done_face if data->fd is -1 to do the unmap ro gfree? I'm confused, because _ft_done_face should fail for embedded fonts, or it's simply never called due to the cairo cache being always alive.
Ok, I'm seeing patch in bug #89951 now, it does what I suggested indeed. I think we should still free the bytes also when FT_New_Memory_Face fails. I think all these are pretty much the same thing, we are leaking the data of embedded fonts, so we could probably merge both bugs into a single one with a single patch.
*** Bug 89951 has been marked as a duplicate of this bug. ***
Created attachment 114973 [details] [review] Fix memory leak in CairoFreeTypeFont::create I was hoping it would be less confusing as separate patches. :) I've squashed the patches. This frees things in three places: - In _ft_done_face, free the embedded font data. - In _ft_new_face, free the embedded font data if it's found to be a duplicate. - In CairoFreeTypeFont::create, free the embedded font data and codeToGID array if font creation fails.
(In reply to Jason Crain from comment #5) > Created attachment 114973 [details] [review] [review] > Fix memory leak in CairoFreeTypeFont::create > > I was hoping it would be less confusing as separate patches. :) And confused me :-P > I've squashed the patches. This frees things in three places: > > - In _ft_done_face, free the embedded font data. > - In _ft_new_face, free the embedded font data if it's found to be a > duplicate. You should also free the bytes in _ft_new_face when FT_New_Memory_Face fails, no? > - In CairoFreeTypeFont::create, free the embedded font data and codeToGID > array if font creation fails. Good catch! This one is probably more unrelated and could be in a separate patch, but I'm fine with a single patch too.
(In reply to Carlos Garcia Campos from comment #6) > You should also free the bytes in _ft_new_face when FT_New_Memory_Face > fails, no? It should be fine because when FT_New_Memory_Face fails, _ft_new_face will return false, and CairoFreeTypeFont::create will free it in the err2 label.
(In reply to Jason Crain from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > You should also free the bytes in _ft_new_face when FT_New_Memory_Face > > fails, no? > > It should be fine because when FT_New_Memory_Face fails, _ft_new_face will > return false, and CairoFreeTypeFont::create will free it in the err2 label. Ah, I missed the gfree(font_data) in err2: label part. Pushed to git master now, thanks!
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.