Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix use-after-free and 3 memory leaks; enforce AddressSanitizer in CI #133

Merged
merged 5 commits into from
Feb 26, 2023

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Feb 26, 2023

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.

The object pointer may become dangling if the object is destroyed, thus calling `object_get_instance_id`
via FFI can cause UB. The only reliable way to check object validity is through `is_instance_id_valid`.

This change increases the size of `Gd` by 8 bytes, but is needed for safety.
@Bromeon Bromeon added bug c: tooling CI, automation, tools c: ffi Low-level components and interaction with GDExtension API labels Feb 26, 2023
@Bromeon
Copy link
Member Author

Bromeon commented Feb 26, 2023

bors try

bors bot added a commit that referenced this pull request Feb 26, 2023
@bors
Copy link
Contributor

bors bot commented Feb 26, 2023

try

Build succeeded:

…t() before running the init fn

Impacts:
* Simplifies ffi_methods! usage for Variant, GodotString, StringName, TypedArray, PackedArray, Dictionary
* impl Default does not need to re-implement from_sys_init()
* Functions which do *not* expect a pre-initialized instance can directly call from_sys_init() in the future, avoiding 2 inits (memory leak)
Affected:
* TypedArray::share()
* Dictionary::share()
* PackedArray::from(&Array)

Output from Godot compiled with LeakSanitizer, prior to this commit:

   SUMMARY: AddressSanitizer: 864 byte(s) leaked in 10 allocation(s).
@Bromeon Bromeon force-pushed the bugfix/mem-corruption branch from 08b934b to cbd22fa Compare February 26, 2023 22:14
@Bromeon
Copy link
Member Author

Bromeon commented Feb 26, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 26, 2023

Build succeeded:

@bors bors bot merged commit f9db5d6 into master Feb 26, 2023
@bors bors bot deleted the bugfix/mem-corruption branch February 26, 2023 22:27
@Bromeon Bromeon mentioned this pull request Feb 27, 2023
17 tasks
bors bot added a commit that referenced this pull request Jul 6, 2023
325: Address sanitizers for Rust r=Bromeon a=Bromeon

We have missed a lot of UB that occurred only in Rust code and wasn't detected by AddressSanitizer outside of C++ code. This PR enables ASan for Rust code using `-Zsanitizer=address` in the two memcheck CI jobs. As such, this is a follow-up to #133.

The CI currently fails because there is still some UB on `master`... ideally we can fix this soon.

bors try

Co-authored-by: Jan Haller <bromeon@gmail.com>
@Bromeon Bromeon added the ub Undefined behavior label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: ffi Low-level components and interaction with GDExtension API c: tooling CI, automation, tools ub Undefined behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AddressSanitizer reports heap-use-after-free in object_instance_id_when_freed integration test
1 participant