Fscanf not copying to string

Question

You know that moment when your trying to learn but you're missing some small detail and just can't figure it out? I'm having one of those and would be extremely grateful if it were over. Anyway here's my code:

// I'm trying to implement a load function for a trie, a dictionary in particular

// I'm getting a segfault when it arrives at strcpy, but when I check the value of 'word' there, it's NULL

bool load(const char *dictionary) {
    FILE *file = fopen(dictionary, "r");
    char *word = NULL;
    char input[45] = { '\0' };
    int size = sizeof(node);

    while (fscanf(file, "%s", word) != EOF) {
        printf("%s", word);
        strcpy(input, word);
        int cycle = 0;
        node *next = &root;

        while (input[cycle] != '\0') {
            int position = toupper(input[cycle]) % 65;
            if (position == 39) {
                position = 26;
            }

            if (next->children[position] == NULL) {
                next->children[position] = malloc(size);
                next = next->children[position];
            } else {
                next = next->children[position];
            }
        }

        *sizePointer = *sizePointer + 1;
        next->wordHere = true;
    }
    fclose(file);
    return true;
}

You tell `fscanf` to write the string to a *null pointer*. That leads to *undefined behavior* which makes you program *ill-formed* an invalid. You're (un)lucky the program didn't crash. Why don't you use `input` directly in the `fscanf` call? — Some programmer dude, Aug 08 '17 at 12:15
You should allocate memory for word, in which ``fscanf`` can write to. — cmdLP, Aug 08 '17 at 12:15
Thank you, but then I'm not entirely sure on how to initialize it properly EDIT: Is there a way to do it so I don't have to free it in the end? — , Aug 08 '17 at 12:17
`calloc` initializes the variable. Then, you must call `free(word); when finished using it. — ryyker, Aug 08 '17 at 12:18
If you use `input` directly, you don't need to initialize it, and you can remove the `word` variable. And no need to allocate or free anything else. ***Why*** don't you do that? It's as simple as `fscanf(file, "%44s", input)`. — Some programmer dude, Aug 08 '17 at 12:19
Yes, create a string buffer: `char word[80];`. But as programmer dude has already pointed out, `input` is already in that form, and ready to use. — ryyker, Aug 08 '17 at 12:22
@MichaelKeller for each `malloc`/`calloc` you need to call `free` exactly once. — Jabberwocky, Aug 08 '17 at 12:23
@Some programmer dude Hey thanks, that's a good idea. I thought I couldn't do that b.c. I thought fscanf only could return char*'s — , Aug 08 '17 at 12:25
Another general recommendation: Try to avoid [*magic numbers*](https://en.wikipedia.org/wiki/Magic_number_(programming)). For example, what does `65` stand for? [ASCII](https://en.wikipedia.org/wiki/Magic_number_(programming)) encoded upper-case `'A'`? Then *use* `'A'` instead. — Some programmer dude, Aug 08 '17 at 12:25
Arrays naturally decays to pointers to their first element. If a function wants a pointer to the first element of an array, you can use either `input` or `&input[0]`. They are the same. As for your worry about pointers and arrays in `fscanf`, you don't seem to have that worry about `strcpy` which *also* takes a `char *` as its first argument. :) — Some programmer dude, Aug 08 '17 at 12:27
I thought the strcpy() function was here to deal with those conversion I thought existed :) — , Aug 08 '17 at 12:30

score 0 · Answer 1 · answered Aug 08 '17 at 16:26

When using fscanf , you are required to ensure that sufficient memory is allocated for the arguments that it will write to.

For instance you could declare "word" as follows:

char word[100];

This allows for a character string up to 100 characters (including the terminating null character). The code you submitted didn't allocate any space and you are running into undefined behavior. Most likely fscanf is overwriting your "input" variable, but it really depends on the compiler and what compile settings are enabled.

I suggest you forget about the variable "word" altogether and just pass "input" into your fscanf. Then you can remove the call to strcpy as well.

Additionally, fscanf is a function that requires great care when using. Please see this question on SO: When/why is it a bad idea to use the fscanf() function?

score 0 · Answer 2 · answered Aug 08 '17 at 16:41

You must define word either as an array or as a pointer that points to an actual array. Passing NULL as the destination for the %s conversion specifier has undefined behavior.

Furthermore, you should pass the maximum number of bytes to store into the array to prevent potential buffer overflows.

Finally, the test on the return value should be == 1 instead of != EOF. In this particular case, the behavior would be very much the same, but not in general.

Here is a modified version:

bool load(const char *dictionary) {
    FILE *file = fopen(dictionary, "r");
    char word[45];
    char input[45] = { '\0' };
    int size = sizeof(node);

    if (file == NULL)
        return false;

    while (fscanf(file, "%44s", word) == 1) {
        printf("%s\n", word);
        strcpy(input, word);
        ...

Fscanf not copying to string

2 Answers2