Malloc pointer error with queue implementation

Question

I'm trying to implement a queue in C++. The queue holds strings.

I'm using the following code:

#include <iostream> 
#include <stdlib.h> 

struct qnode {
    std::string data;//it stores a string
    qnode *link;//and has a pointer to the next node in the structure.
};

class queue { 
    private: 
        qnode *front; // Pointer to first node.
        qnode *rear;  // Pointer to last node.

    public: 
        // Constructor.
        queue();

        //Is the queue empty?
        bool empty() ;

        //peeks first element
        int first(std::string *c);

        // Adds element to the queue.
        int enqueue(std::string c);

        // Extracts an element from the queue.
        int dequeue(std::string *c);

        // Destructor.
        ~queue();
};

//////////// ACTUAL CODE /////////////

//Constructor, front and rear initially point to null.
queue::queue() { 
    front = NULL;
    rear = NULL;
}

// If front points to null then the queue is empty.
bool queue::empty() {
    return (front == NULL);     
}

int queue::first(std::string *c) {
    int res = 1;

    if (!empty()) {
        res = 0;
        *c = front->data;
    }  

    return res;
}

//Function for adding an element to the end of queue.
int queue::enqueue (std::string c){
    int res = 1;

    qnode *tmp; // Auxiliar pointer.
    tmp = new qnode; //Auxiliar new node.

    if(tmp != NULL) {
        // If the queue is empty then we store the data in tmp and make its link point to null and front and rear become tmp.
        if(empty()){
            tmp->data = c;
            tmp->link = NULL;
            front = tmp;
            rear = tmp;
            rear->link = tmp;
        } else {
            // If it is not, then last node's link point to tmp, we store the data in tmp and tmp's link point to null.
            rear->link = tmp;
            tmp->data = c;
            tmp->link = NULL;
            // Rear of the queue points to tmp now.
            rear = tmp;
        }

        res = 0;
    }

    return res;
}

// Function for extracting an element from the queue.
int queue::dequeue(std::string *c){
    int res = 1;

    // We make sure queue is not empty and access first node's data with the help of tmp.
    if(!empty()) {
        qnode *tmp;
        tmp = front;
        //The data extracted is stored in the character container c, passed by reference.
        *c = tmp->data;
        // Front now has to point to the next node and the old element has to be deleted.
        front = front->link;
        delete tmp;

        res = 0;
    }

    return res;
}

// Destructor
queue::~queue() {
    // If it is already empty we don't have to do anything.
    if (empty()){
        return;
    }

    qnode *tmp;

    // We take front element and delete it throught a tmp node till the queue is empty.
    while (!empty()){
        tmp = front;
        front = front->link;
        delete tmp;
    }
}

//////////// MAIN ///////////

int main(int argc, char *argv[]) {
    queue queue;
    std::string str2;

    queue.enqueue("hello");
    queue.dequeue(&str2);

    std::cout << str2;
}

Sadly, I get the following error:

$ g++ -o issue issue.cpp $ ./issue issue(23060,0x7fff71058310) malloc: * error for object 0x7f962b403920: pointer being freed was not allocated * set a breakpoint in malloc_error_break to debug helloAbort trap: 6

If I comment the queue destructor (the "while" block), the implementation works fine. The implementation also worked fine using char elements (just single letters).

Can anyone enlighten me?

Answer 1

Your problem is that you're initializing rear->link to tmp in your enqueue function - change the line rear->link = tmp to rear->link = NULL in the empty-queue part of enqueue() and it works fine for me ^^ your last node shouldn't point to anything, you're trying to delete the same memory twice when you get to the end because that memory is linked to itself.

EDIT: And now for the far more important answer. Both your list and your queue implementations don't follow the Rule of Big Three. A basic rundown of the rule is this (though you should DEFINITELY still read the link - it's one of the most important idioms in C++):

If a class manages a resource dynamically, it should provide a copy constructor, assignment operator AND a destructor. Any class that breaks this rule (ie. yours) run the risk of creating copies of pointers, then deleting them. In your example, what it looks like is this:

Your queue is initialized with the string you expected. Everything looks fine. Then you call the function l.insert(queue, 0), which is prototyped as follows: int insert(queue c, int position);

The important thing to notice is that the queue is passed by VALUE, meaning that a COPY of it gets made (using the copy constructor - rule of big three #1), then the list node gets created with the data of THAT COPY - meaning the head pointer, most specifically. Since you didn't provide a copy constructor, that pointer just gets copied bitwise - meaning the copy of your queue (the data of which you're inserting into the list) has the EXACT SAME head pointer as the queue you passed in, but it's a different object.

Then, you write the line tmp->data=c; - which is another one of your Big Three functions, the assignment operator. Again, you don't provide one - so that pointer is assigned bitwise again, leading to the same problem as with the copy constructor.

So far we've got three copies of our queue object we passed in, all of them with the same head pointer. See where we're going yet?

Now, when your list::insert() function finishes up, it destroys all the local objects it created. What does that mean? It means calling the destructor of the copy you created when you passed the queue by value. Since it has the same head pointer as the other two copies of that queue you have, it goes through their lists as well as its own, deleting everything willy-nilly. So now, your list node's queue's data is garbage, but we haven't quite crashed and burned fully yet. Let's see what happens when your program exits.

Now the fun starts - the double-free error you were seeing (in fact, it's a triple free, since we made three copies, remember?). As your program exits, both of the other copies' destructors are called as well - and they try to go through the list, deleting everything again.

So, how do we solve these problems?

The link provided will give you a more in-depth explanation - but what you need to do is ensure those classes both provide ALL of the functions in the rule of big three, and that those functions make a deep copy wherever necessary - instead of copying the pointer to the head (for example), your queue should create a new qnode;, set that as its head, and fill its data with a copy of the other list's head's data. For example (and I didn't compile this, and there's no error checking, and etcetera) your copy constructor for queue could look like this:

Queue::Queue(const Queue &other)
{
    head = new qnode; //IMPORTANT: don't copy the value of the pointer (like the compiler
                  // will by default if you let it)
    head->data = other.head->data;

    qnode *tmp = other.head->link;
    while (tmp != nullptr)
    {
        qnode *tmp2 = new qnode; //again, don't copy the value of the pointer!
        tmp2->data = tmp.data; //make a new node and copy the data
        tmp2->link = nullptr;
        rear->link = tmp2; //update the tail of the list
        rear = tmp2;
    }
}

Hope that helps... it was more typing than I expected! but you gave a REALLY good example of why the Rule of Big Three is one of the most important idioms in C++. These things will happen EVERY TIME you break the Rule of Big Three.