C++

Forum

 
Double free error...but the memory is only being freed once!

declan (87)
Hey guys. I made a change to my program, which was working fine, and now it is freaking out about me double freeing a pointer. Here's the error it throws:

1
2
3
4
5
6
7
8
9
10
11
12
 
rundsmc3d(81558,0xa00cb720) malloc: *** error for object 0x5450: Non-aligned pointer being freed (2)
*** set a breakpoint in malloc_error_break to debug
rundsmc3d(81558,0xa00cb720) malloc: *** error for object 0x13127660: double free
*** set a breakpoint in malloc_error_break to debug
rundsmc3d(81558,0xa00cb720) malloc: *** error for object 0x13127660: double free
*** set a breakpoint in malloc_error_break to debug
rundsmc3d(81558,0xa00cb720) malloc: *** error for object 0x13127660: double free
*** set a breakpoint in malloc_error_break to debug
rundsmc3d(81558,0xa00cb720) malloc: *** error for object 0x13127660: double free
*** set a breakpoint in malloc_error_break to debug
rundsmc3d(81558,0xa00cb720) malloc: *** error for object 0x13127660: double free
*** set a breakpoint in malloc_error_break to debug


And continues that until I kill it. First of all, yes, the section of code in question, and the whole program, is looping, but why doesn't it crash after the first error? Why does it keep looping and producing more errors?

Secondly, here is the source of the ticking. Letting it crash in the debugger and backtracing invariably leads to it being at this line: Unless there's something I've missed, there's no possible way to try and delete a pointer that has already been deleted, or never had any memory allocated to it:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
176 bool flag = true;
177while(flag){
178          //This goes from the _original_ position to the new position. If the "resultant" line crosses the plane we're leaving on its path, we need new velocities.
179         double * correctionPtr = last_wall_hit->lineIntersect(position_old_old(i,1),position_old_old(i,2),position_old_old(i,3),partList.position(i,1),partList.position(i,2),partList.position(i,3));
180         if(0<=correctionPtr[3] && correctionPtr[3]<=1){//Reassign the velocities.
181                                                 
188                  velPtr = last_wall_hit->redistributeVels(partList.velocity(i,1),partList.velocity(i,2),partList.velocity(i,3));
189                  partList.velocity(i,1) = velPtr[0];
190                  partList.velocity(i,2) = velPtr[1];
191                  partList.velocity(i,3) = velPtr[2];
192                  delete [] velPtr;
193                  velPtr = NULL;
194                  partList.position(i,1) = partList.velocity(i,1)*tau_remaining + position_old(i,1);      //Here we move the particle for the remaining timestep.
195                  partList.position(i,2) = partList.velocity(i,2)*tau_remaining + position_old(i,2);
196                  partList.position(i,3) = partList.velocity(i,3)*tau_remaining + position_old(i,3);
197            }       
198            else{//If we don't need to because it doesn't collide with the wall along the "resultant" line, the new velocities are fine.
199                  flag = false;
200           }       
201           delete [] correctionPtr;
202           correctionPtr = NULL;
203//cout << "New velocities: " << vel(i,1) << ", " << vel(i,2) << ", " << vel(i,3) << endl;
204}//while


The line in question is line 201 here, delete [] correctionPtr. But it will never be double deleting anything, will it? I'm confused.

Thanks!

Ugh, sorry about the stupid indentation, it messed it up from vim to here and I had to do it by hand.
Last edited on
Disch (13742)
Returning allocated buffers from functions is a really bad idea for many reasons. You probably should just be passing the buffer to the function as an 'out' parameter. That will avoid all of these sloppy new/delete mismatch problems because you won't have to new/delete anything.

It'd be better for performance too, since you wouldn't be needlessly allocating a freeing tiny chunks of memory constantly, which could fragment memory pretty bad.


But that said... the problem doesn't seem to be in the code you pasted. I might be in lineIntersect. Are you always returning a new[]'d pointer?
declan (87)
Ok, I will actually probably take your advice here and redo a lot of code, because I've been doing this new/delete crap too long and I can tell it's gotta be risky. I actually asked this before, but haven't really had the time to change it:

http://www.cplusplus.com/forum/general/26052/

But yes, here's lineIntersect:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
 
