• Advertisement
Sign in to follow this  

Why is delete [] causing heap corruption?

This topic is 4180 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I can't for the life of me figure this error out and it's driving me crazy, so hopefully someone on here can help me out.
// allocate a temporary buffer
int bufferSize = bitmapPtr->infoHeader.biSizeImage / 4;
unsigned int *tempBuffer = new unsigned int[ bufferSize ];

// read in the buffer, bitmapFile is an ifstream
bitmapFile.read( (char *)tempBuffer, bitmapPtr->infoHeader.biSizeImage );

// this doesn't work
delete [] tempBuffer;
When that code runs, I get this error: HEAP CORRUPTION DETECTED: after Normal block(#134) at 0x00386CA0. CRT detected that the application wrote to memory after end of heap buffer. But I don't get the error with this code...
// allocate a temporary buffer
int bufferSize = bitmapPtr->infoHeader.biSizeImage / 4;
unsigned int *tempBuffer = new unsigned int[ bufferSize ];

// this does work, why!?
delete [] tempBuffer;

// read in the buffer, bitmapFile is an ifstream
bitmapFile.read( (char *)tempBuffer, bitmapPtr->infoHeader.biSizeImage );
What is happening to tempBuffer during the bitmapFile.read to cause it have heap curruption if I try to delete it after that line? Am I'm using the pointer wrong by casting it to a char pointer? The problem isn't directy in this part of the code, but I added the delete there to figure out where my program was messing up. The real problem happens later when I try to delete the memory that I allocated here and I get the crazy heap corruption business :( I figure this little snippet of code is basically the problem. I'm using VS2005, not sure if that helps or not or if I'm missing some other information, but if I forgot something important, tell me and I'll let you know.

Share this post


Link to post
Share on other sites
Advertisement
You a creating a buffer of size bitmapPtr->infoHeader.biSizeImage / 4 bytes.
You are then trying to read in bitmapPtr->infoHeader.biSizeImage bytes from the stream, causing it to write over the end of the buffer, and corrupting your heap.

Solution:
Allocate a buffer of size bitmapPtr->infoHeader.biSizeImage instead.

Share this post


Link to post
Share on other sites
Quote:
Original post by mattd
You a creating a buffer of size bitmapPtr->infoHeader.biSizeImage / 4 bytes.
You are then trying to read in bitmapPtr->infoHeader.biSizeImage bytes from the stream, causing it to write over the end of the buffer, and corrupting your heap.

Solution:
Allocate a buffer of size bitmapPtr->infoHeader.biSizeImage instead.

I'm reading in bitmapPtr->infoHeader.biSizeImage and not bitmapPtr->infoHeader.biSizeImage/4 because the it's a char* in bitmapFile.read(), instead of an unsigned int*. So isn't that what I'm supposed to do?

Share this post


Link to post
Share on other sites
Quote:
Original post by sammyjojo
Quote:
Original post by mattd
You a creating a buffer of size bitmapPtr->infoHeader.biSizeImage / 4 bytes.
You are then trying to read in bitmapPtr->infoHeader.biSizeImage bytes from the stream, causing it to write over the end of the buffer, and corrupting your heap.

Solution:
Allocate a buffer of size bitmapPtr->infoHeader.biSizeImage instead.

I'm reading in bitmapPtr->infoHeader.biSizeImage and not bitmapPtr->infoHeader.biSizeImage/4 because the it's a char* in bitmapFile.read(), instead of an unsigned int*. So isn't that what I'm supposed to do?

Oops, sorry, you're correct :)

Are you sure that bitmapPtr->infoHeader.biSizeImage is unsigned int aligned, ie, a multiple of 4? If it wasn't, the read call could be trying to write an extra 1-3 bytes that are unaccounted for by the integer division by 4.

BMPs are meant to have a DWORD-aligned pitch, but some BMP writers don't do this.

Share this post


Link to post
Share on other sites
Quote:
You a creating a buffer of size bitmapPtr->infoHeader.biSizeImage / 4 bytes


Wrong. He's creating biSizeImage/4 ints -- each int is 4 bytes, so it multiplies out, unless biSizeImage is not divisible by 4. You could add one to the number of ints you create to make really sure.

In debug builds, there's a function available called _CrtCheckMemory(), which will stop with a breakpoint if something in the heap is corrupted. It's not super fast, but it's thorough. You might want to scatter calls to _CrtCheckMemory() around your source, so that you can catch problems sooner rather than later -- ideally, call it right before you allocate or delete any memory, plus after every line in pieces of code where you suspect problems.

If you don't have the header, you can just forward declare it (assuming MSVC here):

extern "C" int _CrtCheckMemory();

More expensive tools, such as BoundsChecker or Purify, will catch all your memory writes (and reads) and validate that they are always within a valid block. They do cost money (but may have demo versions).

Share this post


Link to post
Share on other sites
Quote:
Original post by Fruny
Why don't you directly do new char[whatever] ?

I could just have the pointer be char* instead of unsigned int*, but I'm just wondering what I'm doing wrong. In my mind the code that I posted that doesn't work... should work, but it for some reason it doesn't and I don't understand why. The main reason I'm using unsigned ints is because I'm working with a 32-bit pixels.

Share this post


Link to post
Share on other sites
Compare the address at which the corruption happens with the address of the beginning and the end of the array (i.e; print them out). That might give us a hint.

Share this post


Link to post
Share on other sites
This is probably not that important, but for portability sake you should change the code like this:

// allocate a temporary buffer
int bufferSize = bitmapPtr->infoHeader.biSizeImage / sizeof(unsigned int);// <= not 4
unsigned int *tempBuffer = new unsigned int[ bufferSize ];

// read in the buffer, bitmapFile is an ifstream
bitmapFile.read( (char *)tempBuffer, bitmapPtr->infoHeader.biSizeImage );

// this doesn't work
delete [] tempBuffer;

Share this post


Link to post
Share on other sites
I can see two possible issues with the original code (besides the portability issue noted elsewhere):
  • As already mentioned, bitmapPtr->infoHeader.biSizeImage may not be a multiple of four, in which case the division truncates and (bitmapPtr->infoHeader.biSizeImage / 4) * 4 < bitmapPtr->infoHeader.biSizeImage. To test for this print out the value of bitmapPtr->infoHeader.biSizeImage.

  • bitmapPtr could be a dangling pointer such that bitmapPtr->infoHeader.biSizeImage happens to point to the same memory location as tempBuffer and so gets overwritten. To test for this print out the addresses of bitmapPtr->infoHeader.biSizeImage and tempBuffer.
In addition you should use an RAII wrapper like std::vector or boost::scoped_array to ensure exception safety, even in the face of future changes.

Σnigma

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Paulius Maruska
This is probably not that important, but for portability sake you should change the code like this:

// allocate a temporary buffer
int bufferSize = bitmapPtr->infoHeader.biSizeImage / sizeof(unsigned int);// <= not 4
unsigned int *tempBuffer = new unsigned int[ bufferSize ];

// read in the buffer, bitmapFile is an ifstream
bitmapFile.read( (char *)tempBuffer, bitmapPtr->infoHeader.biSizeImage );

// this doesn't work
delete [] tempBuffer;


That won't really make it more portable... if sizeof(unsigned int) isn't 4, your version and the original version will probably work equally bad, assuming the allocated buffer is actually used for something.

Share this post


Link to post
Share on other sites
Okay, so I tested the size of bitmapPtr->infoHeader.biSizeImage and it's 25602 bytes, which is not divisble by 4. It winds up being 6400.5, which I guess makes it 6400 in the allocation line. Then in the read line it reading 6400.5, which I guess is a problem. Would this cause heap corruption?

In this part of the code I'm reading in a 32-bit bitmap's pixels, so I was assuming the size of the image should be divisible by 4, which is not. Should I just treat the tempBuffer as a char* to avoid this problem?

Here's the function code so you can see what's going on


struct Bitmap32
{
BITMAPFILEHEADER fileHeader;
BITMAPINFOHEADER infoHeader;
unsigned int *buffer;
};

int MyLoadBitmap( char *fileName, Bitmap32 *bitmapPtr )
{
// - loads a bitmap with the specified filename and saves the image in a 32-bit buffer
// R [1] success
// R [0] failure

ifstream bitmapFile;

bitmapFile.open( fileName, ios::binary | ios::in );
if( bitmapFile.is_open() )
{
// read in the file header
bitmapFile.read( (char *)&(bitmapPtr->fileHeader), sizeof(BITMAPFILEHEADER) );

// make sure it's a bitmap file
if( bitmapPtr->fileHeader.bfType != 0x4D42 )
{
bitmapFile.close();
return 0; // file was not a bitmap
}

// read in the info header
bitmapFile.read( (char *)&(bitmapPtr->infoHeader), sizeof(BITMAPINFOHEADER) );

// make sure the bitmap is 32-bit
if( bitmapPtr->infoHeader.biBitCount != 32 )
{
bitmapFile.close();
return 0; // bitmap was not 32-bit
}

// allocate a temporary buffer
int bufferSize = bitmapPtr->infoHeader.biSizeImage / 4;
unsigned int *tempBuffer = new unsigned int[ bufferSize ];

// read in the buffer
bitmapFile.read( (char *)tempBuffer, bitmapPtr->infoHeader.biSizeImage );

delete [] tempBuffer; // this is just to test the delete problem

// flip the buffer because windows stores the colors are BGRA and opengl wants RGBA

unsigned char red = 0;
unsigned char green = 0;
unsigned char blue = 0;
unsigned char alpha = 0;

unsigned int *curPos = tempBuffer;

for( int i = 0; i < bufferSize; i++ )
{
blue = *curPos;
green = *curPos >> 8;
red = *curPos >> 16;
alpha = *curPos >> 24;

*curPos = ((alpha << 24) + ((blue) << 16) + ((green) << 8) + (red));
curPos++;
}

// delete any previous bitmap buffer data
if( bitmapPtr->buffer != NULL )
{
delete [] bitmapPtr->buffer;
bitmapPtr->buffer = NULL;
}

// assign the buffer to point to the new buffer
bitmapPtr->buffer = tempBuffer;

return 1;
}
else
return 0; // file could not be opened
} // end of MyLoadBitmap

Share this post


Link to post
Share on other sites
That's the point; you end up not allocating those last two bytes because the .5 gets truncated. Allocate char's instead of doing weird division stuff; that way, you don't depend on the size of an int being 4 and only your data being aligned to 4 bytes (which it isn't).

Share this post


Link to post
Share on other sites
Additionally the bitmap format is stored as BGR, not BGRA, so it's only three bytes per pixel. Note that each scanline is padded to a multiple of four bytes though.

Σnigma

[Edited by - Enigma on August 12, 2006 1:38:24 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma
Additionally the bitmap format is stored as BGR, not BGRA, so it's only three bytes per pixel. Note that each scanline is padded to a multiple of four bytes though.

Since it's a 32-bit bitmap wouldn't that make it BGRA because the alpha channel needs to be stored in there too, right?

Share this post


Link to post
Share on other sites
Yes, you're right. My apologies. It's so long since I've dealt with anything other than 24-bit bitmaps I'd forgotten that 32-bit versions actually exist. However, since your size is not a multiple of four there is something wrong. Do you actually have an uncompressed bitmap? Try printing out the biCompression member of your BITMAPINFOHEADER. If it's not zero then you're dealing with a compressed bitmap. If it is zero then something is seriously wrong!

EDIT: Technically there's still no alpha channel in a 32-bit bitmap though. According to MSDN the high order byte of each pixel DWORD is simply unused.

Σnigma

Share this post


Link to post
Share on other sites
Well I switched tempBuffer from unsigned int* to unsigned char* and modified everything else accordingly and it works now! No more heap corruption :)

I really need to not assume things while programming because it looks like that's what got me trouble this time. As you probably already know, I was reading in more then I allocated memory for.

Enigma, my bitmap is uncompressed according to biCompression which is zero, so the byte size of the image should have been divisble by 4, but I was wrong to assume so. I honestly don't know why my image has 2 extra bytes, because it's 80x80 and 32-bit so it should take up 25600 bytes, but instead it takes up 25602 according to biSizeImage in my BITMAPINFOHEADER.

Well, thanks for the help everyone. These forums are always a great help :)

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by sammyjojo
I really need to not assume things while programming because it looks like that's what got me trouble this time. As you probably already know, I was reading in more then I allocated memory for.


