Is there any gcc compiler warning which could have caught this memory bug?

I haven't programmed C for quite some time and my pointer-fu had degraded. I made a very elementary mistake and it took me well over an hour this morning to find what I'd done. The bug is minimally reproduced here: https://godbolt.org/z/3MdzarP67 (I am aware the program is absurd memory-management wise, just showing what happens).

The first call to realloc() breaks because of course, the pointer it's given points to stack memory, valgrind made this quite obvious.

I have a rule with myself where any time I track down a bug, if there is a warning that could have caught it I enable it on my projects. Often times this is not the case, since many bugs come from logic errors the compiler can't be expected to check.

However here I am a bit surprised. We malloc() and then immediately reassign that pointer which leaves the allocated memory inaccessible. It's obvious the returned pointer does not live outside the scope of that if block, and is never free()'d. Maybe it's too much to expect the compiler to analyze the calls and realize we're attempting to realloc() stack memory but I am surprised that I can't find anything to yell at me about the leaking of the malloc() returned pointer. Even clang's static analyzer scan-build doesn't pick up on it, I've tried various relevant options.

The best I could find was -fsanitize=address which at least prints out some cluing information during the crash instead of:

mremap_chunk(): invalid pointer

on Godbolt, or

realloc(): invalid old size
Aborted (core dumped)

on my machine, both of which are somewhat cryptic (although yes they do show plainly that there is some memory issue occurring). Still, this compiles without issues.

Since Godbolt links don't live forever here is the critical section of the code:

void add_foo_to_bar(struct Bar** b, Foo* f) {
    if ((*b)->foos == NULL) {
        (*b)->foos = (Foo*)malloc(sizeof(Foo));
        // uncomment for correction
        //(*b)->foos[(*b)->n_foos] = *f;      
        // obvious bug here, we leak memory by losing the returned pointer from malloc
        // and assign the pointer to a stack address (&f1)
        // comment below line for correction
        (*b)->foos = f; // completely wrong
        (*b)->n_foos++;
    } else {
        (*b)->foos = (Foo*)realloc((*b)->foos, ((*b)->n_foos + 1) * sizeof(Foo));
        (*b)->foos[(*b)->n_foos] = *f;
        (*b)->n_foos++;
    }
}

the error occurs because f is a pointer to stack memory (intentional) but we obviously can't assign something that was supposed to have been malloc()'d to that.

Solution 1:

Try -fanalyzer if your compiler is recent enough. When running it I get:

../main.c:30:28: warning: ‘realloc’ of ‘&f1’ which points to memory not on the heap [CWE-590] [-Wanalyzer-free-of-non-heap]
   30 |         (*b)->foos = (Foo*)realloc((*b)->foos, ((*b)->n_foos + 1) * sizeof(Foo));
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-2
    |
    |   37 | int main() {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |......
    |   45 |     add_foo_to_bar(&b, &f1);
    |      |     ~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (2) calling ‘add_foo_to_bar’ from ‘main’
    |
    +--> ‘add_foo_to_bar’: events 3-5
           |
           |   19 | void add_foo_to_bar(struct Bar** b, Foo* f) {
           |      |      ^~~~~~~~~~~~~~
           |      |      |
           |      |      (3) entry to ‘add_foo_to_bar’
           |   20 |     if ((*b)->foos == NULL) {
           |      |        ~
           |      |        |
           |      |        (4) following ‘true’ branch...
           |   21 |         (*b)->foos = (Foo*)malloc(sizeof(Foo));
           |      |         ~~~~
           |      |          |
           |      |          (5) ...to here
           |
    <------+
    |
  ‘main’: events 6-7
    |
    |   45 |     add_foo_to_bar(&b, &f1);
    |      |     ^~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (6) returning to ‘main’ from ‘add_foo_to_bar’
    |   46 |     add_foo_to_bar(&b, &f2);
    |      |     ~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (7) calling ‘add_foo_to_bar’ from ‘main’
    |
    +--> ‘add_foo_to_bar’: events 8-11
           |
           |   19 | void add_foo_to_bar(struct Bar** b, Foo* f) {
           |      |      ^~~~~~~~~~~~~~
           |      |      |
           |      |      (8) entry to ‘add_foo_to_bar’
           |   20 |     if ((*b)->foos == NULL) {
           |      |        ~
           |      |        |
           |      |        (9) following ‘false’ branch...
           |......
           |   30 |         (*b)->foos = (Foo*)realloc((*b)->foos, ((*b)->n_foos + 1) * sizeof(Foo));
           |      |                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                            |                     |
           |      |                            |                     (10) ...to here
           |      |                            (11) call to ‘realloc’ here
           |

Solution 2:

No, but, runtime testing can save you.

If you can spare the execution overhead, I have seen many applications add an extra layer to memory allocation to track the allocations made and find leaks/errors. Usually they replace malloc() and free() with a macros that include FILE and LINE

One example can be seen here (check the Heap.c and Heap.h files) https://github.com/eclipse/paho.mqtt.c/tree/master/src

Googling "memory heap debugger" will probably turn up other examples. Or you could roll your own.