C - Why does char array only return last value that's put into it?

Question

I'm writing in C. I'm trying to read lines from a text file, parse the line, and put some info into an array of strings. When I test my code, every value in the array seems to be the last value inserted. What causes this?

int r;
char *users[51]; //given no more than 50 users
for (r = 0; r < 51; r++) {
    int n = 15; //arbitrary guess at length of unknown usernames
    users[r] = malloc((n + 1) * sizeof(char));
}
FILE *fp;
fp = fopen(argv[1], "r");

char *username;

int counter = 0;
char line[100];
while (fgets(line, 100, fp) != NULL) {
    username = strtok(line, ":");

    users[counter] = username;
    printf("%s\n", username);
    printf("%s\n", users[counter]);

    //counter increase for later
    counter += 1;

`users[counter] = username;` should be `strcpy(users[counter], username);` — Iłya Bursov, Apr 18 '19 at 17:39
Because they share a common buffer `char line[100];` (a frequently asked question). Also note the possible buffer over run (when copying as above) so `fgets(line, 100, fp)` should be `fgets(line, n, fp)`. — Weather Vane, Apr 18 '19 at 17:40
Note that you could have created users with `char users[51][16];` — stark, Apr 18 '19 at 17:50
related: https://stackoverflow.com/questions/6901090/c-why-is-strcpy-necessary — Ryan Haining, Apr 18 '19 at 17:51

score 1 · Answer 1 · answered Apr 18 '19 at 17:47

You put a sensible value into each entry in the array:

users[r] = malloc((n+1) * sizeof(char));

But then you overwrite it with a nonsensical value (a pointer into line):

users[counter] = username;

What you presumably wanted to do was copy the string pointed to by username into the space allocated at users[counter]. The strcpy function can do this.

score 1 · Accepted Answer · answered Apr 18 '19 at 18:18

strtok is a very confusing function:

it modifies the array it receives a pointer to
it returns a pointer to an element of this array.
it keeps an internal state which makes it non-reentrant and non thread-safe.

Hence username points inside line. You store this pointer into users[counter]. At the end of the loop, all entries in users point to the same array that has been overwritten by every call to fgets().

You should duplicate the contents of the array with strdup():

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

int main(int argc, char *argv[) {
    char *users[51]; //given no more than 50 users
    int r;
    FILE *fp;

    if (argc < 2) {
        fprintf(stderr, "missing filename argument\n");
        return 1;
    }
    fp = fopen(argv[1], "r");
    if (fp == NULL) {
        fprintf(stderr, "cannot open file %s\n", argv[1]);
        return 1;
    }
    char line[100];
    int counter = 0;
    while (counter < 50 && fgets(line, 100, fp) != NULL) {
        char *username = strtok(line, ":");
        if (username != NULL) {
            users[counter] = strdup(username);
            //counter increase for later
            counter += 1;
        }
    }
    users[counter] = NULL;

    ...
}

score -1 · Answer 3 · answered Apr 18 '19 at 17:57

-1

I would just like to add to David's answer that you should also check the username string is null terminated before using strcpy(). I can't remember if strtok() null terminates but you shouldn't rely on it anyway.

answered Apr 18 '19 at 17:57

user1160866

157
2
10

1

`strtok` does NUL terminate (by overwriting the source string) but that's not the same issue as returning a `NULL` pointer. – Weather Vane Apr 18 '19 at 17:59

C - Why does char array only return last value that's put into it?

3 Answers3