Trying to delete already-deleted pointer in destructor

Question

I have the following class and I am getting an error when the destructor is called and it tries to delete the pointers to a and b. It looks like they don't exist. This is the line of code triggering the problem:

unordered_map<string,Stock>* SDict = new unordered_map<string,S>();
SDict->insert(make_pair("38363",S("38363",i,w)));

Header

class O{
public:
    O();
    ~O();
    O(const O& tocopy);
    O& operator=(const O& toassign);


private:
    unordered_map<int,PQL>* b;
    unordered_map<int,PQL>* a;
};

Source

O::O(){
    a = new unordered_map<int,PQL>();
    b = new unordered_map<int,PQL>();
}

O::~O(){
    delete b; //I get the exception here- b doesn't exist before the delete.
    delete a;
}

O& O::operator=(const O& src){
    if(this != &src){
        delete b;
        delete a;

        b = new unordered_map<int,PQL>();
        b = src.b;
        a = new unordered_map<int,PQL>();
        a = src.a;
    }
    return *this;
}

O::O(const O& src){
    b = new unordered_map<int,PQL>();
    b = src.b;
    a = new unordered_map<int,PQL>();
    a = src.a;
}

PQL is a class which just has three ints. Is there something obvious which is causing this error?

class O is a data member of the following class:

Header

class S{
    public:
        S();
        S(string sid, vector<string>* indexids, vector<double>* sw);
        ~S();
        S(const S& tocopy);
        S& operator=(const S& toassign);

    private:
        string sid;
        O* o;
        vector<Strategy*> ts;
        unordered_map<string,double>* iw;
};

Source

    S::S(){}

    S::S(string sid, vector<string>* iis, vector<double>* sw){
        sid = sid;
        iw = new unordered_map<string,double>();
        o = new o();

        if(iis->size() == sw->size()){
            for(size_t i=0; i<iis->size(); i++){
                string key = iis->at(i);
                if(iw->count(key) == 0 ){
                    double weighting = sw->at(i);
                    iw->insert(make_pair(key,weighting));
                }
                else{
                    throw new exception();
                }
            }
        }
        else{
            throw new exception();
        }
    }

    S::S(const S& rhs){
        sid = rhs.sid;
        ts = rhs.ts;
        o = new O();
        o = rhs.o;
        iw = new unordered_map<string,double>();
        iw = rhs.iw;
    }

    S& S::operator=(const S& src){

        if(this != &src){
            delete o;
            delete iw;

            sid = src.sid;
            ts = src.ts;
            o = new o();
            o = src.o;
            iw = new unordered_map<string,double>();
            iw = src.iw;
        }

        return *this;
    }

    S::~S(){
        delete o;
        delete iw;
    }

I edited my answer to include the unordered_map insertion (which I think is creating a temporary S object, which causes the copy constructors and destructors to be called. — user997112, Sep 25 '13 at 18:40
wow I can not remember if I saw more memory leaks in my life o_O — BЈовић, Sep 25 '13 at 18:45
Your copy constructor is not copying the map, but copying a pointer (and leaking the map you just allocated). The copy line needs to be `b = new unordered_map(*src.b);` (similarly change `a`'s assignment). But why on earth does your class contain pointers to maps, the data member should be `unordered_map b;` — Praetorian, Sep 25 '13 at 18:46
@Everyone- i'm a newbie C++ programmer so all constructive comments most welcome — user997112, Sep 25 '13 at 18:47
[Pointers... pointers everywhere.](http://memegenerator.net/instance/32854198) But seriously though, so many naked pointers, it's not even worth fixing the code. Just rewrite everything to either use references, automatic variables, or `unique_ptr`/`shared_ptr`. — DanielKO, Sep 25 '13 at 18:48

Dietmar Kühl · Accepted Answer · 2013-09-25T19:05:07.530

3

There is an obvious problem in your copy constructor:

O::O(const O& src){
    b = new unordered_map<int,PQL>();
    b = src.b;
    a = new unordered_map<int,PQL>();
    a = src.a;
}

The assignment does assign the wrong entities: it assigns the pointers, i.e., the assignment b = src.b leaks the just allocated memory and duplicates the pointer. You probably meant to write:

O::O(O const& src)
    : b(new std::unordered_map<int, PQL>(*src.b)
    , a(new std::unordered_map<int, PQL>(*src.a)
{
}

Note, that your assignment operator is not exception safe! The general conjecture is that if your code needs to check for self-assignment to work in case of self-assignment, it is not exception safe.

The canonical implementation of assignment leverages the copy constructor and the destructor and use another common function, swap(), to exchange the resources:

O& O::operator= (O other) {
    this->swap(other);
    return *this;
}
void O::swap(O& other) {
    using std::swap;
    swap(this->b, other.b);
    swap(this->a, other.a);
}

edited Sep 25 '13 at 19:05

answered Sep 25 '13 at 18:44

Dietmar Kühl

150,225
13
225
380

Could you elaborate on your comment regarding exception-safe please? I am interested. – user997112 Sep 25 '13 at 18:55
@user997112: If an exception is thrown while allocating/copying the `std::unordered_map<...>`s the object ends up in an inconsistent state: the objects are already `delete`d and can't be recovered. – Dietmar Kühl Sep 25 '13 at 19:01
1

You might add that the first problem with his code is that he's using dynamic allocation when he shouldn't. There's almost never a case where you'd want to allocate one of the standard containers dynamically. – James Kanze Sep 25 '13 at 19:11

score 1 · Answer 2 · answered Sep 25 '13 at 18:45

Following code:

   b = new unordered_map<int,PQL>();
   b = src.b;
   a = new unordered_map<int,PQL>();
   a = src.a;

Should be changed to:

   b = new unordered_map<int,PQL>();
   *b = *src.b;
   a = new unordered_map<int,PQL>();
   *a = *src.a;

Both in copy ctor and assignment operator. So it would copy map content instead of copying pointers which is wrong behaviour. But even better way is:

   b = new unordered_map<int,PQL>( *src.b );
   a = new unordered_map<int,PQL>( *src.a );

score 1 · Answer 3 · answered Sep 25 '13 at 18:48

There are several problems with your code

O& O::operator=(const O& src){
if(this != &src){
    delete b;
    delete a;
    //b will point to newly allocated memory
    b = new unordered_map<int,PQL>();
    //now you set b to point to the same unordered map as src (double deletion will occur)
    //you will leak the memory you just allocated
    b = src.b;

Same problems with the copy constructors...

score 0 · Answer 4 · answered Sep 25 '13 at 18:43

0

your copy constructor is wrong. You need to do a deep copy instead of just use some thing like

b = src.b;

you need to copy each member of the map from source to destination.

And your assignment operator too.

answered Sep 25 '13 at 18:43

CS Pei

10,869
1
27
46

No, you don't need to copy each element in the map, `unordered_map` has a copy constructor that'll do that for you. – Praetorian Sep 25 '13 at 18:47
that's true. but that constructor still copies each element, right? – CS Pei Sep 25 '13 at 18:48

Trying to delete already-deleted pointer in destructor

4 Answers4