C pointer (core dumped)

Question

For some reason I get problems in my structure, I believe its related to memory allocation For some reason, I get an error when running:

Segmentation fault (core dumped)

I understand that there might be some problem with memory allocation, but I can't detect the problem. I would like to know how to build it correctly and make it more stable. I’ve added comments to th code to make it clear

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

/* define the typedef struct node (self-referential structure) */
typedef struct node{
    char data;
    struct node *next

void print_linked_list(node *head){
    unsigned int char_counter = 0;
    node *temp = head;
    /*iterate the entire linked list and print the data */
    while(head->next != NULL){
         ++char_counter;
         printf("%c", temp->data);
         temp = temp->next;
         if (char_counter%10==0){
        ******** */
/* this function walk the entire linked-list and free each node */
void free_linked_list(node* head){
    while (head != NULL){
        node *temp = head;
        head = head->next;
        free(temp);
    }
    return;
}


/* ********************************* Main ********************************** */

int main(int argc, char *argv[]){
    char ch;
    FILE *fog;

    /* create the head of the linked list */
    node *head = (node*)malloc(sizeof(node));
    
    /* check if the memory allocation succeed */
    if (!head){
        printf("error 2: memory allocation failed\n");
        exit(-2);
    }
    
    /* define the file descriptor*/
    if (!(fd = fopen(argv[1], "r"))){
        printf("error 1: cannot open the selected file.\n");
        exit(-1);
    }
    /* read file char by char until reached EOF */
    while ((character = fgetc(fd) != EOF)){
        add_node(head, character);
    }
    print_linked_list(head);
    free_linked_list(head);
    if (fclose(fd)){
        printf("error 3: close error\n");
        exit (-3);
    }
    else {
    printf("file closed successfully.\n");
    }
    return 0;
}

Answer 1

This memory allocation

/* create the head of the linked list */
node *head = (node*)malloc(sizeof(node));

does not make a sense because at least data members of the allocated node are not initialized.

Just write

/* create the head of the linked list */
node *head = NULL;

The function add_node excepts the pointer head declared in main by value. That means that the function deals with a copy of the value of the pointer head. Changing the copy within the function does not reflect on the value of the original pointer head.

Using your approach to the function definition what you need is to change the return value of the function and to return its parameter. For example

/* ************************************************************************ */
/* this function add a node in the end of the linked list */
node * add_node(node *head, char character){
    /* check if the head point to NULL (it's an empty linked list),*/
    /* otherwise find the last node and add the new node. */
    /* create a new node */
    node *last_node = NULL;
    node *temp = NULL;
    if (head == NULL){
        //...
    }
    else{
        //...
    }

    return head;
}

In main you will need to write

head = add_node(head, character);

And there is a bug within the function print_linked_list

while(head->next != NULL){

Change it the following way

void print_linked_list( const node *head){
    unsigned int char_counter = 0;

    /*iterate the entire linked list and print the data */
    for ( const node *temp = head; temp != NULL; temp = temp->next )
    {
         ++char_counter;
         printf("%c", temp->data);
         if (char_counter%10==0){
         printf("\n");
         }
    }
    printf("NULL\n");
}

Answer 2

There are the following issues:

• The node allocated for *head is not initialised. So its next member should not be dereferenced, yet that is what happens in the add_node function. You should not start with a new node that has no purpose. Instead leave the head NULL, which represents an empty linked list.

• add_node receives the head pointer by value, so it can never hope to change the caller's head variable. One way to solve this, is to make that parameter a pointer to the head pointer, and the caller would pass the address of their head pointer.

• print_linked_list has a wrong while condition. It should not test head->next, but temp, since that is the variable that performs the traversal.

• To make the output readable, print_linked_list should output some delimiter between the node values, so they can be distinguished and do not come out as one string.

• It would be a good idea to also pass the address of the head pointer to the free_linked_list function, so that it can set the caller's head pointer to NULL.

• Not a problem, but it is better to avoid code repetition, and only have one line where you call malloc for creating a node.

• In C it is advised to not cast the value returned by malloc, as that is implied by the variable to which that value is assigned.

Here is the adapted code (without the file handling):

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

typedef struct node{
    char data;
    struct node *next;
} node;

// **head: address of head-pointer
void add_node(node **head, char character) {
    // One code block for node creation
    node *new_node = malloc(sizeof(node));
    if (new_node == NULL) {
        printf("error 2: memory allocation failed\n");
        exit(-2);
    }
    new_node->data = character;
    new_node->next = NULL;
    if (*head == NULL) {
        *head = new_node;
    } else {
        node *last_node = *head;
        while (last_node->next != NULL) {
            last_node = last_node->next;
        }
        last_node->next = new_node;
    }
}

void print_linked_list(node *head) {
    unsigned int char_counter = 0;
    node *temp = head;
    while (temp != NULL) { // corrected condition
         ++char_counter;
         printf("%c ", temp->data); // delimit
         temp = temp->next;
         if (char_counter % 10 == 0) {
             printf("\n");
         }
    }
    printf("NULL\n");
}

void free_linked_list(node** head) { // pointer pointer
    while (*head != NULL) {
        node *temp = *head;
        *head = (*head)->next;
        free(temp);
    }
    return;
}

int main(int argc, char *argv[]) {
    node *head = NULL; // empty list
    add_node(&head, 'a'); // pass address
    add_node(&head, 'b');
    print_linked_list(head);
    free_linked_list(&head); // pass address
    return 0;
}