Single Linked List seg fault issue

Question

I tried to implement by myself a single linked list. For now, I only wrote an addition of an element to the end of the list, and a function which prints lists content. But when I want to print out a list my program gives me a segmentation fault. Heres my code:

#include <cstdlib>
#include <iostream>
#include <string>
using namespace std;

class Stuff;

class List
{
    private :

        Stuff *first, *last;

    public :
        List();
        void addfront(Stuff *s);
        void print();
        ~List();
};

class Stuff
{
    private :

        string name;
        double price;

    public :

        Stuff();
        Stuff(string, double);
        void print();
        Stuff *next;
};

Stuff::Stuff()
{
    name = "";
    price = 0;
    next = NULL;
}

Stuff::Stuff(string n, double p)
{
    name = n;
    price = p;
}

void Stuff::print()
{
    cout << name << " " << price << "\n";
}

List::~List()
{
}

void List::addfront(Stuff *s)
{
    if(first == NULL )
       first = s;
   last->next = s;
   s->next = NULL;
   last = s;
}

void List::print()
{
    Stuff *p;
    if(last == first == NULL)
        cout << "list is empty!\n";
    else
        for (p = first; p != NULL; p = p->next)
            p->print();
}

List::List()
{
    first = last = NULL;
}

int main(int argc, char **argv)
{
    List l;

    Stuff *s1 = new Stuff("Coffe", 4.50);
    Stuff *s2 = new Stuff("Apple", 2.50);

    l.addfront(s1);
    l.addfront(s2);

    l.print();

    return 0;
}

Juste one remark, `last == first == NULL` will not work as you expect. Use `last == first && first == NULL` instead. — Matthieu Rouget, May 30 '13 at 09:21
@bash.d Well it's more likely OP is using a compiler that supports C++11 rather than one that doesn't define NULL to 0. — Djon, May 30 '13 at 09:27

score 4 · Accepted Answer · edited May 23 '17 at 10:27

It seems like you forgot to check if last != NULL before setting last->next to s.

Deferencing a NULL pointer results to an undefined behaviour.

Your addFront function should look like that:

void List::addfront(Stuff *s) {
    if(!first)
        first = s;
    if (last)
        last->next = s;
    s->next = NULL;
    last = s;
}

Btw: Use if(last == first && last == NULL) instead of if(last == first == NULL).

Suvarna Pattayil · Answer 2 · 2013-05-30T10:16:16.970

3

One issue is

if(last == first == NULL)

make it

if(last == NULL && first == NULL)

also you need to do,

void List::addfront(Stuff *s)
{
    if(!first)
       first = s;
    if (last)
        last->next = s;
   s->next = NULL;
   last = s;
}

edited May 30 '13 at 10:16

answered May 30 '13 at 09:26

Suvarna Pattayil

5,136
5
32
59

This is **exactly** my answer. – aldeb May 30 '13 at 09:27
1

@segfolt Yeps I know I am late! I was trying to find some links relevant to the issue. No probs I upvoted yours. Will add some links to mine when i find them – Suvarna Pattayil May 30 '13 at 09:28
Thanks. No prob. I just didn't understand because it was looking like a pure copy paste :p – aldeb May 30 '13 at 09:30

score 2 · Answer 3 · answered May 30 '13 at 09:35

It is because of this line in addfront method

last->next = s;

Here last is a NULL pointer.

(gdb) p last
 $1 = (Stuff *) 0x0

Deferencing a NULL pointer will cause memory fault/segmentation violation.

Always check whether it is NULL and then deference.

if (last)
  last->next = s;

If you are in Linux machine, then you can run the program in gdb. Once segmentation violation happens put use backtrace command to see the call stack to understand which statement is crashed.

Single Linked List seg fault issue

3 Answers3