Why is fread setting the output pointer to NULL and causing a stack-smashing error?

Question

I asked this earlier but failed to provide a minimally reproducible example. I appreciate the feedback. I am trying to write to a binary file an int followed by an array of bools, where the int represents the length of that array.

The following code compiles and seems to produce the binary file correctly. When fread is called it sets the void* argument I pass it to NULL and triggers a stack-smashing error.

example.c

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

typedef struct cutlogTag{
    int len;
    bool *parts_cut;
} Cutlog;


size_t save_cutlog(const char *path, const Cutlog *p){
    
    FILE *fp;
    size_t written = 0;

    fp = fopen(path, "wb");
    if (fp == NULL){
        fprintf(stderr, "Failed to save cutlog file\n");
        return 0;
    }
    written = fwrite(&(p->len), sizeof(p->len), 1, fp);
    written += fwrite(p->parts_cut, sizeof(bool), p->len, fp);
    if(written != 1 + p->len)
        fprintf(stderr, "error writing file\n");
    else fprintf(stdout, "cutlog written to %s\n", path);
    fclose(fp);
    return written;
}

//returns cutlog with length of -1 on failure to load log
Cutlog load_cutlog(const char *path){
    
    Cutlog ret;
    FILE *fp;
    size_t read = 0;

    ret.len = -1;
    ret.parts_cut = NULL;
    
    fp = fopen(path, "rb");
    assert(fp != NULL);

    fseek(fp, 0, SEEK_SET);
    fread(&ret.len, sizeof(ret.len), 1, fp);
    ret.parts_cut = malloc(sizeof(bool) * ret.len);
    assert(ret.parts_cut);
    read = fread(&ret.parts_cut, sizeof(bool), ret.len, fp);
    if(read != ret.len){
        fprintf(stderr, "read unexpected size of data\n");
        ret.len = -1;
    }
    if (getc(fp) != EOF){
        fprintf(stderr, "expected file end. something went wrong. \n");
        ret.len = -1;
    }
    fclose(fp);
    return ret;
}

int main(int argc, char *argv[]){
    Cutlog clog;
    const char* path = "testbinary";
//initialize cutlog struct
    clog.len = 687;
    clog.parts_cut = malloc(sizeof(bool) * clog.len );
    assert(clog.parts_cut);
    for (int i = 0; i < clog.len; i++){
        clog.parts_cut[i] = false;
    }
//save to binary file and free from memory
    save_cutlog(path, &clog);
    free(clog.parts_cut);
//load from binary file
    clog = load_cutlog(path);
    fprintf(stdout, "len is %d\n", clog.len);
    return 0;
}

Tried to write a binary file an int followed by an array of bools, where the int represents the length of that array, then load the file back.

The file is written correctly but on reading it I cause a stack smashing crash.

Answer 1

read = fread(&ret.parts_cut, sizeof(bool), ret.len, fp);

&ret.parts_cut is the address of ret.parts_cut, which is itself a pointer that exists on the stack, so typically four or eight bytes at the time this question was asked (but could of course be more).

If you have more boolean values that will fit in the space of that pointer, then you will damage other things on the stack.

But even if it does fit, you will almost certainly still get incorrect behaviour because your pointer is now set to something not pointing to the memory you allocated, some weird alias of a set of boolean values like 0x0101000101000001.

This is unlikely to end well if you later try to de reference that pointer with something like *(ret->parts_cut).

You should be using ret.parts_cut in the fread call, which is the value of the pointer, pointing to the thing you just allocated enough space for:

read = fread(ret.parts_cut, sizeof(bool), ret.len, fp);

---

And just a quick note: it may not be the best idea to rely on the assert macro to catch things that may cause problems. If the NDEBUG macro is defined (which it may well be in production code), the assert macro does nothing.

You're better off using if (! condition) handle_it(), rather than assert(condition), to handle these situations.

Answer 2

Building your source with clang -fsanitize=address produces:

=================================================================
==485824==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffd74354d0 at pc 0x558ecaa48b4e bp 0x7fffd7435490 sp 0x7fffd7434c60
WRITE of size 687 at 0x7fffd74354d0 thread T0
    #0 0x558ecaa48b4d in fread (/tmp/a.out+0x3cb4d) (BuildId: 532318f475d6a04e8d9ae1be96e2fd96a96490f1)
    #1 0x558ecaaeb41c in load_cutlog /tmp/t.c:48:12

Address 0x7fffd74354d0 is located in stack of thread T0 at offset 48 in frame
    #0 0x558ecaaeb11f in load_cutlog /tmp/t.c:32

  This frame has 1 object(s):
    [32, 48) 'retval' <== Memory access at offset 48 overflows this variable

