Problems with Linked List of String in C

Question

I'm trying to make a minishell and store all the commands the user types, and when the user enters history it should display all the commands the user has typed so far, and when the user types history -c then it should clear the linked list.

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <dirent.h>
#include <errno.h>
#include <signal.h>
#include <sys/wait.h>

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

node *create_node(char* data)
{
    node *ptr = malloc(sizeof(node));
    if (ptr == NULL)
    {
        fprintf(stderr, "Error: Out of memory in create_node()\n");
        exit(1);
    }
    
    ptr->data = data;
    ptr->next = NULL;
    return ptr;
}

node *insert_node(node *head, char* data)
{
    node *temp;
    if (head == NULL)
        return create_node(data);
    temp = head;
    while (temp->next != NULL)
    {     
        temp = temp->next;
    }
    // 'temp' is pointing to the last node in the list now.
    temp->next = create_node(data);
    return head;
}

void print_list(node *head)
{
    node *temp;
    if (head == NULL)
    {
        printf("(empty list)\n");
        return;
    }

    for (temp = head; temp != NULL; temp = temp->next)
        printf("%s%c", (char*)temp->data, '\n');
}

int main (int argc, char ** argv)
{
    char buf[1024];
    char * args[MAX_ARGS];
    char ** arg;
    node *head = NULL;
    while (1)
    {
        printf("#");
        if (fgets (buf, 1024, stdin ))
        {
            head = insert_node(head, buf);
            arg = args;
            *arg++ = strtok(buf, SEPARATORS);  // tokenize input
            while ((*arg++ = strtok(NULL, SEPARATORS)));
            if (args[0])
            {
                //#byebye command, exist the while loop.
                if (!strcmp(args[0], "byebye")) {
                    break;
                }      
  
                if (!strcmp(args[0], "history"))
                {
                    // command history with flag c

                    if (args[1] && !strcmp(args[1], "-c"))
                    {
                        // clear the linked list
                    } 
                    else
                    {
                        print_list(head);
                        printf("\n");
                    }
                    continue;
                }
                arg = args;
                while (*arg) fprintf(stdout, "%s ", *arg++);
                fputs ("\n", stdout);
            }
        }
    }
    return 0;
}

But this is my output:

#hello

hello

#adding

adding

#to list

to list

#history

history

history

history

history

So, instead of printing out all the commands, it prints out history and I don't know what I am doing wrong. Please help, it has been awhile since I touched C and pointers.

Answer 1

Your create_node() is implemented wrong.

You are leaking the memory you malloc() for the node::data member, as you are immediately re-assigning the node::data to point at the same memory that the input char* parameter is pointing at, thus losing the malloc'ed memory.

As such, all of your created nodes end up pointing at the same char[] buffer in main(), which is being reused for every string the user types in.

create_node() needs to make a copy of the char data, which is probably what you intended to do, but didn't do so correctly. Try this:

node *create_node(char* data)
{
    node *ptr = malloc(sizeof(node));
    if (ptr == NULL)
    {
        fprintf(stderr, "Error: Out of memory in create_node()\n");
        exit(1);
    }

    ptr->data = strdup(data);
    ptr->next = NULL;

    if (ptr->data == NULL)
    {
        fprintf(stderr, "Error: Out of memory in create_node()\n");
        free(ptr);
        exit(1);
    }

    return ptr;
}

And then you need to add a new function to free each node and its data when you are done using it, eg:

node* free_node(node* n)
{
    node *next = n->next;
    free(n->data);
    free(n);
    return next;
}

...

int main (int argc, char ** argv)
{
    node *head = NULL;
    ...

    while (head != NULL) {
        head = free_node(head);
    }

    return 0;
}