malloc overwriting struct elements

Question

Calling malloc before assigning 0 to count works as expected. But, if I call malloc after assigning 0 to count then it overwrites the value of count. I get a random number printed out for count every time I run it.

Could someone explain why count gets overwritten?

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

typedef struct list list;

struct list {
    size_t element_size;
    size_t capacity;
    size_t count;
    void* data;
};

list* create_list(size_t element_size) {
    list* list = malloc(sizeof(list));

    list->element_size = element_size;
    list->capacity = 8;
    list->data = malloc(8 * element_size);
    list->count = 0;

    // If malloc is called here instead then count is overwritten
    // list->data = malloc(8 * element_size);

    printf("count: %zu\n", list->count);

    return list;
}

int main(int argc, char** argv) {
    list* l = create_list(sizeof(int));
}

Does this even compile? You can't use the name `list` like that — Lightness Races in Orbit, Aug 31 '19 at 20:45
ok sorry i did not include the whole code. i'll edit it in a sec — ohhnoes_, Aug 31 '19 at 20:46
Heed [your compiler warnings](https://coliru.stacked-crooked.com/a/26498a68525ce2d9)! — Lightness Races in Orbit, Aug 31 '19 at 20:46
Still doesn't work with %zu in printf. I just get a random number. — ohhnoes_, Aug 31 '19 at 21:00
@ohhnoes_ Is this *all* of your code? I can't reproduce on either 32-bit or 64-bit Linux. What's your compiler invocation? — S.S. Anne, Aug 31 '19 at 21:02
Put `#define __USE_MINGW_ANSI_STDIO 1` at the top of your source file. — S.S. Anne, Aug 31 '19 at 21:10
That screenshot shows you compiling `main.cpp` into `main`. But then you run `main.exe`. Those are two different files. — Lightness Races in Orbit, Aug 31 '19 at 21:11
@LightnessRacesinOrbit GCC automatically puts a `.exe` suffix on executables created for Windows. — S.S. Anne, Aug 31 '19 at 21:18
VTC'ing as "simple typographical error"; sorry, but this is unlikely to be useful to future readers. — S.S. Anne, Aug 31 '19 at 21:20
Ah, I know why this "worked" on Linux: `malloc` doesn't keep its metadata directly before and after the allocation. My personal `malloc` would've caught this, but I didn't use it. — S.S. Anne, Aug 31 '19 at 21:55

hyde · Accepted Answer · 2019-08-31T21:38:36.397

Ah, I think problem is here:

list* list = malloc(sizeof(list));

sizeof(list) gives size of pointer, because at that point list is a pointer, not the struct type any more. So you allocate too little memory, and hit undefined behavior when you initialize the struct data by writing in unallocated memory.

Fix by changing the variable or type name... Or use sizeof(*list) to get the correct size. Or, if you want my opinion/preference, do not use typedef with structs. Or several of these, so here you might have struct list *lst = malloc(sizeof(*lst));.

Even though it is usually not very productive with Undefined Behavior, we can take an educated guess at what probably happens: If you call malloc after writing that 0 beyond what you allocated, then malloc creates a new allocation and overwrites the 0 with its own allocation data and you get that value in output. If you do it the other way, writing that 0 will instead overwrite the allocation data. Both are quite fatal in the long run, so you can consider yourself lucky you noticed the strange output and spotted the UB early, instead of having mysterious crashes later.

Definitely recommend a different name; even with the `struct` shenanigans, it's just poor style to re-use names like this (as evidenced by the bug!) — Lightness Races in Orbit, Aug 31 '19 at 21:17
That's a wonderful explanation; never would've thought of it myself (even having written a `malloc` implementation). — S.S. Anne, Aug 31 '19 at 21:53

S.S. Anne · Answer 2 · 2019-08-31T21:54:09.373

You're allocating the size of a pointer here:

list* list = malloc(sizeof(list));

Instead you should do this:

list* list = malloc(sizeof(*list));

Although I would prefer removing

typedef struct list list;

and using:

struct list *list = malloc(sizeof(*list));

as this makes it more clear that you're dealing with the size of the structure and not the size of the pointer.

On another note, MSVCRT does not support the %zu or %zd format specifiers for printf. Start your C file with this line:

#define __USE_MINGW_ANSI_STDIO 1

and you should be able to use those specifiers.

malloc overwriting struct elements

2 Answers2