Memory Leak in MFC CMap::SetAt/CList::AddTail. plex.cpp(29)

Question

I was using VS2008 SP1.

It's a long time since I last time wrote memory leak code :), until I met this one. it is reported from: \atlmfc\src\mfc\plex.cpp(29) :

the MFC "CPlex::Create" is invoked when the CMap::SetAt is called while "pAssoc = NewAssoc()" is invoked to assign memory.

I understand that, release a container by just calling "RemoveAll()" is not enough, need to iterate each entry and delete each of them.

But in this case, all I wanted is to used CMap to store the pair, I don't want the CMap to delete the value pointer it stored. (The other container handles that and I did released the pointers when releasing those containers.)

Note: this memory leak also happened in "CMapStringToPtr"(reported by other developer and googled them, not tested to confirm)

01    typedef CMap <int, int, CNode*, CNode*&> CNodeIndexMap;
02    CNodeIndexMap m_mapIndexToNode;
03    CNode* pNode = ... //This pNode is from another container, which is responsible for the nodes' clean
04    m_mapIndexToNode.SetAt(nIndex, pNode);
      ....
05    m_mapIndexToNode.RemoveAll();

      //Clean node list
06    for(int i = 0; i < lstNode.GetCount(); i++)
07    {
08        CNode* pNode = lstNode.GetAt(i);
09        delete pNode;
10    }
11    lstNode.RemoveAll();

I guess this is a microsoft bug. If that is really the case, please confirm and what is the data structure I can use to create an "index table" and how to use it properly? (use stl::map?)

Any idea or comment on this memory leak?

Answer 1

On 04/03/2012 23:19, milesma80 wrote:

I was using VS2008 SP1.

It's a long time since I last time wrote memory leak code :), until I met this one. it is reported from: \atlmfc\src\mfc\plex.cpp(29) :

the MFC "CPlex::Create" is invoked when the CMap::SetAt is called while "pAssoc = NewAssoc()" is invoked to assign memory.

I understand that, release a container by just calling "RemoveAll()" is not enough, need to iterate each entry and delete each of them.

But in this case, all I wanted is to used CMap to store the pair, I don't want the CMap to delete the value pointer it stored. (The other container handles that and I did released the pointers when releasing those containers.)

Note: this memory leak also happened in "CMapStringToPtr"(reported by other developer and googled them, not tested to confirm)

Did the leak appear in the output window?
Please post some compilable code to reproduce this leak.
 > what is the data structure I can use to create an "index table" and how to use it properly? (use stl::map?)

There is no stl::map, but there is std::map (which is part of STL).

If you want to have a map from integer to some non-owning pointer to some class X, you can simply use:

#include <map>

std::map<int, X *> myMap;

You can use operator[] to set and get the values (pointers) associated to the key.

If you want pointer ownership, you can use shared_ptr wit std::map.

map<int, shared_ptr<X>> m;

You can find std::map tutorials on the Internet, and there is an interesting video lesson on Channel 9 (by the VC++ STL developer/maintainer himself):

http://channel9.msdn.com/Shows/Going+Deep/C9-Lectures-Stephan-T-Lavavej-Standard-Template-Library-STL-2-of-n

Giovanni

Answer 2

Thanks Giovanni.

It might be a memory leak introduced by my codes.

Because I tried following code and NO memory leak at all.

I will update thread again after I have more investigation.

#include "stdafx.h"
#include "CMapLeak.h"
#include <iostream>

#ifdef _DEBUG
#define new DEBUG_NEW
#endif
CWinApp theApp;
typedef CMap <int, int, CPoint*, CPoint*&> CPointMap;
using namespace std;

int _tmain(int argc, TCHAR* argv[], TCHAR* envp[])
{
	int nRetCode = 0;
	if (!AfxWinInit(::GetModuleHandle(NULL), NULL, ::GetCommandLine(), 0))
	{
		_tprintf(_T("Fatal Error: MFC initialization failed\n"));
		nRetCode = 1;
	}
	else
	{
		//1. Initialize a list of pointers
		CList<CPoint*> lstPoint;
		lstPoint.AddTail(new CPoint(0, 0));
		lstPoint.AddTail(new CPoint(1, 1));
		lstPoint.AddTail(new CPoint(2, 2));
		//2. the map (int to *) will temperately use the instances
		CMap <int, int, CPoint*, CPoint*&> mapIndex;
		int nIndex = 0;
		POSITION pos1 = lstPoint.GetHeadPosition();	
		while(pos1)
		{
			CPoint* pPoint = lstPoint.GetNext(pos1);
			if(pPoint)
			{
				mapIndex.SetAt(nIndex, pPoint);
				nIndex++;
			}
		}
		_tprintf(_T("There are %d entries in the map\n"), mapIndex.GetCount());
		//3. RemoveAll (Not make sense to iterate the map and call delete)
		mapIndex.RemoveAll();

		//4. Clean the list
		POSITION pos2 = lstPoint.GetHeadPosition();	
		while(pos2)
		{
			CPoint* pPoint = lstPoint.GetNext(pos2);
			if(pPoint)
			{
				delete pPoint;
				pPoint = NULL;
			}
		}
		lstPoint.RemoveAll();
	}
	std::cin.get();
	return nRetCode;
}

Answer 3

On 06/03/2012 22:04, milesma80 wrote:

Thanks Giovanni.

You're welcome.

It might be a memory leak introduced by my codes.

Yes.

Note that if you (properly) use STL containers with smart pointers instead of raw pointers and manual memory management, you will have to work really hard to introduce leaks :)

Thanks to C++ destructors, well-designed container classes and smart pointers it is possible to write C++ code that is structurally incapable of leaking.

Giovanni

Answer 4

Thanks Giovanni.

In short, my code has memory leak like: (I used CList as example)

CList<T>* pList = new CList<T>();

pList.AddTail(...);

if I forgot to "delete pList;", Of course I will have memory leak. reported as plex.cpp(29)

The point is: I didn't add the macro:

#ifdef _DEBUG
#undef THIS_FILE
static char THIS_FILE[]=__FILE__;
#define new DEBUG_NEW
#endif

While, if I add them to the .cpp files, these memory leaks will be reported with correct file name and line number.

As I did not add this macro, the first .CPP file that has "#define new DEBUG_NEW" defined are responsible to report the memory leak.

That's why plex.cpp prompt in the debug messages.

Answer 5

On 19/03/2012 06:17, milesma80 wrote:

In short, my code has memory leak like: (I used CList as example)

CList<T>* pList = new CList<T>();

pList.AddTail(...);

Maybe you had a typo here; I think this should be:

pList->AddTail(...);
 > if I forgot to "delete pList;", Of course I will have memory leak.

Yes.

May I ask why you allocated the CList instance on the heap?

CList<T> list;

is simpler and automatically calls CList<T>'s destructor.

If you allocated the CList instance on the heap because you are sharing it between different owners, consider using shared_ptr<> smart pointer to wrap this shared CList resource.
shared_ptr is reference counted, and as soon as the ref count reaches 0, the (no more shared) CList instance is automatically deleted.

Giovanni

Answer 6

Thanks again.

1. it is pList->AddTail(...);

2. I will keep in mind to allocate the list on stack by CList<T> list;

    As to the reason, I was just lazy. Another function takes an CList<T>* as parameter, so I declared one and passed in.

    I should pass the argument as "&list".

It's really a good lesson, thanks Giovanni.