Memory leaks (windows) in dcmtk-3.6.1_20110225.tar.gz

[vsalomoni]

Dear forum readers,
I have tried the newest snapshot (dcmtk-3.6.1_20110225.tar.gz) and I think I have found that in the windows versions (I am using Visual Studio 2008 32bit) there are some memory leaks when linking my application to dcmdata.lib, ofstd.lib, oflog.lib.
I have not had the same problem with version 3.6.0.

I think that the memory not released when the application is closed is allocated in the oflog/libsrc/globinit.cc line 90 during the call to the function log4cplus::initializeLog4cplus();

Looking to the modifications between 3.6.0 and 3.6.1_20110225 I have found that you removed the virtual keyword from the destructor of OFListLink, OFListBase, OFListLinkBase in ofstd/include/dcmtk/ofstd/oflist.h
If I restore the virtual keyword in the destructors of the list classes I cannot see the memory leaks anymore. I hope this can help.

Best regards,

Valerio

[Uli Schlachter]

Hi,

I think that the memory not released when the application is closed is allocated in the oflog/libsrc/globinit.cc line 90 during the call to the function log4cplus::initializeLog4cplus();

Why do you think that this is where the memleak occurs? How do you check for memory leaks? Since initializeLog4cplus() calls various functions, can you test which one of these causes the problem?

On Linux, I ran both dcmdump and storescp under valgrind. In both cases it didn't detect any memory leak in oflog.

Looking to the modifications between 3.6.0 and 3.6.1_20110225 I have found that you removed the virtual keyword from the destructor of OFListLink, OFListBase, OFListLinkBase in ofstd/include/dcmtk/ofstd/oflist.h

This was changed because OFList is supposed to be a portable implementation of std::list (and when HAVE_STL is defined, OFList turns into a simple typedef for std::list). Since std::list isn't virtual, OFList shouldn't be virtual either.

Cheers,
Uli

[vsalomoni]

Hello Uli,

Hi,

I think that the memory not released when the application is closed is allocated in the oflog/libsrc/globinit.cc line 90 during the call to the function log4cplus::initializeLog4cplus();

Why do you think that this is where the memleak occurs? How do you check for memory leaks? Since initializeLog4cplus() calls various functions, can you test which one of these causes the problem?

On Linux, I ran both dcmdump and storescp under valgrind. In both cases it didn't detect any memory leak in oflog.

Looking to the modifications between 3.6.0 and 3.6.1_20110225 I have found that you removed the virtual keyword from the destructor of OFListLink, OFListBase, OFListLinkBase in ofstd/include/dcmtk/ofstd/oflist.h

This was changed because OFList is supposed to be a portable implementation of std::list (and when HAVE_STL is defined, OFList turns into a simple typedef for std::list). Since std::list isn't virtual, OFList shouldn't be virtual either.

Cheers,
Uli

I've checked memory leaks with the Visual Studio 2009 using MFC and Debug flags (which calls the _CrtDumpMemoryLeaks function before closing application).

In the mean time I have made many tests on the Offis source code and in my opinion the virtual keyword is needed at least in the base classes you are using (OFListBase, OFListLinkBase).
What happens is that if you don't use the virtual destructor in the OFListLinkBase class, when you delete the OFListLink<T> objects created in the OFList<T> class you just call the destructor of the OFListLinkBase class. Both destructors are empty, thus avoiding the call to the OFListLink class destructor would not be a big problem, but the real problem is that in class OFListLink the destructor of the class member "T info;" is not called (and when you make list of complex data you need to call it).

Here is a code sample of what happens (simplyfing the situation as much as possible):

Code: Select all

struct T
{
	T::T()
	{
		printf("Constructor T\n");
		m_string = new char[256];
	}
	T::~T()
	{
		printf("Destructor T\n");
		delete [] m_string;
	}
	char *m_string;
};

struct DemoListLinkBase
{
	DemoListLinkBase::DemoListLinkBase()
	{
		printf("Constructor DemoListLinkBase\n");
	}
	DemoListLinkBase::~DemoListLinkBase()
	{
		printf("Destructor DemoListLinkBase\n");
	}
};

struct DemoListLink : public DemoListLinkBase
{
	T info;

	DemoListLink::DemoListLink()
	{
		printf("Constructor DemoListLink\n");
	}
	DemoListLink::~DemoListLink()
	{
		printf("Destructor DemoListLink\n");
	}

};

int main(int argc, char *argv[])
{
	DemoListLinkBase *BasePointerToDerivedClass = new DemoListLink;

	delete BasePointerToDerivedClass;
	return 0;
}

The output is the following:

Constructor DemoListLinkBase
Constructor T
Constructor DemoListLink
Destructor DemoListLinkBase

