How come my fread returns an empty string?

Question

When I write my string to file, I first write the length of the string as an int, followed by the string itself. Here is my code:

int wordLength = strlen(words);
fwrite(&wordLength,sizeof(int),1, outputFile);
fwrite(&words,sizeof(char),strlen(words), outputFile);

However, when I fread it back, I get an empty string. Here is my reading code:

int strLength;
fread(&strLength, sizeof(int), 1, f);
char* word = (char*) malloc(strLength*sizeof(char));
fread(&word, sizeof(char), strLength, f);

Why is this happening?

The second fwrite is suspicious. If `words` is a pointer, then you're writing the pointer instead of what it points to. If it's an array, you're OK, but you'd also be OK without the `&`. Posting incomplete program fragments makes answering questions hard! The second fread is definitely wrong; `&word` is the address of a pointer. The buffer it points to is what you want to be fread'ing into. — , Apr 09 '17 at 01:45
@WumpusQ.Wumbley I removed both `&` and I still get an empty string. Words is an array. — Jonathan Allen Grant, Apr 09 '17 at 02:01
if `words` is an array of strings, then how do you expect `strlen()` to return the total size of the array? Perhaps what you actually want is `sizeof( words )` — user3629249, Apr 09 '17 at 03:33
when calling any of the heap allocation functions: (malloc, calloc, realloc), do not cast the returned value. The returned type is `void*` so can be assigned to any other pointer. Casting just clutters the code, making it more difficult to understand, debug, etc. Note: `sizeof(char)` is defined in the standard as 1, so just use 1. Also multiplying anything by 1 has absolutely no effect other than cluttering the code. — user3629249, Apr 09 '17 at 03:36
when calling `fread()`, always check the returned value against the 3rd parameter to assure the operation was successful — user3629249, Apr 09 '17 at 03:37
in general, when preparing to read a file that was just written to, first call `fflush()` on the file. — user3629249, Apr 09 '17 at 03:39
have you examined the output file to determine if anything was written and if it is correct? — user3629249, Apr 09 '17 at 03:43
@user3629249 Regarding your first comment, I suspect the identifier `words` may have be chosen it's a *group of words in a single string*, not a *group of strings each containing a single word*. — autistic, Apr 09 '17 at 04:10

chux - Reinstate Monica · Answer 1 · 2017-04-09T06:13:23.223

when I fread it back, I get an empty string. Here is my reading code:
Why is this happening?

fread(&strLength, sizeof(int), 1, f);
char* word = (char*) malloc(strLength*sizeof(char));
fread(&word, sizeof(char), strLength, f);

Code allocates insufficient memory. strLength*sizeof(char) is enough for the text yet not the terminating null character to make a string.
```
// char* word = (char*) malloc(strLength*sizeof(char));
char* word = malloc(strLength + 1u); // add 1
```
fread(&word, ...); is attempting to read data into the address of word, rather than into the memory just allocated.
```
// fread(&word, sizeof(char), strLength, f);
fread(word, sizeof *word, strLength, f);  // drop &
```

The null character is never appended.

size_t count = fread(word, sizeof *word, strLength, f);
if (count != strLength) puts("Error");
else {
  word[strLength] = '\0';
  puts(word);
}

Notes: Better to use size_t wordLength

Checking the return value of malloc() makes for good code.

size_t wordLength = strlen(words);
...
char* word = malloc(strLength + 1);
if (word == NULL) Hanlde_OutOfMemory();

Post does not show file open/closing details. Code may need to rewind(f) before reading data written.

I'm all about dotting `i`'s and crossing `t`'s, but `strLength + 1u` is quite impressive. I'd never considered the possibility of **sign extension** resulting from `+ 1` and `-pendantic` doesn't complain about `addition` of *signed* and *unsigned* types `:)` — David C. Rankin, Apr 09 '17 at 06:25
@DavidC.Rankin It is more of `int` overflow with `int wordLength ... strLength + 1` and to draw attention that allocations should use unsigned math. Consider the effect of `int strLength = -99; fread(&strLength, sizeof(int), 1, inputFile); char* buff = (char*) malloc(strLength*sizeof(char));` should `fread()` failure. — chux - Reinstate Monica, Apr 09 '17 at 06:29
I got you. That would be one long-a.. string `:)`, but very well put. — David C. Rankin, Apr 09 '17 at 06:30

score 1 · Accepted Answer · answered Apr 09 '17 at 03:16

1

This works on Ubuntu:

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

int main(int argc, char **argv)
{

    FILE *outputFile;
    FILE *inputFile;
    char words[] = "This is a series of words";

    int wordLength = strlen(words);

    outputFile = fopen("outputFile", "w");
    if ( outputFile == NULL )
    {
        perror("fopen failed: ");
    exit(1);
    }


    fwrite(&wordLength,sizeof(int),1, outputFile);
    fwrite(words,sizeof(char),strlen(words), outputFile);

    fclose(outputFile);

    inputFile = fopen("outputFile", "r");
    if ( inputFile == NULL )
    {
        perror("fopen(2) failed: ");
    exit(1);
    }

    int strLength = -99;
    fread(&strLength, sizeof(int), 1, inputFile);

    char* buff = (char*) malloc(strLength*sizeof(char));
    fread(buff, sizeof(char), strLength, inputFile);

    buff[strLength] = 0x00;

    printf("Input Str: -->%s<--\n", buff);

}

answered Apr 09 '17 at 03:16

TonyB

927
6
13

3

While this code may answer the question, providing additional context regarding how and/or why it solves the problem would improve the answers long-term value. – autistic Apr 09 '17 at 03:47
Additionally, [`strlen` returns `size_t`](http://www.iso-9899.info/n1570.html#7.24.6.3) (which is what `wordLength` should be declared as, and what your first `fwrite` and `fread` should be writing and reading), [`sizeof(char)` is *guaranteed* to be 1](http://www.iso-9899.info/n1570.html#6.5.3.4p4), and [you shouldn't cast `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – autistic Apr 09 '17 at 04:01
@Seb I agree with all of your comments, but my goal was to make minimum modifications to his source to show a working model... He should be able to compare and determine the differences. – TonyB Apr 09 '17 at 05:11
Very well. The comments should be for the question, then... My bad. – autistic Apr 09 '17 at 05:22
`char* buff = (char*) malloc(strLength*sizeof(char)); ... buff[strLength] = 0x00;` is poor (UB) as code attempts to write beyond allocated memory. – chux - Reinstate Monica Apr 09 '17 at 06:14
1

@chux good catch, yes... my bad. Should have been `char* buff = (char*) malloc(1+strLength*sizeof(char));` – TonyB Apr 12 '17 at 21:20
Yes the +1 is needed. `1+strLength*sizeof(char)` is curious. `*sizeof(char)` is the same as `*1` and serves little value here as `sizeof(char)` is always 1. Had code been some other type then likely you would want `(1+strLength)*sizeof(some_other_type)`, add before `*sizeof(some_other_type)`. IAC, recommend `buff = malloc(strLength+1u);` here. – chux - Reinstate Monica Apr 12 '17 at 22:02

How come my fread returns an empty string?

2 Answers2