Error with delete[]

The image didn''t show up.

I''m taking a wild guess and saying you instantiated one of these objects in debug mode and tried to delete rubbish.

oh, the screen did popup in the edit window, and I was right Its VC++ debug mode, prefills arrays with rubbish to help you find mistakes (see VC docs).

Try this:
CBitmap() { bmpBuffer = NULL; }
~CBitmap() { if ( bmpBuffer != NULL ) delete[] bmpBuffer; }

I recommend the mmgr library, which can be found on flipcode for tracking this stuff - its more thorough than VC++''s debug mode and its portable.

~~~
Cheers!
Brett Porter
PortaLib3D : A portable 3D game/demo libary for OpenGL
Community Service Announcement: Read How to ask questions the smart way before posting!

Thanks for responding so quickly!

Hm... I tried that, but the same popup still came up. Any other ideas?

Chances are that you:

1. change bmpBuffer somewhere else
OR
2. sometimes create CBitmap with another constructor that does not alloicate that array.
OR
3. delete it two times.
OR
4. that you write outside the bounderies of the allocated array thereby messing up the block list and causing the assertion to fail. This is probably the most likely candidate for the error.

Check for all these things.

[edited by - felonius on June 11, 2002 8:13:51 AM]

i''ve come upon this problem several times, most recently it was (as far as I understood) not caused directly by freeing the memory (I had: if(NULL!=Icon) delete Icon, but because something else. The same thing happened with a simple int* blah. I fixed the problem by not calling that piece of code twice, EVEN THOUGH it shouldn''t have broken.

I''m positive this is what''s happening with ur code too, not the twice-deletion, but the pointer being deleted.

I realise i''m talking jibberish, but I don''t know the answer to this.

That is correct. If the heap is already corrupted due to for example writing too far into an array, a delete could certainly crash.

zilch_

What you say makes sense. I''ll try it when I get home from work.

Thanks again!

If I recall correctly, the delete operator has a built-in security to make sure it doesn''t delete NULL pointers, so

if (ptr != NULL)
delete [] ptr;


is pretty much the same as:
delete [] ptr;

What I do think is going wrong is that you are deleting the object twice, so that the destructor gets called twice, and since your ptr still points to the (already deleted) old object, it is not NULL and it tries to delete it there. I suggest you do the following:



And of course check anywhere where you might delete the object twice. This might occur when you delete an object created on the stack:




Hope this helps,

Dormeur

You mean that local objects are automatically destroyed when they go out of scope? Meaning, I don''t have to delete a local object when I''m done with it?

I thought C++ didn''t have such sophisticated garbage collection. Please correct me if I''m mistaken.

quote:

---

Original post by Teric
You mean that local objects are automatically destroyed when they go out of scope? Meaning, I don''t have to delete a local object when I''m done with it?

---

That is correct; only objects allocated with "new" must be explicitly deallocated by the programmer.

Multiple deletes are almost always caused by a bad copy constructor or assignment operator. Try putting CBitmap(const CBitmap &bm) and CBitmap & operator = (const CBitmap &bm) in the private section of your class. If your program refuses to compile then this is your problem and you have three options to fix it.

1.) Stop copying CBitmaps, use const &''s This is the most restrictive, but fastest.

2.) Use boost''s shared_array. The only restriction here is that CBitmaps cannot refer to other CBitmaps, if doing so would create a cycle. This is probably what I would do.

3.) Deep copy the bitmap. This is slow, but the most general.

CBitmap(const CBitmap &bm) : size(bm.size) {
    bmpBuffer = new UCHAR[size];
    memcpy(bmpBuffer,bm.bmpBuffer,size);
}

CBitmap & operator = (const CBitmap &bm) {
    size = bm.size;
    delete [] bmpBuffer;
    bmpBuffer = new UCHAR[size];
    memcpy(bmpBuffer,bm.bmpBuffer,size);
}
 

Hm... I tried what you said but I'm still getting the same popup. No, I'm not trying to copy the CBitMap object; I'm simply trying to allocate memory for bmpBuffer and then deallocate that memory in the destructor. I'll attach my whole class object so that maybe someone could see what I'm doing wrong:

EDIT: GRRRRR, source tags aren't working right.

Class Header:


// BitMap.h: interface for the CBitMap class.
//
//////////////////////////////////////////////////////////////////////

#ifndef __BITMAP_H__
#define __BITMAP_H__

#include <windows.h>
#include <windowsx.h>
#include <mmsystem.h>
#include <stdio.h>
#include <io.h>
#include <ddraw.h>

#include "DDrawMain.h"

#define BITMAP_ID 0x4D42 // universal id for a bitmap
#define MAX_PALETTE_COLORS 256
#define _RGB32BIT(a,r,g,b) (b + ((g)<<8) + ((r)<<16) + ((a) << 24))

