Re: [hatari-devel] fixing errors reported by GCC 10 -fanalyzer

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Sun, 10 May 2020 23:19:24 +0200
schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:

> Hi
> 
> I compiled Hatari with GCC10 -fanalyzer to give it a try.
> It reports some possible NULL pointer after malloc, as well as some 
> other double free.
> 
> It's possible some reported errors are false positives, because GCC 
> can't analyze the full code flow depending on how Hatari will really
> run (for example in debug/*c, many errors are reported, not sure
> they're really errors)

I recently also had a look at the output of -fanalyzer, and I came to
the same conclusion: All those "double free" warnings seem wrong to me
(or I simply don't get it how a double free could occur there).

> Still, if people want to compile with -fanalyzer, maybe we can spot
> some issues that need fixing.
> 
> I fixed a few mallocs not testing NULL after return.

Right, I did the same by replacing some of the string-related malloc()s
with Str_Alloc().

> In ide.c, some fixes are needed here :
>                  s->io_buffer = malloc(MAX_MULT_SECTORS * 
> MAX_SECTOR_SIZE + 4);
>                  assert(s->io_buffer);
> [...]
>                  hd_table[i] = malloc(sizeof(BlockDriverState));
>                  assert(hd_table[i]);
> 
> The problem is that assert can be disabled completely depending on 
> compilation flags, so NULL won't be tested in these 2 cases.
> ide_init2() and Ide_Init() return void, maybe they should return true
> or false, false if init fails ?
> 
> Similar assert should be changed in statusbar.c :
> 
> void Statusbar_AddMessage(const char *msg, Uint32 msecs)
> ...
>          item = calloc(1, sizeof(msg_item_t));
>          assert(item);
> 
> NOTE : assert() is used in several places ; as the condition/error 
> tested by assert() can also happen when assert is compiled to no-code 
> "{}", maybe assert() should be replaced by proper printf+exit (or
> return if possible) ?

Actually, I think the best thing that we can do, is to introduce
another wrapper around malloc() in our code base, that prints an error
message and exits the program completely in case of errors, and we then
should use that wrapper for allocating small amounts of memory instead
of using malloc + error handling everywhere around in the code. malloc()
should never fail for small amounts of memory, otherwise the system is
in a catastrophic out-of-memory situation anyway where the OOM-killer
of the kernel will reap out some processes soon. So aborting the
program is likely the best thing that you can do in such a situation
anyway.
This is e.g. also what is done in the normal glib malloc functions -
they abort the program in OOM situations, and you rather have to use the
try_malloc functions instead if you want to avoid that, see e.g.:

 https://developer.gnome.org/glib/unstable/glib-Memory-Allocation.html#g-try-malloc

And while we're at it, our malloc-wrapper could also clear the
allocated memory block at the end - this is currently done manually in
most spots that malloc some memory anyway, so we could simplify the
code in many spots that way. Thus, what about calling the wrapper
mzalloc() or malloc0() or something similar?

 Thomas