CODEWITHSUNDEEP
c++
wolfPack88
wolfPack88

Reputation: 4203

"Trying to free invalid pointer" error detected in destructor

I am in the beginning stages of writing a fairly large code. I have defined one class as such:

class GPUMD {
    private:
        double xhi, xlo, yhi, ylo, zhi, zlo;
        int numAtoms;
        Atom *atoms;
    public:
        GPUMD();
        ~GPUMD();
};

The destructor is defined as below:

GPUMD::~GPUMD() {
    if(atoms != NULL)
        delete [] atoms;
}

Right now, the code does this:

int main(int argc, char *argv[]) {
    GPUMD gpumd;
    exit(0);
}

I get a glibc detected error: trying to free invalid pointer. Using valgrind, I see that this error traces to my destructor for GPUMD. For some reason, the atoms != NULL test is returning true even though I have not assigned anything to that pointer. Why is that?

EDIT: The constructor is defined as:

GPUMD::GPUMD() {}

Upvotes: 2

Views: 7196

Answers (2)

hmjd
hmjd

Reputation: 121971

Because atoms has not been explicitly initialised to NULL or a valid pointer in the constructor. Change constructor to:

GPUMD::GPUMD() : numAtoms(0), atoms(NULL) {}

Note that the atoms != NULL check prior to the delete[] is superfluous as delete[], or delete, on a NULL pointer is no-op. The following is safe:

GPUMD::~GPUMD() {
    delete [] atoms;
}

As there is a dynamically allocated member in GPUMD you need to prevent copying of instances of GPUMD or implement the assignment operator and copy constructor (see What is The Rule of Three?).

As C++, consider using a vector<Atom> (or a vector of smart pointers) instead which will manage dynamic memory for you.

Upvotes: 6

Nikos C.
Nikos C.

Reputation: 51840

If you don't assign anything to the pointer (in other words if you don't initialize it) then its value is undefined. It's not NULL (well, it could be NULL by pure chance, but that chance is slim to none.) Only static variables are automatically zero-initialized.

To make a long story short, initialize it to NULL in the constructor:

GPUMD::GPUMD()
: atoms(NULL)
{ }

Or, if you don't like initializers for POD types (why not?), then:

GPUMD::GPUMD()
{ atoms = NULL; }

Upvotes: 1

Related Questions