Segmentation Fault (Core Dumped) in C++ with linked List basic implementation

Question

I know there are many questions that are based on segmentation fault core dumped in C++ in here but I am not able to resolve this error. My code is simple to create a link list and print it. I want to know why it is wrong because I am new to this topic and want to learn more and how it works and what errors means.

The code is below:


using namespace std;
struct node
{
    int data;
    node *next;
};

node *head =NULL;
void addnode(int x)
{
    node *temp =new node;
    temp->data = x;
    temp ->next = NULL;
    if(head==NULL)
    {
        head = temp;
    }
    else
    {
        node *p =head;
        while(p!=NULL)
        {
            p=p->next;
        }
        p->next =temp;
        temp =NULL;
    }
}

void display(node *head)
{
    node *temp=new node;
    temp = head;
    while(temp!=NULL)
    {
        cout<<temp->data<<" , ";
        temp = temp->next;
    }
    cout<<endl;
}

int main()
{
    cout<<"Hello World";
    node work;
    addnode(12);
    addnode(11);
    addnode(1);
    addnode(22);
    display(head);

    return 0;
}

Most probably I am messing with the head pointer and that's why such an error is occurring but I want to know with respect of my code what I am doing wrong in here.

Thank You. Much Appreciated .

`while (p != NULL) { }` - what will the value of `p` be when the loop terminates? What then happens on the line that follows, `p->next = temp`? — 1201ProgramAlarm, Jan 29 '20 at 19:25
@1201ProgramAlarm What I tried to do is to insert at the end of the linked list. So the `while` loop is traversing to the last node of the link list and then at the last node which becomes p at the end of `while` we link using `p->next=temp` — Sayanto Roy, Jan 29 '20 at 19:28
@Chaos problem is `p` does not become end of the list but one behind it - null pointer. Btw you should use `nullptr` in C++ instead of `NULL`. And use ctor to initialize your data - it will make your code cleaner and less error prone. — Slava, Jan 29 '20 at 19:30
Unrelated: Here is an easier (but conceptually a bit harder) way [to do insertions to a linked list](https://stackoverflow.com/a/59779376/4581301). In your case you're only looking for the end of the list, so you can ignore the `&& (*curr)->data <= item` test for the ordered insertion point. — user4581301, Jan 29 '20 at 20:15

score 2 · Accepted Answer · answered Jan 29 '20 at 19:27

2

Use of

while(p!=NULL)
{
    p=p->next;
}
p->next =temp;

is a problem. When the loop breaks, p is a NULL pointer. After that, p->next is not good. You need to use:

while (p->next != NULL)
{
    p = p->next;
}
p->next = temp;

answered Jan 29 '20 at 19:27

R Sahu

204,454
14
159
270

OK. So it worked and Thank You. But now I have another thing to ask.... – Sayanto Roy Jan 29 '20 at 19:30
What if I want to make a head pointer in my main instead of global variable>? That will require to be used as arguments but won't that would be a good implementation in terms of secure coding?? – Sayanto Roy Jan 29 '20 at 19:32
1

@Chaos proper coding would be to put your list implementation into a class and put that head pointer to be a private member of that class. – Slava Jan 29 '20 at 19:33
@Chaos, I agree with Slava. User code shouldn't have to deal with the abstraction of the nodes of a list. They should deal with just the list abstraction. – R Sahu Jan 29 '20 at 19:35
@Slava OK I get that now. Thanks. – Sayanto Roy Jan 29 '20 at 19:36

Vlad from Moscow · Answer 2 · 2020-01-29T19:37:51.617

For starters this declaration

node work;

does not make sense. You already declared a pointer to the head node.

node *head =NULL;

In the function addnode in this code snippet

else
{
    node *p =head;
    while(p!=NULL)
    {
        p=p->next;
    }
    p->next =temp;
    temp =NULL;
}

the loop

    while(p!=NULL)
    {
        p=p->next;
    }

is executed until p is equal to NULL. So using a null-pointer in the next statement

    p->next =temp;

results in undefined behavior.

This code snippet should look like

else
{
    node *p = head;
    while( p->next != NULL )
    {
        p = p->next;
    }
    p->next = temp;
}

The function display has a memory leak because a memory at first is allocated and its address is assigned to the pointer temp and then the pointer is reassigned. So the allocated memory will not be deleted.

node *temp=new node;
temp = head;

The function can look like

void display(node *head)
{
    for( node *temp = head; temp !=NULL; temp = temp->next )
    {
        cout<<temp->data<<" , ";
    }
    cout<<endl;
}

Or even like

void display( const node *head )
{
    for( const node *temp = head; temp !=NULL; temp = temp->next )
    {
        cout<<temp->data<<" , ";
    }
    cout<<endl;
}

In any case the list interface is inconsistent. The function addnode deals with the global variable head while the function display gets a parameter. It is a bad idea to use a global variable especially when functions depend on such a variable.

@Slava Yes I got that right... It worked when I implemented it using the `p-next!=NULL` — Sayanto Roy, Jan 29 '20 at 19:34

Segmentation Fault (Core Dumped) in C++ with linked List basic implementation

2 Answers2