Created attachment 6395 [details] [review]
patch to fix mem leak problem

I get memory corruption with your patch, can you check this works for you?

Created attachment 6396 [details] [review]
a new patch

Created attachment 6397 [details]
PDF that shows the memory leak

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.

BTW, the code path in the leak scenario is:

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

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

2. 
      if (!(fontFile = fontEngine->loadTrueTypeFont(
			   id,
			   fontsrc,
			   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.

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).

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)
  src->unref();
#endif

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

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.

ok, fixed, thanks again for diggind thrugh the problem.