Segmentation fault while returning a Node Linkedlist

Question

In find(T item) function I am checking if the data inputted is a string then I am handling it separately and for other data types I am handling it in else case, because I am using the string function compare() to check for the match. The program runs fine when there is a matching string in the linked list and returns the node and prints the node data, but when matching string is not found then instead of returning an empty tmp node the programs crashes and say "Segmentation fault (core dumped)". Is there a way I can fix it?

#include <iostream>
#include <typeinfo>
using namespace std;

template <typename T>
class Node {
public:
    T data;
    Node *next;
    Node *prev;

    Node(){};
};

template <typename T>
class LinkedList {
private:
    Node<T> *head;
    Node<T> *tail;

public:
    LinkedList(){
        head = NULL;
        tail = NULL;
    }

    void insertHead(T item){
        Node<T> *tmp = new Node<T>();
        if (head == NULL){
            head = tmp;
            head->prev = NULL;
            head->data = item;
            head->next = NULL;
            tail = tmp;
        } else {
            tmp->next = head;
            tmp->data = item;
            tmp->prev = NULL;
            head->prev = tmp;
            head = tmp;
        }
    }

    void insertLast(T item){
        Node<T> *tmp = new Node<T>();
        tmp->data = item;
        if (head == NULL){
            head = tmp;
            head->prev = NULL;
            head->next = NULL;
            tail = tmp;
        } else {
            tmp->prev = tail;
            tail->next = tmp;
            tmp->next = NULL;
            tail = tmp;
        }
    }

    Node<T>* find(T item){
        Node<T> *tmp = head;
        if (typeid(item) == typeid(string)){
            string s, d;
            s = item;
            d = tmp->data;
            while(tmp != NULL){
                if(s.compare(d) == 0){
                    return tmp;
                }
                tmp = tmp->next;
                d = tmp->data;
            }
            Node<string> *tmp = new Node<string>();
            tmp->data = "";
            return tmp;
        } else {
            while(tmp != NULL){
                if(tmp->data == item){
                    return tmp;
                }
                tmp = tmp->next;
            }

            tmp = new Node<T>();
            tmp->data = -1;
            return tmp;
        }
    }

    void print(){
        Node<T> *tmp;
        tmp = head;
        while (tmp != NULL){
            cout << tmp->data << "->";
            tmp = tmp->next;
        }
        cout << endl;
    }
};

int main(){

    LinkedList<string> l;
    l.insertHead("Abc");
    l.insertHead("def");
    l.insertHead("ghi jk");

    Node<string>* tmp = new Node<string>();
    tmp = l.find("ghi");
    cout << tmp << endl;

    l.print();
}

Vlad from Moscow · Accepted Answer · 2020-12-09T18:23:41.697

For starters the member function find can invoke undefined behavior in the very beginning

