Error EXC_BAD_ACCESS in list with SmartPointer

Question

I'm facing this problem when I remove an element from my list.

Here my list.h:

class AwesomeList {
friend class Iteratore;
private:
class Nodo;

class SmartPointer {
public:
    Nodo* punt;

    SmartPointer(Nodo* n = 0): punt(n) {}
    SmartPointer(const SmartPointer& ptr): punt(ptr.punt) {}
    ~SmartPointer() {
        delete punt;
    }

    SmartPointer& operator=(const SmartPointer& ptr) {
        if (this != &ptr) {
            delete punt;
            punt = ptr.punt;
        }
        return *this;
    }
    bool operator==(const SmartPointer& ptr) const {
        return ptr.punt == punt;
    }
    bool operator!=(const SmartPointer& ptr) const {
        return ptr.punt != punt;
    }
    Nodo* operator->() const {
        return punt;
    }
    Nodo& operator*() const {
        return *punt;
    }
};

class Nodo {
public:
    T* value;
    SmartPointer next;

    Nodo(T* t = T(), const SmartPointer& ptr = SmartPointer()): value(t), next(ptr) {}
};

SmartPointer head;
SmartPointer tail;

public:
class Iteratore{
    friend class AwesomeList;
private:
    AwesomeList::SmartPointer punt;
public:
    bool operator==(const Iteratore& it) const {
        return it.punt == punt;
    }
    bool operator!=(const Iteratore& it) const {
        return it.punt != punt;
    }
    Iteratore& operator++() {
        if(punt != 0) punt = punt->next;
        return *this;
    }
    Iteratore& operator++(int) {
        if(punt != 0) punt = punt->next;
        return *this;
    }
    T* operator*() const {
        if (punt != 0) return punt->value;
    }
};

AwesomeList(const SmartPointer& ptr = 0): head(ptr), tail(0) {
    if (head != 0) {
        SmartPointer p = head;
        while (p != 0)
            p = p->next;
        tail = p;
    }
}
AwesomeList(const AwesomeList& list): head(list.head), tail(list.tail) {}

AwesomeList& operator=(const AwesomeList& list) {
    head = list.head;
    tail = list.tail;
}

int getSize() const {
    int count = 0;
    SmartPointer p = head;
    while (p != 0) {
        p = p->next;
        count++;
    }
    return count;
}
bool isEmpty() const {
    return getSize() == 0;
}
T* at(int pos) const {
    if (pos > -1 && pos < getSize()) {
        SmartPointer p = head;
        while (pos--) {
            p = p->next;
        }
        return p->value;
    } else return 0;
}
void add(const T& t) {
    if (head == 0) {
        head = SmartPointer(new Nodo(&(const_cast<T&>(t))));
        tail = head;
    } else {
        tail->next = SmartPointer(new Nodo(&(const_cast<T&>(t))));
        tail = tail->next;
    }
}
void remove(int pos) {
    if (pos > -1 && pos < getSize()) {
        SmartPointer newHead = head;
        SmartPointer p = newHead;
        head = 0;
        while (pos--) {
            add(*p->value);
            p = p->next;
        }
        p = p->next;
        while (p != 0) {
            add(*p->value);
            p = p->next;
        }
    }
}
void replace(int pos, T* t) {
    if (pos > -1 && pos < getSize()) {
        SmartPointer p = head;
        while (pos--)
            p = p->next;
        p->value = t;
    }
}
void replace(int pos, const T& t) {
    if (pos > -1 && pos < getSize()) {
        SmartPointer p = head;
        while (pos--)
            p = p->next;
        T& t_obj = const_cast<T&>(t);
        p->value = &t_obj;
    }
}

Iteratore begin() const {
    Iteratore it;
    it.punt = head;
    return it;
}
Iteratore end() const {
    Iteratore it;
    it.punt = 0;
    return it;
}
T* operator[](const Iteratore& it) const {
    return it.punt->value;
}
};

This are the tests I made:

AwesomeList<int> list = AwesomeList<int>();
list.add(1);
list.add(2);
list.add(3);
for (int i = 0; i < list.getSize(); i++)
    qDebug() <<*(list.at(i)) <<" ";

list.remove(-1);
for (int i = 0; i < list.getSize(); i++)
    qDebug() <<*(list.at(i)) <<" ";

list.remove(2);
for (int i = 0; i < list.getSize(); i++)
    qDebug() <<*(list.at(i)) <<" ";

list.replace(0, 5);
qDebug() <<"Replace in posizione 0";
auto cit = list.begin();
for (; cit != list.end(); cit++)
    qDebug() <<*(*cit) <<" ";

qDebug() <<"Size";
qDebug() <<list.getSize() <<endl;

This is the lines where errors appears:

AwesomeList::Nodo::~Nodo() + 16 (awesomelist.h:8)
AwesomeList::SmartPointer::~SmartPointer() + 42 (awesomelist.h:21)
AwesomeList::SmartPointer::~SmartPointer() + 21 (awesomelist.h:22)

Any help is appreciate. Thanks!

UPDATE

I solved my problem changing SmartPointer and Nodo classes like this:

