2

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..

Tommy
  • 12,588
  • 14
  • 59
  • 110
  • I see only a destructor. Where are your copy constructor and (copy) assignement operator? – peppe Oct 22 '13 at 13:35
  • I didn't have them, hence, "rule of three failure". My question is, why does not having them kill me here? – Tommy Oct 22 '13 at 13:36
  • You'll get the default copy constructor and assignment operator. Which will copy the pointer. Now you have two objects that use the same pointer. The first delete works. The second deletes a pointer that was already deleted. – Hans Passant Oct 22 '13 at 13:38
  • 1
    Please reduce the code to something compilable. You'll be surprised how self-helpful such an exercise is. – rubenvb Oct 22 '13 at 13:43

3 Answers3

5

[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?).

thelamb
  • 486
  • 3
  • 10
  • But, if newFoo deletes X, why on Earth would the `cout` statement work as intended? – Tommy Oct 22 '13 at 13:39
  • I edited my code. I never explicitly created newfOO: not sure if that changes anything. – Tommy Oct 22 '13 at 13:40
  • Undefined behaviour. It could do pretty much anything. – Simple Oct 22 '13 at 13:41
  • There is no double `free` in C++ (as long as you don't use `free`), because you can call `delete` on a null pointer without issue. – rubenvb Oct 22 '13 at 13:42
  • @Tommy What do you expect? You have undefined behavior, so anything might happen. Including the appearance of working. – James Kanze Oct 22 '13 at 13:43
  • 2
    When you free memory, it is not *destroyed*. You're just saying `this piece of memory here can now be used for something else`. Accessing memory that was freed *might* work, or it might launch a nuclear attack against Russia. In other words, it is *undefined behavior*. @rubenvb after `delete[] x`, x is *not* NULL. Even setting it to NULL in the destructor won't help, since the copy of `newFoo` will still have the original pointer value, and it will try to delete it. – thelamb Oct 22 '13 at 13:44
  • @rubenvb We generally speak of double delete, rather than double free, but it certainly can happen in C++ (and does in this example). – James Kanze Oct 22 '13 at 13:44
  • I would expect cout << `FooVec[0].[X][0]` to fail with something like "this memory no longer allocated". But instead it prints all of the correct values that were indeed initialized in Y. – Tommy Oct 22 '13 at 13:44
  • Right, I was wrong. Blame me for using smart pointers all the time :-). – rubenvb Oct 22 '13 at 13:45
  • @Tommy So. Undefined behavior is, well, undefined. In practice, the memory will probably not be returned to the OS, or unmapped; it will simply be returned to the in process free space arena, for use in the next `new`. – James Kanze Oct 22 '13 at 13:47
  • So the `cout` statement works "luckily" due to the fact that those values are still sitting in those memory cells out in boonland because nothing new has re-claimed that space yet? Kind of like deleting files from your hard drive but being able to recover them before a new file overwrites that now "free" space? – Tommy Oct 22 '13 at 13:48
  • @Tommy Yes, pretty much. There's a great analogy for a similar situation in [this answer](http://stackoverflow.com/a/6445794/1782465). – Angew is no longer proud of SO Oct 22 '13 at 13:55
  • I guess my question is "why is it undefined then?". This means there could be lots of code out there "working" due to chance like this code was for weeks. Wouldn't defining the behavior to fail with something like "this memory no longer allocated" be much safer? – Tommy Oct 22 '13 at 13:55
  • @Tommy It would be safer, but it would be slower. Remember, C(++) happily lets you blow your foot off with a shotgun in return for a performance gain. There are tools out there that help you find and diagnose these problems though, most notably: `valgrind` – thelamb Oct 22 '13 at 13:57
  • 1
    @Tommy It would be safer, but it would be expensive. The core C++ philosophy is "you only pay for what you use." You can create a smart pointer which will trash the memory when it deallocates it. Or a memory pool which will guard access. Or anything. But why should correct programs pay in performance for catching bugs in bad programs? – Angew is no longer proud of SO Oct 22 '13 at 13:58
  • I actually do know the length of X because that is stored as another member. Making an explicit copy constructor did fix the problem. I've yet to make an explicit assignment operator, though, but will do so. I havent had my other leg blown off by that yet, but will prevent it from happening. – Tommy Oct 22 '13 at 14:03
  • @Tommy Since this is actual code, why are you not using a `std::vector` for x? Or, if you can use C++11, why not a `std::array` if the size of `x` is static, as in the example. – thelamb Oct 22 '13 at 14:06
  • 1
    I should be using vector now. However, I have avoided knowing about the rule of three in the past because I always use vector which I suppose comes with a deep copy constructor by default. Hence, using a dynamically allocated array here (size is not static but read at runtime) actually made me do a bit of learning that vector was taking care of for me. – Tommy Oct 22 '13 at 14:12
3

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).

James Kanze
  • 150,581
  • 18
  • 184
  • 329
  • "Works as intended" meaning prints the values that were indeed initialized in Y. – Tommy Oct 22 '13 at 13:43
  • 1
    @Tommy That's one possible result of undefined behavior. Not the only one, of course, and in different contexts, your code might do different things. – James Kanze Oct 22 '13 at 13:48
2

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
Grimm The Opiner
  • 1,778
  • 11
  • 29
  • newFoo should be changed to Foo(Y). SOrry about that, but I never actually instantiated newFoo, I just did it implicitly in the push_back statement. Sorry again about changing my question afterwards. – Tommy Oct 22 '13 at 14:01
  • @Tommy Your edit doesn't really change anything in regard to my answer, just replace `newfoo` with `the temporary foo created in the call to pushback()`. The temporary object goes out of scope and deletes the memory pointed to by Y. – Grimm The Opiner Oct 22 '13 at 14:06
  • Also note, that the vector will do additional copy constructor / destructor calls on all its objects whenever its storage is exhausted. So, even if you leak all the objects and avoid creating temporaries that would delete the array, the `vector<>` can still trigger the same kind of crash all on its own. – cmaster - reinstate monica Oct 22 '13 at 14:57