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 memory leak caused by LLVMVerifyModule function call #507

Merged

Conversation

antonbaliasnikov
Copy link
Contributor

Description

Fix a memory leak in the verify function that calls the LLVMVerifyModule function of LLVM.

If we build inkwell with Address sanitizer enabled:
RUSTFLAGS="-Z sanitizer=address" cargo +nightly build --target aarch64-unknown-linux-gnu -Zbuild-std

And try to use the the inkwell::module::Module::verify function, then sanitizer will report the following memory leak (if the function succeeds):

==31844==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2 byte(s) in 2 object(s) allocated from:
    #0 0x56420a9edfda in strdup /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:570:3
    #1 0x564210600990 in LLVMVerifyModule /__w/era-compiler-tester/era-compiler-tester/target-llvm/build-final/../../llvm/llvm/lib/Analysis/Analysis.cpp:94:20
    #2 0x56420bfd2fef in inkwell::module::Module::verify::hc511d5bc97d2c81f /usr/local/cargo/git/checkouts/inkwell-ae33c16eecfbdc50/c0821d7/src/module.rs:736:29
...

This is caused by not freeing the memory allocated in the strdup() call in the LLVMVerifyModule LLVM function .

This fix calls LLVMDisposeMessage to explicitly call free() for the err_str pointer fixing the memory leak.

Related Issue

N/A

How This Has Been Tested

Manually by building with sanitizers including this fix and calling verify function.

Option<Breaking Changes>

N/A

Checklist

@TheDan64
Copy link
Owner

TheDan64 commented Jun 19, 2024

@antonbaliasnikov there are test failures that need to be addressed

Edit: May be unrelated

@antonbaliasnikov
Copy link
Contributor Author

antonbaliasnikov commented Jun 20, 2024

@antonbaliasnikov there are test failures that need to be addressed

Edit: May be unrelated

Hey @TheDan64 ,

I checked tests locally for a failed configuration of LLVM 4.0 on Ubuntu 20.04. All tests passed:

cargo test --release --features llvm4-0 --verbose
...
test result: ok. 296 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 11.67s

I'm attaching the full log for the reference.

inkwell-llvm-4.0-log.txt

This CI error (invalid memory reference) is odd (hardware related?) and looks not feasible to debug from my side, especially without repository access (I can't even restart workflows to check if it's sporadic or not).

Could you, please, check it with a test PR from the master? Let me know if I can help somehow.

@TheDan64
Copy link
Owner

Not sure why, but master started failing, even though it passed before. Perhaps GH changed hardware or something under us which revealed existing UB

@TheDan64 TheDan64 added this to the 0.5.0 milestone Jul 2, 2024
Copy link
Owner

@TheDan64 TheDan64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, thank you!

@TheDan64 TheDan64 merged commit c3f6e05 into TheDan64:master Jul 27, 2024
16 checks passed
@antonbaliasnikov antonbaliasnikov deleted the ml-aba-fix-llvm-verify-memory-leak branch July 29, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants