C segmentation fault when using pointer to structure in a linked list

Question

I write a very basic linked list in C supporting only two operations - insert of a node to the top of the list and iterate over the list in order to print the value of each node. The problem I am facing is that I have a segmentation fault when executing. Here is my code:

#include<stdio.h>
#include<string.h>
#include<stdlib.h>
struct linkedList
{
    int data;
    struct linkedList *next;
};
struct linkedList* createNode(int value)
{
    struct linkedList *node;
    node = malloc(sizeof(struct linkedList));
    node->next = malloc(sizeof(struct linkedList));
    node->next = NULL;
    node = NULL;
    return node;
}
//insert a node at the top of the linked list
struct linkedList* insertTop(struct linkedList* top,struct linkedList* node)
{
    if(top == NULL)//the element we insert is the 1st element for the linked list
    {
        node->next = NULL;
        //the first element points to NULL since it has no successors
    }
    else//there is already an element in the list
    {
        node->next = top;
    }
    return node;
}
void iterate(struct linkedList* top)
{
    while(top->next != NULL)
    {
        printf("Data = %d\n", top->data);
        top = top->next;
    }
}
int main()
{
    struct linkedList *a,*b,*c,*root;
    a = createNode(2);
    b = createNode(10);
    c = createNode(23);
    root = insertTop(NULL,a);//these 3 lines provoke a segmentation fault
    root = insertTop(root,b);
    root = insertTop(root,c);
    iterate(root);//the result I expect here is 23,10,2 
    return 0;
}

I know this question has been asked many times on stackoverflow and yet I still can't figure out why my code does not work as expected. Can you please explain to me where is the problem and how can I fix it? Thank you

In createNode() why are you putting node = NULL before returning. You are losing the memory that you just malloced. — Haris
– Haris, Commented Feb 12, 2016 at 15:45
You allocate node->next, then throw it away by making it NULL. Then you make the node pointer to NULL and return that... — Sami Kuhmonen
– Sami Kuhmonen, Commented Feb 12, 2016 at 15:45
It is also creating the next node and nullifying it. That's fun. — Eugene Sh.
– Eugene Sh., Commented Feb 12, 2016 at 15:47

dbush · Accepted Answer · 2016-02-12 17:08:16Z

3

There are two main issues. First createNode:

You allocate space for a struct linkedList and assign it to node, which is good. But then you do the same for node->next. So you're actually creating two nodes, but then you set node->next to NULL ,loosing the reference to the second bit of memory you allocated, creating a memory leak. The same happens when you do the same for node. The end result is that you always return NULL for your new node. Also, you don't assign value to anything.

Just do the first allocation, assign value to data, and initialize next to NULL:

struct linkedList* createNode(int value)
{
    struct linkedList *node;
    node = malloc(sizeof(struct linkedList));
    node->data = value;
    node->next = NULL;
    return node;
}

The next issue is in iterate:

while(top->next != NULL)

This causes the iteration to stop when the current node is the last one, so you don't print the last node. Also, as a result of how createNode was originally implemented, your root pointer is NULL, so you end up dereferencing a NULL pointer, which causes a core dump.

You want to test if top is NULL instead:

while(top != NULL)

A third issue is cleanup. You need to define a function to free the memory you allocated before your program exits. You can do that like this:

void cleanup(struct linkedList *top)
{
    struct linkedList *temp;

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

edited Feb 12, 2016 at 17:08

answered Feb 12, 2016 at 16:22

dbush

233k27 gold badges261 silver badges334 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

traviata Over a year ago

Thank you for the complete answer! In the [cleanup] function, you've declared temp as a variable of type struct linkedList and it seems to me it should be a pointer to the linkedList type. Is it necessary the temp to be a pointer to linkedList and if yes, for what reason it should be a pointer? When using the temp as a simple variable of type linkedList the compiler says "incompatible types" between node and top.

dbush Over a year ago

@traviata That was a typo. It should have been a struct linkedList *. I've editted The answer.

unwind · Accepted Answer · 2016-02-12 15:45:02Z

3

This:

node = NULL;

doesn't look so good in the createNode() function. In fact, that entire function doesn't look so good. It should be something like this:

struct linkedList* createNode(int value)
{
    struct linkedList *node = malloc(sizeof *node);
    node->next = NULL;
    node->data = value;
    return node;
}

It really shouldn't have been allocating more than one node, that's not sane.

answered Feb 12, 2016 at 15:45

unwind

402k64 gold badges492 silver badges620 bronze badges

Comments

Rabbid76 · Accepted Answer · 2016-02-12 16:07:51Z

If you like to create a node you only have to allocate mamory for one node. Adapt your code like this:

struct linkedList* createNode(int value)
{
    struct linkedList *node = malloc(sizeof(struct linkedList));
    node->next = NULL;
    node->data = value;
    return node;
}

Apart from this you have to change the termination condition of the while loop in your function iterate:

void iterate( struct linkedList *top )
{
    while( top != NULL) // if to is not NULL you can print its data
        // ^^^ 
    {
        printf("Data = %d\n", top->data);
        top = top->next;
    } 
}

And you can simplify your function insertTop:

struct linkedList* insertTop(struct linkedList* top,struct linkedList* node)
{
    if( node == NULL ) // test if node is not NULL
        return top;
    node->next = top;  // successor of new node is top (top possibly is NULL)
    return node;
}

Dont't forget to free your list at the end of main:

int main()
{
    struct linkedList *root = NULL;
    root = insertTop( root, createNode(2) );
    root = insertTop( root, createNode(10) );
    root = insertTop( root, createNode(23) );

    iterate(root);

    while ( root != NULL )
    {
        struct linkedList *temp = root;
        root = root->next; 
        free( temp );
    }
    return 0; 
}

Apart from this you can combine createNode and insertTop to one function createTop:

struct linkedList* createTop( struct linkedList* top, int value )
{
    struct linkedList *node = malloc(sizeof(struct linkedList));
    node->next = top;
    node->data = value;
    return node;
}

Haris · Accepted Answer · 2016-02-12 15:55:49Z

0

You have quite a few problems there..

In the function createNode() you are allocatting memory to node and then throwing it away by doing node = NULL.

Loose those statements, as they create a memory leak.

struct linkedList* createNode(int value)
{
    struct linkedList *node;
    node = malloc(sizeof(struct linkedList));
    node->data = value;
    node->next = NULL;
    return node;
}

edited Feb 12, 2016 at 15:55

answered Feb 12, 2016 at 15:50

Haris

12.3k6 gold badges48 silver badges75 bronze badges

Collectives™ on Stack Overflow

C segmentation fault when using pointer to structure in a linked list

4 Answers 4

2 Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

2 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related