Linked list in c adding to beginning of the list

Question

Im trying to create a simple programme to add a value to a linked list. the code does compile with out errors. Im getting a segmentation fault when trying to execute the file. I tried to debug using printf statements, but I don't get any output anywhere. could someone point out what im doing wrong.

typedef struct in separate .h file, include files also in separate .h file

typedef struct      s_list
    {
        struct s_list   *next;
        void            *data;
    }                   t_list;


void    list_push_front(t_list **begin_list, void *data)
{
    t_list *l;

    l = (t_list*)malloc(sizeof(t_list));
    if(l == NULL){
        printf("No allocation");
    }
    printf("%s\n", l->data);
    l->data = data;
    l->next = *begin_list;
    *begin_list = l;
    printf("%s\n", l->data);

}

int     main(void)
{
    t_list *k;
    k = (t_list*)malloc(sizeof(t_list));
    if(k == NULL){
        printf("No allocation");
    }
    printf("allocation");
    char s[] = "Woow!";
    k->data = "Hello";
    k->next->data = NULL;
//  k->next->next->data = NULL;
    list_push_front(&k, s);
    return(0);
}

Why are you addressing `k->next->data` when `k->next` is not initialized? — Rafael, Mar 11 '20 at 18:23

score 1 · Accepted Answer · answered Mar 11 '20 at 18:27

In the printf call

l = (t_list*)malloc(sizeof(t_list));
if(l == NULL){
    printf("No allocation");
}
printf("%s\n", l->data);

you are trying to output non-initialized memory pointed to by the pointer l->data. So the function invokes undefined behavior. Remove this call of printf. It does not make sense.

Also in main this statement

k->next->data = NULL;

is incorrect and also invokes undefined behavior. It seems you mean

k->next = NULL;

score 0 · Answer 2 · answered Mar 11 '20 at 19:00

As a general point, always compile with the -Wall -Werror flags and run your code frequently (every couple of lines). This should help avoid a lot of the problems here. Use valgrind, asan or gdb to detect and diagnose memory issues like the ones in this program.

k->next->data = NULL; is illegal because k->next is uninitialized.
printf("%s\n", l->data);, same problem. You must initialize a value before use.
Functions should not produce side effects like printing. It's OK for temporary debugging, but beyond that it makes for noisy programs and essentially unusable functions. If you want errors, print to stderr and exit or use return values such as an enum or NULL to indicate errors.
Always free allocated memory.
No need to cast the result of malloc.
Use consistent indentation and formatting.

A possible rewrite:

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

typedef struct ListNode {
    struct ListNode *next;
    void *data;
} ListNode;

ListNode *list_create(void *data) {
    ListNode *node = malloc(sizeof(*node));

    if (!node) {
        fprintf(stderr, "%s %d: malloc failed\n", __FILE__, __LINE__);
        exit(1);
    }

    node->data = data;
    node->next = NULL;
    return node;
}

void list_push_front(ListNode **head, void *data) {
    ListNode *node = list_create(data);
    node->next = *head;
    *head = node;
}

void list_free(ListNode *head) {
    while (head) {
        ListNode *dead = head;
        head = head->next;
        free(dead);
    }
}

int main(void) {
    ListNode *list = list_create("a");
    list_push_front(&list, "b");
    list_push_front(&list, "c");

    for (ListNode *curr = list; curr; curr = curr->next) {
        printf("%s\n", (char *)curr->data);
    }

    list_free(list);
    return 0;
}

Linked list in c adding to beginning of the list

2 Answers2