WIP: BUG: Fix pointer type in memory allocation

[jhlegarreta]

Fix pointer type in memory allocation call.

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)

[jhlegarreta]

I'd be grateful if anyone could run Valgrind on this branch before making further changes. I want to know if this fixes at least some of the issues.

[N-Dekker]

Looks like a proper fix to me. In this particular case (IOComponentEnum::INT), int must have been intended, instead of unsigned int. However, I don't expect any observable behavior change. In C++, a pointer to unsigned int may be converted to a void pointer, and then casted to and "interpreted" as a pointer to signed int, without runtime errors.

[issakomi]

I look from time to time at Valgrind builds. IMHO, the whole approach, that AllocateBuffer and void**, should be reviewed. Exactly after #3403 the amount of Valgrind defects went from 1 to 33, they are all from mesh tests and about the allocation helper.

30.05.2022

31.05.2022

The errors are like this:

FMM ==16981== Mismatched free() / delete / delete []
==16981==    at 0x483CFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==16981==    by 0x1FF10D: itkOBJMeshIOTest(int, char**) (itkOBJMeshIOTest.cxx:183)
==16981==    by 0x12ED77: main (ITKIOMeshOBJTestDriver.cxx:142)
==16981==  Address 0x89e4ab0 is 0 bytes inside a block of size 400,000 alloc'd
==16981==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==16981==    by 0x1FB27A: AllocateBuffer(void**, itk::CommonEnums::IOComponent, unsigned long) (itkMeshIOTestHelper.h:416)
==16981==    by 0x1FDB56: itkOBJMeshIOTest(int, char**) (itkOBJMeshIOTest.cxx:106)
==16981==    by 0x12ED77: main (ITKIOMeshOBJTestDriver.cxx:142)

[issakomi]

Fixed typo in links to builds.

[issakomi]

I'd be grateful if anyone could run Valgrind on this branch before making further changes. I want to know if this fixes at least some of the issues.

