Memory leak in `itkFreeSurferMeshIOTestBinary`

[jhlegarreta]

Description

The dashboard is reporting a memory leak in the itkFreeSurferMeshIOTestBinary test: https://open.cdash.org/viewDynamicAnalysisFile.php?id=9817770

The leak has been there ever since the test was added in: https://github.com/InsightSoftwareConsortium/ITK/commit/0fcfaf1288928a76c3ef2b3fcc8b6e53246bfbba

Steps to Reproduce

1. Have Valgrind or a dependable dynamic analysis tool.
2. Run the itkFreeSurferMeshIOTestBinary test: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshFreeSurfer/test/CMakeLists.txt#L18
3. Check the memory leak.

Expected behavior

No memory leaks are present.

Actual behavior

The following leak is reported:

UMR ==24113== Syscall param writev(vector[...]) points to uninitialised byte(s)
(...)
==24113==    by 0x50D38D2: itk::ByteSwapper::SwapWrite4Range(void const*, unsigned long, std::ostream*) (itkByteSwapper.hxx:367)
==24113==    by 0x50D07A7: itk::ByteSwapper::SwapWriteRangeFromSystemToBigEndian(unsigned int const*, int, std::ostream*) (itkByteSwapper.hxx:160)
==24113==    by 0x50D1FD9: void itk::FreeSurferBinaryMeshIO::WriteCells(unsigned int*, std::basic_ofstream >&) (itkFreeSurferBinaryMeshIO.h:147)
==24113==    by 0x50CF1E2: itk::FreeSurferBinaryMeshIO::WriteCells(void*) (itkFreeSurferBinaryMeshIO.cxx:481)
==24113==    by 0x144C45: int itkFreeSurferMeshIOTestHelper(itk::FreeSurferBinaryMeshIO::Pointer, itk::FreeSurferBinaryMeshIO::Pointer, char*, char*, char*, char*, bool, bool, bool, bool, bool, itk::FreeSurferBinaryMeshIO::SizeValueType, itk::FreeSurferBinaryMeshIO::SizeValueType, itk::FreeSurferBinaryMeshIO::SizeValueType, itk::FreeSurferBinaryMeshIO::SizeValueType) (itkFreeSurferMeshIOTest.cxx:118)
==24113==    by 0x13C881: itkFreeSurferMeshIOTest(int, char**) (itkFreeSurferMeshIOTest.cxx:210)

Reproducibility

%100.

Versions

master

Environment

Any.

Additional Information

• The leak in FreeSurferMeshIO is only present for the BINARY case in the dashboard.

• I am not persuaded that the problem lies in itk::ByteSwapper as the leak's call stack reports. The previous call points to itk::FreeSurferBinaryMeshIO::WriteCells(void*) (itkFreeSurferBinaryMeshIO.cxx:481).

• The itkBYUMeshIOTest, itkOFFMeshIOTest, itkVTKPolyDataMeshIOTest and itkOBJMeshIOTest do the same thing: fist use the cell buffer to test the exception cases, then use it to write to the output file.

The only things that I can think of:

