sf::Image destructor causes SIGABRT crash

[drummerp]

Hello all! I'm trying to create a Skybox object, which when loaded loads a single image file and splits it into 5 or 6 (depending on if there's a bottom image) sf::Image objects, represented as a std::vector<sf::Image>. I then pass the image data (from image.getPixelsPtr() ) to glTexImage2D and call a number of other OpenGL functions to set up my textures for rendering.

My issue comes at the end of my function: when the vector is cleared off the stack, it results in a crash dump to stderr and then a SIGABRT crash. If I run it in my debugger, it becomes apparent it's resulting from in the sf::Image destructor. Here's my code (with some irrelevant portions removed):

bool Skybox::loadSkybox() {
    glGenTextures(6, ids);

    //Loads the image file, then splits into 5-6 images using sf::Image::copy, returning vector of images
    //returns a list of skybox images, in order: Top, Bottom?, Left, Right, Front, Back
    const std::vector<sf::Image> images = Image->getAsSkybox();
    if (images.empty() || images.at(0).getSize().x == 0) //Empty list or empty images is an error with loading
        return false;

    for (unsigned int i = 0; i < images.size(); i++) {

        glBindTexture(GL_TEXTURE_2D, ids[i]);

        GLuint width = images.at(i).getSize().x, height = images.at(i).getSize().y;
        unsigned char* image = const_cast<unsigned char*>(images.at(i).getPixelsPtr());
        if (image == NULL || width <= 0 || height <= 0) {
            std::err << "Error loading image file." << std::endl;
            glDeleteTextures(6, ids);
            return false;
        }

        glTexImage2D(GL_TEXTURE_2D, 0, GL_COMPRESSED_RGBA, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, image);

        GLenum err;
        if ((err = glGetError()) != GL_NO_ERROR) {
            std::err << "OpenGL error in loading image number " << i << ": " << gluErrorString(err) << std::endl;
            glDeleteTextures(6, ids);
            return false;
        }

        //Call other OpenGL functions to set it up

        free(image); //Clear our temporary image data
    }

    return true;
}

I'm using SFML2,  Code::Blocks for my IDE and GCC 4.6 on a 32-bit on an Ubuntu 12.04 LTS environment.

[binary1248]

unsigned char* image = const_cast<unsigned char*>(images.at(i).getPixelsPtr());
...
free(image); //Clear our temporary image data

What the... never ever use free in your own C++ code. Here you kind of asked for it by doing something really dumb. The pointer returned from getPixelsPtr() is const for a reason. If you cast away the constness, and free the memory allocated by the underlying object yourself, you can't complain you get all sorts of undefined behaviour. In this case, it results in a double free when the sf::Image destructors are called.

Rule #1 of C dynamic memory management: For every malloc() there must be a free(), and vice versa.

[FRex]

Fix:

// free(image); //Clear our temporary image data

What the... never ever use free in your own C++ code.

What if I'm biding C to c++ and things might get allocated or freed in libs with cstdlib functions?

[binary1248]

What if I'm biding C to c++ and things might get allocated or free in libs with cstdlib functions?

I said in your own C++ code. If your code is C then there is no other way. If you want to use a library that makes you call free() on objects that malloc() themselves instead of providing a cleanup function for them, then... just use another library.

[Nexus]

The memory inside sf::Image doesn't belong to you. In modern C++, memory (and other resources) always has a clear owner -- if ownership is transferred, then this is done explicitly, usually through an encapsulated object or a smart pointer. In this respect, SFML is well-designed and uses the RAII idiom to relieve the user from the burden of memory management. That in turn implies that there are no implicit cases in SFML where you have to interfere with raw memory.

And as binary1248 mentioned, don't use const_cast unless you're absolutely sure what you're doing. It is almost only necessary when you interact with code that is not const-correct, i.e. bad code.

[drummerp]

