vector causing memory errors on destruction

[tuukka lah]

std vector seems to cause errors when destructing
here some simplified code:

#include <stdlib.h>
#include <stdio.h>
#include <vector>

class Foo{
	public:
	int* child;
	
	Foo(){
		child=NULL;
		if(rand()%10) child=new int(5);
	}
	
	~Foo(){
		if(child!=NULL) {
			delete child;
			child=NULL;
		}
	}
};

int main(int argc, char **argv)
{	
	std::vector<Foo> arr;
	arr.resize(100,Foo());
	
	printf("hello world\n");
	return 0;
}

compiled with
g++ -c "/home/tuukka/codelite/bugtest/main.cpp" -g -o ./Debug/main.o "-I." "-I."
g++ -o ./Debug/bugtest ./Debug/main.o "-L."


here is my output:

hello world
*** glibc detected *** ./bugtest: double free or corruption (fasttop): 0x09abb008 ***
======= Backtrace: =========
/lib/i386-linux-gnu/libc.so.6(+0x75ee2)[0xd05ee2]
/usr/lib/i386-linux-gnu/libstdc++.so.6(_ZdlPv+0x1f)[0x88951f]
./bugtest[0x80487e6]
./bugtest[0x8049170]
./bugtest[0x8048f56]
./bugtest[0x8048abd]
./bugtest[0x804899c]
./bugtest[0x8048833]
./bugtest[0x804873b]
/lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0xca94d3]
./bugtest[0x8048641]
======= Memory map: ========
00110000-0013a000 r-xp 00000000 08:01 5243915    /lib/i386-linux-gnu/libm-2.15.so
0013a000-0013b000 r--p 00029000 08:01 5243915    /lib/i386-linux-gnu/libm-2.15.so
0013b000-0013c000 rw-p 0002a000 08:01 5243915    /lib/i386-linux-gnu/libm-2.15.so
00517000-00518000 r-xp 00000000 00:00 0          [vdso]
00737000-00757000 r-xp 00000000 08:01 5243916    /lib/i386-linux-gnu/ld-2.15.so
00757000-00758000 r--p 0001f000 08:01 5243916    /lib/i386-linux-gnu/ld-2.15.so
00758000-00759000 rw-p 00020000 08:01 5243916    /lib/i386-linux-gnu/ld-2.15.so
007e0000-008b8000 r-xp 00000000 08:01 7080912    /usr/lib/i386-linux-gnu/libstdc++.so.6.0.16
008b8000-008b9000 ---p 000d8000 08:01 7080912    /usr/lib/i386-linux-gnu/libstdc++.so.6.0.16
008b9000-008bd000 r--p 000d8000 08:01 7080912    /usr/lib/i386-linux-gnu/libstdc++.so.6.0.16
008bd000-008be000 rw-p 000dc000 08:01 7080912    /usr/lib/i386-linux-gnu/libstdc++.so.6.0.16
008be000-008c5000 rw-p 00000000 00:00 0 
00ba5000-00bc1000 r-xp 00000000 08:01 5243778    /lib/i386-linux-gnu/libgcc_s.so.1
00bc1000-00bc2000 r--p 0001b000 08:01 5243778    /lib/i386-linux-gnu/libgcc_s.so.1
00bc2000-00bc3000 rw-p 0001c000 08:01 5243778    /lib/i386-linux-gnu/libgcc_s.so.1
00c90000-00e33000 r-xp 00000000 08:01 5242936    /lib/i386-linux-gnu/libc-2.15.so
00e33000-00e35000 r--p 001a3000 08:01 5242936    /lib/i386-linux-gnu/libc-2.15.so
00e35000-00e36000 rw-p 001a5000 08:01 5242936    /lib/i386-linux-gnu/libc-2.15.so
00e36000-00e39000 rw-p 00000000 00:00 0 
08048000-0804a000 r-xp 00000000 08:01 7488008    /home/tuukka/codelite/bugtest/Debug/bugtest
0804a000-0804b000 r--p 00002000 08:01 7488008    /home/tuukka/codelite/bugtest/Debug/bugtest
0804b000-0804c000 rw-p 00003000 08:01 7488008    /home/tuukka/codelite/bugtest/Debug/bugtest
09abb000-09adc000 rw-p 00000000 00:00 0          [heap]
b7794000-b7797000 rw-p 00000000 00:00 0 
b77b0000-b77b4000 rw-p 00000000 00:00 0 
bf8c9000-bf8ea000 rw-p 00000000 00:00 0          [stack]
Aborted (core dumped)
Press ENTER to continue...

[Cubbi]

The class Foo needs a copy constructor and a copy assignment operator (look up "Rule of Three"). Or, better yet, it shouldn't store raw pointers.

http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)
http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three

[Disch]

The problem is not with vector, it's with your class. Your class is doing a shallow copy.

Here's your problem:

int main()
{
    {
        Foo a;  // <- 'a' creates a new int for its child.
        {
            Foo b(a);  // <- 'b' is a copy of 'a'.  Note that you did not write a copy ctor for
              // Foo, so this will do a shallow copy.  That is... a.child and b.child point to the same
              // memory.
        } // <- 'b's destructor is called.  b.child is deleted

        // at this point, a.child is a BAD POINTER because it was deleted when b's destructor
        //  deleted it (remember both pointers pointed to the same data)

    } // <- a's dtor called... a.child is deleted AGAIN.  This is where you are crashing
}

This is happening with vector because vector sometimes has to move the memory around when you resize it, in order to keep all elements continuous in memory.


Possible Solutions (in the order in which I recommend them):

1) Don't use dynamic allocation

There's no need for it here. 'child' does not have to be a pointer. You can just make it a normal int and remove the dtor completely and remove the 'new' from the ctor.

2) If you must use dynamic allocation, use a smart pointer

unique_ptr will take care of deletion and copying/moving effects so you don't have to worry about them.

class Foo{
	public:
	std::unique_ptr<int> child;
	
	Foo(){
		if(rand()%10) child = std::unique_ptr<int>( new int(5) );
	}

	// no need for the dtor... unique_ptr will take care of deletion for you
};

3) Write a proper copy constructor + assignment operator and/or move constructor + move assignment operator.

The copy constructor and assignment operators would need to do a deep copy of the 'child' member (ie: allocate new memory and copy the pointed-to data... rather than just copying the pointer).

But as this is error prone and a lot of work... it's best to avoid this.



Typically... if you're managing memory yourself... you're probably doing it wrong.


EDIT:

ninja'd by Cubbi.

[kempofighter]

// incorrect - undefined behavior because child is an array
delete child;

// correct
delete []  child;

The other issues with copy construction and assignment are valid issues as well, and if you take the advice for that then perhaps the rework will result in the destructor being simplified. Option 1 above seems like the best option. Unique_Ptr is C++11 so if you try that then the compiler needs to support it.

[Disch]

child actually isn't an array, it's a single int. So delete child; was correct.

Note that he's doing new int(5); (parenthesis) and not new int[5]; (brackets).

[tuukka lah]

Thank you.
I actually need the dynamic allocation since the int was type *Foo before I tried to simplify.

will use unique_ptr from now on.