• The file is open within some methods but is not closed at the end of such methods and vice-versa (e.g. https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshFreeSurfer/src/itkFreeSurferBinaryMeshIO.cxx#L244). But the same happens in other MeshIO classes (e.g. https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshFreeSurfer/src/itkFreeSurferAsciiMeshIO.cxx#L91 does not call CloseFile(); or still https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshOFF/src/itkOFFMeshIO.cxx#L98). However, the leak does not point to one of such methods, and he output stream is effectively closed in https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshFreeSurfer/src/itkFreeSurferBinaryMeshIO.cxx#L530
• The template argument for make_unique_for_overwrite is not the appropriate one. Not sure whether it should be the class template typename T everywhere.
• As the buffer is used for the exception cases first, then to write to the output file, maybe separating both uses within a local scope {} may solve the issue.

Not sure if the MSVC memory leak detector is dependable, as it shows me a leak even if I remove all contents of the test and leave only the instantiation. https://learn.microsoft.com/en-us/visualstudio/debugger/finding-memory-leaks-using-the-crt-library?view=vs-2022

I have been willing to resolve this since some time/unwilling to open an issue, but I have spent time trying to investigate this. Having the above issue with the MSVC leak detector I am not able to effectively test my guesses, and the apparently incoherent file opening/closing patterns across the files but the absence of more leaks adds some confusion.

Suggestions are welcome.

[issakomi]

There is, BTW, no memory leak, but uninitialized bytes. Running Valgrind with --track-origins=yes shows the origin of uninitialized memory in ReadCells

==49404==  Uninitialised value was created by a heap allocation
==49404==    at 0x484220F: operator new[](unsigned long) (vg_replace_malloc.c:640)
==49404==    by 0x16D492: auto itk::make_unique_for_overwrite<unsigned int []>(unsigned long) (itkMakeUniqueForOverwrite.h:67)
==49404==    by 0x173900: itk::FreeSurferBinaryMeshIO::ReadCells(void*) (itkFreeSurferBinaryMeshIO.cxx:247)

I looked at the test some time ago and gave up, sorry. I can not open and validate files, also Write() is empty, don't know how to write properly. BTW, FreeSurfer requires license AFAIK (free, but with too much personal information). If there are users of the class, perhaps they could help.

[jhlegarreta]

Thanks for the investigation @issakomi.

==49404== by 0x173900: itk::FreeSurferBinaryMeshIO::ReadCells(void*) (itkFreeSurferBinaryMeshIO.cxx:247)

Not sure how to look at that:

• the member m_NumberOfCells is initialized here: https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Modules/IO/MeshBase/src/itkMeshIOBase.cxx#L25 And modified when reading the mesh information https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshFreeSurfer/src/itkFreeSurferBinaryMeshIO.cxx#L160

• Otherwise, the line it points to itk::make_unique_for_overwrite<unsigned int []>. The leak was there before itk::make_unique_for_overwrite was introduced, but maybe @N-Dekker can hit on an idea?

• Also, the ReadCells method is called directly in the test as well, so it seems weird that the Valgrind report does not point to that call as well: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MeshFreeSurfer/test/itkFreeSurferMeshIOTest.cxx#L92

[issakomi]

it seems weird that the Valgrind report does not point to that call as well

it will point only with the --track-origins=yes

I shall try to get FreeSurfer license. I want to see that binary mesh first.

[N-Dekker]

maybe @N-Dekker can hit on an idea?

Thanks for asking, Jon. Could it possibly be that the m_InputFile.read has failed? If m_InputFile.read fails, the data buffer passed as argument might not be fully initialized. Looking at:

https://github.com/InsightSoftwareConsortium/ITK/blob/bd4d46e053bb8d5c3dc22e1c3ca2c86dd5ac7a40/Modules/IO/MeshFreeSurfer/src/itkFreeSurferBinaryMeshIO.cxx#L243-L252

BTW, Instead of calling make_unique_for_overwrite<itk::uint32_t[]> the original code (before my PR #3590 commit 15d09d6c756b46b4d1c296d8bb13b85f82654ef9) just did new itk::uint32_t[this->m_NumberOfCells * numberOfCellPoints], which also would have left the data buffer uninitialized, after a read failure.

[issakomi]

BTW, the fsmeshiosphere.fsa file written by the test (ascii test doesn't fail, no defects) looks like this:

#!ascii version of /home/r/itk/build/Testing/Temporary/fsmeshiosphere.fsa
162    320

The binary file fsmeshiosphere.fsb written by the test (binary test doesn't fail, but with defect) is 29 bytes large.

But the test reports that comparison with original is OK.

There are so many question about the tests and the class, the post would be too long.

[issakomi]

#!ascii version of /home/r/itk/build/Testing/Temporary/fsmeshiosphere.fsa
162    320

OK, i know why, the last write call ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->WriteMeshInformation()); overrides the file, the order is wrong, it should be first call

[issakomi]

I have got the license and installed FreeSurfer (latest release). The results:

sphere.fsa (input file) crashes FreeView, so skipped.

sphere.fsb works

This program produces exactly (also checksum is identical) the same output as input and doesn't have Valgrind defects:

#include "itkFreeSurferBinaryMeshIO.h"
#include "itkMeshIOTestHelper.h"
#include "itkTestingMacros.h"
int main(int argc, char ** argv)
{
  auto fsMeshIO = itk::FreeSurferBinaryMeshIO::New();
  fsMeshIO->SetFileName(argv[1]);
  ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->ReadMeshInformation());
  // sphere.fsb: point component type is float, cell component type is uint32,
  // not tested with other files.
  const std::shared_ptr<void> pointBuffer =
    itk::MeshIOTestHelper::AllocateBuffer(fsMeshIO->GetPointComponentType(), 3 * sizeof(float) * fsMeshIO->GetNumberOfPoints());
  const std::shared_ptr<void> cellBuffer =
    itk::MeshIOTestHelper::AllocateBuffer(fsMeshIO->GetCellComponentType(), 3 * sizeof(uint32_t) * fsMeshIO->GetNumberOfCells());
  ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->ReadPoints(pointBuffer.get()));
  ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->ReadCells(cellBuffer.get()));
  fsMeshIO->SetFileName("out.fsb");
  ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->WriteMeshInformation());
  ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->WritePoints(pointBuffer.get()));
  ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->WriteCells(cellBuffer.get()));
  return 0;
}

