linked list implemetation from scratch, segmentation fault-CodePudding

I want to implement linked list from scratch. I programmed it, but when I run it gives me a Segmentation fault in my node_adder(int data) function. I don't know what caused this.

struct linked_list
{
    int value;
    struct linked_list *next;
};

struct linked_list *head = NULL;
struct linked_list *tail = NULL;

void node_adder(int data)
{
    struct linked_list *new = NULL;

    new->value = data;
    new->next = NULL;

    if (head == NULL)
    {
        head = new;
        tail = new;
        printf("head added\n");
    }
    else
    {
        tail->next = new;
        tail = new;
        printf("tail added\n");
    }
}

CodePudding user response：

In node_adder(), you are not allocating any memory for the new node before accessing its members.

Try this instead:

struct linked_list
{
    int value;
    struct linked_list *next;
};
    
struct linked_list *head = NULL;
struct linked_list *tail = NULL;

void node_adder(int data)
{
    struct linked_list *newNode;

    // ADD THIS!!!
    // in C:
    newNode = malloc(sizeof(struct linked_list));
    // in C  :
    newNode = new linked_list;
    //

    newNode->value = data;
    newNode->next = NULL;

    if (head == NULL)
    {
        head = tail = newNode;
        printf("head added\n");
    }
    else
    {
        tail->next = newNode;
        tail = newNode;
        printf("tail added\n");
    }
}

CodePudding user response：

This line initializes a pointer to a node to NULL:

struct linked_list *new = NULL;

Yet, no memory is allocated for this node. Therefore, in this line, attempting to assign a value to one of the members of the node leads to a segmentation fault:

new->value = data;

You probably meant to do this:

struct linked_list *new = malloc(sizeof(struct linked_list));

Of course, a corresponding free(..) should also be added when the allocated memory will no longer be used.

CodePudding user response：

For starters it is a bad idea to declare global variables and define functions that depend on the global variables as you are doing

struct linked_list *head = NULL;
struct linked_list *tail = NULL;

because in this case you will not be able to define for example more than one list.

It is much better to introduce one more structure like for example

struct node
{
    int value;
    struct node *next;
};

struct linked_list
{
    struct node *head;
    struct node *tail;
};

As for your problem then you are using a null pointer to access memory that results in undefined behavior

void node_adder(int data)
{
    struct linked_list *new = NULL;

    new->value = data;
    new->next = NULL;
    //...

You need to allocate a node dynamically as for example

int node_adder(int data)
{
    struct linked_list *new = malloc( sizeof( *new ) );
    int success = new != NULL;

    if ( success )
    {
        new->value = data;
        new->next = NULL;

        if (head == NULL)
        {
            head = new;
        }
        else
        {
            tail->next = new;
        }

        tail = new;
    }

    return success;
}

But using the approach I showed in the beginning of my answer You could write instead

struct node
{
    int value;
    struct node *next;
};

struct linked_list
{
    struct node *head;
    struct node *tail;
};

int node_adder( struct linked_list *list, int data )
{
    struct node *new = malloc( sizeof( *new ) );
    int success = new != NULL;

    if ( success )
    {
        new->value = data;
        new->next = NULL;

        if (list->head == NULL)
        {
            list->head = new;
        }
        else
        {
            list->tail->next = new;
        }

        list->tail = new;
    }

    return success;
}

and in main you can write

int main( void )
{
    struct linked_list list = { .head = NULL, .tail = NULL };

    node_adder( &list, 10 );
    //...
}