Node<T>* find(T item){
    Node<T> *tmp = head;
    if (typeid(item) == typeid(string)){
        string s, d;
        s = item;
        d = tmp->data;
        ^^^^^^^^^^^^^
        //

because in general head can be equal to nullptr.

In the while loop you also are not checking whether the pointer tmp is equal to nullptr.

tmp = tmp->next;
d = tmp->data;

Creating a dummy node

Node<string> *tmp = new Node<string>();
tmp->data = "";
return tmp;

is a bad idea and can lead to a memory leak. It is better to return a null pointer.

This allocation of a node in main

Node<string>* tmp = new Node<string>();
tmp = l.find("ghi");

produces a memory leak.

The function can be declared and defined at least the following way

Node<T> * find( const T &item )
{
    Node <T> *result = head;

    while ( result && result->data != item ) result = result->next;

    return result;
}

Moreover it can be overloaded the following way

template <typename BinaryPredicate>
Node<T> * find( const T &item, BinaryPredicate binary_predicate )
{
    Node <T> *result = head;

    while ( result && not binary_predicate( result->data, item ) ) result = result->next;

    return result;
}

Gary Strivin' · Answer 2 · 2020-12-09T18:35:34.453

Problem:

In the lines:

s = item;
d = tmp->data;
while(tmp != NULL){
    if(s.compare(d) == 0){
        return tmp;
    }
    tmp = tmp->next;
    d = tmp->data;
}

When tmp->next becomes NULL you will take NULL->data for d = tmp->data; which results in segmentation fault.

Solution:

Replace the code with this:

s = item;
while(tmp != NULL){
    d = tmp->data;
    if(s.compare(d) == 0){
        return tmp;
    }
    tmp = tmp->next;
}

Adittional info:

As churill said, C++ string comparison is not like Java, you can use s == d. Actually because of this the string specialization is required.
I would also consider to use nullptr instead of NULL.
You should not use using namespace std; (More info here).
Printing tmp in cout << tmp << endl; prints the address of tmp, not the value it stores.
tmp->data = -1; return tmp; will cause an error if you try to use the template with a type that cannot be represented as an integer. You should probably use return NULL; or return nullptr; instead.
Node<string>* tmp = new Node<string>(); tmp = l.find("ghi"); use extra space that is not freed. Use Node<string>* tmp; tmp = l.find("ghi"); instead.

Full code:

#include <iostream>
using std::endl;
using std::string;
using std::cout;

template <typename T>
class Node {
public:
    T data;
    Node *next;
    Node *prev;

    Node(){};
};

template <typename T>
class LinkedList {
private:
    Node<T> *head;
    Node<T> *tail;

public:
    LinkedList(){
        head = nullptr;
        tail = nullptr;
    }

    void insertHead(T item){
        Node<T> *tmp = new Node<T>();
        if (head == nullptr){
            head = tmp;
            head->prev = nullptr;
            head->data = item;
            head->next = nullptr;
            tail = tmp;
        } else {
            tmp->next = head;
            tmp->data = item;
            tmp->prev = nullptr;
            head->prev = tmp;
            head = tmp;
        }
    }

    void insertLast(T item){
        Node<T> *tmp = new Node<T>();
        tmp->data = item;
        if (head == nullptr){
            head = tmp;
            head->prev = nullptr;
            head->next = nullptr;
            tail = tmp;
        } else {
            tmp->prev = tail;
            tail->next = tmp;
            tmp->next = nullptr;
            tail = tmp;
        }
    }

    Node<T>* find(T item){
        Node<T> *tmp = head;
        while(tmp != nullptr){
            if(tmp->data == item){
                return tmp;
            }
            tmp = tmp->next;
        }
        return nullptr;
    }

    void print(){
        Node<T> *tmp;
        tmp = head;
        while (tmp != nullptr){
            cout << tmp->data << "->";
            tmp = tmp->next;
        }
        cout << endl;
    }
};

int main(){

    LinkedList<string> l;
    l.insertHead("Abc");
    l.insertHead("def");
    l.insertHead("ghi jk");

    Node<string>* tmp;
    tmp = l.find("ghi");

    cout << ((tmp == nullptr) ? "nullptr":tmp->data) << endl;

    l.print();
}

score 0 · Answer 3 · answered Dec 09 '20 at 18:02

The problem is this loop:

while(tmp != NULL){     // tmp is not null, good
    if(s.compare(d) == 0){
         return tmp;
    }
    tmp = tmp->next;    // after this tmp MIGHT be null!
    d = tmp->data;      // possibly dereferencing null-pointer, bad!
}

As soon as this loop reaches the end of the list and didn't find the searched for string it dereferences a null-pointer and you get undefined behaviour, which manifests as segmentation fault.

In the non-string-version you've done it right and as far as I see you shouldn't need the two versions at all.

s.compare(d) == 0

is exactly the same as s == d, which is identical to item == tmp->data.

The only difference would then be the return value, where I suggesst to return a nullptr if the item was not found. What if the list contains the value "" or -1? You wouldn't be able to tell the value from list apart from the "default value".

Segmentation fault while returning a Node Linkedlist

3 Answers3