double free()??

[arathalion]

What causes the following that isn't a double free()?

*** glibc detected *** ./a.out: double free or corruption (out): 0x080887a8 ***
======= Backtrace: =========

* code is at the bottom of my post

I allocated an array of class screen with new[] (ln: 224, inside int tmpProg::setupStates())
I then create an SDL_Surface with SDL_CreateRGBSurface() (ln: 315, inside int screen::createWindow(SData &d))

When I call delete[] (ln: 159, inside tmpProg::~tmpProg()) it causes my error. Stepping through with gdb and writing some of those fprintf lines identified that the problem was caused when calling SDL_FreeSurface() on the Surface created as mentioned above (ln: 281, inside screen::~screen()).
It happens when freeing the last screen in the array, which holds position ZERO (apparently delete[] frees backwards through the array), which is the only screen to be assigned a background IMG SDL_Surface.

Having no idea what was causing the problem I went and compiled Valgrind, hoping that it would help me find and identify my problem, however when I ran my program through Valgrind it exited without error.

as such I wrote a quick test:

#include <stdlib.h>

int main()
{
    void *ptr = malloc(1);
    free(ptr);
    free(ptr);

    return 0;
}

and discovered that when running this through valgrind it erred:

==9773== Invalid free() / delete / delete[]

I am lost as to what is causing my little problem. I can't see anywhere that I could have freed that surface, and printing it in gdb right before calling SDL_FreeSurface() prints information that makes me think it is still there. Perhaps someone will be able to point out what I'm doing incorrectly. Thanks in advance for any help with this reasonably sized post.

#include <SDL/SDL.h>
#include "SDL/SDL_image.h"
#include <cstdio>
        

//global functions  
SDL_Surface* loadIMG(char *filename)
{           
    SDL_Surface* img;
    if (!(img = IMG_Load(filename)))
    {       
        char *SDLerr= IMG_GetError();
        size_t errlen = 1 + strlen(SDLerr);
        char errstr[errlen];
        snprintf(errstr, errlen, "%s\n", SDLerr);
        //free(SDLerr);
        throw errstr;
        fprintf(stderr, errstr);
        return NULL;    
    }   
    else
        return img;
}

//data type for screen data
typedef struct {
    int w;
    int h;
    int bpp; 
    int fps;
    int running;
    char *title;
} SData;
    

/*
 * class screen
 */
    
class screen
{   
    public:
        screen(); 
        ~screen();
        
        int createWindow(SData &d);
        int setBackground(SDL_Surface* bg);

        SDL_Surface* render(); //function for "rendering"  the screen to a single SDL_Surface then returning a pointer to it. 

    private:
        SDL_Surface *sCurrent;
        SDL_Surface *sbg;
};  
        
        
/*  
 * class tmpProg
 */     
class tmpProg
{   
    public: 
        tmpProg(); 
        ~tmpProg();
        
        int run();
        
    private:
        SData sdata; //for holding screen dimensions

        SDL_Surface *sScreen; // Surface for the Screen

        enum states {
            ZERO,
            ONE,
            TWO,
            THREE,
            FOUR,
            FIVE,
            SIX     };

        screen* state;
        screen* cState;
        states enumState;

        int init();     //for initiating the states
        int loadImg();
        int setupStates();

};


/*
 * main()
 */
int main(int argc, char** args)
{
    tmpProg *prog = new tmpProg();
    prog->run();
    delete prog;
    return 0;
}

tmpProg::tmpProg()
{
    sdata.w = 800;
    sdata.h = 600;
    sdata.bpp = 32;
    sdata.fps = 5;
    sdata.running = 0;
    sdata.title=strdup("tmpProg");
    sScreen = NULL;
    state = NULL;
    cState = NULL;

    /*
     * init SDL
     */
    printf("Initialising SDL\n");

    if (SDL_Init( SDL_INIT_EVERYTHING) <0)
    {
        printf("Failed to init SDL\n");
        exit(1);
    }
    else
        printf("SDL Initialised\n");

    // set the screen
    sScreen = SDL_SetVideoMode(sdata.w, sdata.h, sdata.bpp, SDL_SWSURFACE);
    if (sScreen == NULL)
    {
        printf("Failed to Set Video Mode\n");
        SDL_Quit();
        exit(1);
    }
    else
    {
        printf("\nVideo Mode Set:\n");
        printf("Width: %d\n", sScreen->w);
        printf("Height: %d\n", sScreen->h);
        printf("Bit Deapth: %d bpp\n", sdata.bpp);
        printf("FPS: %d\n", sdata.fps);
    }

    // set the window caption
    SDL_WM_SetCaption(sdata.title, NULL);

}

/*
 * ~tmpProg()
 * cleans up tmpProg
 */

tmpProg::~tmpProg()
{
    if (state)
        delete[] state; // should give double free() message in here
    state=NULL;
    cState=NULL;

    free(sdata.title);
    sdata.title=NULL;

    if (sScreen)
        SDL_FreeSurface(sScreen);
    sScreen=NULL;

    SDL_Quit();
}

