Check for Memory Leaks

[andreeaflorescu]

We have a bunch of unsafe code & usage of CString which needs to be deallocated explicitly.
We should check to see if we find any memory leaks. We could use valgrind. See https://creativcoder.github.io/post/checking_memory_leaks_in_rust_ffi/ as it might not work out of the box.

[alexandruag]

I wanted to start by trying the sanitizers rustc ships with (https://github.com/japaric/rust-san), but they apparently don't work with musl at this point. Running valgrind does not work with the stable toolchain, because there's no way to switch to the system allocator from jemalloc.

Unfortunately, even if this is possible on nightly, valgrind still appears to miss detecting any heap allocation. This does not appear to be related to musl.

We should definitely have this issue in mind as an extra precaution, but it seems we need to wait for better/newer tools and/or toolchain versions before getting some relevant results.

[memoryruins]

As of 1.28, the #[global_allocator] attribute was stabilized, enabling the switch to the system or a custom allocator on a stable toolchain. Recently, nightly switched the default allocator to System on all platforms.

[dianpopa]

Hi @memoryruins , This is really useful information that we were not aware of. Thank you!
Indeed, it seems that in the meantime there appeared some new additional datapoints from our last investigation that we could make use of:

• As per your information, jemalloc can now be replaced with the System allocator in the stable toolchain. We could try do that and give it a go with the valgrind tool.
• As per sys_util: enable build for non-musl libraries #639, firecracker can be built with glibc also. This means that we should check again to see if the provided rustc sanitizers can now work with musl. If not, we could investigate with the glibc built binary and see what we find there.
  For sure, this issue reached a state where it could use some fresh investigation and any help from community is greatly appreciated 👍 .

[AE1020]

An issue that arises is that Firecracker's main() does not return "cleanly". Termination is done in a mid-subscriber callback with unsafe { libc::exit() }. Rust's std::process::exit() would do some more cleanup than that, but even still would not run destructors for objects on the stack.

If one wants to return an integer exit status -and- guarantee all the destructors have run, the idiom is to put your main code in a wrapped function that returns an integer. Then have the true main() call std::process::exit() with the result of the wrapper. This way, all the code that could need destruction has finished.

I've done that, and tried to make some minimally invasive changes to get an Option<ExitCode> to bubble up for clean exit. Also I've made all the threads join() so that they can release their resources and have a fully clean output.

While clean shutdown is nice for Valgrind, it is slower, and it's also introducing new code paths that could wind up in deadlocks. So I put it under an option... --cleanup-level 2

Now submitted as PR #2535

[acatangiu]

Valgrind tests can now be run after merging #2599

[dianpopa]

Closing this in favor of #1662