Maybe this information will help:

If i comment the line 90 in the test ITK_TRY_EXPECT_NO_EXCEPTION(fsMeshIO->ReadPointData(pointDataBuffer.get())); there is no Valgrind defect, i am not 100% sure, but may be this line ReadPointData() m_InputFile.seekg(m_FilePosition, std::ios::beg); causes the problem, you call ReadCells after.

[jhlegarreta]

Thanks both @issakomi and @N-Dekker. Thanks for the pointers and effort :100:. I'll try to have a look at it in the coming weeks.

[issakomi]

I found FreeSurfer useful for some of my tasks.

sphere.fsa (input file) crashes FreeView, so skipped.

Unfortunately FreeSurferAsciiMeshIO is broken. First. The mesh doesn't look like the class imagine. Points look like 0.000000 0.850651 0.525731 0.000000 It is wrong, FreeSurfer will crash, the 4th component is integer, probably saying "Is in patch" (maybe unused, not yet sure, 0 is safe).

The template

  template <typename T>
  void
  WritePoints(T * buffer, std::ofstream & outputFile, T label = itk::NumericTraits<T>::ZeroValue())

is doing wrong job, it should be

  template <typename T>
  void
  WritePoints(T * buffer, std::ofstream & outputFile, int label = 0)

The same for WriteCells.

Second. FreeView doesn't know extension fsa (assumes binary format and crashes) and fsb, BTW too, but for binary (native) mesh format it doesn't play any role. There is the utility _mrisconvert:

These file formats are supported:
  ASCII:       .asc
  ICO:         .ico, .tri
  GEO:         .geo
  STL:         .stl
  VTK:         .vtk
  GIFTI:       .gii
  MGH surface-encoded 'volume': .mgh, .mgz
Freesurfer binary format assumed for all other extensions.

So ITK's ascii mesh class should be fixed.

Edit: The binary class works, that defect with uninitialized bytes is from the test. The binary test should be reviewed and produce proper outputs, some methods are for curvature data, not for a mesh, e.g. ReadPointData, WritePointData. The class is difficult to use, but works (at least with a mesh, not yet tested with curvature data). With proper usage that Valgrind's uninitialized bytes defect will disappear (it is caused by rewinding the file by the wrong call ReadPointData and later read failure in the following ReadCells call). The ascii test is useless before the class will be repaired, IMHO.

Edit:

The binary class works

Hmm, works, but among FreeSurfer models there is not a single file with .fsb extension, a user have to know and re-name to use CanReadFile. CanReadFile should be based on magic number, IMHO. 'Curvature data' should be checked (there is no test file and many questions).

[jhlegarreta]

Thanks for all this work @issakomi. Thanks @N-Dekker for having spared a thought as well.