Heap corruption when wrapping unmanaged pointers in C++/CLI

Question

I am having some trouble with heap corruption in a .NET application that makes use of native C code, C++/CLI, and C#. This is my first time really getting into the weeds here.

The structure of the application is C# for GUI and overall control flow, C++/CLI for wrapping native C functions, and native C functions for processing data. These native C functions typically accept as inputs native pointers to arrays (eg: int*) and a dimension. C++/CLI wraps up those low level functions into higher level combined processing functions, and C# calls the high level functions.

Occasionally, I do need to allocate unmanaged memory at the C# level, then pass the same parcel of memory to a few different C++/CLI functions.

In order to pass these arrays around freely through my C# and C++/CLI layers, I created a thin wrapper class around managed pointers. This wrapper, called ContiguousArray, defined at the C++/CLI layer, looks something like this:

template <typename T>
public ref class ContiguousArray
{
public:
  ContiguousArray<T>(int size)
  {
    _size = size;
    p = (T*) calloc(_size,sizeof(T));
  }

  T& operator[](int i)
  {
    return p[i];
  }

  int GetLength()
  {
    return _size;
  }
  ~ContiguousArray<T>()
  {
    this->!ContiguousArray<T>();
  }

  !ContiguousArray<T>()
  {
    if (p != nullptr)
    {
      free(p);
      p = nullptr;
    }
  }

  T* p;
  int _size;
};

// Some non-templated variants of ContiguousArray for passing out to other .NET languages
public ref class ContiguousArrayInt16 : public ContiguousArray<Int16>
{
  ContiguousArrayInt16(int size) : ContiguousArray<Int16>(size) {}
};

I use this wrapper class in a few ways.

Use Case 1 (C++/CLI):

{
  // Create an array for the low level code
  ContiguousArray<float> unmanagedArray(1024);

  // Call some native functions
  someNativeCFunction(unmanagedArray.p, unmanagedArray.GetLength());
  float* unmanagedArrayPointer = unmanagedArray.p;
  anotherNativeCFunction(unmanagedArrayPointer, unmanagedArray.GetLength());
  int returnCode = theLastNativeCFunction(unmanagedArray.p, unmanagedArray.GetLength());

  return returnCode;
} // unmanagedArray goes out of scope, freeing the memory

Use Case 2 (C++/CLI):

{
  // Create an array for the low level code
  ContiguousArray<float>^ unmanagedArray = gcnew ContiguousArray<float>(1024);
  cliFunction(unmanagedArray);
  anotherCLIFunction(unmanagedArray);
  float* unmanagedArrayPointer = unmanagedArray->p;
  int returnCode = nativeFunction(unmanagedArrayPointer, unmanagedArray->GetLength());
  return returnCode;
} // unmanagedArray goes out of scope, the garbage collector will take care of it at some point

