Add ASAN

[jakub-gonet]

It was immensely helpful when developing so I'm opting to run it by default. Open to discussion, though.

On MacOS running an AVM shows a warning "malloc: nano zone abandoned due to inability to reserve vm space.". To avoid this warning, set MallocNanoZone=0 env variable before running the VM.

Also adds debug symbols when compiling with Clang.

---

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[pguyot]

I do use ASAN in a stash as well.
We need to fix the leak found in packbeam.c though (file_data isn't freed when iterating on args).

[jakub-gonet]

...or we can ignore for the time being, running ASAN_OPTIONS=detect_leaks=0 packBeam .... I checked implementation and the most non-intrusive way would be to use atexit with global variable. Good alternative (IMO) is to bubble up errors and exit in one point in program.

[bettio]

I'm not sure if this should be the default: it is very useful during development, but it should be off during release.
Also I'm not sure how interacts with platforms that do not properly support -fsanitize=address such as ESP32.

So maybe it should be manually enabled (and we might suggest it in our documentation) and of course we should add a job in our CI that enables it. After all what really matter is not merging code that fails those checks.

I think we should have jobs and options for undefined behavior sanitizer, thread sanitizer and memory sanitizer and they should be part of our CI as well.

[jakub-gonet]

Makes sense. I don't have means to test embedded platforms so most of my testing comes from unix and WASM port.

What do you think about adding

set(SANITIZER none) # available values: address, memory, undefined, none

and corresponding ifs in libAtomVM CMakeList?

Is there any way to create local CMakeLists or you need to use env variables when running CMake to set those options?

[bettio]

I think that -g is already the default when building with cmake in Debug mode.

[jakub-gonet]

Is build mode dictated by DEBUG env variable? I couldn't find reference to debug/release in the code.

[bettio]

Is -fno-omit-frame-pointer cross platform? I remember that was something quite common on i386 that has few available registers, but I think it is not the default on several platforms (and maybe it is not even available as an option on other archs).

[jakub-gonet]

Good question. Logically, omitting frame pointer is an optimization so every platform should support that (making one less available register if disabled) but from reading GCC docs it's noop on archs that don't benefit from it.

https://gcc.gnu.org/onlinedocs/gcc-4.9.2/gcc/Optimize-Options.html
look for -fomit-frame-pointer

[bettio]

...or we can ignore for the time being, running ASAN_OPTIONS=detect_leaks=0 packBeam .... I checked implementation and the most non-intrusive way would be to use atexit with global variable. Good alternative (IMO) is to bubble up errors and exit in one point in program.

I'm not a huge fan of this workaround, I would rather go for the clean way. By the way, isn't adding free() on that loop an option?

[jakub-gonet]

I'm not a huge fan of this workaround, I would rather go for the clean way. By the way, isn't adding free() on that loop an option?

From reading the code, we malloc file_data in do_pack which uses assert_fread to exit if it can't read specified number of bytes. This means we need to change assert_fread to either know about unfreed allocations or not to exit.
That's why I suggested atexit.

ASAN does report it as a violation even at exit (typically OS collects unfreed memory by then) and I didn't find any way to skip reporting exit-freed allocations.
I also looked into adding ASAN dynamically but I couldn't get it to work on MacOS (I remember reading that when comes to GCC and Clang one links it statically, one dynamically and the other configuration for both is less tested. This means it's better to use the default linking strategy).

[bettio]

I tried [1] but then it started crashing on my PC with sanitize=memory. Also I feel like we are leaking file descriptors too.

[1]

--- a/tools/packbeam/packbeam.c
+++ b/tools/packbeam/packbeam.c
@@ -271,6 +271,8 @@ static int do_pack(int argc, char **argv, int is_archive, bool include_lines)
             char *filename = basename(argv[i]);
             pack_beam_file(pack, file_data, file_size, filename, !is_archive && i == 1, include_lines);
         }
+
+        free(file_data);
     }

     add_module_header(pack, "end", END_OF_FILE);

[jakub-gonet]

I spent ~2h on this draft, this is how I'd rewrite the file (I didn't run it at all, so expect errors if you want to work on it further). It isn't the most readable code but I'm not a C expert.

Bubbling the errors up is really painful. Related: what's your opinion on Zig? :P

We leak descriptors and memory mostly when checking if files are AVM or BEAM ones. We also do a bit of unnecessary full file reads. I think it would be nice to mmap all of them instead of doing a mix of mmap and fopens.