Fix use-after-free and 3 memory leaks; enforce AddressSanitizer in CI by Bromeon · Pull Request #133 · godot-rust/gdext (original) (raw)

Changes implementation of the Gd smart pointer to cache the instance ID in each object. To my knowledge, instance IDs are the only reliable way to check for object validity in Godot, as raw object pointers may become dangling.

In addition, this PR fixes 3 memory leaks around arrays and dictionaries. Those occurred due to from_sys_init() using T::default() in too many places. This essentially lead to allocating a default-constructed object, which is then immediately overwritten by the init function, which also allocates a new object. I refactored the GodotFfi::from_sys_init() to never call default(), and add an explicit from_sys_init_default() for cases where this is desired. This also cuts down on some of the boilerplate.

Both use-after-free and memory leaks were discovered using AddressSanitizer/LeakSanitizer. Great tooling from the C++ world, which also proves useful for us, as miri can't be used in FFI contexts. From now on, UB or leaks detected by ASan/LSan will cause a hard error in CI. The tools are not 100% bullet-proof; they didn't detect the following UAF case in a short test, but they are still of great value as a more systematic counter to memory errors.

use-after-free, false negative

let mut boks = Box::new(44); let ptr = std::ptr::addr_of_mut!(*boks); println!("deref={}", unsafe { *ptr }); // output: deref=44 drop(boks); println!("deref={}", unsafe { *ptr }); // output: deref=1719666059

On a side note, getting these working correctly in CI was a bit of a marathon because ASan/LSan don't have stacktraces for dynamically loaded libraries (a known wontfix problem, see google/sanitizers#89). Additionally, false positives for memory leaks were reported: a simple println! would cause 1024 bytes of non-reclaimable memory. Therefore, I had to compile a special version of nightly Godot that disables dynamic library unloading via dlclose, to keep the stacktrace around, and this seemed to fix the false-positive issue as well. Although likely unrelated, what I also found during research was rust-lang/rust#19776.

Fixes #89.