Like Tree5Likes
  • 3 Post By laserlight
  • 2 Post By Elysia

Problem using free() with and array of objects

This is a discussion on Problem using free() with and array of objects within the C++ Programming forums, part of the General Programming Boards category; I am making an object that converts any four points into tessellated plane with a certain precision. To do that, ...

  1. #1
    Registered User
    Join Date
    Jul 2011
    Posts
    22

    Question 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
    Last edited by jakebird451; 08-08-2011 at 07:08 PM.

  2. #2
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    Using x+1 when x+1 is not a valid subscript seems like a Bad Idea. (Ditto for y+1.)

  3. #3
    Registered User
    Join Date
    Jul 2011
    Posts
    22
    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!

  4. #4
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    You're going over the line in your other for loop as well.

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,401
    Quote 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;
    Salem, Elysia and iMalc like this.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Registered User
    Join Date
    Jul 2011
    Posts
    22
    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);
    }

  7. #7
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    C++ doesn't complain about out-of-bounds array accesses, ever, for anything.

  8. #8
    Registered User
    Join Date
    Jul 2011
    Posts
    22
    Quote Originally Posted by laserlight View Post
    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!

  9. #9
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,420
    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.
    jakebird451 and iMalc like this.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    Registered User
    Join Date
    Jul 2011
    Posts
    22
    Quote Originally Posted by Elysia View Post
    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.

  11. #11
    Registered User
    Join Date
    Jul 2011
    Posts
    22
    Could I use the following code?

    Code:
    pts = new Point3D[(tessCountX+1)*(tessCountY+1)];
    Or would such blasphemy make the C++ Gods angry?

  12. #12
    Registered User
    Join Date
    May 2011
    Location
    Around 8.3 light-minutes from the Sun
    Posts
    1,866
    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.
    Quote Originally Posted by anduril462 View Post
    Now, please, for the love of all things good and holy, think about what you're doing! Don't just run around willy-nilly, coding like a drunk two-year-old....
    Quote Originally Posted by quzah View Post
    ..... Just don't be surprised when I say you aren't using standard C anymore, and as such,are off in your own little universe that I will completely disregard.
    Warning: Some or all of my posted code may be non-standard and as such should not be used and in no case looked at.

  13. #13
    Registered User
    Join Date
    Jul 2011
    Posts
    22
    Quote Originally Posted by AndrewHunter View Post
    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)

  14. #14
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,401
    Quote 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.

    Quote 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++.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  15. #15
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,420
    Quote Originally Posted by jakebird451 View Post
    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."
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Dynamic two dimensional array free problem
    By nickman in forum C Programming
    Replies: 14
    Last Post: 01-13-2011, 11:44 AM
  2. problem with loading array of class objects
    By McElmurry in forum C++ Programming
    Replies: 1
    Last Post: 12-04-2010, 02:34 AM
  3. C array/malloc/free problem
    By Weasel in forum C Programming
    Replies: 6
    Last Post: 10-17-2007, 03:24 AM
  4. Stack and Free Store Objects
    By Frank_Li in forum C++ Programming
    Replies: 9
    Last Post: 05-16-2006, 01:37 PM
  5. Replies: 4
    Last Post: 10-16-2003, 11:26 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21