In this sample the destructor of T and DemoListLink classes are never called and the string allocated in the T class is never freed.

At the end I would suggest to restore the virtual keywords at least for the two base classes (OFListBase, OFListLinkBase).

Regards,

Valerio

[Uli Schlachter]

Sorry, I was compiling DCMTK with HAVE_STL which means that OFList was replaced with std::list. This is why I didn't see any memory leaks.

Of course you are right. I have made the destructors of OFListLinkBase and OFListBase virtual again which fixes this problem. This change should appear in git soon.

Uli

Edit:
Here's the link:
http://git.dcmtk.org/web?p=dcmtk.git;a= ... f24d2d6ad2

[st80rules]

I recently downloaded dcmtk-3.6.1_200110707.tar.gz, and a tool I use to detect memory leaks has gone wild with traces of the like:

9122: f:\vs70builds\6030\vc\mfcatl\ship\atlmfc\src\mfc\afxmem.cpp(343) +16 bytes (operator new)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\ofstd\libsrc\oflist.cc(44) +7 bytes (OFListBase::OFListBase)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\ofstd\include\dcmtk\ofstd\oflist.h(329) +15 bytes (OFList<DcmDictEntry *>::OFList<DcmDictEntry *>)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\include\dcmtk\dcmdata\dchashdi.h(53) +15 bytes (DcmDictEntryList::DcmDictEntryList)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dchashdi.cc(471) +34 bytes (DcmHashDict::put)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dcdict.cc(686) +0 bytes (DcmDataDictionary::addEntry)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dcdictzz.cc(24991) +0 bytes (DcmDataDictionary::loadBuiltinDictionary)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dcdict.cc(424) +0 bytes (DcmDataDictionary::reloadDictionaries)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dcdict.cc(125) +0 bytes (DcmDataDictionary::DcmDataDictionary)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dcdict.cc(823) +38 bytes (GlobalDcmDataDictionary::createDataDict)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dcdict.cc(842) +0 bytes (GlobalDcmDataDictionary::rdlock)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dctag.cc(128) +10 bytes (DcmTag::lookupVRinDictionary)
9122: e:\rootreferences\dcmtk-3.6.1_20110707\dcmdata\libsrc\dctag.cc(66) +0 bytes (DcmTag::DcmTag)
9122: d:\rootreferences\xvisionapi\xvision_core\cxvtextpresenter.cpp(699) +21 bytes (CxvTextPresenter::getConvertedString)
9122: d:\rootreferences\xvisionapi\xvision_core\cxvtextpresenter.cpp(162) +35 bytes (CxvTextPresenter::createTextPresenterItemsFrom)

The line in my code just declares a tag so:

DcmTag tag(iGroup, iElement);

That tool has never failed in the past, so something smells fishy. Previous versions of dcmtk did not get reported by my tool. What the tool does is basically override new and delete and reports any mismatches.

Can you guys check into this?

Thanks

[J. Riesmeier]

Some of the DCMTK command line tools contain the following code at the end of the main function (before return):

Code: Select all

#ifdef DEBUG
    dcmDataDict.clear();  /* useful for debugging with dmalloc */
#endif

Maybe, this is also useful to convince your checking tool that there is no memory leak.

The problem is that your code initializes the global data dictionary, which is usually never cleared (unless something like the above is used in the code before the program exists).

[st80rules]

Yep, that did the trick. Something must have changed recently because the tool never reported this before (going back to 3.5.4).

Thanks!

[J. Riesmeier]

You are right. I guess this is the corresponding commit. It has been introduced in order to solve other issues that were present with the "old approach".

Of course, the global data dictionary is cleared in the destructor of the class, but that's too late for most memory leak checking tools

[st80rules]

Hi,

on a related topic, I have reports of memory leaks in the DcmQueryRetrieveConfig class, and after reviewing they seem founded, as memory allocated by the readAETable method for example is never freed (the CNF_Config member).

[st80rules]

Has anyone had a chance to check this?

[Michael Onken]

Sorry, we did not check so far but it's still on our radar -- we'll post an update when we have checked that. Intuitively I would guess you are right since the wrapping of the original C code in dcmqrdb into C++ may have been done not as careful as necessary.

Michael

[Uli Schlachter]

Should be fixed now. I tested with valgrind. dcmqrscp was run with --allow-shutdown --single-process and the only SCU used was termscu.

Valgrind now reports:

All heap blocks were freed -- no leaks are possible

A link to the git commit will be given as soon as this appears in git.

Uli

Edit:
http://git.dcmtk.org/web?p=dcmtk.git;a= ... 8bad6d88fc

[st80rules]

I tested as well, and confirm that I no longer see the memory leaks.

Thanks!