Error every time the destructor is called

Question

I got a weird message everytime the destructor is called. Since one of my private variable is dynamic allocated array (int *member;), I write the destructor like this:

ClassSet::~ClassSet(){delete []member;}

Everytime the destructor for ClassSet is called, I got an error message:

Windows has triggered a breakpoint in Hw1.exe.

This may be due to a corruption of the heap, which indicates a bug in Hw1.exe or any of the DLLs it has loaded.

This may also be due to the user pressing F12 while Hw1.exe has focus.

entire class:

class ClassSet
{
  public:
    ClassSet(int n = DEFAULT_MAX_ITEMS);
ClassSet(const ClassSet& other);
ClassSet &operator=(const ClassSet& other);
~ClassSet();
  private:
    int size;
int *member;
 };

ClassSet::ClassSet(int n){
   size = n;
   member = new int[n];
}

ClassSet::ClassSet(const ClassSet& other){
    int i = 0;
    this->size = other.size;
member = new int [capacity];
while (i<size)
{
    this->member[i] = other.member[i];
    i++;
}
 }

 Multiset& Multiset::operator=(const Multiset &other)
 {
    if (&other == this){return *this;}
this->size = other.size;
int i = 0;
    delete [] member;
    member = new int[size];
while (i<other.size)
{
    this->member[i] = other.member[i];
    i++;
}
return *this;
}

Any idea what's wrong with this destructor?

[Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — hmjd, Jan 21 '13 at 20:55
Show us how you initialize `member` and where you allocate it. Better yet, as @NPE said, show us the entire class. — Nik Bougalis, Jan 21 '13 at 20:56
If member is not an array, you should just delete member, not using []:s, you only need to use delete if you used member = new foo(); in the constructor (or in other way created it using memory on the heap) — Anders Lindén, Jan 21 '13 at 20:58
no way this is too localized. This happens to everybody. Could very likely be a dup though. — Doug T., Jan 22 '13 at 01:19

score 4 · Answer 1 · answered Jan 21 '13 at 20:57

4

You failed to implement (or you have implemented incorrectly) one of ClassSet::ClassSet(const ClassSet&) or ClassSet::operator=(const ClassSet&).

In other words, you have violated the Rule of Three.

The best solution, however, is likely not to implement them, but rather to change how you allocate space for your dynamic array. Instead of using new[] and delete[], try replacing that member with a std::vector<>.

answered Jan 21 '13 at 20:57

Robᵩ

163,533
20
239
308

I think I implement the copy constructor and operator= correctly. because when I assign "ClassSet aaa = b or ClassSet aaa; aaa=b" when I change an element in aaa, it does not change any element in b. – user1988385 Jan 21 '13 at 21:05
If you have implemented those correctly, then my theory is wrong. Regardless, I recommend that you do not use `new[]` directly, but use `std::vector` instead. – Robᵩ Jan 21 '13 at 21:18

Doug T. · Answer 2 · 2013-01-21T23:03:42.237

3

Heap corruption is often something detected after-the-fact. It may have to do with your destructor, or as I've seen, can likely happen well before the heap access the error occurs at.

Basically "Heap corruption detected" simply means that on a given access of the heap, Windows decided that the current state of the heap was inconsistent/invalid. Something went bad a while earlier.

These bugs can be really hard to track down. One common cause of heap corruption though is double deletion you deleted something twice inadvertently. This can point at deeper issues with how your data is copied around your code and your design.

This can happen, as others have said, when you don't have an appropriate copy constructor/assignment operator that copies dynamic memory. The "copy" deletes your memory, then the initial class deletes again, causing a double delete.

edited Jan 21 '13 at 23:03

answered Jan 21 '13 at 20:58

Doug T.

64,223
27
138
202

1

Although, `delete` is silent over null pointers, so that way you won't spot potential double deletes. – Matteo Italia Jan 21 '13 at 20:59
1

setting to NULL just hides issues since you shouldn't be calling delete on something twice to begin with – Jesus Ramos Jan 21 '13 at 20:59
@JesusRamos: not setting NULL just hides the issue that you access members after deletion. There's arguments both ways, both ways hide some information. – Mooing Duck Jan 21 '13 at 22:37
1

@MooingDuck And for everything else there's Valgrind :P – Jesus Ramos Jan 21 '13 at 22:38
@JesusRamos yeah in this case you guys are absolutely correct. Corrected my answer. – Doug T. Jan 22 '13 at 01:06

score 1 · Answer 3 · edited May 23 '17 at 12:12

If you've posted you actual code, then I think the problem is here:

ClassSet::ClassSet(const ClassSet& other){
    int i = 0;
    this->size = other.size;
    member = new int [capacity];  // <--- what is capacity?
    while (i<size)
    {
        this->member[i] = other.member[i];
        i++;
    }
}

You're sizing the copied array based on something named capacity which doesn't have any obvious relationship to other.size. If capacity is smaller than size the loop that copies elements will corrupt the heap.

Assuming that this is an academic exercise, once you solve this problem you should look into the copy/swap idiom that used for classes like these to ensure exception safety.

If this isn't an academic exercise, then you should be looking at std::vector or other containers that are provided in libraries.

capacity is a global constant (the number of element that the array member can have). size it the number element in the array that have been filled by user (not some random number that fill the array when the array is initiated) — user1988385, Jan 21 '13 at 22:39

score 0 · Answer 4 · answered Jan 21 '13 at 21:53

0

This problem is quite common. The default copy constructor is equivalent to

ClassSet(const ClassSet& other) {
    size = other.size;
    member = other.member;
}

The problem with this is that when an instance ClassSet is copied, both the original instance and the new instance hold a raw pointer to member. Both destructors will free member, causing the double free problem you are seeing.

For example,

{
    ClassSet a
    ClassSet b(a); // assert(b.member == a.member)
} // At this point, both a and b will free the same pointer.

You can mitigate this by not allowing copying, or moving the pointer instead of copying.

answered Jan 21 '13 at 21:53

Alex Chamberlain

4,147
2
22
49

am not using the default copy constructor and default operator = I think I implement the copy constructor and operator= correctly. because when I assign "ClassSet aaa = b or ClassSet aaa; aaa=b" when I change an element in aaa, it does not change any element in b. – user1988385 Jan 21 '13 at 22:06
@user1988385 Post it and we can check. – Alex Chamberlain Jan 21 '13 at 22:07

Error every time the destructor is called

4 Answers4