C: fgets usage for building a linked list of char*

Question

Am I using fgets() incorrectly?

I am trying to build a linked list of strings (char *) adding each new line to the end of the LL. I am reading these lines from a file, but for some reason every line gets overwritten by the current line being processed, only when using fgets() inside the while loop, but the add function seems to be receiving each line correctly.

If I add lines individually in main() there are no problems.

Here is a sample input file:

input.txt:

This life, which had been the
tomb of his virtue and of his
honour, is but a walking
shadow; a poor player, that
struts and frets his hour upon
the stage, and then is heard
no more: it is a tale told by an
idiot, full of sound and fury,
signifying nothing.
    --William Shakespeare

The code:

#include <stdio.h> //printf, fopen
#include <stdlib.h> //exit, EXIT_FAILURE
#include <string.h> //strlen

struct node {
    char *line;
    struct node *next;
};

void print(struct node *node);

void add(struct node **head, char *newLine) {
    //printf("%s", newLine);

    struct node *new_node = (struct node *)malloc(sizeof(struct node));
    struct node *curr = *head;

    new_node->line = newLine;
    new_node->next = NULL;

    if (*head == NULL) {
        *head = new_node;
    } else {
        while (curr->next != NULL) {
            curr = curr->next;
        }
        curr->next = new_node;
    }
    print(*head);
}

void print(struct node *node) {
    printf("\n");

    while (node != NULL) {
        printf("%s\n", node->line);
        node = node->next;
    }
}

int main(int argc, char *argv[16]) {
    char newLine[81];
    struct node *head = NULL;
    FILE *fp = fopen(argv[1], "r");

    if (fp == NULL) {
        printf("ERROR: file open failed");
        exit(EXIT_FAILURE);
    }

    while (fgets(newLine, 81, fp)) {
        add(&head, newLine);
    }

    add(&head, "why");
    add(&head, "does");
    add(&head, "this");
    add(&head, "work??");

    fclose(fp);

    print(head);

    return 0;
}

Could someone please explain to me what is happening? I have been banging my head against a wall for too long. There are already some commented print statements I have been attempting to use, unsuccessfully for debugging.

Answer 1

You must allocate memory for each line. As currently coded, all nodes point to the local array in main() whose the contents is overwritten by each call to fgets().

Also note that each line added to the list contains a terminating newline which you probably should get rid of before the call.

Here is a corrected version:

#include <stdio.h>  // printf, fopen
#include <stdlib.h> // exit, EXIT_FAILURE
#include <string.h> // strlen, strdup

struct node {
    char *line;
    struct node *next;
};

void print(struct node *node);

void add(struct node **head, char *newLine) {
    //printf("%s", newLine);

    struct node *new_node = malloc(sizeof(struct node));
    struct node *curr = *head;

    new_node->line = strdup(newLine);
    new_node->next = NULL;

    if (*head == NULL) {
        *head = new_node;
        return;
    }

    while (curr->next != NULL) {
        curr = curr->next;
    }

    curr->next = new_node;
    print(*head);
}

void print(const struct node *node) {
    printf("\n");

    while (node != NULL) {
        printf("%s\n", node->line);
        node = node->next;
    }
}

int main(int argc, char *argv[16]) {
    char newLine[81];
    struct node *head = NULL;
    FILE *fp = fopen(argv[1], "r");

    if (fp == NULL) {
        printf("ERROR: file open failed");
        exit(EXIT_FAILURE);
    }

    while (fgets(newLine, sizeof newLine, fp)) {
        newLine[strcspn(newLine, "\n")] = '\0'; // strip the newline if present
        add(&head, newLine);
    }

    add(&head, "why");
    add(&head, "does");
    add(&head, "this");
    add(&head, "work??");

    fclose(fp);

    print(head);

    return 0;
}

Answer 2

Your problem is in the add() method. It keeps adding the same buffer pointer to the list. You need to copy the buffer in your list to newly allocated space, ie. node->line needs to be malloced, too, and the newLine copied into it. Don't forget to malloc (strlen (newLine) + 1).

Answer 3

You have one buffer where you store the input. And you pass a pointer to the first element of this single buffer when adding nodes. That means the string pointer in all nodes will point to the same single buffer. Which will in the end contain the last string you read.

The simplest solution is to make the string in the node-structure an array, and copy the string into it.

Another solution is to allocate memory for the string dynamically (remembering the terminating null character) and again copy the string into that memory.

The difference when using constant string literals is that each of the strings will be a different array.