double* lineIntersect(double x_a,double y_a,double z_a,double x_b,double y_b,double z_b){//Find the point where a line intersects a plane. Also returns t, which can tell whether the point is between the two given points, or not.

	/*The old method.
	double t = (-1*d - (x_a*normal[0] + y_a*normal[1] + z_a*normal[2]))/((x_b-x_a)*normal[0] + (y_b-y_a)*normal[1] + (z_b-z_a)*normal[2]);
	*/
	double x_a0 = x_a - p0[0];
	double y_a0 = y_a - p0[1];
	double z_a0 = z_a - p0[2];
	
	double original[4][4];
	original[1][1] = x_a-x_b;
	original[1][2] = vec1[0];
	original[1][3] = vec2[0];
	original[2][1] = y_a-y_b;
	original[2][2] = vec1[1];
	original[2][3] = vec2[1];
	original[3][1] = z_a-z_b;
	original[3][2] = vec1[2];
	original[3][3] = vec2[2];
	double inverseOfDet = 1/(threeDet(original[1][1],original[1][2],original[1][3],original[2][1],original[2][2],original[2][3],original[3][1],original[3][2],original[3][3]));	//We need 1/|A|.
	//cout << "invDet: " << inverseOfDet << endl;
	//Here we simulataneously find the inverse and multiply it by a vector. Look at "line-plane intersection" on wiki for more info.
	double t = x_a0*twoDet(original[2][2],original[2][3],original[3][2],original[3][3]) + y_a0*twoDet(original[1][3],original[1][2],original[3][3],original[3][2])+ z_a0*twoDet(original[1][2],original[1][3],original[2][2],original[2][3]);
	double u = x_a0*twoDet(original[2][3],original[2][1],original[3][3],original[3][1]) + y_a0*twoDet(original[1][1],original[1][3],original[3][1],original[3][3])+ z_a0*twoDet(original[1][3],original[1][1],original[2][3],original[2][1]);
	double v = x_a0*twoDet(original[2][1],original[2][2],original[3][1],original[3][2]) + y_a0*twoDet(original[1][2],original[1][1],original[3][2],original[3][1])+ z_a0*twoDet(original[1][1],original[1][2],original[2][1],original[2][2]);

	t *= inverseOfDet;	//Add the 1/|A| term in.
	u *= inverseOfDet;
	v *= inverseOfDet;
	
	double x_isect = x_a + (x_b-x_a)*t;	//Find where the line actually intersects the plane.
	double y_isect = y_a + (y_b-y_a)*t;
	double z_isect = z_a + (z_b-z_a)*t;
	double * intersect;
	try{
		intersect = new double[6];
	}
	catch(bad_alloc){
		cout << endl << "Error when allocating memory for lineIntersect in Plane.h" << endl;
	}
	intersect[0] = x_isect;
	intersect[1] = y_isect;
	intersect[2] = z_isect;
	intersect[3] = t;
	intersect[4] = u;
	intersect[5] = v;

	return intersect;
}//lineintersect


Allocates it in the try statement.
Last edited on
declan (87)
Returning allocated buffers from functions is a really bad idea for many reasons. You probably should just be passing the buffer to the function as an 'out' parameter. That will avoid all of these sloppy new/delete mismatch problems because you won't have to new/delete anything.

It'd be better for performance too, since you wouldn't be needlessly allocating a freeing tiny chunks of memory constantly, which could fragment memory pretty bad.


I think I get what you mean, but I'm a little unclear. Do you mean, I pass the function an array I locally declared from stack memory, so I don't have to worry about it when it goes out of scope?
Disch (13742)
Basically you're doing this:

1
2
3
4
5
6
7
8
9
10
11
12
13
 
int* func()
{
  int* temp = new int[6];
  // fill temp
  return temp;
}

int main()
{
  int* foo = func();
  // use foo
  delete[] foo;
}


This is bad because it overuses dynamic memory allocation, and passes ownership of the buffer between functions, which makes for very sloppy and hard to maintain code (which leads to the problems you're having now.

Here is the generally better way to do it:

1
2
3
4
5
6
7
8
9
10
11
 
void func(int* foo)
{
  // fill foo here
}

int main()
{
  int foo[6];
  func(foo);
  // use foo here
}
declan (87)
Cool, that's actually what I just implemented. Is this actually faster than the dynamic allocation thing?
Disch (13742)
It can be. Though the speed difference (if any) will be tiny.

Really, the main benefit is it keeps the code much cleaner, and reduces memory fragmenting (which could slow the program down after a while, and make the program consume much more memory than it really need to)
declan (87)
Well, I never found the error, but I changed all my code to that, and it is gone now. Bad practice indeed!
Topic archived. No new replies allowed.