Itk: 5k+ Valgrind defects from MetaImageIO

[blowekamp]

Sample leak trace from the dashboard:

{

   Memcheck:Leak
   match-leak-kinds: definite
   fun:_Znwm
   fun:_ZN9MetaImageC1Ev
   fun:_ZN3itk11MetaImageIOC1Ev
   fun:_ZN3itk11MetaImageIO3NewEv
   fun:_ZN3itk20CreateObjectFunctionINS_11MetaImageIOEE12CreateObjectEv
   fun:_ZN3itk17ObjectFactoryBase15CreateAllObjectEPKc
   fun:_ZN3itk17ObjectFactoryBase17CreateAllInstanceEPKc
   fun:_ZN3itk14ImageIOFactory13CreateImageIOEPKcNS_11CommonEnums10IOFileModeE
   fun:_ZN3itk15ImageFileWriterINS_5ImageItLj2EEEE5WriteEv
   fun:_ZN3itk15ImageFileWriterINS_5ImageItLj2EEEE6UpdateEv
   fun:_Z35itkMapGrayscaleErodeImageFilterTestiPPc
   fun:main
}

Also see: Kitware/MetaIO#100

[issakomi]

@blowekamp

Yes, there is a memory leak. Tested with trivial program (load MHA image, save MHD), looks like the leak is coming from metaImage.cxx

m_CompressionTable = new MET_CompressionTableType;

which is called in all constructors, but never deleted.

Here is readable _Valgrind_ output (both ITK and test program compiled in _debug_ mode to get lines in source code):

r@deb:~/Desktop/test_rgb/build$ valgrind --leak-check=full  ./test007 
==11408== Memcheck, a memory error detector
==11408== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==11408== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==11408== Command: ./test007
==11408== 
==11408== 
==11408== HEAP SUMMARY:
==11408==     in use at exit: 96 bytes in 2 blocks
==11408==   total heap usage: 109,182 allocs, 109,180 frees, 14,202,584 bytes allocated
==11408== 
==11408== 48 bytes in 1 blocks are definitely lost in loss record 1 of 2
==11408==    at 0x4835DEF: operator new(unsigned long) (vg_replace_malloc.c:334)
==11408==    by 0xDD1BD8: MetaImage::MetaImage() (metaImage.cxx:98)
==11408==    by 0x705CA8: itk::MetaImageIO::MetaImageIO() (itkMetaImageIO.cxx:34)
==11408==    by 0x705423: itk::MetaImageIO::New() (itkMetaImageIO.h:54)
==11408==    by 0x705AE5: itk::CreateObjectFunction<itk::MetaImageIO>::CreateObject() (itkCreateObjectFunction.h:74)
==11408==    by 0x64B3D4: itk::ObjectFactoryBase::CreateAllObject[abi:cxx11](char const*) (itkObjectFactoryBase.cxx:802)
==11408==    by 0x648CEA: itk::ObjectFactoryBase::CreateAllInstance[abi:cxx11](char const*) (itkObjectFactoryBase.cxx:221)
==11408==    by 0x664B6D: itk::ImageIOFactory::CreateImageIO(char const*, itk::CommonEnums::IOFileMode) (itkImageIOFactory.cxx:39)
==11408==    by 0x5B6EA3: itk::ImageFileReader<itk::Image<itk::RGBPixel<unsigned char>, 3u>, itk::DefaultConvertPixelTraits<itk::RGBPixel<unsigned char> > >::GenerateOutputInformation() (itkImageFileReader.hxx:101)
==11408==    by 0x61F671: itk::ProcessObject::UpdateOutputInformation() (itkProcessObject.cxx:1478)
==11408==    by 0x5ACF20: itk::ImageBase<3u>::UpdateOutputInformation() (itkImageBase.hxx:225)
==11408==    by 0x63DCC2: itk::DataObject::Update() (itkDataObject.cxx:306)
==11408== 
==11408== 48 bytes in 1 blocks are definitely lost in loss record 2 of 2
==11408==    at 0x4835DEF: operator new(unsigned long) (vg_replace_malloc.c:334)
==11408==    by 0xDD1BD8: MetaImage::MetaImage() (metaImage.cxx:98)
==11408==    by 0x705CA8: itk::MetaImageIO::MetaImageIO() (itkMetaImageIO.cxx:34)
==11408==    by 0x705423: itk::MetaImageIO::New() (itkMetaImageIO.h:54)
==11408==    by 0x705AE5: itk::CreateObjectFunction<itk::MetaImageIO>::CreateObject() (itkCreateObjectFunction.h:74)
==11408==    by 0x64B3D4: itk::ObjectFactoryBase::CreateAllObject[abi:cxx11](char const*) (itkObjectFactoryBase.cxx:802)
==11408==    by 0x648CEA: itk::ObjectFactoryBase::CreateAllInstance[abi:cxx11](char const*) (itkObjectFactoryBase.cxx:221)
==11408==    by 0x664B6D: itk::ImageIOFactory::CreateImageIO(char const*, itk::CommonEnums::IOFileMode) (itkImageIOFactory.cxx:39)
==11408==    by 0x5AA9B1: itk::ImageFileWriter<itk::Image<itk::RGBPixel<unsigned char>, 3u> >::Write() (itkImageFileWriter.hxx:112)
==11408==    by 0x5A86AE: itk::ImageFileWriter<itk::Image<itk::RGBPixel<unsigned char>, 3u> >::Update() (itkImageFileWriter.h:178)
==11408==    by 0x5A633E: main (test007.cpp:29)
==11408== 
==11408== LEAK SUMMARY:
==11408==    definitely lost: 96 bytes in 2 blocks
==11408==    indirectly lost: 0 bytes in 0 blocks
==11408==      possibly lost: 0 bytes in 0 blocks
==11408==    still reachable: 0 bytes in 0 blocks
==11408==         suppressed: 0 bytes in 0 blocks
==11408== 
==11408== For counts of detected and suppressed errors, rerun with: -v
==11408== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

