CODEWITHSUNDEEP
c++pointerssdl
defect
defect

Reputation: 558

Pointer Invalidation with std::vector

I'm writing toy C++ code to learn it and game dev, wrapping SDL along the way; all it does right now is load a BMP image and display it on the screen in a loop. Posting only relevant code.

EDIT: I'm revising this post down to the MVCE. The problem can be found much more directly.

#include <SDL.h>
#include <vector>

class Surface {
public:
    Surface(SDL_Surface*);
    ~Surface();
    SDL_Surface* mSurface;
}

Surface::Surface(SDL_Surface* surface) {
    mSurface = surface;
}

Surface::~Surface() {
    SDL_FreeSurface(mSurface);
}

int main(int argc, char* args[])
{
    if (SDL_Init(SDL_INIT_VIDEO) < 0) {
        throw("Failed to SDL_Init: " + std::string(SDL_GetError()));
    }

    SDL_Window* window = SDL_CreateWindow("SDL TEST", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 1280, 960, SDL_WINDOW_SHOWN);
    SDL_Surface* screenSurface = SDL_GetWindowSurface(window);

    const char * imgPath = "test.bmp";
    std::vector<Surface> surfaces;
    surfaces.push_back(Surface(SDL_LoadBMP(imgPath)));
    SDL_BlitSurface(surfaces.begin()->mSurface, // memory access violation here
                            NULL, screenSurface, NULL);
    SDL_UpdateWindowSurface(window);
    SDL_Delay(2000);

    SDL_Quit();
    return 0;
}

What it comes down to is that the pointer inside the Surface object loaded into a vector becomes invalidated. I read around and thought that maybe it had to do with needing a custom copy constructor, but making one like this didn't solve it:

Surface::Surface(const Surface& other) {
    mSurface = other.mSurface;
    mIsWindowSurface = other.mIsWindowSurface;
}

Upvotes: 1

Views: 253

Answers (1)

M.M
M.M

Reputation: 141628

You didn't define a copy-constructor, so the default copy constructor is used and this doesn't do what you want.

In the line

 surfaces.push_back(Surface(SDL_LoadBMP(imgPath)));

here is what happens:

  1. Create a temporary Surface whose mSurface pointers to the newly allocated surface
  2. Copy this Surface into the vector using the copy-constructor (or copy-assignment operator)
  3. Destruct the temporary Surface, which calls SDL_FreeSurface.

Then when you go onto do surfaces.begin()->mSurface,, you are trying to work with a surface that has already been freed.

To fix this prior to C++11 you will need to make your copy-constructor and copy-assignment operator semantically make a copy, or use a reference-count mechanism to ensure that SDL_FreeSurface is only called once there are no more active pointers to the surface.

In C++11 there are a lot more out-of-the-box fixes; one of them is to disable copying for your class, and implement a move-constructor and move-assignment operator, and/or use emplace_back( SDL_LoadBMP... instead of push_back.

Upvotes: 1

Related Questions