Seeing as you're on the "anti-assuming-track", don't you think it might be a good idea to not trust the bitmap for the allocation size at all? The file might be corrupt, you know - giving completely insane sizes...

Trusting arbitrary input that way never leads to anything good. As an additional stability measure, how about adding some restrictions or sanity checks? The simplest sanity check might be to never allocate anything above a given size of your own choice. Exact numbers would depend on your usage patterns in that case. A more elaborate sanity check might be to compare the file size to the size of the pixel data given in the file, and see it it seems reasonable...

Share this post


Link to post
Share on other sites
Quote:
Original post by sammyjojo
I honestly don't know why my image has 2 extra bytes, because it's 80x80 and 32-bit so it should take up 25600 bytes, but instead it takes up 25602 according to biSizeImage in my BITMAPINFOHEADER.


- Where did the file come from?
- Does the value you're getting check with the file size (accounting for header)?

Share this post


Link to post
Share on other sites
Does this work?

// allocate a temporary buffer
int bufferSize = (bitmapPtr->infoHeader.biSizeImage + 3) >> 2;
unsigned int* tempBuffer = new unsigned int[ bufferSize ];

// read in the buffer, bitmapFile is an ifstream
bitmapFile.read( (char *)tempBuffer, bitmapPtr->infoHeader.biSizeImage );

// this doesn't work
delete [] tempBuffer;

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Well obviously I'm here a bit late but:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/bitmaps_1rw2.asp

shows that biSizeImage may be set to zero for uncompressed bitmaps so I wouldn't trust this field at all. The image size should be calculated from required fields instead.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by RDragon1
vector<char> buffer( size );


Yay. Why did no one think of that before.

I swear, what ever the question is, someone always will always post that the answer is std::vector.

I think it's time to formulate this observation into a law.

Anonymous Cowards law: As the length of the thread approaches infinity*, the probability that someone will mention std::vector as the solution approaches 1.


*By infinity I guess I mean page 2-ish...

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement