Singly Linked List program in C - Segmentation Fault Error

Question

I have written a Code for Implementing Singly Linked List in C. While my code does compile, I get a segmentation fault when attempting to run my code.

The Code is:

#include <stdio.h>
#include<stdlib.h>

struct Node{
    int data;
    struct Node* next;
};
struct Node* head;

void InsertAtPosition(int data, int n){
    int i;
    struct Node* temp1 = (struct Node*)malloc(sizeof(struct Node*));
    temp1->data = data;
    temp1->next = NULL;
    if(n == 1){
            temp1->next = head;
            head = temp1;
            return;
    }
    struct Node* temp2 = head;
    for(i=0; i<n-2; i++){
            temp2 = temp2->next;
    }
    temp1->next = temp2->next;
    temp2->next = temp1;
}

void Print(){
    struct Node* temp = head;
    printf("List is: \n");
    while(temp != NULL){
            printf("\t%d\n", temp->data);
            temp = temp->next;
    }
}

int main(){
    head = NULL;
    InsertAtPosition(10, 1);
    InsertAtPosition(11, 2);
    InsertAtPosition(12, 3);
    Print();
    return 0;
}

The code is giving an Error Segmentation fault (core dumped).

What exactly am I doing Wrong?

`struct Node* temp1 = (struct Node*)malloc(sizeof(struct Node*));` Wrong. — EOF, Sep 27 '16 at 07:41
Useful link: [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) — Mathieu, Sep 27 '16 at 07:41
[Never cast result of malloc](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc)... Also, as EOF as said, you allocate memory for a pointer to a `Node`, not for a `Node` — Garf365, Sep 27 '16 at 07:45
As [EOF](http://stackoverflow.com/questions/39718810/singly-linked-list-program-in-c-segmentation-fault-error#comment66735185_39718810) has said, you are allocating memory for a pointer-to-node (sizeof(pointer)) instead of a node. Better way to do it: `malloc(sizeof(struct Node))`. Even better: `struct Node* temp1 = malloc(sizeof(*temp1))` — lx., Sep 27 '16 at 07:47
I modified my Code. I was actually passing the values to the Function `InsertAtPosition()` in an opposite manner. But Now, it's fixed. but, still I am getting Segmentation fault — aki2all, Sep 27 '16 at 07:48
@aki2all what is your new code ? Also, as already proposed, use debugger to know where seg fault occured — Garf365, Sep 27 '16 at 07:50
@lx. after I modified your code by adding `struct Node* temp1 = malloc(sizeof(*temp1))` instead of `struct Node* temp1; = (struct Node*)malloc(sizeof(struct Node*));` The Segmentation Fault error is gone. But the Printed list shows Garbage values. — aki2all, Sep 27 '16 at 07:51
It seems to work if you correct the `malloc`: http://ideone.com/Q1gX2Y — mch, Sep 27 '16 at 07:52
@aki2all That's partly due to the `for (...; i < n-2; ...)`. When `n` is 2, the loop won't run. Your code should be checking whether the insert position is valid. The code should do something sensible If it reaches the end of the list before it reaches the insert position. — user3386109, Sep 27 '16 at 07:53
The memory allocation issue notwithstanding. the day you get solid on pointers and move to pointers-to-pointers, you'll realize how much [simpler this becomes](http://pastebin.com/8ufAHW8H). — WhozCraig, Sep 27 '16 at 07:58
also you are passing the content of a node as an argument, it is not that bad if you only store a int but now you are stuck with it, also you dont pass your node as argument , its not very modulable/extensible, — Arnaud Aliès, Sep 27 '16 at 08:29

score 3 · Answer 1 · answered Sep 27 '16 at 07:52

3

(struct Node*)malloc(sizeof(struct Node*))

is wrong, you are creating a piece of memory of a pointer size. Try

(struct Node*)malloc(sizeof(struct Node))

answered Sep 27 '16 at 07:52

Alexi

389
1
8

And in ` for(i=0; inext; }` you should check the size of the list. If you pass an "n" greater than the list, you'll get another segmentation fault – Alexi Sep 27 '16 at 07:54
1

As a good practice, I'd recommend writing `type* var = malloc(sizeof(*var))` instead of `sizeof(type)`, as this makes future changes less error prone (and is more concise ;) ) – lx. Sep 27 '16 at 08:03

score 0 · Answer 2 · answered Sep 27 '16 at 08:01

The previous answer covered it. But I can see one more segfault in the program. This part of code will break if value of n exceeds the size of list.

for(i=0; i<n-2; i++){
        temp2 = temp2->next;
}

In your main function if you change the order of function call, then you will see this segfault

InsertAtPosition(11, 2);
InsertAtPosition(10, 1);
InsertAtPosition(12, 3);

To fix this make the following change

for(i=0; i<n-2; i++){
        if (temp2 == NULL){
            return;

            /*or break instead of return as per your requirement. 
            Adding break will add the element to the end of 
            the list whenever `n` exceeds size of list*/
        }
        temp2 = temp2->next;

}

Singly Linked List program in C - Segmentation Fault Error

2 Answers2