extern const int SCREEN_WIDTH;
extern const int SCREEN_HEIGHT;
extern const int SCREEN_BPP;

class CBitMap
{
private:
BITMAPFILEHEADER bmpFileHeader; //File header
BITMAPINFOHEADER bmpInfoHeader; //Bitmap info including the palette
PALETTEENTRY bmpPalette[256]; //To store the palette (if any)
UCHAR *bmpBuffer; //Actual bitmap data

int FlipBitmap(); //Flip Bitmaps that are upside-down

public:
CBitMap();
~CBitMap();
int LoadBMPFile(char *filename);
int Draw(RECT SourceRect, LPDIRECTDRAWSURFACE7 Dest);
};

#endif


Class Definition:

                
// BitMap.cpp: implementation of the CBitMap class.
//
//////////////////////////////////////////////////////////////////////

#include "CBitMap.h"

void WriteLog(char*);

//////////////////////////////////////////////////////////////////////
// Construction/Destruction
//////////////////////////////////////////////////////////////////////

CBitMap::CBitMap()
{
}//CBitMap

CBitMap::~CBitMap()
{
  //Free memory
  if(bmpBuffer != NULL)
  {
    delete[] bmpBuffer;
    bmpBuffer = NULL;
  }
}//~CBitMap

//LoadBMPFile///////////////////////////////////////////////////

