Bug 7687 - Memory leak when loading embedded font files
Summary: Memory leak when loading embedded font files
Alias: None
Product: poppler
Classification: Unclassified
Component: splash backend (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Windows (All)
: high major
Assignee: Albert Astals Cid
QA Contact:
Depends on:
Reported: 2006-07-29 14:40 UTC by Krzysztof Kowalczyk
Modified: 2006-07-30 02:45 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

patch to fix mem leak problem (899 bytes, patch)
2006-07-29 14:41 UTC, Krzysztof Kowalczyk
Details | Splinter Review
a new patch (1.02 KB, patch)
2006-07-29 15:24 UTC, Albert Astals Cid
Details | Splinter Review
PDF that shows the memory leak (176.23 KB, application/octet-stream)
2006-07-29 15:50 UTC, Krzysztof Kowalczyk

Description Krzysztof Kowalczyk 2006-07-29 14:40:35 UTC
There is a memory leak in SplashOutputDev::updateFont() when loading embedded
font. fontsrc is not correctly unrefed and the tmpBuf from readEmbFontFile() is
not freed.

The leak is in tens of kilobytes per font (obviously depends on the size of
embedded font file).

See attached diff for a fix.
Comment 1 Krzysztof Kowalczyk 2006-07-29 14:41:14 UTC
Created attachment 6395 [details] [review]
patch to fix mem leak problem
Comment 2 Albert Astals Cid 2006-07-29 15:23:52 UTC
I get memory corruption with your patch, can you check this works for you?
Comment 3 Albert Astals Cid 2006-07-29 15:24:41 UTC
Created attachment 6396 [details] [review]
a new patch
Comment 4 Krzysztof Kowalczyk 2006-07-29 15:50:03 UTC
Created attachment 6397 [details]
PDF that shows the memory leak
Comment 5 Krzysztof Kowalczyk 2006-07-29 15:55:10 UTC
The new patch also has a memory leak. The problem is that in the code path that
uses embedded font, the fontsrc has one reference too many and is never deleted.
Both the object and the buffer (font data) are never freed.

Adding ref() essentially undoes the unref() that I've added.

It's possible that in the other codepath, which uses filename, there would be
memory corruption. Can you attach the PDF file that gives you corruption with my
patch? Also, how do you test for memory corruption?

I've attached the PDF file that shows the memory leak.
Comment 6 Krzysztof Kowalczyk 2006-07-29 16:04:20 UTC
BTW, the code path in the leak scenario is:

    fontsrc = new SplashFontSrc;
    if (fileName)
      fontsrc->setFile(fileName, gFalse);
      fontsrc->setBuf(tmpBuf, tmpBufLen, gTrue);

taking the !fileName branch. At this point fontsrc has refcnt 1

      if (!(fontFile = fontEngine->loadTrueTypeFont(
			   codeToGID, n))) {
	error(-1, "Couldn't create a font for '%s'",
	      gfxFont->getName() ? gfxFont->getName()->getCString()
	                         : "(unnamed)");

fontFile takes ownership of fontsrc and increases refcount to 2. When fontFile
is deleted, it'll decrease refcount to 1. No other place decreses the refcount,
hence fontsrc stays around forever.

This is a behaviour currently in poppler. 

My patch decreases the refcount at the end of
SplashOutputDev::updateFont(GfxState *state). It also makes fontsrc the owner of
the buffer, so that it gets deleted along with the fontsrc object, since no
other place seems to be deleting this buffer.
Comment 7 Krzysztof Kowalczyk 2006-07-29 19:36:52 UTC
One more thing: if there is a corruption due to refcount being too small, an
assert(refcnt >= 0) in the destructor would be helpful in catching this early e.g.:
#include <assert.h>
void SplashFontSrc::unref() {
  assert(refcnt >= 0);
  if (! --refcnt)
    delete this;

(there are more places that use refcounting and would benefit from such check).
Comment 8 Krzysztof Kowalczyk 2006-07-29 23:07:32 UTC
The mystery solved. I was doing my tests on Windows and was not seeing the
memory corruption. Now I figured out why it was corrupting memory on Unix.

In SplashFontEngine::loadTrueTypeFont() there is unix-only code that does
another fontsrc unref():

#ifndef WIN32
  // delete the (temporary) font file -- with Unix hard link
  // semantics, this will remove the last link; otherwise it will
  // return an error, leaving the file to be deleted later (if
  // loadXYZFont failed, the file will always be deleted)

I think that the full fix would be to apply my original patch plus this change
to the above code:
  if (src->isFile)

since the comment implies that it's only to force deleting files. I think it
correctly covers all possible code-paths (file and memory).

I've tested that with this change it works on Unix.
Comment 9 Albert Astals Cid 2006-07-30 02:45:24 UTC
ok, fixed, thanks again for diggind thrugh the problem.

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.