Use Case 3 (C#):

{
  ContiguousArrayInt16 unmanagedArray = new UnmanagedArray(1024);
  cliFunction(unmanagedArray);
  unmanagedArray = anotherCLIFunctionThatReplacesUnmanagedArray(unmanagedArray); // Unmanaged array is possibly replaced, original gets collected at some point
  returnCode = finalCLIFunction(unmanagedArray);
  // Do something with return code like show the user
} // Memory gets freed at some point

I thought that I was being pretty careful with handling the unmanaged memory by using this wrapper class, but I keep seeing heap corruption and access violation issues in my application. I never keep a native pointer to the unmanaged memory outside of the scope where the ContiguousArray object is valid.

Is there anything wrong with any of these three use cases that could, in theory, cause heap corruption? Am I missing something key in my ContiguousArray implementation? I am worried that perhaps the garbage collector is getting a little overzealous and cleaning up my managed objects before I'm really done with them.

Use case 1: Am I guaranteed that the finalizer won't be called until the closing brace? Is it possible that .NET has decided that object is no longer of use and it gets cleaned up while I still have a pointer to its internal memory? Does GC::KeepAlive need to be used for stack objects?

Use case 2: Do I need GC::KeepAlive at the end to guarantee the object isn't disposed before the third function call? Would I still need it if I instead wrote: nativeFunction(unmanagedArray->p, unmanagedArray->GetLength());

Use case 3: I can't see anything wrong here, but maybe I am missing something?

The C functions I am invoking are not build-in Windows DLLs, but code that I am compiling myself. I don't want to use pinvoke to call the native function, the exercise is to wrap these myself for high-performance. — Zack Schilling
It is definitely wrong code, the finalizer can run while the native code is executing. Happens when another thread in the program triggers a garbage collection. Using GC::KeepAlive() is a workaround but you have a much better one: destroy the array deterministically. Append delete unmanagedArray; or use stack semantics, in the C# code use using. — Hans Passant

Zack Schilling Zack Schilling · Accepted Answer · 2016-07-08T15:11:47

Thanks to the magic of writing out my question (the best teacher) and the advice of tsandy and Hans, I've researched at length the behavior of the garbage collector when dealing with unmanaged resources. Here's what I found:

The design pattern that I am using is flawed. If the garbage collector decides that a managed object handle (^) is no longer in use, even if the handle is still in scope, it can be garbage collected. A proper (but slower) design pattern does not allow access to unmanaged resources except through methods of its managed wrapper class. If pointers or references to unmanaged resources are allowed to leak out of the wrapper, the code that gets ahold of them needs to take great care to make sure the wrapper that owns them does not get collected/finalized. For that reason, wrapper classes designed like ContiguousArray aren't a good idea.

That said, this pattern is fast! So here's how to salvage things case-by-case.

Use Case 1 is actually ok! Using stack semantics in C++/CLI ensures deterministic finalization when the wrapper goes out of scope. Holding on to pointers after the wrapper has gone out of scope is still an error, but in all this is safe. I changed a great deal of my C++/CLI code to strongly favor stack semantics, including using handle references (%) whenever possible as arguments to functions called only by my C++/CLI code.

Use Case 2 is dangerous and needs fixing. Sometimes you cannot avoid using handles, so you need to use GC::KeepAlive(unmanagedArray) to force the garbage collector to hold on to the objects up until the KeepAlive call.

{
  // Create an array for the low level code
  ContiguousArray<float>^ unmanagedArray = gcnew ContiguousArray<float>(1024);
  cliFunction(unmanagedArray);
  anotherCLIFunction(unmanagedArray);
  float* unmanagedArrayPointer = unmanagedArray->p;
  int returnCode = nativeFunction(unmanagedArrayPointer, unmanagedArray->GetLength());
  GC::KeepAlive(unmanagedArray); // Force the wrapper to stay alive while native operations finish.
  return returnCode;
}

Use Case 3 is technically unsafe. Immediately after the call to finalCLIFunction, the .NET garbage collector might decide it no longer needs unmanagedArray (depending on the implementation of finalCLIFunction). But doesn't make sense to burden C# code with implementation details like KeepAlive if we don't need to. Instead, never attempt to access anything unmanaged from C# code and make sure that the implementation of all our C++/CLI functions call KeepAlive for their own arguments, if those arguments are handles.

int finalCLIFunction(ContiguousArrayInt16^ unmanagedArray)
{
  // Do a bunch of work with the unmanaged array
  Int16* ptr = unmanagedArray->p;
  for(int i=0; i < unmanagedArray->GetLength(); i++)
  {
    ptr[i]++;
  }

  // Call KeepAlive on the calling arguments to ensure they stay alive
  GC::KeepAlive(unmanagedArray);

  return 0;
}

Then that's it. Use stack semantics whenever possible. When you cannot, use GC::KeepAlive() after the final line where you need objects to be alive. Remember to also do this for calling arguments to C++/CLI functions. Keep all this garbage collection wrangling out of sight of your C# code, which shouldn't need to know these implementation details.

I followed all these conventions and my heap corruption and access violations are gone. Hopefully this will be of some help to someone.

Heap corruption when wrapping unmanaged pointers in C++/CLI

2 Answers