Tommy
Tommy

Reputation: 13672

why does this "rule of three" failure actually fail?

Yesterday I learned an extremely valuable lesson: follow the rule of three.

I think I would have learned it easier, but the error only came on the delete statement. Here was the scenario:

foo.h
    class foo{
    public:
        ...
        foo(int *Y); 
        ~foo();  
        int *X;
    }
 foo.cpp
     ...
     (.. constructor sets X to Y..)
     foo:~foo(){
         delete [] X;
     }

main.cpp
    vector<Foo> fooVec;
    { // this is just to demonstrate scope problems more easily.  
         Y = new int[10000];
         (...init Y...)
         fooVec.push(Foo(Y)) // I get it: this calls copy constructor, violating rule of three
         (...forget to delete Y...)
    } 
    // Y is now out of scope so this is a memory leak
    cout << fooVec[0].[X][0] << " " <<  fooVec[0].[X][1] // THIS WORKS AS INTENDED DUE TO MEMORY LEAK
    // program ends, fooVec goes out of scope

which bombs with "pointer being freed has not been allocated". I tracked this back to the point where fooVec goes out of scope, which calls foos destructor, which tries to delete X. My main question: Why does the delete actually fail? I never deleted Y in the code (I get this is a memory leak), so I am not actually double deleting a pointer. Moreover, the memory clearly exists, because the cout line works. If this line had failed I would have figured out the problem much sooner.

Comments below seem to indicate "when Foo(Y) goes out of scope, X is deleted". But if this is the case, why on Earth does the cout statement work?

Note: I did not have a copy constructor and assignment overload, hence "rule of three failure". I get that I should have because of the vector push_back statement. I'm asking why exactly not having them killed me here, because I forgot to free Y so I am not actually deleting a pointer twice.

EDIT:

Thank you all for your help. user1158692s answer summed it up, but all the comments in thelambs answer also helped me figure out what exactly was going on, and they stuck around to answer many questions for me. Would accept both if I could..

Upvotes: 2

Views: 204

Answers (3)

thelamb
thelamb

Reputation: 486

[Note: newFoo is no longer part of the original post, it refers to the object being pushed into the vector fooVec, which is now done inplace: foovec.push_back( Foo(Y) )]

It does double-delete. First, when newFoo goes out of scope, it does delete[] x (this is the first delete). You wrote forget to delete Y here, but Y is in fact deleted by the desctuctor of newFoo.

The second delete is when the copy of newFoo is deleted (when fooVec goes out of scope). The copy of newFoo also does delete[] x, and since you don't have a copy constructor, x is the same in newFoo and in the copy of newFoo, hence it is a double-delete.

Now, you would not be able to solve this problem easily. Since in the copy constructor that you're going to write, you have no idea how to copy x (how many elements does it have? 1? 100000?).

Upvotes: 5

Grimm The Opiner
Grimm The Opiner

Reputation: 1806

Without an explicit copy constructor, the pointer value in X is copied. Here's what's happening to you:

  • Instantiate newFoo which holds pointer value Y
  • push newfoo onto vector, vector now holds another foo which also has a pointer value Y
  • newFoo goes out of scope, destructor deletes memory pointed to by value Y
  • cout uses pointer value Y, but as deleted memory has yet to be overwritten it "works"
  • fooVec goes out of scope, the destructor of the foo it holds attempts to delete memory pointed to by Y
  • BANG

Upvotes: 2

James Kanze
James Kanze

Reputation: 154017

I'm not sure what you mean by the "this works as intended" comment. There is no memory leak in your code, but rather a double delete of the same memory. At the end of the scope in which it is defined, the destructor of newFoo is called; this in turn deletes the memory allocated at Y = new int[10000], and invalidates the pointer in the Foo object in fooVec. Your undefined behavior begins at that moment, because fooVec now contains an object which is formally not copyable. After that, anything can happen: undefined means just that, undefined.

You go on to access the memory (undefined behavior), and to delete it a second time in the destructor of the object in fooVec (undefined behavior).

Upvotes: 3

Related Questions