Getting error reading from 0xfefefefe when destructor is called

Question

I have made a c++ wrapper for an allegro bitmap. I create an AguiBitmap as a global variable for testing, then later I say,

bitmap = AguiBitmap("somepath");

after allegro has been initialized.

However, when I close the application, it crashes in the bitmap's destructor. If I do al_destroy_bitmap(0); its fine, but there cant be anything wrong with my bitmap pointer because I use it to render.

AguiBitmap::~AguiBitmap()
{
        al_destroy_bitmap(nativeBitmapPtr); 
}

AguiBitmap::AguiBitmap()
{
    nativeBitmapPtr = 0;
    width = 0;
    height = 0;
}

AguiBitmap::AguiBitmap( char *filename )
{

    if(!filename)
    {
        nativeBitmapPtr = 0;
        return;
    }

    nativeBitmapPtr = al_load_bitmap(filename);

    if(nativeBitmapPtr)
    {

        width = al_get_bitmap_width(nativeBitmapPtr);
        height = al_get_bitmap_height(nativeBitmapPtr);
    }
    else
    {
        width = 0;
        height = 0;
    }
}

AguiBitmap::AguiBitmap( std::string filename )
{
    AguiBitmap((char*)filename.c_str());
}

ALLEGRO_BITMAP* AguiBitmap::getBitmap() const
{
    return nativeBitmapPtr;
}

int AguiBitmap::getWidth() const
{
    return width;
}

int AguiBitmap::getHeight() const
{
    return height;
}

Thanks

André Caron · Accepted Answer · 2010-10-20T05:31:52.080

3

You have not defined a copy constructor (edit: or copy-assignment operator).

bitmap = AguiBitmap("somepath");

That line constructs a temporary AguiBitmap which allocates the bitmap, then assigns to bitmap variable and the temporary is destroyed, releasing the bitmap. Therefore, any use of bitmap after this line is invalid!

Edit: specifically, when bitmap goes out of scope, the destructor is invoked for the 2nd object and the same bitmap is deleted again.

edited Oct 20 '10 at 05:31

answered Oct 20 '10 at 04:02

André Caron

44,541
12
67
125

how should I make my copy constructor? – jmasterx Oct 20 '10 at 04:03
1

@Milo: [A good introductory C++ book](http://stackoverflow.com/questions/388242/the-definitive-c++-book-guide-and-list) will explain proper C++ memory management, object lifetimes, and issues related to implementing resource-owning containers. Specifically, I'd recommend reading Herb Sutter's _Exceptional C++_. Items 8-19 and 35-41 cover this subject in depth. – James McNellis Oct 20 '10 at 04:05
1

That line invokes the copy assignment operator, not the copy constructor. – James McNellis Oct 20 '10 at 04:10
How would I create my copy constructor for this? Thanks – jmasterx Oct 20 '10 at 04:12
@Milo: in this case, I would personally go for a non-copyable class. Declare the copy constructor and copy assignment operators as private and don't defined them. It will prevent the kind of mistake that occured here. You can read more on this idiom: http://dev-faqs.blogspot.com/2010/07/c-idioms-non-copyable.html – André Caron Oct 20 '10 at 05:34

score 2 · Answer 2 · answered Oct 20 '10 at 04:00

If you are using Visual C++, the Windows debug heap overwrites freed memory with the 0xfe memory pattern to help you to debug memory issues.

You've freed something and either (a) you are trying to access that thing or (b) you are trying to free that thing again.

Since your class manually manages resources, and since you don't show the copy constructor and copy assignment operator implementations, make sure you are following the rule of three.

score 1 · Answer 3 · answered Oct 20 '10 at 04:01

Is Allegro being shut down in main? If so, then when the global destructs after main, I suspect al_destroy_bitmap should fail.

And like James says, you're breaking the rule of three; this probably plays a role. This bitmap = AguiBitmap("somepath"); will call al_destroy_bitmap(nativeBitmapPtr); when the temporary dies; does this leave bitmap with a bad pointer?

Aside from the question, why do you have a char* overload of the constructor? That's nasty. The C-style cast is acting like a const_cast, that should be a hint something is wrong. std::string can initialize from literals anyway, so I'm not sure why you even have two. Just have one, and don't cast any const's away.

Getting error reading from 0xfefefefe when destructor is called

3 Answers3