Tuesday, March 17, 2015

Don't use an MFC CArray with types that contain self pointers or back pointers

CArray is MFC's resizing array class. Its semantics are similar to std::vector's, and in new code you should prefer vector because of the standard library support that comes with it (and because of the gotcha I'm about to describe). But if you're working with an MFC legacy codebase or interacting with the MFC framework itself, you will at times find yourself needing to use CArray. And CArray can get you into trouble if you use it with the wrong types.

The difficulty comes from the way CArray handles the copy operation for its elements when its buffer is relocated in memory as part of a resize operation. vector and the other standard library containers copy their elements one at a time using the contained type's copy/move constructor, but CArray just does a single bulk memcpy from the old buffer space into the new one. If your type is POD this is fine, but if it defines a copy constructor, it won't be called and things will probably blow up (and Microsoft does document this on their CArray page). In particular, if there are any sort of self pointers or back pointers involved, they will become wild. For example, consider this code:

void TraceAddress(const CString& prefix, void* p)
{
 CString str;
 str.Format(_T("%s: %p\n"), prefix.GetString(), p);
 TRACE(str);
}

void Demo()
{
 struct Foo
 {
  Foo() : me(this) {}
  Foo(const Foo&) : me(this) {}
  Foo* me;
 };

 TRACE(_T("\nCArray:\n"));
 CArray<Foo> aFoo;
 
 aFoo.SetSize(1);
 TraceAddress(_T("Address of aFoo[0]"), &aFoo[0]);
 TraceAddress(_T("me ptr  of aFoo[0]"), aFoo[0].me);
 
 aFoo.SetSize(20);
 TRACE(_T("Array resized\n"));
 TraceAddress(_T("Address of aFoo[0]"), &aFoo[0]);
 TraceAddress(_T("me ptr  of aFoo[0]"), aFoo[0].me);

 TRACE(_T("\nVector:\n"));
 std::vector<Foo> vFoo;
 
 vFoo.resize(1);
 TraceAddress(_T("Address of vFoo[0]"), &vFoo[0]);
 TraceAddress(_T("me ptr  of vFoo[0]"), vFoo[0].me);
 
 vFoo.resize(20);
 TRACE(_T("Vector resized\n"));
 TraceAddress(_T("Address of vFoo[0]"), &vFoo[0]);
 TraceAddress(_T("me ptr  of vFoo[0]"), vFoo[0].me);
}

The output I saw when I ran this on my machine was:

CArray:
Address of aFoo[0]: 0063AC90
me ptr  of aFoo[0]: 0063AC90
Array resized
Address of aFoo[0]: 0063ACD0
me ptr  of aFoo[0]: 0063AC90

Vector:
Address of vFoo[0]: 0063AC90
me ptr  of vFoo[0]: 0063AC90
Vector resized
Address of vFoo[0]: 0063ADA8
me ptr  of vFoo[0]: 0063ADA8

What happened here? When we called aFoo.SetSize(1), the CArray reserved a buffer large enough to hold a single Foo at address 0063AC90. It then constructed a Foo there using the class's default constructor, which correctly initializes the me pointer. Then, when we called aFoo.SetSize(20), it reserved a new buffer large enough to hold 20 Foos at address 006ACD0 and copied the existing Foo object to this location. But instead of using the copy constructor to do this, it just memcpyied the thing, which left the me member pointing to the old address. This is a bug, but you won't discover it until the next time someone tries to use that me pointer. By then, the thread could be miles away from the resize operation leaving you with little clue how things went bad. vector, by contrast, uses Foo's copy constructor when it needs to relocate its buffer, so everything works out ok.

This case may seem obvious and contrived, but sometimes the self references are indirect and less easy to detect. For example, a co-worker recently called me over to help him with a tough, reproducible bug he couldn't figure out. A function took a const vector<float>& argument and then performed a std::max_element operation on it, but when max_element went to construct an iterator for its return value, a checked iterator failure was occurring. He was convinced he'd found a bug in the standard library, but I wasn't so sure. Tracing back a ways, it turned out that the input vector was ultimately a member of a struct that was being stored in a CArray. Something like this:

struct S
{
   std::vector<float> v;
};

CArray<S> sArray;
//...
FunctionThatFailed(sArray[0].v);



Upon seeing this, I immediately suspected that he'd stumbled into a CArray resize bug. And sure enough, it turns out that under our standard library implementation (MSVC++ 2012), vector contains a pointer to a proxy (_Myproxy) which in turn contains a pointer back to the container it proxies for (_Mycont). When that CArray got resized it broke all those _Mycont back pointers for the existing elements, but this didn't show up until much later when iterator creation was triggered by calling std::max_element on one of the individual vectors. I suggested he replace the CArray with a vector, and once he did, everything worked fine.

No comments:

Post a Comment