SmartPointer(Nodo* n = 0): punt(n) {
        if (punt) punt->references++;
    }
    SmartPointer(const SmartPointer& ptr): punt(ptr.punt) {
        if (punt) punt->references++;
    }
    ~SmartPointer() {
        if (punt) {
            punt->references--;
            if (punt->references == 0) delete punt;
        }
    }

    SmartPointer& operator=(const SmartPointer& ptr) {
        if (this != &ptr) {
            Nodo* n = punt;
            punt = ptr.punt;
            if (punt) punt->references++;
            if (n) {
                n->references--;
                if (n->references == 0) delete n;
            }
        }
        return *this;
    }
    bool operator==(const SmartPointer& ptr) const {
        return ptr.punt == punt;
    }
    bool operator!=(const SmartPointer& ptr) const {
        return ptr.punt != punt;
    }
    Nodo* operator->() const {
        return punt;
    }
    Nodo& operator*() const {
        return *punt;
    }
};

class Nodo {
public:
    T* value;
    SmartPointer next;
    int references;

    Nodo(T* t = T(), const SmartPointer& ptr = SmartPointer()): value(t), next(ptr), references(0) {}
};

Your error list doesn't actually list what the errors are... — kmdreko, May 13 '16 at 17:32
My friend, I think you are trying to fool us. `SmartPointer head;` cannot be compiled with only a forward declaration of `SmartPointer`. You need the full definition of `SmartPointer` to instantiate a `SmartPointer`. — user4581301, May 13 '16 at 18:57
Nodo is clearly supposed to be a template, but is not declared as one. Methinks you dropped the `template` ahead of `class AwesomeList` — user4581301, May 13 '16 at 19:02
@user4581301, the program compiles, don't worry. I asked help for the error EXC_BAD_ACCESS. template is present. I have not included in the message because it created problems. — Daniele, May 13 '16 at 19:06
If the code compiles, you are doing something else that you aren't showing. To allocate storage for `head`, the size of `SmartPointer` must be available. It won't be available for another 30-or-so lines. — user4581301, May 13 '16 at 19:18

score 1 · Accepted Answer · answered May 13 '16 at 18:53

1

Sorry but I don't understand the ratio beneath your SmartPointer class.

It carry a pointer to a Nodo and delete it with the constructor. Good.

But, if I'm not wrong

(1) when you create a SmartPointer with copy constructor, you copy the pointer from the SmartPointer copied so you have two object with a punt with the same value; when you destroy the two objects, you call delete two times over the same pointer; this can crash the program

(2) when you call operator=, you have the same problem with the copy constructor but, moreover, you don't delete the old pointed value

By example, look at add()

    head = SmartPointer(new Nodo(&(const_cast<T&>(t))));
    tail = head;

You create a temporary SmartPointer object initializing it with new Nodo(&(const_cast<T&>(t))). Next you copy this temporary object in head, so both head and the temporary object are carrying the same not-NULL pointer. Now the temporary object is destroyed, so the memory pointed by punt is deleted but head (his punt) continue to point to a memory area that is deleted. Now you copy head in tail, and you have both head and tail that are pointing to the same deleted area.

Look at the else case

    tail->next = SmartPointer(new Nodo(&(const_cast<T&>(t))));
    tail = tail->next;

In this case, tail->next (and take in count that take point to a deleted area) receive a pointer from a temporary object that delete it. So you write in a deleted area a pointer that is immediately deleted.

I hope it is clear how much all this is dangerous.

Suggestion: redesign SmartPointer class.

p.s.: sorry for my bad English.

answered May 13 '16 at 18:53

max66

65,235
10
71
111

You have been very cleared. Thanks! In your opinion, if I use a private field SmartPointer* head, and then I write head = new SmartPointer(...), could solve my problem? – Daniele May 13 '16 at 19:12
Think on the logic of that for a moment. You want to create a raw pointer to a smart pointer. This defeats the point of the smart pointer. Max is correct. You need to re-implement `SmartPointer`, make it [rule of 5 compliant](http://en.cppreference.com/w/cpp/language/rule_of_three), and [use move semantics](http://stackoverflow.com/questions/3106110/what-are-move-semantics). An alternative is [use `std::unique_ptr`](http://en.cppreference.com/w/cpp/memory/unique_ptr) instead of `SmartPointer`. – user4581301 May 13 '16 at 19:27
Yes, you right. Any suggestion? PS:I've update my code with a correct operator=. – Daniele May 13 '16 at 19:51
@ Daniele - Yes, checking if you're copying a object on itself is good. But the main problem remain: you can have multiple instances of `SmartPointer` with the same value for `punt` and, so, you risk to delete two or more time the same pointer. – max66 May 13 '16 at 20:50
1

@Daniele - I suppose you're developing this code to make practice with C++ (otherwise, my suggestion is: use `std::list` and, if you can use C++11, `std::shared_ptr`) so I suggest (just to play with C++): add a counter in `Nodo` and initialize it to zero; every time you copy a `Nodo` pointer to a `SmartPointer`, increment the counter in the node pointed; every time you release a pointer from a `SmartPointe` (destructor and old value in assignment operator), decrement the node counter and verify the value; call `delete` only when the counter (post decrement) is zero. Just to play with C++. – max66 May 13 '16 at 20:52

Error EXC_BAD_ACCESS in list with SmartPointer

1 Answers1