C++ pointer is allocated -> error: pointer being freed was not allocated

Question

I have a destructor which checks if a pointer has been allocated, if it has, it deletes it:

ShadeRec::~ShadeRec(){

    cout << "before deleting ShadeRec" << endl;
    if(material){
        cout << material << endl;
        delete material;
        material = NULL;
    }
    cout << "after deleting ShadeRec" << endl;
}

The first pointer goes through fine and then on the second one the program gives me the error.

I have checked with the couts and there is something inside the pointer, which makes sense as it got into the if statement... so why is it giving me the error?

Constructors:

ShadeRec::ShadeRec(World& world)
    :   hit(false),
        material(NULL),
        hitPoint(),
        localHitPoint(),
        normal(),
        ray(),
        depth(0),
        colour(0),
        t(0.0),
        w(world)
{}

ShadeRec::ShadeRec(const ShadeRec& sr)
    :   hit(sr.hit), 
        material(sr.material), 
        hitPoint(sr.hitPoint), 
        localHitPoint(sr.localHitPoint), 
        normal(sr.normal), 
        ray(sr.ray), 
        depth(sr.depth), 
        colour(sr.colour), 
        t(sr.t),
        w(sr.w)
{}

Material operator=

Material& 
Material::operator= (const Material& rhs) {
    if (this == &rhs)
        return (*this);

    return (*this);
}

Matte is a child of Material:

Matte* matte1 = new Matte;

which as both of these:

Matte& operator= (const Matte& rhs);

virtual Material* clone(void) const;

Are you intializing `material` correctly and have you followed the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — parapura rajkumar, Mar 27 '13 at 21:23
@RupertCobbe-Warburton: Ah, gotcha. :-) Can you post the code where you allocate `material`? — RichieHindle, Mar 27 '13 at 21:25
May you post the constructor of the `ShadeRec` class and the main please? — Shoe, Mar 27 '13 at 21:25
@RupertCobbe-Warburton: Show *all* mehods that set or modify `material`. If these methods have a parameter which is stored in `material`, also show us the caller. And its caller(s), up to where the pointer itself comes from (`new` or whatever else you are using). — Daniel Frey, Mar 27 '13 at 21:59
@RupertCobbe-Warburton: Not really complete, read you question: `material` is set to `NULL` or the pointer is copied. Anyways, I'll guess the rest. See my edited answer in a minute. — Daniel Frey, Mar 27 '13 at 22:11

Daniel Frey · Accepted Answer · 2013-03-27T22:12:15.890

Most likely, it is this:

material is a member variable if ShadeRec
Your constructor of ShadeRec takes/sets the pointer
The copy-ctor is not defined or, if it is, just copies the pointer, not creating a new instance of the object it points to.
There are at least two instances of ShadeRec, maybe it's copied as a parameter or return value.
The first dtor deletes the object and set its own pointer to NULL
The second dtor works on a different object, its pointer is still set
Bam!

To make it visible, also print this in your dtor and you'll see it's a different instance of ShadeRec.

After your edit: It's this line:

material(sr.material),

which should create a copy of the object, not just copy the plain pointer. Another option, often preferable, is using a std::shared_ptr (if you can use C++11, otherwise check out boost::shared_ptr), but be aware that copies of ShadeRec will then share the same material instance.

Given what you have shown so far, replace the above line from the copy-ctor with

material(sr.material->clone()),

and see if it works.

For *all* pointers where it applies? Also, your `operator=` is broken and needs to be fixed in a similar manner. And since you haven't posted all relevant code and are turned your problem into a small, compilable example (which you show always do), I can't help you any further, sorry. — Daniel Frey, Mar 27 '13 at 22:31
I think this is what I use in the Matte case: `Matte& Matte::operator= (const Matte& rhs){ if (this == &rhs) return (*this); Material::operator=(rhs); if (ambientBRDF) { delete ambientBRDF; ambientBRDF = NULL; } if (rhs.ambientBRDF) ambientBRDF = rhs.ambientBRDF->clone(); if (diffuseBRDF) { delete diffuseBRDF; diffuseBRDF = NULL; } if (rhs.diffuseBRDF) diffuseBRDF = rhs.diffuseBRDF->clone(); return (*this); }` — Rupert Cobbe-Warburton, Mar 27 '13 at 22:36

Shoe · Answer 2 · 2013-03-27T21:53:14.823

Problem

You are not defining a deep copy constructor. When you make a copy of a ShadeRec object the pointers are copied as they are therefore leading to a conflict between the two classes.

When using it like:

ShadeRec a;
ShadeRec b(a);

both instances contains the same address, which means that when the first one frees his pointer correctly, the second will still have to free its pointers (which is already freed by the other one).

Solution examples

Here's an example of a correct deep copy constructor. Considering material to be a pointer to a dynamic allocated Material object and that the Material class has a properly defined copy constructor:

ShadeRec::ShadeRec(const ShadeRec& sr)
    :   hit(sr.hit), 
        material(0), // <--- NULL 
        hitPoint(sr.hitPoint), 
        localHitPoint(sr.localHitPoint), 
        normal(sr.normal), 
        ray(sr.ray), 
        depth(sr.depth), 
        colour(sr.colour), 
        t(sr.t),
        w(sr.w)
{
    if (sr.material)
        material = new Material(*(sr.material));
}

Here's an example of how to write a proper assignment operator:

Material& 
Material::operator= (const Material& rhs) {
    if (this != &rhs) {
        if (material)
            delete material;
        // make member per member assignments here
        material = new Material(*(rhs.material));
    }
    return (*this);
}

More

If you can, you should use smart pointers which, as of C++11, are implemented as std::shared_ptr on the STL or, previous to C++11, available in the boost library as boost::shared_ptr.

@RupertCobbe-Warburton, no, one is gettind deallocated and the other one tried to deallocate the same memory. — Shoe, Mar 27 '13 at 21:31
still..I think the problem is deeply rooted by the fact that I actually call a child class "Matte" which then interacts with another child called Lambertian of class BRDF — Rupert Cobbe-Warburton, Mar 27 '13 at 22:07

C++ pointer is allocated -> error: pointer being freed was not allocated

2 Answers2