int CBitMap::LoadBMPFile(char *filename)
{
  int fhandle;        //Handle to the bitmap file on disk
  int i;
  OFSTRUCT file_data; //File data information

  //Load a bitmap file from disk and separate it into its component structures

  //First open the file for reading
  fhandle = OpenFile(filename, &file_data, OF_READ);
  if (fhandle==-1)
    return 0;

  //Load the bitmap file header
  _lread(fhandle, &bmpFileHeader, sizeof(BITMAPFILEHEADER));

  //Make sure its a valid bitmap file
  if (bmpFileHeader.bfType != BITMAP_ID)
  {
    //Close the file and return failure
    _lclose(fhandle);
    return 0;
  }

  //Load the bitmap info header
  _lread(fhandle, &bmpInfoHeader, sizeof(BITMAPINFOHEADER));

  //Check to see if this is an 8-bit bitmap.  If so, load the palette
  if(bmpInfoHeader.biBitCount == 8)
  {
    //Load palette data from the file
    _lread(fhandle, &bmpPalette, MAX_PALETTE_COLORS * sizeof(PALETTEENTRY));

    //Set all palette flags correctly, and fix the reversed RGBQUAD data format
    //(meaning switch BGR to RGB)
    for (i=0; i    {
      int temp_color = bmpPalette.peBlue;
      bmpPalette[i].peBlue = bmpPalette[i].peRed;
      bmpPalette[i].peRed = temp_color;
      bmpPalette[i].peFlags = PC_NOCOLLAPSE;
    }
  }

  //Move the file pointer to the beginning of the actual image data
  _lseek(fhandle, -(int)(bmpInfoHeader.biSizeImage), SEEK_END);

  //Now read image data
  if(bmpInfoHeader.biBitCount==8 ||
     bmpInfoHeader.biBitCount==16 ||
     bmpInfoHeader.biBitCount==24)
  {
    //Clear any extraneous bmpBuffer image data
    //if(bmpBuffer != NULL)
    //  delete[] bmpBuffer;

    //Allocate memory for the image
    bmpBuffer = new UCHAR[bmpInfoHeader.biSizeImage + 1];
    if(!bmpBuffer || (bmpBuffer = NULL))
    {
      //Close the file handle on a failure
      _lclose(fhandle);
      WriteLog("Failed to allocate bmpBuffer\n");
      return 0;
    }

    //Read it
    _lread(fhandle, bmpBuffer, bmpInfoHeader.biSizeImage);
  }
  else
  {
    //Not an 8, 16, nor 24 bit bitmap
    _lclose(fhandle);
    return 0;
  }

  //Close the file
  _lclose(fhandle);

  //If the bitmap is upside-down, flip it.  Positive biHeight means the
  //bitmap has been stored upside down.
  if (bmpInfoHeader.biHeight >= 0)
    if (FAILED(FlipBitmap()))
      return 0;

  //Return 1 if succeeded
  return 1;

}//LoadBMPFile

//FlipBitmap////////////////////////////////////////////////////

int CBitMap::FlipBitmap()
{
  UCHAR *temp_buffer;   //Temporary buffer
  int bytes_per_line;   //Number of bytes per line in the image
  int height;           //Height of image
  int i;

  // this function is used to flip bottom-up .BMP images

  // allocate the temporary buffer
  bytes_per_line = bmpInfoHeader.biWidth * (bmpInfoHeader.biBitCount / 8);
  height = bmpInfoHeader.biHeight;
  temp_buffer = new UCHAR[bytes_per_line * height];
  if (!(temp_buffer))
     return 0;

  // copy image to work area
  memcpy(temp_buffer, bmpBuffer, bytes_per_line * height);

  // flip vertically
  for (i=0; i < height; i++)
      memcpy(&bmpBuffer[((height-1) - i) * bytes_per_line],
             &temp_buffer[i * bytes_per_line], bytes_per_line);

  // release the memory
  delete[] temp_buffer;

  // return success
  return 1;
  
}//FlipBitmap

//Draw//////////////////////////////////////////////////////////

int CBitMap::Draw(RECT SourceRect, LPDIRECTDRAWSURFACE7 Dest)
{
  DDSURFACEDESC2 ddsd;            //Surface descriptor for destination DDraw surface
  UCHAR *dst, *src;               //Pointers to destination and source buffers
  int *dst32;                     //For a 32-bit DDraw surface destination
  int src_width;                  //Width of source image
  int i, j;                       //Loop indices
  int dstIndex;                   //Index to dst32 array
  UCHAR blue, green, red;         //Used in reading color entries in 24-bit bitmaps
  int RGBColor;                   //Used to write a 32-bit color entry
  
  //Initialize ddsd
  DDRAW_INIT_STRUCT(ddsd);

  //Lock DDraw surface
  if(FAILED(Dest->Lock(NULL, &ddsd, DDLOCK_WAIT, NULL)))
    return 0;

  //Copy bitmap to the surface, depending on what format it's in (8-,16-,or 24-bit)
  switch(bmpInfoHeader.biBitCount)
  {
  case 8:
    {
      //Compute pointer locations and source image width
      dst = (UCHAR*)ddsd.lpSurface;
      src = bmpBuffer + (SourceRect.top * bmpInfoHeader.biHeight);
      src_width = SourceRect.right - SourceRect.left;
      if(src_width > ddsd.lPitch)
        src_width = ddsd.lPitch;

      //Copy image data to the DDraw surface
      for (i = SourceRect.top; i<=SourceRect.bottom; i++)
      {
        memcpy(dst, src, src_width);
        dst+=ddsd.lPitch;
        src+=bmpInfoHeader.biWidth;
      }
      break;
    }

  case 16:
    {
      //Compute pointer locations and source image width
      dst = (UCHAR*)ddsd.lpSurface;
      src = bmpBuffer + (SourceRect.top * bmpInfoHeader.biHeight);
      src_width = (SourceRect.right - SourceRect.left) << 1;
      if(src_width > ddsd.lPitch)
        src_width = ddsd.lPitch;

      //Copy image data to the DDraw surface
      for (i = SourceRect.top; i<=SourceRect.bottom; i++)
      {
        memcpy(dst, src, src_width);
        dst+=ddsd.lPitch;
        src+=(bmpInfoHeader.biWidth << 1);
      }
      break;
    }

  case 24:
    {
      //Compute pointer locations and source image width
      dst32 = (int *)ddsd.lpSurface;
      dstIndex = 0;
      src_width = (SourceRect.right - SourceRect.left + 1) * 3;
      if(src_width > (int)(ddsd.lPitch * 0.75))
        src_width = (int)(ddsd.lPitch * 0.75);

      //Copy image data to the DDraw surface
      for (i = SourceRect.top; i<=SourceRect.bottom; i++)
      {
        for (j=(SourceRect.left * 3); j<=(SourceRect.right * 3); j+=3)
        {
          //Grab the BGR entries and reverse them to RGB
          blue = bmpBuffer[(i * bmpInfoHeader.biWidth * 3) + j];
          green = bmpBuffer[(i * bmpInfoHeader.biWidth * 3) + j + 1];
          red = bmpBuffer[(i * bmpInfoHeader.biWidth * 3) + j + 2];
          RGBColor = _RGB32BIT(0, red, green, blue);

          //Write the color entry to a pixel on the DDraw surface
          dst32[dstIndex] = RGBColor;
          dstIndex++;
        }

        //Advance to the next line
        dst32+=(ddsd.lPitch >> 2);
        dstIndex = 0;
      }
      break;
    }
  }

  //Unlock DDraw surface
  Dest->Unlock(NULL);

  return 1;
}//Draw
   

Again, thanks in advance for your help.

[edited by - Teric on June 12, 2002 6:22:11 AM]

You are not assigning NULL to your pointer in your constructor, thus defeating the point of checking for NULL before your delete, though this is generally not needed. Where it will cause problems is in your loadBMPFile(). Here you have must check against null so that you can delete before allocating a new buffer. You have this check, but have commented it out. The big problems seem to come from this:

 if(!bmpBuffer || (bmpBuffer = NULL))
 

I don''t think this meens what you think it means, what this is actually doing is "if bmpBuffer is not null then set it to zero and evaluate to false". Your compiler should be bitching about this, bump up the warning level. What you almost certainly ment to say is:

 if(!bmpBuffer || (bmpBuffer == NULL))
 

This is redundant, you (well me actually) are saying if not nonzero or equal to zero it''s zero. Pick one.

This leads us to a minor bit of style. Using ''NULL'' in C++ is somewhat discoraged. It is a convention that is used in C to provide a bit of type safety because NULL is almost always ''((void *)0)'' This is because a comparison in C between a pointer type and a binary 0 in essance converts the pointer to an int and then compares. Some weirdo architectures might have no easy way of doing this. In C++ the expression if (ptr==0) or if(!ptr) is required by the language to be eqivilent to "is this a ''NULL'' pointer using whatever wierdo method your sick and twisted architecture finds most efficent" Meanwhile, if(ptr==((void *)0)) is an error if ptr has any other type but void*, and if it is a void*, the the expression could be false when ptr is ''NULL'' and true when you have a perfectly valid pointer. In the real world the only issue is when you mix C and C++ code NULL can break in the C++ code so we spell NULL as 0

Wow, that was long, I am clearly drinking too much coffee.

Yes, I caught the ''bmpBuffer = NULL'' error about 5 minutes after I posted this morning. It was one of those AAARGGH moments where after an hour trying to debug I finally saw it and smacked myself in the head.

However, my original problem still exists--the popup window still comes up when I exit the program. *sigh* back to the drawing board. I wish remote debugging would work properly when using DirectDraw...

I was having pretty much the same problem that you are (same assert failure, different circumstances), and I fixed what was wrong within an hour after trying out Paul Nettles mmgr. Someone mentioned this, I believe, but didn''t give a URL, so here it is (the download link is a little bit down the page):
http://www.flipcode.com/cgi-bin/msg.cgi?showThread=12September2000-PresentingAMemoryManager&forum=askmid&id=-1

It works SO well...
Good luck.

Does bmpInfoHeader.biSizeImage==0?

Gumby--

bmpBuffer != 0

Kranman--

Ok, I''ve downloaded the mmgr files, but they seem somewhat complicated. Can you give me a few pointers on how to get started using them?

Thanks!

Still trying to figure out this problem myself.

Question: Is there a way to tell how large of a memory chunk has been allocated to an array? I''m dumping sizeof(bmpBuffer) out to a log file, but they tell me that bmpBuffer is only 4 bytes. I tried strlen((char*)bmpBuffer), but that returned 1. Is there another way?

quote:

---

Original post by Teric
Still trying to figure out this problem myself.

Question: Is there a way to tell how large of a memory chunk has been allocated to an array? I''m dumping sizeof(bmpBuffer) out to a log file, but they tell me that bmpBuffer is only 4 bytes. I tried strlen((char*)bmpBuffer), but that returned 1. Is there another way?

---

The size of bmpBuffer is 4 bytes(on 32 bit machines), you were thinking of sizeof(*bmpBuffer), which doesn''t work. There are ways to use read either the debug information provided by MSVC, and the number of elements must be stored somewhere for arrays allocated with new[] whose elements have non-trivial destructors. But this will not help you a bit, and is an all around Bad Idea. If new type[size] is not returning a pointer to enough memory for size type''s or returning 0 (MSVC, default) or throwing bad_alloc (everywhere else in the world) then things are far more broken than you have any hope of fixing.

Rather than assuming that your compiler is broken in some very subtle way why not log the construction and destruction of each object and see if you are destroying an object that you never called LoadBMPFile() on? Have you changed your constructor to initalize bmpBuffer to 0? Have you added the priviate copy ctor to see if you are making any shallow copies?

(Insane, Maniacal Laughter) HAAAAHAHAHAHAAA!!

I''ve done it, Igor! It''s alive!!! HAHAHA!!

*ahem*

I figured out the problem. I was passing a CBitMap object directly to the CSprite::Load function (i.e. MySprite.Load(MyBitMap) instead of MySprite.Load(&MyBitMap)). This caused the BitMap object to be treated as a local variable within the scope of the Load function. Thus, when the function completed, it went out of scope and was destroyed (calling the destructor, which would delete[] my bmpBuffer array). Then, at the end of the program, the destructor was called again when the global bitmap object went out of scope, and there''s where it choked.

SO... I altered the definition of CSprite::Load to use a *CBitMap parameter instead of a straight CBitMap parameter. It worked like a charm.

You experienced coders are probably saying "Geez, that was a dumb mistake." Yep. It was dumb. But man, oh, man, did I LEARN from it!

Thanks again to all who posted advice on this thread.