MFC multithreading problem with delete[] , dbgheap.c

Question

Hi!

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

I made my application multithreaded 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? thanks a lot!

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'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

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.