unsigned char* image = const_cast<unsigned char*>(images.at(i).getPixelsPtr());
...
free(image); //Clear our temporary image data

What the... never ever use free in your own C++ code. Here you kind of asked for it by doing something really dumb. The pointer returned from getPixelsPtr() is const for a reason. If you cast away the constness, and free the memory allocated by the underlying object yourself, you can't complain you get all sorts of undefined behaviour. In this case, it results in a double free when the sf::Image destructors are called.

Rule #1 of C dynamic memory management: For every malloc() there must be a free(), and vice versa.

Ah, wow. I'd love to chalk that one up to a lack of sleep or something like that, but all I can really say is that I just pretty well screwed that up. But I understand my problem at least.

Anyway, I removed the call to free(), and that solved the problem. Behavior's now as expected. Thank you for the help.

[drummerp]

And as binary1248 mentioned, don't use const_cast unless you're absolutely sure what you're doing. It is almost only necessary when you interact with code that is not const-correct, i.e. bad code.

I'm uncertain how else I'm supposed to approach this: glTexImage2D requires an unsigned char*, but getPixelsPtr() returns a const unsigned char*, and the two are not interchangeable -- trying to pass the return value of getPixelsPtr() directly to glTexImage2D without the cast results in an error. I'm probably going about this wrong, or otherwise not seeing something obvious, but I'm unsure what else I should do here.

[FRex]

What the... never ever use free in your own C++ code.

/*
 * File:   main.cpp
 * Author: superprogrammer
 *
 * Created on October 24, 2013, 2:08 PM
 */

#include <iostream>
#include <cstdlib>
#include <cstring>

int main()
{
char * buffer=std::malloc(100);
std::strcpy(buffer,"Hello Modern C++");
std::cout<<buffer<<std::endl;
//std::free(buffer) // - commented cus binary1248 said to never use free in C++
}

glTexImage2D requires an unsigned char*

Not for me on my Linux and not in docs of GL. Both take const void * and const pointer of any kind should be implicitly convertible to that(= what is the error message and how is that function declared for you??). Even if it wasn't const, casting away const doesn't mean that you can free pointers you didn't malloc...

[Nexus]

glTexImage2D requires an unsigned char*

No, it does not. It expects const GLvoid*:

void glTexImage2D( GLenum target,  
  GLint level,  
  GLint internalFormat,  
  GLsizei width,  
  GLsizei height,  
  GLint border,  
  GLenum format,  
  GLenum type,  
  const GLvoid * data);

binary1248's message concerning malloc/free was pretty clear: Don't use it in C++. Of course there are exceptional cases, but they are extremely rare, and completely irrelevant for this thread.

[FRex]

For added comedic value I just wanted to run "image+freeing it's ptr" code with debugging on* to make a point of running debugger first and asking for help later and the call stack is more or less yelling profanities at me for touching that pointer that belongs to std::vector in sf::Image pointer, as seen on attached picture.
The added comedic value comes from the fact that free requires a cast to get rid of const because it takes a void * unlike GL that takes const void *... sooo.. does the error for const cast come from GL function or not? It's bug, mistake or very weird compiler config if it does.

*admittedly it's Fedora + CodeLite but since Ubuntu/Mint and C::B are apparently 'beginner friendly' in popular opinion I can only assume that on them the output is just as helpful as here.

[eXpl0it3r]

I've no experience in OpenGL so I might not get what you're doing, but why don't you just use an sf::Texture and call sf::Texture::bind(...) instead of the detour through sf::Image?

@FRex: Can please either start posting useful/non-trolling/non-conspiracy-theory stuff or just not post at all?
If you find C sooooo cool then stick with it, but don't start off-topic discussions whenever possible.
Thank you!

[drummerp]

It's strange that it doesn't require a const. I never use a const_cast unless a compile error needs me to. But you are right, it's a const GLvoid*, so the const_cast is unnecessary. I corrected that, and it does still work. Thank you all for your help.