Problem using free() with and array of objects

I am making an object that converts any four points into tessellated plane with a certain precision. To do that, I have an array of "Point3D"s that I use to store information for where each part is do be drawn. Most of this is no problem, just some simple math. I probably have an error with the implementation somewhere to make the boxes to draw properly (I couldn't test it because I am caught in this snag). I have encountered a similar error in another project, but I forgot how I overcame it.

I would rather not post all of Point.h and Point3D.cpp since they are fairly large and does not contain any pointers (its a fairly simple class with 3 private doubles with implementation though it's various methods). However, the error is isolated within this small piece of code as you can see below. The program causes a sigabrt at free() which I noted in the code. The full error reported to me from XCode was:

"Project(11947,0x7fff765d5960) malloc: *** error for object 0x10035df88: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug"

Which doesn't make too much sense to me since free is ment to dealloc the entire variable at that location, not read it. I also set a breakpoint to see the memory before I tired freeing it and something seems to be there.

Code:

void Plane3D::drawTessellated(int tessCountX, int tessCountY)
{
    //assuming:
    //a---b
    //|   |
    //d---c
    
    Point3D *pts,left,right;
    int pos;
    
    pts = (Point3D*)malloc(tessCountX*tessCountY*sizeof(Point3D));
    if (!pts){errLog.print("Could not malloc. [Plane3D::drawRessellated(int,int)]");return;}
    pos = 0;
    
    for (double y=0;y<=1.0;y+=1.0/tessCountY)
    {
        avg(a,d,y,left);
        avg(b,c,y,right);
        for (double x=0;x<=1.0;x+=1.0/tessCountX)
        {
            avg(left,right,x,pts[pos]);
            pos++;
        }
    }
    glBegin(GL_QUADS);
    for (int y=0;y<tessCountY-1;y++)
    {
        for (int x=0;x<tessCountX-1;x++)
        {
            pts[x  +( y   *tessCountX)].useAsVertex();
            pts[x+1+( y   *tessCountX)].useAsVertex();
            pts[x+1+((y+1)*tessCountX)].useAsVertex();
            pts[x  +((y+1)*tessCountX)].useAsVertex();
        }
    }
    glEnd();
    
    if (pts){free(pts);} // death by sigabrt
}

void Plane3D::avg(Point3D in0, Point3D in1, double input, Point3D &output)
{
    output.set(avg(in0.getX(), in1.getX(), input),
               avg(in0.getY(), in1.getY(), input),
               avg(in0.getZ(), in1.getZ(), input));
}

double Plane3D::avg(double in0, double in1, double input)
{
    return (in0*(1.0-input))+(in1*input);
}

Point3D& Point3D::set(double x,double y,double z)
{
    this->x=x;
    this->y=y;
    this->z=z;
    return *this;
}

I sure hope it is a simple fix. Thanks for your help!

EDIT: Changed the max ranges inside the for statements to stay within malloced bounds of the array

Using x+1 when x+1 is not a valid subscript seems like a Bad Idea. (Ditto for y+1.)

Yes, good catch. I changed the for statement ranges to be: for (int x=0;x<tessCountX-1;x++) now. I'm not sure why that didn't just cause an abort right there. However, the same error still occurs. Thanks for the catch!

You're going over the line in your other for loop as well.

Originally Posted by jakebird451

Which doesn't make too much sense to me since free is ment to dealloc the entire variable at that location, not read it. I also set a breakpoint to see the memory before I tired freeing it and something seems to be there.

Generally, unless you are doing something special like allocating memory for use with placement new, you should be using new/new[] and delete/delete[] instead of malloc and free. In this case, you could make life even easier by using a std::vector<Point3D>.

Also, look carefully at this line:

Code:

Point3D *left,right;

You probably want right to be a pointer as well, so it should be:

Code:

Point3D *left, *right;

though I recommend:

Code:

Point3D* left;
Point3D* right;

Ah, I fixed it. It seems that arrays generated by malloc do not complain when instances are placed out of its designated range, but only when free() is called to clean it? But anyways, the final code is:

Code:

void Plane3D::drawTessellated(int tessCountX, int tessCountY)
{
    //assuming:
    //a---b
    //|   |
    //d---c
    
    Point3D *pts,left,right;
    int pos,x,y;
    
    if (tessCountX<0 || tessCountY<0){return;}
    
    pts = (Point3D*)malloc((tessCountX+1)*(tessCountY+1)*sizeof(Point3D));
    if (!pts){errLog.print("Could not malloc. [Plane3D::drawRessellated(int,int)]");return;}
    pos = 0;
    
    for (y=0;y<=tessCountY;y++)
    {
        avg(a,d,y/(float)tessCountY,left);
        avg(b,c,y/(float)tessCountY,right);
        for (x=0;x<=tessCountX;x++)
        {
            avg(left,right,x/(float)tessCountX,pts[pos]);
            pos++;
        }
    }
    glBegin(GL_QUADS);
    for (y=0;y<tessCountY;y++)
    {
        for (x=0;x<tessCountX;x++)
        {
            pts[x  +( y   *(tessCountX+1))].useAsVertex();
            pts[x+1+( y   *(tessCountX+1))].useAsVertex();
            pts[x+1+((y+1)*(tessCountX+1))].useAsVertex();
            pts[x  +((y+1)*(tessCountX+1))].useAsVertex();
        }
    }
    glEnd();
    
    free(pts);
}

C++ doesn't complain about out-of-bounds array accesses, ever, for anything.

Originally Posted by laserlight

Also, look carefully at this line:

Code:

Point3D *left,right;

You probably want right to be a pointer as well, so it should be:

Code:

Point3D *left, *right;

The line was originally:

Code:

Point3D *pts,left,right;

This line was to allocate a pointer for 1 dimensional array of points named "pts" while allocating local non-pointers of left and right both as Point3D's. I would rather have them removed automatically whenever possible to relieve the stress of unneeded pointer use.

However, the use of new and delete is a good recommendation. I use malloc only because I am more familiar with it since an MIT mentor taught me how to use malloc/realloc/free instead of new/delete back in the day. I'll have to look into it.

By the way, thank you guys for replying so fast! It was truly amazing that I got replies this fast!

malloc is, in fact, dangerous in C++, because it doesn't call the constructor. Similarly, free is dangerous too, since it doesn't call the destructor. Hence, I would suggest you avoid them.
Also by using std::vector (as has been suggested), you won't have to worry about memory management at all. It will free itself by the end of the function, just like a local variable. I figure you might like that.

Originally Posted by Elysia

malloc is, in fact, dangerous in C++, because it doesn't call the constructor. Similarly, free is dangerous too, since it doesn't call the destructor. Hence, I would suggest you avoid them.
Also by using std::vector (as has been suggested), you won't have to worry about memory management at all. It will free itself by the end of the function, just like a local variable. I figure you might like that.

Thank you very much! I never realized that. That is a very good point, I believe I will be using vectors or new for allocations from now on. Especially when allocating for objects.

Could I use the following code?

Code:

pts = new Point3D[(tessCountX+1)*(tessCountY+1)];

Or would such blasphemy make the C++ Gods angry?

This would work, however note you are limited to the fact that you cannot resize this array if you needed to later. You would have to allocate a new block and then copy the values over. Honestly, as has been suggested, using std::vector would be the way to go with this.

Also did you fix your pointer allocation line? Do note that:

Code:

Point3D *ptr, pt2, pt3

creates 1 pointer and two objects of type Point3D.

Originally Posted by AndrewHunter

Also did you fix your pointer allocation line? Do note that:

Code:

Point3D *ptr, pt2, pt3

creates 1 pointer and two objects of type Point3D.

I intended for the first variable to be a pointer and the rest to be normal instances. I should probably have left those variables on separate lines to avoid confusion.

The reason why I am going with the new[] implementation instead of the vector<Point3D> is because I only need to allocate a fixed block. Using a class to help me allocate a fixed block is unneeded. If the size was undeterminable, I would have gone with vectors. However, I do want to make sure I am using a good programming practice with my "new[]" usage. I am hesitant of using this notation due to the knowledge of a bad programming practice of "Point3D pts[calculationHere()];" (which also "works" if I am not mistaken)

Originally Posted by jakebird451

The reason why I am going with the new[] implementation instead of the vector<Point3D> is because I only need to allocate a fixed block. Using a class to help me allocate a fixed block is unneeded. If the size was undeterminable, I would have gone with vectors. However, I do want to make sure I am using a good programming practice with my "new[]" usage.

The thing is, the use of a std::vector provides you the benefit of RAII: you no longer have to worry about delete[] because the lifetime of the array is tied to the lifetime of the container object that will be destroyed when control leaves the scope of the function, whether it leaves at the end of the function or sometime before because of a return statement inserted during maintenance, or perhaps because an exception was somehow thrown or propagated.

That said, you don't have to use std::vector to get this benefit. For example, you could use a std::unique_ptr with a custom deleter if your compiler has sufficient support for the upcoming version of C++, or you could use a boost::scoped_array if Boost is available.

Originally Posted by jakebird451

I am hesitant of using this notation due to the knowledge of a bad programming practice of "Point3D pts[calculationHere()];" (which also "works" if I am not mistaken)

It is not necessarily bad programming practice, but variable length arrays are not part of standard C++.

Originally Posted by jakebird451

The reason why I am going with the new[] implementation instead of the vector<Point3D> is because I only need to allocate a fixed block. Using a class to help me allocate a fixed block is unneeded. If the size was undeterminable, I would have gone with vectors. However, I do want to make sure I am using a good programming practice with my "new[]" usage.

If you want to know good programming practice, then you would be delighted when I say that I would call using new instead of vector here bad programming practice.
The simple reason is that you do not have the benefit of RAII with new, plus you won't have the debugging support of vector which can do bounds checking and most implementations (if I'm not mistaken) will alert you if you go out of bounds with the index operator.
In short, you are giving up benefits here that you could use by not considering using vector.

I am hesitant of using this notation due to the knowledge of a bad programming practice of "Point3D pts[calculationHere()];" (which also "works" if I am not mistaken)

It doesn't work unless you have some special compiler extensions. This is not standard C++, hence it is "illegal."