Determining the proper predefined array size in C?

Question

In the following code I have the array size set to 20. In Valgrind the code tests clean. But as soon as I change the size to 30, it gives me errors (showed further below). The part that confuses me is that I can change the value to 40 and the errors go away. Change it to 50, errors again. Then 60 tests clean and so on. Keeps going like that. So I was hoping someone might be able to explain this to me. Because it's not quite coming clear to me despite my best efforts to wrap my head around it. These errors were hard to pinpoint because the code by all appearances was valid.

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

struct record {
    int number;
    char text[30];
};

int main(int argc, char *argv[])
{
    FILE *file = fopen("testfile.bin", "w+");
    if (ferror(file)) {
        printf("%d: Failed to open file.", ferror(file));
    }

    struct record rec = { 69, "Some testing" };

    fwrite(&rec, sizeof(struct record), 1, file);
    if (ferror(file)) {
        fprintf(stdout,"Error writing file.");
    }

    fflush(file);
    fclose(file);
}

Valgrind errors:

valgrind --leak-check=full --show-leak-kinds=all\
                --track-origins=yes ./fileio
==6675== Memcheck, a memory error detector
==6675== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6675== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==6675== Command: ./fileio
==6675== 
==6675== Syscall param write(buf) points to uninitialised byte(s)
==6675==    at 0x496A818: write (in /usr/lib/libc-2.28.so)
==6675==    by 0x48FA85C: _IO_file_write@@GLIBC_2.2.5 (in /usr/lib/libc-2.28.so)
==6675==    by 0x48F9BBE: new_do_write (in /usr/lib/libc-2.28.so)
==6675==    by 0x48FB9D8: _IO_do_write@@GLIBC_2.2.5 (in /usr/lib/libc-2.28.so)
==6675==    by 0x48F9A67: _IO_file_sync@@GLIBC_2.2.5 (in /usr/lib/libc-2.28.so)
==6675==    by 0x48EEDB0: fflush (in /usr/lib/libc-2.28.so)
==6675==    by 0x109288: main (fileio.c:24)
==6675==  Address 0x4a452d2 is 34 bytes inside a block of size 4,096 alloc'd
==6675==    at 0x483777F: malloc (vg_replace_malloc.c:299)
==6675==    by 0x48EE790: _IO_file_doallocate (in /usr/lib/libc-2.28.so)
==6675==    by 0x48FCBBF: _IO_doallocbuf (in /usr/lib/libc-2.28.so)
==6675==    by 0x48FBE47: _IO_file_overflow@@GLIBC_2.2.5 (in /usr/lib/libc-2.28.so)
==6675==    by 0x48FAF36: _IO_file_xsputn@@GLIBC_2.2.5 (in /usr/lib/libc-2.28.so)
==6675==    by 0x48EFBFB: fwrite (in /usr/lib/libc-2.28.so)
==6675==    by 0x10924C: main (fileio.c:19)
==6675==  Uninitialised value was created by a stack allocation
==6675==    at 0x109199: main (fileio.c:11)
==6675== 
==6675== 
==6675== HEAP SUMMARY:
==6675==     in use at exit: 0 bytes in 0 blocks
==6675==   total heap usage: 2 allocs, 2 frees, 4,648 bytes allocated
==6675== 
==6675== All heap blocks were freed -- no leaks are possible
==6675== 
==6675== For counts of detected and suppressed errors, rerun with: -v
==6675== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Answer 1

The problem is that there is padding in the structure to make the int a always aligned by 4 in memory, even within an array of struct records. Now, 20+4 is divisible by 4, and so is 40+4 and 60+4. But 30+4 and 50+4 are not. Hence 2 padding bytes need to be added to make the sizeof (struct record) divisible by 4.

When you're running the code with array size 34, sizeof (struct record) == 36, and bytes 35 and 36 contain indeterminate values - even if the struct record is otherwise fully initialized. What is worse, code that writes indeterminate values can leak sensitive information - the Heartbleed bug being a prime example.

The solution is actually to not write the structure using fwrite. Instead write the members individually - this improves portability too. There isn't much performance difference either, as fwrite buffers the writes and so does fread.

---

P.S. the road to hell is paved with packed structs, you want to avoid them like plague in generic code.

---

P.P.S. ferror(file) will almost certainly never be true just after fopen - and in normal failures fopen will return NULL and ferror(NULL) will probably lead to a crash.

Answer 2

[edit]

My answer relates to a weakness in OP's code, yet the Valgrind write(buf) points to uninitialized byte(s) is due to other reasons answered by others.

---

When the open fails, ferror(file) is undefined behavior (UB).

if (ferror(file)) is not the right test for determining open success.

FILE *file = fopen("testfile.bin", "w+");
// if (ferror(file)) {
//    printf("%d: Failed to open file.", ferror(file));
// }
if (file == NULL) {
    printf("Failed to open file.");
    return -1;  // exit code, do not continue
}

I do not see other obvious errors.

---

ferror(file) is useful to test the result of I/O, not of opening a file.

Answer 3

I initially misinterpreted the valgrind output, so @chux's deserves acceptance. I'll try to put together the best answer I can though.

Checking errors

The first error (the one I didn't immediately consider) is to check the value returned by fopen(3) with ferror(3). The fopen(3) call returns NULL on error (and sets errno), so checking NULL with ferror(3) is wrong.

Serializing a structure on a file.

With the initialization you write all the fields of your structure, but you don't initialize all the memory it covers. Your compiler might for example leave some padding in the structure, in order to get better performance while accessing data. As you write the whole structure on the file, you are actually passing non-initialized data to the fwrite(3) function.

By changing the size of the array you change Valgrind's behaviour. Probably this is due to the fact that the compiler changes the layout of the structure in memory, and it uses a different padding.

Try wiping the rec variable with memset(&rec, 0, sizeof(rec)); and Valgrind should stop complaining. This will only fix the symptom though: since you are serializing binary data, you should mark struct record with __attribute__((packed)).

Initializing memory

Your original initialization is good.

An alternative way of initializing data is to use strncpy(3). Strncpy will accept as parameters a pointer to the destination to write, a pointer to the source memory chunk (where data should be taken from) and the available write size.

By using strncpy(&rec.text, "hello world", sizeof(rec.text) you write "hello world" over the rec.text buffer. But you should pay attention to the termination of the string: strncpy won't write beyond the given size, and if the source string is longer than that, there won't be any string terminator.

Strncpy can be used safely as follows

strncpy(&rec.text, "hello world", sizeof(rec.text) - 1);
rec.text[sizeof(rec.text) - 1] = '\0';

The first line copies "hello world" to the target string. sizeof(rec.text) - 1 is passed as size, so that we leave room for the \0 terminator, which is written explicitly as last character to cover the case in which sizeof(rec.text) is shorter than "hello world".

Nitpicks

Finally, error notifications should go to stderr, while stdout is for results.