"Pointer being freed was not allocated" when adding elements to a storage

0
votes

I'm getting the error "Pointer being freed was not allocated" everytime i want to add an element to a std::map in a storage class. I have added some "couts" in the constructor and destructor to debug and the output is:

new del del

So it seems the destructor is called twice, in the main() and when adding a new Element.

Here is the Code:

Container.h

#include <map>
#include "Element.h"

using std::map;

class Container
{
private:
    map<string, Element> storage;

public:
    Container();
    ~Container();

    void add(Element e);
    void remove(string s);

};

Container.cpp

#include "Container.h"

Container::Container()
{
}

Container::~Container()
{
}

void Container::add(Element e)
{
    storage.insert(pair<string, Element>(e.getS(), e)); // CRASH
}

void Container::remove(string s)
{
    storage.erase(s);
}

Element.h

#include <iostream>
#include <string>

using namespace std;

class Element
{
private:
    string *s;

public:
    Element();
    Element(string s);

    ~Element();

    string getS();
};

Element.cpp

#include <iostream>
#include "Element.h"


Element::Element()
{
    s = new string("std cons");
    std::cout << "new" << std::endl;
}

Element::Element(string s2)
{
    s = new string(s2);

    std::cout << "new" << std::endl;
}

Element::~Element()
{
    std::cout << "del" << std::endl;
    delete s;
}

string Element::getS()
{
    return *this->s;
}

main.cpp

#include <iostream>
#include "Element.h"
#include "Container.h"

int main(int argc, const char * argv[])
{
    Element e("l");
    Container c;
    c.add(e);

    return EXIT_SUCCESS;
}
2
c++c++11

2 Answers

1
votes

The problem here is there is the default copy constructor on Element. It essentially does a memberwise copy of the fields which doesn't allocate new memory. 

Element e1("l");
Element e2(e1);

At this point there are 2 Element values but the memory for E::s has only been allocated once. Each Element value though will attempt to free the memory and hence you end up with a double free error.

To fix this you need to ensure every copy of Element has its own copy of the memory to manage

Element::Element(Element& other) 
{
  s = new string(other.s);
}

Or better yet just hold Element:s by value 

class Element
{
  string s;
  ...
};
1
votes

The reason of the error is that you defined neither a copy constructor nor a copy assignment operator for class Element.

In this case implicitly defined by the compiler corresponding functions simply copy the pointer to the string.

As the result two objects point to the same string. When one object is deleted then the second object has invalid pointer. And when it is deleted in turn you get the error.

Also there is no any need to define data member of type pointer to std::string. It would be much simpler if you would defined this data member as

class Element
{
private:
    string s;

public:
    Element();
    Element( const string &s );

    ~Element();

    string getS() const;
};

In this case the definitions could look as

#include <iostream>
#include "Element.h"


Element::Element() : s( "std cons" )
{
    std::cout << "new" << std::endl;
}

Element::Element( const string &s2 ) : s( s2 )
{
    std::cout << "new" << std::endl;
}

Element::~Element()
{
    std::cout << "del" << std::endl;
}

string Element::getS() const 
{
    return s;
}