/*
 * run()
 * runs the tmpPrpg
 */
int tmpProg::run()
{
    // setup the states, exit the program if something fails
    if (!(init()))
        return 0;

    enumState = ZERO;
    sdata.running = 1;

    //while(sdata.running)
        cState=&state[ZERO];

        //render the screen
        SDL_BlitSurface(cState->render(), NULL, sScreen, NULL);

        // Update the Screen
        if (SDL_Flip(sScreen)<0)
        {
            fprintf(stderr, "Failed to update the Screen. \n");
            return 0;
        }

    return 1;
}

/*
 * init()
 * runs the init functions for screens
 * returns 0 on falure
 */

int tmpProg::init()
{
    if (!(setupStates()))
        return 0;
    if (!(loadImg()))
        return 0;
    return 1;
}

/*
 * setupStates()
 * sets up screen array
 */
int tmpProg::setupStates()
{
    //Create an array of screens for gameStates
    state = new screen [SIX];

    for (int i = 0; i<=SIX; i++)
    {
        state[i].createWindow(sdata);
        printf("Game State %d's render() func returns a pointer to %p.\n", i, (state[i].render()));
    }
    return 1;
}

/*
 * loadImg()
 * loads images
 */
int tmpProg::loadImg()
{
    //if ((!(state[ZERO].setBackground(loadIMG("./background.png")))))
    //    return 0;

    //unworking code (did throw segfaults)
    
    try
    {
        state[ZERO].setBackground(loadIMG("./background.png"));
    }
    catch (char *err) // doesnt catch anything meaningful
    {
        fprintf(stderr, "%s",err);
        //free(str);
        return 0;
    }
    

    return 1;
}

/*
 * screen()
 */

screen::screen()
{
    sCurrent = NULL;
    sbg = NULL;
}

/*
 * ~screen()
 * the double free() gets called in here
 */

screen::~screen()
{
    // if sCurrent exists, print its pointer loc and free it, do the same with sbg
    if (sCurrent)
    {
        printf("Freeing memory location %p, assigned to sCurrent in ~screen().\n", sCurrent);
        SDL_FreeSurface(sCurrent); //this line causes the double free()
        sCurrent = NULL;
    }

    if (sbg)
    {
        printf("Freeing memory location %p, assigned to sbg in ~screen().\n", sbg);
        SDL_FreeSurface(sbg);
        sbg = NULL;
    }
}

/*
 * render()
 * Blit the screen in a single SDL_Surface and return a pointer to it
 * return a pointer of the screen
 */


SDL_Surface* screen::render()
{
    if (sbg)
        SDL_BlitSurface(sbg, NULL, sCurrent, NULL);
    return sCurrent;
}


/*
 * createWindow()
 * creates an SDL_Surface with the same dimensions as the screen and returns a 
 * pointer to it to assign to sCurrent.
 */
int screen::createWindow(SData &d)
{
    if ((sCurrent = SDL_CreateRGBSurface(SDL_SWSURFACE, d.w, d.h, d.bpp, 0, 0, 0, 0)))
        return  1;
    else
        return 0;
}

/*
 * setBackground()
 * sets bg
 */

int screen::setBackground(SDL_Surface* bg)
{
    if (bg)
    {
        if (sbg)
            SDL_FreeSurface(sbg);

        sbg=bg;
    }
    else
    {
        fprintf(stderr, "Dont call setBackground without a surface!\n");
        return 0;
    }

    return 1;
}

[kbw]

I've not read all of it, but should you be freeing the return from IMG_GetError? I'm not certain myself, but SDL_GetError returns a static buffer.

[arathalion]

lol thanks.

That's something I added as I wrote the example program. You are correct, It also causes the same problems as it tries to free unallocated memory, but it only goes in there when there is no background.png file.

however, it's also doing it when it tries to free state[0].sCurrent.

[kbw]

You've gone off the end of your array in:

    state = new screen [SIX];
    for (int i = 0; i<=SIX; i++)

You don't need to check for null on delete in:

    if (state)
        delete[] state; // should give double free() message in here 

but it's not an error.

[arathalion]

Ahh, because its an enum that translates to 6 Im creating the array one smaller than I want it. Thanks very much.

One last question, less important but still annoying.

My throw statement on line 17, annoyingly enough it doesn't segfault in this program, but it does in my other one, and I can't see a difference between those functions. In any case, with this program, it throws garble, or perhaps instead garble is caught in my catch statement on 249. Any ideas?

ps. i have commented the above code and I have realised that instead of doing all that in loadIMG to add a \n to the end of the string I could just throw IMG_GetError() and add it in my fprintf statement on line 251.

[kbw]

It's throwing a local variable. It's probably overwritten by the time you get to use it. You may want to throw a std::string.

I still think you've gone off the end of your array.

[arathalion]

lol, yeah, I did go off the end of my array. I was confused about why I got the error when in the first element instead of the end+1.
Adding an eighth enum to the set and changing the for loop to i<LAST; fixes it.

You are also correct about the throw. Thanks for the help.