Memory leak in CPngImage

A memory leak in a surprising place

We’ve recently been doing some work switching our resources in our programs from BMP (using CBitmap) to PNG (using CPngImage).

At some point, we got around to dog-fooding, which we do with our tools all the time, and we were surprised to see memory leaks (in the form of HGLOBAL handles) in our tools being reported by Memory Validator.

We took a look and found they were coming from CPngImage::LoadFromBuffer(). Here’s the code, with the leaking lines highlighted.

BOOL CPngImage::LoadFromBuffer(LPBYTE lpBuffer, UINT uiSize)
{
        ASSERT(lpBuffer != NULL);

        HGLOBAL hRes = ::GlobalAlloc(GMEM_MOVEABLE, uiSize); // this line leaks
        if (hRes == NULL)
        {
                return FALSE;
        }

        IStream* pStream = NULL;
        LPVOID lpResBuffer = ::GlobalLock(hRes);             // this line leaks
        ASSERT (lpResBuffer != NULL);

        memcpy(lpResBuffer, lpBuffer, uiSize);

        HRESULT hResult = ::CreateStreamOnHGlobal(hRes, FALSE, &pStream);

        if (hResult != S_OK)
        {
                return FALSE;
        }

        if (CMFCToolBarImages::m_bMultiThreaded)
        {
                CMFCToolBarImages::m_CriticalSection.Lock();
        }

        if (m_pImage == NULL)
        {
                m_pImage = new CImage;
                ENSURE(m_pImage != NULL);
        }

        m_pImage->Load(pStream);
        pStream->Release();

        BOOL bRes = Attach(m_pImage->Detach());

        if (CMFCToolBarImages::m_bMultiThreaded)
        {
                CMFCToolBarImages::m_CriticalSection.Unlock();
        }

        return bRes;
}

Verifying the memory leak

At first we thought C++ Memory Validator might have made a mistake, as it seemed unlikely that such a well used class would contain a mistake like a memory leak.

To check if this was correct memory leak report or a FALSE positive we created a test program where we can repeatedly create and destroy images in rapid succession. If the leak is real the toy program will soon fail to allocate memory. If the leak is a false positive by C++ Memory Validator, the toy program will run forever with no problems. The toy program demonstrated the leak is real – after just over 65,000 loads of an image with CPngImage, all GlobalAlloc() allocations fail.

The test program allows you to repeatedly load a BMP, a PNG into CPngImage and a PNG into an svlPngImage. The image is destroyed after loading. You can specify how many times to perform the load, 1, 10, 100, 1000, 10000, 64K and 100,000 times.

We have verified that his memory leak was present in CPngImage that ships with Visual Studio.

Fixing the memory leak

The fix is to add two lines just before the return bRes; statement.

        ::GlobalUnlock(hRes);
        ::GlobalFree(hRes);

Unfortunately, CPngImage loads it data from methods that are not virtual, so we couldn’t replace them in the implementation. We created a drop-in replacement for CPngImage called svlPngImage. It’s identical to the CPngImage class with the exception that the CMFCToolBarImages calls are commented out, and the two additional lines to prevent the memory leak are added. If you’d like to use svlPngImage the source code for this drop-in replacement is in the download.

Test Program Source Code

You can download the test program source code and project files.

Update, after response from Microsoft

I was surprised when I got a reply saying this couldn’t happen because the second parameter to ::CreateStreamOnHGlobal() was TRUE. I searched my machine again, and it was FALSE. I then searched a different machine and it was TRUE. So most likely this bug did exist (and we’re using the version of Visual Studio that has it) but has been fixed in a more recent service pack. My claim that the bug was in all versions of Visual Studio was incorrect because I was checking for the absence of the memory freeing calls without checking the value passed to ::CreateStreamOnHGlobal() was FALSE. The drop-in replacement class is still valid.

If you’re using CPngImage, check CPngImage::LoadFromBuffer() and if the second parameter passed to ::CreateStreamOnHGlobal() is FALSE, use our svlPngImage class instead. Otherwise carry on as you are.

Update 2, after further response from Microsoft

Microsoft has modified this function to improve the error handling to fix a memory leak if ::CreateStreamOnHGlobal() fails, in response to this bug report, which prompted them to reexamine the function. Ironic that the report I made, although demonstrating a real bug, that bug had already been fixed, but it still prompted a fresh look at the function behaviour.