I have case where I have a class that allocates memory in the constructor, and frees it in the destructor -- pretty basic stuff. The problem happens if I reuse the class instance variable for a new instance of the class. When the final (and only the final) instance gets destroyed when it goes out of scope, it will crash with a SIGABRT on the call to free/delete:

malloc: *** error for object 0xXXXXX: pointer being freed was not allocated

I feel like I'm missing something fundamental and I'd like to understand my mistake so I can avoid it in the future.

Here's a simple repro:

class AllocTest {
public:
    AllocTest(const char *c) {
        this->data = new char[strlen(c)   1];
        strcpy(this->data, c);
    }
    ~AllocTest() {
        if (this->data != NULL) {
            delete[] data;
        }
    }
private:
    char* data;
};

int main(int argc, const char *argv[]) {
    const char* c = "test test test";
    AllocTest t(c);
    t = AllocTest(c);
    t = AllocTest(c);
    return 0;
}

I can see in the debugger that the destructor being called each time t gets reassigned against the previous instance, why does only the final destructor cause a crash? It doesn't matter if I use new/delete or malloc/free -- it's the same behavior either way -- and only on the final deallocation. It also doesn't matter if I move the scopes around or anything -- as soon as the final scope leaves, the crash happens.

This does not reproduce if confining the variable 't' to its own scope and not trying to reuse it -- for instance, everything's fine if I do this:

for (int i = 0; i < 100; i  ) {
    AllocTest t(c);
}

Working around the issue is simple enough but I'd much rather understand why I'm having this problem to start with.

UPDATE: Thanks to the answer from @user17732522, I now understand what the problem is here. I didn't realize that when I was reassigning t that it was actually making a copy of the class -- I was working on an assumption that like other languages that I typically work with that the assignment overwrites it. Upon realizing this things all made sense as I was unwittingly stumbling into a classic "double free" problem. The documentation on copy initialization and the pointers to the documentation about the rule of three pattern helped fill in the rest of the gaps here.

Simply modifying my class to define the implicit copy semantic was enough to cause the code to work as expected:

    AllocTest& operator=(const AllocTest& t) {
        if (this == &t) {
            return *this;
        }
        size_t newDataLen = strlen(t.data)   1;
        char* newData = new char[newDataLen];
        strcpy(newData, t.data);
        delete this->data;
        this->data = newData;
        return *this;
    }

Thanks, folks!

CodePudding user response：

First of all the constructor itself has undefined behavior, because you allocate only enough space for the length of the string (strlen(c)) which misses the additional element required for the null-terminator.

Assuming in the following that you fix that.

The destructor on an object/instance is only ever called once.

In your code t is always the same instance. It is never replaced with a new one. You are only assigning to it, which uses the implicit copy assignment operator to copy-assign the members from the temporary object to t's members one-by-one.

The destructor on t is called only once, when its scope ends after the return statement.

This destructor call has undefined behavior because the destructor of the last AllocTest(c) temporary object has already deleted the allocated array to which t.data points at this point (it had been assigned that value with t = AllocTest(c);). Additionally, the first allocation in AllocTest t(c); has been leaked, since you overwrote the t.data pointer with the first t = AllocTest(c);.

With just AllocTest t(c); you are not copying any pointer and so this can't happen.

The underlying issue here is that your class violates the rule of 0/3/5: If you have a destructor you should also define copy/move constructor and assignment operators with the correct semantics. The implicit ones (which you are using here) are likely to do the wrong thing if you need a custom destructor.

Or even better, make the rule-of-zero work by not manually allocating memory. Use std::string instead and you don't have to define either the destructor or any special member function. This also automatically solves the length mismatch issue.