Pointer being freed was not allocated - destructor

Question

Slowly learning about copy/move constructors, rule of 5 etc. Mixed with not-so-well understanding of usage of pointers/reference, I cannot understand why my destructor throws the error below. I know that it's caused by destructor and destructor only, and after the copy constructor.

untitled(19615,0x104a60580) malloc: *** error for object 0x600000b74000: pointer being freed was not allocated

untitled(19615,0x104a60580) malloc: *** set a breakpoint in malloc_error_break to debug

Code:

#include <string>

using namespace std;
typedef int size_type;

class user{
public:
    user():
    rank(1),
    name("default"){
    }
private:
    int rank;
    string name;
};

class account{
public:
    account(size_type max_size):
    owners(new user[max_size]),
    max_size(max_size){
        cout<<"normal constructor"<<endl;
    }
    account(account& other):
    owners(other.owners),
    max_size(sizeof(owners)){
        cout<<"copy constructor called"<<endl;
    }
    account(account&& other):
    owners(other.owners),
    max_size(sizeof(owners)){
        cout<<"move constructor called"<<endl;
    }
    ~account(){
        if(owners!= NULL) {
            delete[] owners;
            owners= 0;
        }
        cout<<"destructor called"<<endl;
    }

private:
    user* owners;
    size_type max_size;
};

int main(){
    account f(3);
    account n(f);
}

The copy constructor is fundamentally wrong, as it fails to do a deep copy, which is why the same pointer value is being deleted twice (or more times). Also you are missing a user-defined assignment operator. In general, you are violating the [rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). Go to the **Managing resources** section of that link, and read it carefully. — PaulMcKenzie, Jan 30 '22 at 23:34
You also don't need to check for `NULL` when issuing a call to `delete`. — PaulMcKenzie, Jan 30 '22 at 23:35
`sizeof(owners)` is the size of a pointer, not the size of the data pointed to. Why wouldn't you just use `other.max_size`? — Retired Ninja, Jan 30 '22 at 23:35
@RetiredNinja next thing I forgot about using pointer, thank you. — fiflak, Jan 30 '22 at 23:37

eerorika · Accepted Answer · 2022-01-30T23:54:49.847

I cannot understand why my destructor throws the error below. I know that it's caused by destructor and destructor only

account f(3);

The constructor of this object allocates an array.

account n(f);

The copy constructor that you wrote copies the pointer that points to the dynamic array.

n is destroyed first. Its destructor deletes the array. f is destroyed after that. Its destructor also deletes the same array and the behaviour of the program is undefined, which lead to the output that you observed.

To prevent the allocation from being deleted multiple times, you must enforce a class invariant (a post condition that applies to all member functions) that any non-null owners value must be unique across all instances of the class. To maintain that invariant in the copy constructor (as well as move constructor and copy and move assignment operators), you must not copy the pointer, as you do now. I recommend studying the RAII idiom.

Better solution: Don't use owning bare pointers. Use std::vector<user> and don't have user defined special member functions at all.

P.S. The copy constructor should accept a reference to const.

Also:

if(owners!= NULL) {
    delete[] owners;
    owners= 0;
}

The check is redundant because it's safe to delete null. The assignment is redundant because the object is about to be destroyed and the member cannot be accessed after that anyway.

Thank you very much! Did not realise that I copied the pointer, there goes not being used to pointers and reference. Long way to go. — fiflak, Jan 30 '22 at 23:55

Pointer being freed was not allocated - destructor

1 Answers1