Thread: 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

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;

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.

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