I have a console application written in C, which takes inputs from the user in order to manipulate a linked list.
When the user inputs "add" to add an item, it will add the item, but all nodes added this way will share the name of the last item made using the "add" command.
list.h
typedef struct node {
char *name;
int value;
struct node *next;
} node_js;
static node_js *currentNode;
void printList(node_js *thing) {
currentNode = thing;
while (currentNode != NULL) {
printf("%s\n", currentNode->name);
printf(">%d\n", currentNode->value);
currentNode = currentNode->next;
}
printf("\n");
}
void addLast(node_js **thing, const char *name, int val) {
if (*thing == NULL) {
*thing = (node_js *)malloc(sizeof(node_js));
//(*thing)->name = name;
(*thing)->name = name;
(*thing)->value = val;
(*thing)->next = NULL;
} else {
currentNode = *thing;
while (currentNode->next != NULL) {
currentNode = currentNode->next;
}
currentNode->next = (node_js *)malloc(sizeof(node_js));
//currentNode->next->name = strdup(name);
currentNode->next->name = name;
currentNode->next->value = val;
currentNode->next->next = NULL;
}
}
void removeLast(node_js **thing) {
if (*thing == NULL) {
return;
}
if ((*thing)->next == NULL) {
free(*thing);
*thing = NULL;
return;
}
currentNode = *thing;
while (currentNode->next->next != NULL) {
currentNode = currentNode->next;
}
free(currentNode->next);
currentNode->next = NULL;
}
main.c
//the problem is on the addLast() inside the while(1), the probelm only occur with items that are
//added when user inputs "add"
#include <stdio.h>
#include <stdlib.h>
#include "list.h"
static int numInput;
static char textInput[32];
static char uInput[16];
node_js *list = NULL;
int main() {
printf("--- Welcome to the program! ---\n>Type 'exit', 'quit' or 'q' to exit\n>Type 'help' for a list of the commands\n\n");
//make list
list = (node_js *)malloc(sizeof(node_js));
list->name = "joe";
list->value = 10;
list->next = (node_js *)malloc(sizeof(node_js));
list->next->name = "james";
list->next->value = 20;
list->next->next = NULL;
addLast(&list, "jane", 30); //here it works fine
while (1) {
printf("input: ");
scanf_s("%s", textInput, sizeof(textInput));
if (strcmp(textInput, "quit") == 0 || strcmp(textInput, "q") == 0 || strcmp(textInput, "exit") == 0) {
printf("Bye!");
free(list);
exit(1);
}
if (strcmp(textInput, "print") == 0) {
printList(list);
}
if (strcmp(textInput, "add") == 0) {
printf("name: ");
scanf_s("%s", uInput, sizeof(uInput));/*the problem is on the addLast() inside the while(1), the probelm only occur with items that are added when user inputs "add"*/
printf("number: ");
scanf_s("%d", &numInput, sizeof(numInput));
addLast(&list, uInput, numInput);
printf("--- added (%s, %d) ---\n", uInput, numInput);
}
if (strcmp(textInput, "remove") == 0) {
removeLast(&list);
printf("--- Removed last item of list ---\n");
}
}
free(list);
return 0;
}
Instead of using textInput to scan the users input, I tried making another array just for that. Could not find anything on this, and also tried asking ChatGPT which did not help.
To largely summarize the comments:
addLast(&list, uInput, numInput)
and in turn,
(*thing)->name = name;
sets every node's name member as the same pointer value: a pointer to the first element of the uInput array that exists in main. The contents of uInput are overwritten every time the add command is used, and the result is that all nodes in the list share the same string, which is always the most recently input name.
list->name = "joe";, list->next->name = "james";, and addLast(&list, "jane", 30); do not share this problem, because each string literal is a pointer to a distinct1, statically allocated, read-only character array.
Instead, you must create a copy of each string, and store a pointer to the copy in each associated node.
This can be done by allocating memory for the length of the string in bytes, plus one additional byte for the null-terminating character ('\0').
The general pattern is
char *create_copy_of_a_string(const char *a_string)
{
char *copy = malloc(1 + strlen(a_string));
if (copy)
strcpy(copy, a_string);
return copy;
}
On many platforms (e.g., POSIX, and in the upcoming C23), strdup is available, which does just this.
Note that with this approach, creating a list by mixing pointers to dynamically allocated strings (malloc) and pointers to statically allocated strings (string literals) will create problems when it comes time to free the former. The general advice would be to avoid mixing these two types of pointers.
(addLast(&list, "jane", 30); would be fine in the example below, wheres list->name = "joe"; would lead to undefined behaviour.)
Alternatively, change the name member to an array, and copy the input to it. Beware of buffer overflows with this approach.
Other issues include:
The single free(list); only frees the memory for the first node in the list.
General repetition of code (addLast in particular).
Use of scanf (scanf_s) for user input can lead to frustrating results when something is incorrectly entered, and the input buffer becomes a mess. The general advice is to read entire lines of input into a buffer with fgets, and then process that data however you want (commonly sscanf, strtol, strtok, etc.). This isn't foolproof, but it does simplify the problem a great deal.
Implementations of functions should be placed in source files (.c), not header files (.h).
In the cursory example below, the signatures of the library functions (printList, addLast, removeLast) remain unchanged. This does not leave a lot of room for cleanly handling/propagating errors, so error handling is largely remiss. In particular, a robust program should handle the event that malloc (calloc) returns NULL, and avoid dereferencing a null pointer value.
Otherwise, it is refactored a fair amount (simplified for the sake of brevity).
list.h:
#ifndef LIST_H
#define LIST_H
typedef struct node {
char *name;
int value;
struct node *next;
} node_js;
void printList(node_js *);
void addLast(node_js **, const char *, int);
void removeLast(node_js **);
#endif
list.c:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "list.h"
void printList(node_js *node)
{
while (node) {
printf("<%s> <%d>\n", node->name, node->value);
node = node->next;
}
}
void addLast(node_js **head, const char *name, const int value)
{
node_js *node = calloc(1, sizeof *node);
/* create a copy of the string */
node->name = malloc(1 + strlen(name));
strcpy(node->name, name);
node->value = value;
if (!*head)
*head = node;
else {
node_js *current = *head;
while (current->next)
current = current->next;
current->next = node;
}
}
void removeLast(node_js **node)
{
if (!*node)
return;
while ((*node)->next)
node = &(*node)->next;
free((*node)->name);
free(*node);
*node = NULL;
}
main.c:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "list.h"
#define match(x, y) (0 == strcmp((x), (y)))
static int read_a_line(char *buf, int len)
{
if (!fgets(buf, len, stdin))
return 0;
/* remove the newline character, if present */
buf[strcspn(buf, "\n")] = 0;
return 1;
}
static int add_to_list(node_js **list)
{
char name[256];
char value[256];
printf("Name: ");
if (!read_a_line(name, sizeof name))
return 0;
printf("Value: ");
if (!read_a_line(value, sizeof value))
return 0;
/* assume this succeeds */
addLast(list, name, strtol(value, NULL, 10));
return 1;
}
int main(void)
{
node_js *list = NULL;
puts("------- Welcome to the program! -------");
while (1) {
char selection[256];
printf("Enter a command (quit, print, add, remove): ");
if (!read_a_line(selection, sizeof selection) || match(selection, "quit"))
break;
if (match(selection, "print"))
printList(list);
else if (match(selection, "add"))
add_to_list(&list);
else if (match(selection, "remove"))
removeLast(&list);
else
printf("Invalid selection <%s>\n", selection);
}
while (list) {
node_js *next = list->next;
free(list->name);
free(list);
list = next;
}
puts("Goodbye.");
}
In use:
------- Welcome to the program! -------
Enter a command (quit, print, add, remove): add
Name: foo
Value: 123
Enter a command (quit, print, add, remove): add
Name: bar
Value: 456
Enter a command (quit, print, add, remove): add
Name: qux
Value: 789
Enter a command (quit, print, add, remove): print
<foo> <123>
<bar> <456>
<qux> <789>
Enter a command (quit, print, add, remove): remove
Enter a command (quit, print, add, remove): print
<foo> <123>
<bar> <456>
Enter a command (quit, print, add, remove): quit
Goodbye.
1. String literals of the same string value may or may not occupy the same memory (i.e., two pointers to string literals are not guaranteed to compare equal, even if equal in terms of content). Ref.