Destructor causes error upon delete double array

[Kaisky]

Hi!
I have a class "VEC". It looks like this:

class VEC {
public:
	VEC();
	VEC(int);
	VEC(int, double*);
	~VEC();
	double get(int);
	void set(int, double);
	void copy(VEC);
	void add(VEC);
	void subtract(VEC);
	void mult(double);
	void mult(MAT);
	void print();
	int dim;
private:
	double* data;
};

There are 3 Constructers (i dont use the 3rd in my code) working fine.

// default constructer
VEC::VEC() {}

// constructer dimension only
VEC::VEC(int d) {
	dim = d;
	data = new double[d];
}

// constructer dimension with pointer to the data
VEC::VEC(int d, double* data_) {
	dim = d;
	data = new double[d];
	data = data_;
}

But if i implement the destructor i immediately get a breakpoint.

// destructor releases array
VEC::~VEC() {
	delete[] data;
}

If I comment
delete[] data;
everything works fine, but my RAM usage increases steadily...
Not sure what exactly causes the problem ... my only suggestion is that the default constructer doesnt define data and then he tries to delete something not existent. So I tried to implement if(data != NULL) but it didnt help.

Since im pretty new to c++ I do'nt have any Idea how to solve this. After googling a bit I think it has to do something with the "Rule Three of C++" or something, but I didn't get what I have to change.

Maybe someone is kind enough to help a newb like me :)
Kaisky

[Thomas1965]

When the destructor is called data_ might not longer exist.
https://en.wikipedia.org/wiki/Dangling_pointer

// constructer dimension with pointer to the data
VEC::VEC(int d, double* data_) {
	dim = d;
	data = new double[d];
	data = data_;
}

You need to copy the elements from data_ to data - either with memcpy or a for loop.
An even better solutions is to use a std::vector instead of pointers.
http://www.cplusplus.com/reference/vector/vector/

[Kaisky]

That cant cause the problem,
because
1. like i said i dont use this constructer at all (could atually just delete it)
2. i tried to check if data still exists, it didnt help
3. if i comment "delete[] data", data gets used about thousand of times in a framework loop - no crashes or bugs at all.

Nevertheless I implemented a copy loop
for (int i = 0; i < d; ++i) data[i] = data_[i];
as you said, but it didnt change anything.

Thanks tho for reading my topic!

I'll try to use Vectors maybe but then i have to change 800 lines of code completely. First I would like to fix this.

[Naughty Albatross]

// Default constructor
VEC::VEC() : data (NULL) {}

You didn't initialize the member (data) with NULL. That means the pointer member starts with a random (garbage) value and when the destructor function is called it will attempt to free the garbage (invalid) address. And that is why your program crashes.


Also, free the (data) only when it is not NULL. Freeing a zero address also causes undefined behavior.

// Destructor releases array
VEC::~VEC() {
   if(data) delete[] data;
}

[Kaisky]

Thank you!
Wasnt even aware of this syntax VEC::VEC() : data (NULL) {}

I changed what you said, but it didnt solve the problem...

I logged the data[0] value like this:

// destructor releases array
VEC::~VEC() {
	Log("data[0]", data[0]);
	if(data) delete[] data;
}

in a .txt file and got

data[0] = 0
data[0] = -1.45682e+144

from it. So the first call works, the 2nd doesnt.
I wish i could find out from where it gets called...
I'll try to find it by inserting more Log("foo bar") everywhere where i create a VEC.
Nevertheless thank you for the first step.

[Naughty Albatross]

for(int i = 0; i < d; ++i) data[i] = data_[i];
Normally, your complier will perform a regular copy method that guarantees that the data of s1 and s2 must be exactly the same after the copy operation. Because s1 and s2 share the same (data) member values (or the same address values), it will cause a problem. For example, s1 frees (data) when its destructor is called, and s2 attempts to free (data) that s1 has already freed. That results in an undefined behavior.

You may want to overload the compiler with a (=) operator function to fix the problem (put the function in your class VEC) :

void operator = (const VEC &v)
{ 
      dim = v.dim;
      data = v.data;
      if(data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
}

[Naughty Albatross]

Does this help you? :)

[Peter87]

The problem is what you suspected, the rule of three. You need to implement the copy constructor and the copy assignment operator so that the VEC objects can be copied correctly.

closed account 5a8Ym39o6 wrote:

Also, free the (data) only when it is not NULL. Freeing a zero address also causes undefined behavior.

This is not true. Using delete on an null pointer is perfectly safe and will do nothing.

[Kaisky]

So after inserting your code i had to delete the '&' to make it work.
void operator = (const VEC v)
Sadly this also didnt fix anything, to the contrary even with commenting the delete now the programm freezes at the very first frame.

I tried to find out from where my destructer gets called, so i implemented Log's everywhere until i got the line.
Well, the destination was before even the main function gets called.