The fix is to change this:

    read = fread(&ret.parts_cut, sizeof(bool), ret.len, fp);

to this:

    read = fread(ret.parts_cut, sizeof(bool), ret.len, fp);

Answer 3

In addition to @paxdiablo noting your primary problem that ret.part_cut is already a pointer and you do not take its address again in fread(), there are other areas where you can improve:

• with sizeof you use parenthesis with sizof(type) (e.g. sizeof(bool)) and you omit parenthesis with sizeof object, (e.g. sizeof p->len). Compilers will generally allow both forms, but better to be precise,
• if you use the dereferenced pointer to set the type-size for an array or allocated block, you will never get the type-size wrong, e.g. sizeof *p->parts_cut. With short code, this is generally not a problem, but when declarations may be thousands of lines apart or the pointer may be to an object with multiple levels of indirection -- it will get more challenging,
• while assert() is fine for short test-code, it is best avoided. Why? it simply halts the program preventing any recovery from the error. You will generally want a way to have your program recover and not simply halt on every error,
• you should validate fclose() after any write to catch stream errors not caught by checking the number of items written (like a file-stream error when flushing the write buffer contents to disk, etc..), e.g.

    /* always validate close-after-write */
    if (fclose (fp) == EOF) {
        perror ("fclose save_cutlog");
        return 0;
    }

• it's always a good idea to initialize your structures to avoid the possibility of referencing a member while its value is indeterminate, e.g. Cutlog ret = { .len = 0 }; (this will set len to the value shown and all other members not explicitly initialized to zero),
• while permissible to use, read is the name of a syscall in C, and it is better to choose a name that will not conflict, e.g. size_t nparts = 0; isntead,
• your choice of checking the cumulative sum of written to validate fwrite() is a bit unorthodox -- but works. Better to check after each write to avoid writing to a stream that is in an error state if the first write fails (this correction is left to you),
• fread() does not distinguish between EOF and stream-error, so to find out which occured, you need to call feof() and ferror() in the event of an error, e.g.

    /* ret.part_cut is already a pointer to allocated memory, no & */
    nparts = fread (ret.parts_cut, sizeof *ret.parts_cut, ret.len, fp);
    if (nparts != (size_t)ret.len) {
        fprintf (stderr, "error: read unexpected size of data\n");
        /* fread doesn't distinguish between EOF and error */
        if (feof (fp)) {  /* must check EOF */
            fputs ("error: EOF on read of ret.parts_cut.\n", stderr);
        }
        else if (ferror (fp)) { /* and stream error */
            fputs ("error: stream error on read of ret.parts_cut.\n", stderr);
        }
        ret.len = -1;
        fclose (fp);
        return ret;
    }

• your call to getc(fp) to check EOF after the read isn't wrong, but isn't really needed either,
• you really don't want to hard-code filenames. You can use "testbinary" as a default filename if one isn't provided as an argument to your code, but you really shouldn't need to recompile your program just to write to a different filename. You can use a simple ternary to set const char *path = argc > 1 ? argv[1] : "testbinary";,
• you should check the return value for each function necessary to the continued defined operation of your program. save_cutlog() returns a value that indicates success or failure, but you fail to use that return in main(), e.g.

    /* validate every function call where result is necessary
     * for the continued defined operations of your code.
     */
    if (!save_cutlog (path, &clog)) {
        exit (EXIT_FAILURE);                /* exit with failure */
    }
    free (clog.parts_cut);

• lastly, tidy up by freeing all memory you allocate before returning from main(). Yes, it will be freed on program exit, but when using memory check tools like valgrind, memory not freed before returning from main() will be shown as in-use at program exit.

All in all, you knew where you were going, you just got tripped up on taking the address of ret.parts_cut in fread(). If you follow all the recommendations above, you can tweak your code similar to the following:

// #include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

typedef struct cutlogTag {
    int len;
    bool *parts_cut;
} Cutlog;


size_t save_cutlog (const char *path, const Cutlog *p)
{
    FILE *fp;
    size_t written = 0;

    fp = fopen (path, "wb");
    if (fp == NULL) {
        fprintf (stderr, "Failed to save cutlog file\n");
        return 0;
    }
    written = fwrite (&(p->len), sizeof p->len, 1, fp);
    written += fwrite (p->parts_cut, sizeof *p->parts_cut, p->len, fp);
    
    if (written != (size_t)(1 + p->len)) {
        fprintf(stderr, "error writing file\n");
    }
    else {
        fprintf(stdout, "cutlog written to %s\n", path);
    }
    
    /* always validate close-after-write */
    if (fclose (fp) == EOF) {
        perror ("fclose save_cutlog");
        return 0;
    }
    
    return written;
}

