cfwritefreadbinary-data

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


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.


Solution

  • 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.