Custom copy assignment operator crashes the program (C++)

Question

I'm creating a custom class for automaticly cleaning up memory. The class contains a pointer to an SDL_Surface object and calls it's refcount when needed. The problem is when I implement the custom copy assigment operator the system crashes with the following code:

"Unhandled exception at 0x771a15de in xyz.exe: 0xC0000005: Access violation reading location 0xcccccd04."

and the object attribute "address" suddenly gets the value "0x0000ffff ", whilst using the default copy assignment operator it runs perfectly fine.

I strongly suggest you make use of `std::shared_ptr` instead of doing reference counting manually. — jrok, Jan 08 '13 at 20:10
Look into the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) for an easy way to get correct behavior. jrok is right though: `shared_ptr` with custom deleter would solve this issue even better. — GManNickG, Jan 08 '13 at 20:11
`SDL_FreeSurface` will decrement the refcount, and when called on a null pointer, is a no-op. So there is no need, (in your destructor or your assignment operator), to check for NULL, decrement the refcount, or compare the refcount to 0. Just call `SDL_FreeSurface`. See the second to last example of this page: http://sdl.beuc.net/sdl.wiki/SDL_Surface — Benjamin Lindley, Jan 08 '13 at 20:19
Actually, come to think of it, that's probably the source of your error. You're decrementing the refcount, then calling SDL_FreeSurface, which decrements it again. — Benjamin Lindley, Jan 08 '13 at 20:27
@BenjaminLindley: I think you should make your comments into an answer. — Fred Larson, Jan 08 '13 at 20:31

score 2 · Accepted Answer · answered Jan 08 '13 at 20:34

You're using the refcount incorrectly. SDL_FreeSurface will decrement the refcount, and when called on a null pointer, is a no-op. So, your assignment operator should look like this:

const Image& Image::operator=(const Image& other){
    if (img != other.img){
        SDL_FreeSurface(img);
        img = other.img;
        if (img != NULL)
            img->refcount++;
    }
    return *this;
}

And your destructor should look like this:

Image::~Image(){
    SDL_FreeSurface(img);
}

Also, in your default constructor, you should initialize img to a null pointer.

Image::Image() :img(NULL) {}

See the second to last example on this page: http://sdl.beuc.net/sdl.wiki/SDL_Surface

Thank you Benjamin, I should have noticed that! It now works like a charm! — Mix, Jan 09 '13 at 11:16

Custom copy assignment operator crashes the program (C++)

1 Answers1