After adding a line to destructor

 MetaImage::~MetaImage()
 {
+ delete m_CompressionTable;
 MetaObject::M_Destroy();
 }

the leak seems to go. Here is output:

r@deb:~/Desktop/test_rgb/build$ valgrind --leak-check=full  ./test007 
==13818== Memcheck, a memory error detector
==13818== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==13818== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==13818== Command: ./test007
==13818== 
==13818== 
==13818== HEAP SUMMARY:
==13818==     in use at exit: 0 bytes in 0 blocks
==13818==   total heap usage: 109,182 allocs, 109,182 frees, 14,202,584 bytes allocated
==13818== 
==13818== All heap blocks were freed -- no leaks are possible
==13818== 
==13818== For counts of detected and suppressed errors, rerun with: -v
==13818== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Edit:
This not a PR, only a quick test, code about that _m_CompressionTable_ should be reviewed more carefully, check members of _MET_CompressionTableType_, etc.

Edit:
also checked version 4.13 (still use for public release builds) - the leak is not present, FYI. In that version was proper cleanup in MetaImage::M_Destroy() - called in _MetaImage::~MetaImage()_. In current version this function is not available. The changes (renamed to _M_ResetValues_) were done in https://github.com/Kitware/MetaIO/commit/632a827b5bac695a366dfc3b61f1b7008cf996b1#diff-5c9aca0d9c6e5a0655392c5f2669b9babe37ddd46be0994dbb0660654e47a2d4 and somewhere else in _MetaImage::~MetaImage()_ it was replaced with _MetaObject::M_Destroy()_

[blowekamp]

@issakomi Great! Work tracking it down. I'll make the PR to MetaImageIO.

The de-mangled output of your valgrind run is nice. Do you add flags to valgrind to de-mangle the symbols?

[blowekamp]

Fix proposed in: Kitware/MetaIO#101

[issakomi]

Thank you.

The de-mangled output of your valgrind run is nice. Do you add flags to valgrind to de-mangle the symbols?

Only compiled ITK and test program (trivial one) in _Debug_ mode and run valgrind --leak-check=full ./test007 from terminal on Linux.

BTW, a side note to that function

void
MetaImage::M_ResetValues()
{
  if (m_AutoFreeElementData && m_ElementData != nullptr)
  {
    delete[]static_cast<char *>(m_ElementData);
  }

  m_ElementData = nullptr;

  if (m_CompressionTable && m_CompressionTable->compressedStream)
  {
    inflateEnd(m_CompressionTable->compressedStream);
    delete m_CompressionTable->compressedStream;
    delete[] m_CompressionTable->buffer;
    m_CompressionTable->buffer = nullptr;
  }
  delete m_CompressionTable;
  m_CompressionTable = nullptr;

  MetaObject::M_Destroy();
}

may be it were not bad to check m_CompressionTable is not null before delete m_CompressionTable; to make it re-entrant. May be it is called somewhere else under some conditions, there were a lot of changes since 4.13. Just a thought, not sure it is required.

[blowekamp]

may be it were not bad to check m_CompressionTable is not null before delete m_CompressionTable

With C++, delete nullptr is well defined to be safe and have not side effects. Good thinking to consider that check. As the method already guards the de-referencing of m_CompressionTable it should be safe.

[issakomi]

Thank you. Sure, sorry, i still use _NULL_ in my code and check (probably not really required with NULL too), forgotten about it, time to switch to _nullptr_.

[issakomi]

I have found where it was removed from destructor, same date _18.12.2020_ https://github.com/Kitware/MetaIO/commit/48650b52d864cdc69d9c0bf2acad6d02b8e16d55#diff-5c9aca0d9c6e5a0655392c5f2669b9babe37ddd46be0994dbb0660654e47a2d4R1169

That MetaImage::M_Destroy() was replaced with MetaObject::M_Destroy() at several places, incl. destructor (fixed now). It was also replaced in _MetaImage::Read()_ and _MetaImage::ReadROI()_. My knowledge of MetaIO is very limited, honestly never looked inside (but use and recommend as main non-DICOM format), can't judge, just FYI. Sorry for noise.

[blowekamp]

@issakomi That looks like some good detective work. Can you make a PR to Kitware/MetaIO with the recommended change 👍

After merging the MetaIO update with the metaImage fix, the related valgrind defects are gone.

[issakomi]

@blowekamp Thank you! Looks like it is not urgent, Clear() function deletes members of _m_CompressionTable_ (or re-creates _m_CompressionTable_ if it is null). Not sure about _m_ElementData_, may be little later i shall do more checks. BTW, there is no indent at some related places, may be it is a reminder that the author planed to return to that point later to look at it again.