int WINAPI WinMain(HINSTANCE hInstance,
	HINSTANCE hPrevInstance,
	LPSTR lpCmdLine,
	int nCmdShow)
{
	Log("WinMain");
...

This Log didnt get called at all, while this one

VEC::~VEC() {
	Log("\nVEC Destructor:\n");
	Log("data[0]", data[0]);
	if(data) delete[] data;
}

gets called even twice, then it breaks.

What does it mean, i thought my program always starts at the main() or WinMain() function?

edit:
didnt read the other 2 comments while i was writing this as answer to the first one.
first i must say sorry, your code didnt cause the freeze, it was my "log" because my programm tries to open and close a .txt file about 100 time per ms. after deleting the logs it worked with your lines, but still only when commenting the delete line.

edit2 @Peter87:
Yeah google told me the same, sadly im not that much into c++ yet to udnerstand what i have to do. What is even an assignment operator? This is probably a dumb question, sorry.

[Naughty Albatross]

> So after inserting your code i had to delete the '&' to make it work.

I put the (&) operator the wrong place. The correct syntax should be :
void operator= (const VEC &v)

[Kaisky]

Ok, done, but like i said many times, i dont use this constructor at all.
I deleted it now from the code, so there are only the two constructers left:

// default constructer
VEC::VEC() : data (NULL) {}

// constructer dimension only
VEC::VEC(int d) {
	dim = d;
	data = new double[d];
}

With an empty destructor everything works now except the increasing RAM usage.
But upon

VEC::~VEC() {
	Log("\nVEC Destructor:\n");
	Log("data[0]", data[0]);
	if(data) delete[] data;
}

it crashes before the WinMain function gets called.
my logs.txt file looks like this:

VEC Destructor:
data[0] = 0
VEC Destructor:
data[0] = -1.45682e+144

I feel like i have to write my program from the beginning...
Would it help if i upload the whole code? (4 code files, ~1400 lines in sum and uses direct x 11 sdk)

[Naughty Albatross]

It is because in your VEC's default constructor, you didn't also initialize the member (dim)

Which may cause a big trouble when you attempt to copy VEC objects one after another.

VEC::VEC() : data (NULL), dim(0) {}

[Naughty Albatross]

Also, when logging, DO NOT attempt to dereference a pointer that you are not sure if it is valid. Attempting to dereference a pointer that points to an invalid or garbage address also causes undefined behavior.

So :

VEC::~VEC() {
	Log("\nVEC Destructor: ");
	Log("data's address == ", data);
	if(data) {delete[] data; data = NULL; dim = 0;}
}

Edit : Resetting your member values is a very effective way to prevent undefined behavior.

[Kaisky]

edit: only read first answer yet, gonna try your 2nd now.

Done, didnt change anything.
But I Log'd the dim as well in the destructor:

VEC::~VEC() {
	Log("\nVEC Destructor:");
	Log("data[0]", data[0]);
	Log("dim", dim);
	if(data) delete[] data;
}

and got in my txt file

VEC Destructor:
data[0] = 0
dim = 2
VEC Destructor:
data[0] = -1.45682e+144
dim = 2

so it seem to be caused by the not default constructer:

// constructer dimension only
VEC::VEC(int d) {
	dim = d;
	data = new double[d];
}

that is really weird, even after appending
for (int i = 0; i < d; ++i) data[i] = 0;
to the end of this constructer, my log file still gives the same result.

so maybe i have one VEC and the destructor gets called twice, thats why it outputs 0, and then a garbage value before crash? but why does if(data) not fix it...

thank you for your patience tho...

[Naughty Albatross]

You can see I just edited my post & solution. Apply my new solution as let us know your program results.

[Kaisky]

Edited the destructor to

VEC::~VEC() {
	Log("\nVEC Destructor:");
	Log("data[0]", data[0]);
	Log("dim", dim);
	if (data) {
		delete[] data;
		data = NULL;
		dim = 0;
	}
}

The result is the same, also the log file still gives

VEC Destructor:
data[0] = 0
dim = 2
VEC Destructor:
data[0] = -1.45682e+144
dim = 2

.. hm i dont even know anymore

[Naughty Albatross]

You are doing the opposite. I have always told you to stop dereferencing the (data) pointer - do not access the element data[0]!!!

So :
Log("data[0] == ", data[0]);

Just log this :

	Log("data's address == ", data);
	Log("dim == ", dim);

[Thomas1965]

Could you maybe post the whole code. The problem might be somewhere else.

[Kaisky]

Sorry, i didnt see the difference at the beginning.

Had do adjust my Log function so it can log the adress.
I get

VEC Destructor:
data's address = 00CECE30
VEC Destructor:
data's address = 00CECE30

yeah... nice so now we know the delete[] data; gets called twice for the same data? that probably causes the error? but why does if(data) not work to prevent this?

[Kaisky]

@Thomas1965 : ill upload the whole code, just a second.
edit: here http://www.file-upload.net/download-11752695/code.zip.html
you need to add $(DXSDK_DIR)Include and $(DXSDK_DIR)Lib\x86 to the properties like this:
http://puu.sh/pVT43/10cb24786e.png (if u use visual studios..)

in this version it should work - in "MATHS.h" line 80 you can remove the comment for delete[] data that causes the crash.