CODEWITHSUNDEEP
cpointersstructlinked-list
traviata
traviata

Reputation: 39

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

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

Upvotes: 1

Views: 541

Answers (4)

dbush
dbush

Reputation: 223872

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);
     }
}

Upvotes: 3

Rabbid76
Rabbid76

Reputation: 210918

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;
}

Upvotes: 1

Haris
Haris

Reputation: 12270

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;
}

Upvotes: 0

unwind
unwind

Reputation: 399813

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.

Upvotes: 3

Related Questions