MFC multithreading with delete[] , dbgheap.c

Question

I've got a strange problem and really don't understand what's going on.

I made my application multi-threaded using the MFC multithreadclasses.

Everything works well so far, but now:

Somewhere in the beginning of the code I create the threads:

            m_bucketCreator = new BucketCreator(128,128,32);
    CEvent* updateEvent = new CEvent(FALSE, FALSE);
    CWinThread** threads = new CWinThread*[numThreads];
    for(int i=0; i<8; i++){
        threads[i]=AfxBeginThread(&MyClass::threadfunction, updateEvent);
        m_activeRenderThreads++;
    }

this creates 8 threads working on this function:

UINT MyClass::threadfunction( LPVOID params ) //executed in new Thread
{

Bucket* bucket=m_bucketCreator.getNextBucket();

    ...do something with bucket...

delete bucket;

}

m_bucketCreator is a static member. Now I get some thread error in the deconstructor of Bucket on the attempt to delete a buffer (however, the way I understand it this buffer should be in the memory of this thread, so I don't get why there is an error). On the attempt of delete[] buffer, the error happens in _CrtIsValidHeapPointer() in dbgheap.c.

Visual studio outputs the message that it trapped a halting point and this can be either due to heap corruption or because the user pressed f12 (I didn't ;) )

class BucketCreator {
public:
    BucketCreator();

~BucketCreator(void);

void init(int resX, int resY, int bucketSize);

Bucket* getNextBucket(){

Bucket* bucket=NULL;
//enter critical section
CSingleLock singleLock(&m_criticalSection);
singleLock.Lock();

int height = min(m_resolutionY-m_nextY,m_bucketSize);
int width = min(m_resolutionX-m_nextX,m_bucketSize);

bucket = new Bucket(width, height);

//leave critical section
singleLock.Unlock();
return bucket;
}

private:

int m_resolutionX;
int m_resolutionY;
int m_bucketSize;

int m_nextX;
int m_nextY;

//multithreading:
CCriticalSection m_criticalSection;
};

and class Bucket:

class Bucket : public CObject{
DECLARE_DYNAMIC(RenderBucket)
public:

Bucket(int a_resX, int a_resY){

resX = a_resX;
resY = a_resY;
buffer = new float[3 * resX * resY];

int buffersize = 3*resX * resY; 
for (int i=0; i<buffersize; i++){
    buffer[i] = 0;
}
}

~Bucket(void){
delete[] buffer;
buffer=NULL;
}


int getResX(){return resX;}
int getResY(){return resY;}
float* getBuffer(){return buffer;}

private:
int resX;
int resY;
float* buffer;

Bucket& operator = (const Bucket& other) { /*..*/}
Bucket(const Bucket& other) {/*..*/}
};

Can anyone tell me what could be the problem here?

edit: this is the other static function I'm calling from the threads. Is this safe to do?

static std::vector<Vector3> generate_poisson(double width, double height, double min_dist, int k, std::vector<std::vector<Vector3> > existingPoints)
{
    CSingleLock singleLock(&m_criticalSection);
    singleLock.Lock();

    std::vector<Vector3> samplePoints = std::vector<Vector3>();

            ...fill the vector...

            singleLock.Unlock();
            return samplePoints;
     }

Answer 1

You haven't made a private copy constructor, or any default constructor. If class Bucket is constructed via one of these implicitly-defined methods, buffer will either be uninitialized, or it will be a copied pointer made by a copy constructor.

The copy constructor for class Bucket is Bucket(const Bucket &B) -- if you do not explicitly declare a copy constructor, the compiler will generate a "naive" copy constructor for you.

In particular, if this object is assigned, returned, or otherwise copied, the copy constructor will copy the pointer to a new object. Eventually, both objects' destructors will attempt to delete[] the same pointer and the second attempt will be a double deletion, a type of heap corruption.

I recommend you make class Bucket's copy constructor private, which will cause attempted copy construction to generate a compile error. As an alternative, you could implement a copy constructor which allocates new space for the copied buffer.

Exactly the same applies to the assignment operator, operator=.

The need for a copy constructor is one of the 55 tips in Scott Meyer's excellent book, Effective C++: 55 Specific Ways to Improve Your Programs and Designs:

This book should be required reading for all C++ programmers.

If you add:

class Bucket {
/* Existing code as-is ... */
private:
    Bucket() { buffer = NULL; }    // No default construction
    Bucket(const Bucket &B) { ; }  // No copy construction
    Bucket& operator= (const Bucket &B) {;}  // No assignment
}

and re-compile, you are likely to find your problem.

There is also another possibility: If your code contains other uses of new and delete, then it is possible these other uses of allocated memory are corrupting the linked-list structure which defines the heap memory. It is common to detect this corruption during a call to delete, because delete must utilize these data structures.

Answer 2

All the previous replies are sound. For the copy constructor, make sure that it doesn't just copy the buffer pointer, otherwise that will cause the problem. It needs to allocate a new buffer, not the pointer value, which would cause an error in 'delete'. But I don't get the impression that the copy contructor will get called in your code.

I've looked at the code and I am not seeing any error in it as is. Note that the thread synchronization isn't even necessary in this GetNextBucket code, since it's returning a local variable and those are pre-thread.

Errors in ValidateHeapPointer occur because something has corrupted the heap, which happens when a pointer writes past a block of memory. Often it's a for() loop that goes too far, a buffer that wasn't allocated large enough, etc.

The error is reported during a call to 'delete' because that's when the heap is validated for bugs in debug mode. However, the error has occurred before that time, it just happens that the heap is checked only in 'new' and 'delete'. Also, it isn't necessarily related to the 'Bucket' class.

What you need to need to find this bug, short of using tools like BoundsChecker or HeapValidator, is comment out sections of your code until it goes away, and then you'll find the offending code.

There is another method to narrow down the problem. In debug mode, include in your code, and sprinkle calls to _CrtCheckMemory() at various points of interest. That will generate the error when the heap is corrupted. Simply move the calls in your code to narrow down at what point the corruption begins to occur.

I don't know which version of Visual C++ you are using. If you're using a earlier one like VC++ 6.0, make sure that you are using the Multitreaded DLL version of the C Run Time Library in the compiler option.

Answer 3

You're constructing a RenderBucket. Are you sure you're calling the 'Bucket' class's constructor from there? It should look like this:

class RenderBucket : public Bucket {
  RenderBucket( int a_resX, int a_resY )
    : Bucket( a_resX, a_resY )
  {
  }
}

Initializers in the Bucket class to set the buffer to NULL is a good idea... Also making the Default constructor and copy constructor private will help to make double sure those aren't being used. Remember.. the compiler will create these automatically if you don't:

Bucket();  <-- default constructor
Bucket( int a_resx = 0, int a_resy = 0 )  <-- Another way to make your default constructor
Bucket(const class Bucket &B)  <-- copy constructor