"Trying to free invalid pointer" error detected in destructor

Question

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() {}

Note, that you can omit comparation with `NULL` before deleting, because deleting null pointer is valid. — Lol4t0, Oct 25 '12 at 19:33

score 6 · Accepted Answer · edited May 23 '17 at 10:28

6

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.

edited May 23 '17 at 10:28

Community

1
1

answered Oct 25 '12 at 19:31

hmjd

120,187
20
207
252

The unguarded `delete[]` is not a good idea generally, since the pointer might get deleted elsewhere. Without explicitly making sure to set it to NULL again in such cases, this would result in a double `delete[]`. – Nikos C. Oct 25 '12 at 19:38
@NikosChantziaras, just because a pointer is not `NULL` does not guarantee that it points to a valid object, as this question nicely shows. Having a guarded delete does provide a 100% guarantee and neither does `NULL`ing a pointer after `delete` as another pointer can be pointing to the same object. – hmjd Oct 25 '12 at 19:39
@hmjd - *Having a guarded delete does provide a 100% guarantee* -- I think you mean something along the lines of "does not guarantee". – David Hammen Oct 25 '12 at 19:51
@NikosChantziaras - With one exception, there is absolutely no reason to guard `delete`. That one exception is if you have overloaded `operator delete` so that it emulates the behavior of `free` in the bad old days, prior to the first C standard. Back then, freeing a null pointer did cause all kinds of havoc. This is the 21st century. Both `free` and `delete` (sans override) are now required to allow a null pointer as input, and to do absolutely nothing when told to free/delete a null pointer. – David Hammen Oct 25 '12 at 19:55

score 1 · Answer 2 · answered Oct 25 '12 at 19:31

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; }

"Trying to free invalid pointer" error detected in destructor

2 Answers2