| Summary: | Memory Leak in CairoFreeTypeFont::create | ||
|---|---|---|---|
| Product: | poppler | Reporter: | Jason Crain <jason> |
| Component: | cairo backend | Assignee: | poppler-bugs <poppler-bugs> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | minor | ||
| Priority: | medium | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
PDF with duplicate TrueType fonts
Free font data when we find duplicate Fix memory leak in CairoFreeTypeFont::create |
||
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.
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)