Shortly, current implementation (after #3403) does following:

void my_alloc(void ** p)
{
  void * i = nullptr;
  i = new int[1000];
  *p = i;
}

int main(int, char**)
{
  void * p = nullptr;
  my_alloc(&p);
  ::operator delete(p);
  return 0;
}

r@deb:~$ valgrind ./a.out 
==8953== Memcheck, a memory error detector
==8953== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==8953== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==8953== Command: ./a.out
==8953== 
==8953== Mismatched free() / delete / delete []
==8953==    at 0x4836EAB: operator delete(void*) (vg_replace_malloc.c:576)
==8953==    by 0x10919B: main (ppp.cpp:11)
==8953==  Address 0x4d7fc80 is 0 bytes inside a block of size 4,000 alloc'd
==8953==    at 0x483650F: operator new[](unsigned long) (vg_replace_malloc.c:423)
==8953==    by 0x10915A: my_alloc(void**) (ppp.cpp:3)
==8953==    by 0x10918F: main (ppp.cpp:10)
==8953== 
==8953== 
==8953== HEAP SUMMARY:
==8953==     in use at exit: 0 bytes in 0 blocks
==8953==   total heap usage: 2 allocs, 2 frees, 76,704 bytes allocated
==8953== 
==8953== All heap blocks were freed -- no leaks are possible
==8953== 
==8953== For counts of detected and suppressed errors, rerun with: -v
==8953== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

It is wrong, BTW that unusual ::operator delete(p) is probably used to suppress the warning warning: deleting ‘void*’ is undefined [-Wdelete-incomplete]? But is doesn't fix the problem.

I don't have time to dig ITK mesh classes, i don't use them, just some ideas (assumed ** is required for some reason).

void my_alloc(void ** p)
{
  int * i = new int[1000];
  p[0] = static_cast<void*>(i);
}

int main(int, char**)
{
  void * p[1];
  my_alloc(p);
  int * i = static_cast<int*>(p[0]);
  delete[] i;
  return 0;
}

will not cause errors and warnings, there are many variants, of course, (*p instead p[0] works too).

In fact, i don't really know much about the mesh tests and what is the best solution.

[N-Dekker]

Thanks @issakomi Your comments do make thinks clearer! So the problem is that the buffers should have been deleted by delete [], instead of ::operator delete(p).

I think this could be solved by RAII -- let the buffers take case of deleting their own pointers automatically, in the appropriate way. However, it isn't entirely trivial, I must admit 🤔

I think something like this could solve the Valgrid defects: master...N-Dekker:Add-Buffer-to-MeshIOTestHelper The added itk::MeshIOTestHelper::Buffer class would then manage the data (both allocation and delete). What do you think?

[N-Dekker]

Another approach could be to have AllocateBuffer return an std::shared_ptr<void> to the allocated buffer:

https://github.com/N-Dekker/ITK/tree/AllocateBuffer-return-shared_ptr-void

What do you think?

[issakomi]

So the problem is that the buffers should have been deleted by delete [], instead of ::operator delete(p).

Yes, i got this version close to the current with ::operator delete[](p);

void my_alloc(void ** p)
{
  int * i = new int[1000];
  *p = static_cast<void*>(i);
}

int main(int, char**)
{
  void * p;
  my_alloc(&p);
  ::operator delete[](p);
  return 0;
}

Where is no Valgrind error, but i am still not sure, simple delete[] triggers the warning deleting ‘void*’ is undefined [-Wdelete-incomplete] delete[] p;
Probably your suggestion with RAII is better and modern (or maybe something like FreeBuffer function with switch could be used to cast back).

[jhlegarreta]

Thanks both @issakomi and @N-Dekker 💯 for your time and insights into the problem, and for proposing possible solutions.

So the problem is that the buffers should have been deleted by delete [], instead of ::operator delete(p).

I had tried this in PR #3403 prior to switching to ::operator delete, as the former was not building cleanly (was giving compiler warnings if I recall correctly).

(or maybe something like FreeBuffer function with switch could be used to cast back).

As a follow up to the commit in the current branch, I had a new commit ready to go doing this: in every test file, a switch statement for every buffer where in each case I would statically cast to the appropriate type prior to calling the delete operator:

  // Free memory
  switch (fsMeshIO->GetPointComponentType())
  {
    case itk::IOComponentEnum::CHAR:
      delete static_cast<char *>(pointBuffer);
      break;
    case itk::IOComponentEnum::UCHAR:
     (...)

Maybe that would have worked/removed the memory leaks, but I wanted to avoid it, as it involves repeating almost the exact same code for every buffer, in each and every test affected by the issue.

Unfortunately, I do not have a better proposal over the ones proposed in #3459 (comment) or in PR #3460. I am not familiar with RAII, but the solution looks very elegant. If we are satisfied with PR #3460 / if it does not incur memory leaks, I would close this in its favor.

Again, thanks both of you !

[N-Dekker]

I am not familiar with RAII, but the solution looks very elegant.

Thanks @jhlegarreta Just to clarify the RAII idiom. It's the very common C++ technique to let destructors of classes take care of deallocation of memory and other resources. Instead of having to call free and delete functions manually, at the end of your functions.

You're already using RAII on many places, even if you're not familiar with the term. For example, when you declare giftiMeshIOBaseTest and inputFileName, here:

ITK/Modules/IO/MeshGifti/test/itkGiftiMeshIOTest.cxx

Lines 48 to 52 in 0fcfaf1

	auto giftiMeshIOBaseTest = itk::GiftiMeshIO::New();
	testStatus = TestBaseClassMethodsMeshIO<itk::GiftiMeshIO>(giftiMeshIOBaseTest);
	// Test reading exceptions
	std::string inputFileName = argv[3];

Both giftiMeshIOBaseTest and inputFileName use dynamically allocated memory. But still you don't need to deallocate them manually, afterwards . That happens automatically, according to RAII.

On the other hand, when you have those operator delete calls at the end of your function, there is a risk that the program execution does not reach those lines of code, because of an exception or an earlier return statement. Which would lead to memory leaks. So RAII is not just a matter of style. It does avoid memory leaks.

HTH, Niels

[N-Dekker]

@issakomi @jhlegarreta Can you possibly confirm that Valgrind is happy now, with git master branch commit db8d797 I do expect so, but I just want to be sure 😃

[issakomi]

@issakomi @jhlegarreta Can you possibly confirm that Valgrind is happy now, with git master branch commit db8d797 I do expect so, but I just want to be sure

Minimal local test runs fine, without compiler warnings (Linux, gcc 8.3.0) or Valgrind defects:

#include <memory>

template <typename T>
std::shared_ptr<void>
MakeSharedArray(const std::size_t bufferSize)
{
  return std::shared_ptr<void>(new T[bufferSize], std::default_delete<T[]>());
}

int main(int,char**)
{
  const std::shared_ptr<void> p = MakeSharedArray<double>(100000);
  return 0;
}

==20258== Memcheck, a memory error detector
==20258== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==20258== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==20258== Command: ./a.out
==20258== 
==20258== 
==20258== HEAP SUMMARY:
==20258==     in use at exit: 0 bytes in 0 blocks
==20258==   total heap usage: 3 allocs, 3 frees, 872,728 bytes allocated
==20258== 
==20258== All heap blocks were freed -- no leaks are possible
==20258== 
==20258== For counts of detected and suppressed errors, rerun with: -v
==20258== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[N-Dekker]

Minimal local test runs fine, without compiler warnings (Linux, gcc 8.3.0) or Valgrind defects

@issakomi Cool, I'm glad to see that std::shared_ptr<void> works fine. Honestly, I think I never created a shared void pointer before in my life 😸 Of course, the next question is if Valgrind now also likes the MeshIO tests as a whole...? Again, I guess it probably will, but I just want to know for sure.

[jhlegarreta]

Superseded by #3460.

[issakomi]

the next question is if Valgrind now also likes the MeshIO tests as a whole...?

@N-Dekker Looks good, no more Mismatched free() / delete / delete []

https://open.cdash.org/viewDynamicAnalysis.php?buildid=7966303