Problem with filling the linked list with strtok

Question

Im having troubles with the next piece of code...

I have defined a linked list struct

    typedef struct node {
        char *cmd;
        char *value; 
        int delay; 
        struct node *next;
    } node_t;

im reading commands from a text file called commands.txt

write:0x02:1
read:0x04:2 
set:0xff:2
reset:0xfa:2

the code below works if i use two buffers (temp and temp1) but if i replace temp1 with temp it does not parse it correctly. it gives me

Tokenized2 read x04 1
Tokenized2 read 0x04 2
Tokenized2 read x04 1
Tokenized2 read 0x04 2

The code is:

    char *filename = "commands.txt";
    FILE *fp = fopen(filename, "r");
    
    node_t *head = (node_t *)malloc(sizeof(node_t));
    node_t *hp = head; 
    char temp[14];
    char temp1[14];
  
    fgets(temp, 14, fp);  
    hp->cmd = strtok(temp, ":");
    hp->value = strtok(0, ":");
    hp->delay = atoi(strtok(0, ":"));
    hp->next = (node_t *)malloc(sizeof(node_t));
    hp = (node_t *)hp->next; Blaz
    
    fgets(temp1, 14, fp);
    hp->cmd = strtok(temp1, ":");
    hp->value = strtok(0, ":");
    hp->delay = atoi(strtok(0, ":"));
    hp->next = NULL; 
    hp = head;

    printf("Tokenized2 %s %s %d\n", hp->cmd, hp->value, hp->delay);
    hp = hp->next;
    printf("Tokenized2 %s %s %d\n", head->next->cmd, head->next->value, head->next->delay);
    printf("Tokenized2 %s %s %d\n", head->cmd, head->value, head->delay);
    printf("Tokenized2 %s %s %d\n", head->next->cmd, head->next->value, head->next->delay);

    fclose(fp);

The bottom printfs are redundant, I was just testing if it works with classic access to head and via pointer. Eventually I would like to make this piece of code work.

    const unsigned MAX_LENGTH = 256;
    char cmdbuffer[MAX_LENGTH];
    node_t *head = (node_t *)malloc(sizeof(node_t));
    node_t *hp = head; 
    while (fgets(cmdbuffer, MAX_LENGTH, fp)) {
        hp->cmd = strtok(cmdbuffer, ":");
        hp->value = strtok(0, ":");
        hp->delay = atoi(strtok(0, ":"));
        hp->next = (node_t *)malloc(sizeof(node_t));
        hp = (node_t *)hp->next;
    }
    hp->next = NULL; 
    printf("Tokenized %s %s %d\n", head->cmd, head->value, head->delay);
    printf("Tokenized2 %s %s %d\n", head->next->cmd, head->next->value, head->next->delay);

How can I solve this problem?

It seems you have a *single* string buffer, and make all nodes strings point to that single buffer. — Some programmer dude, Feb 08 '22 at 17:39

score 1 · Answer 1 · answered Feb 08 '22 at 20:31

I managed to solve it with strdup. Here is the code ...

FILE *fp = fopen("commands.txt", "r");

    if (fp == NULL){
        printf("Error: could not open text file");
        return 1;
    }

    const unsigned MAX_LENGTH = 256;
    char cmdbuffer[MAX_LENGTH];
    node_t* head = malloc(sizeof(node_t));
    node_t* hp = head; 
    char* sdup; 

    while (fgets(cmdbuffer, MAX_LENGTH, fp)){
        sdup = strdup(cmdbuffer);
        hp->cmd = strtok(sdup, ":");
        hp->value = strtok(0, ":");
        hp->delay = atoi(strtok(0, ":"));
        hp->next = malloc(sizeof(node_t));
        hp = (node_t*) hp->next;
    }
    hp->next = NULL; 


    hp = head; 
    while(hp){
        printf("Tokenized: %s %s %d\n", hp->cmd, hp->value, hp->delay);
        free(hp);
        hp = hp->next;
    }

Comments/corrections are welcome!

Marco Bonelli · Accepted Answer · 2022-02-08T18:22:22.760

That's because of these lines:

hp->cmd = strtok(temp, ":");
hp->value = strtok(0, ":");
// ...

fgets(temp, 14, fp); // here temp1 was replaced with temp

You are basically replacing the first and second : in temp with a terminator (\0) and then assigning hp->cmd = temp; and hp->value = temp + some_offset;.

Then, if you read into temp again, the old contents will be overwritten and the old assigned values will reference some bad unrelated offset in the new data in the buffer. Using two buffers fixes the issue because you are no longer overwriting the data of the first read with the second one.

If you only want to use one temporary buffer for reading then you will have to use strdup() to copy it somewhere else before using strtok() and assigning ->cmd and ->value, or just malloc() the buffer right away, so that each time you do these assignments they will refer to different newly allocated strings. If you do this, remember to also free() the allocated buffers later on.

Something along the lines of this (check the code for yourself, I did not test it):

const unsigned MAX_LENGTH = 256;
node_t *head = malloc(sizeof(node_t));
node_t *hp = head; 
char *cmdbuffer;

head->next = NULL;

while (1) {
    cmdbuffer = malloc(MAX_LENGTH);
    if (fgets(cmdbuffer, MAX_LENGTH, fp)) {
        break;
    }

    hp->cmd = strtok(cmdbuffer, ":");
    hp->value = strtok(0, ":");
    hp->delay = atoi(strtok(0, ":"));
    hp->next = malloc(sizeof(node_t));
    hp = hp->next;
}

if (hp->next) {
    // careful here you need to free before assigning NULL 
    // or you leak memory
    free(hp->next);
    hp->next = NULL;
}

// ...

node_t *tmp;
hp = head;

while (hp) {
    tmp = hp;
    hp = hp->next;
    free(tmp->cmd);
    free(tmp);
}

Anyway, do not cast the return value of malloc().

Jesus Christ that was fast! Thank you very much and god bless StackOverflow. Ill try to fix it and will return if not successful! thanks again! — user2856623, Feb 08 '22 at 17:50
@user2856623 you are welcome. If this solved your problem you can accept my answer using the checkmark button on the top left. — Marco Bonelli, Feb 08 '22 at 18:03
Edited my answer: forgot to allocate the buffer inside the loop not outside, whoops. — Marco Bonelli, Feb 08 '22 at 18:05
I've copy/pasted your code and ran it but it doesn't work. i get (null) (null) 0 and then segmentation fault. Should it work "out-of-the box" ? — user2856623, Feb 08 '22 at 18:20
@user2856623 sorry, but I'm not on a machine where I can test it right now. It's just an indication of a solution, not a ready-to-copy-paste snippet. Take your time to review it and understand it. — Marco Bonelli, Feb 08 '22 at 18:22

Problem with filling the linked list with strtok

2 Answers2