//returns cutlog with length of -1 on failure to load log
Cutlog load_cutlog (const char *path)
{
    Cutlog ret = { .len = 0 };    /* initialize all zero */
    FILE *fp;
    size_t nparts = 0;            /* read is a syscall, use another name */

    ret.len = -1;
    ret.parts_cut = NULL;
    
    fp = fopen (path, "rb");
    if (!fp) {                /* assert prevents any type recovery - avoid */
        perror ("fopen");
        return ret;
    }

    // fseek(fp, 0, SEEK_SET);    /* unnecessary */
    
    if (fread (&ret.len, sizeof ret.len, 1, fp) != 1) {
        if (feof (fp)) {
            fputs ("error: EOF on read of ret.len.\n", stderr);
        }
        else if (ferror (fp)) {
            fputs ("error: stream error on read of ret.len.\n", stderr);
        }
        ret.len = -1;
        fclose (fp);
        return ret;
    }
    ret.parts_cut = malloc (sizeof *ret.parts_cut * ret.len);
    /* always validate every allocation, assert just crashes out - avoid */
    if (!ret.parts_cut) {
        perror ("malloc ret.parts_cut");
        ret.len = -1;
        fclose (fp);
        return ret;
    }
    
    /* ret.part_cut is already a pointer to allocated memory, no & */
    nparts = fread (ret.parts_cut, sizeof *ret.parts_cut, ret.len, fp);
    if (nparts != (size_t)ret.len) {
        fprintf (stderr, "error: read unexpected size of data\n");
        /* fread doesn't distinguish between EOF and error */
        if (feof (fp)) {  /* must check EOF */
            fputs ("error: EOF on read of ret.parts_cut.\n", stderr);
        }
        else if (ferror (fp)) { /* and stream error */
            fputs ("error: stream error on read of ret.parts_cut.\n", stderr);
        }
        ret.len = -1;
        fclose (fp);
        return ret;
    }
    
    if (getc(fp) != EOF) { /* not really necessary - but not wrong */
        fprintf(stderr, "expected file end. something went wrong. \n");
        ret.len = -1;
    }
    
    fclose(fp);
    return ret;
}

int main (int argc, char *argv[]) {
    
    Cutlog clog = { .len = 0 };               /* initialize */
    /* use 1st argument as filename (default "testbinary") */
    const char *path = argc > 1 ? argv[1] : "testbinary";
    
    clog.len = 687;
    clog.parts_cut = malloc (sizeof *clog.parts_cut * clog.len );
    if (!clog.parts_cut) {
        perror ("malloc clog.parts_cut");
        exit (EXIT_FAILURE);                /* exit with failure */
    }
    // assert(clog.parts_cut);              /* avoid using assert */
    for (int i = 0; i < clog.len; i++){
        clog.parts_cut[i] = false;
    }
    
    //save to binary file and free from memory
    /* validate every function call where result is necessary
     * for the continued defined operations of your code.
     */
    if (!save_cutlog (path, &clog)) {
        exit (EXIT_FAILURE);                /* exit with failure */
    }
    free (clog.parts_cut);
    
    //load from binary file
    clog = load_cutlog(path);
    if (clog.len == -1) {
        exit (EXIT_FAILURE);                /* exit with failure */
    }
    
    fprintf (stdout, "len is %d\n", clog.len);
    
    free (clog.parts_cut);                  /* don't forget to free mem */
}

Example Use/Output

$ ./bin/fwrite-cutlog
cutlog written to testbinary
len is 687

Memory Use/Error Check

Run the code through valgrind to catch any memory errors and verify all memory is freed (this would have caught your original problem as well -- as well as compiling with full warnings enabled). For example:

$ valgrind ./bin/fwrite-cutlog
==25258== Memcheck, a memory error detector
==25258== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==25258== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==25258== Command: ./bin/fwrite-cutlog
==25258==
cutlog written to testbinary
len is 687
==25258==
==25258== HEAP SUMMARY:
==25258==     in use at exit: 0 bytes in 0 blocks
==25258==   total heap usage: 7 allocs, 7 frees, 11,534 bytes allocated
==25258==
==25258== All heap blocks were freed -- no leaks are possible
==25258==
==25258== For lists of detected and suppressed errors, rerun with: -s
==25258== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Let me know if you have any questions.