Add support for dylibs with Address Sanitizer #42711
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Undefined symbol Probably because LLVM 3.7 is too old (ABI changed in https://reviews.llvm.org/D11004?id=29191 ?) |
|
Possibly. I was worried the manual linking in the rustc process in the test could be part of the issue (note the -lasan). |
|
I'm not sure why the "waiting on review" flag was removed. I explicitly asked for a review, because I need advice from a more experienced developer to help fix the issue from the CI regarding the fact that I have to force add -lasan at the moment (it should be getting added without that). |
|
Sorry about that. I've readded it. |
|
I think You shouldn't need With that removed, I get a different fun error: |
|
This latest commit resolves the test issues that were mentioned by @cuviper as well as resolving the ld issue that was noted. I incorrectly added the runtime of asan to dylibs when it was not required. Observing the test outputs now shows: Additionally, looking at the elf symbols we can see the correct list of asan symbols in the dylib: |
src/librustc_metadata/creader.rs
Outdated
| @@ -861,8 +861,11 @@ impl<'a> CrateLoader<'a> { | |||
| // This crate will be compiled with the required | |||
| // instrumentation pass | |||
| config::CrateTypeRlib => false, | |||
There was a problem hiding this comment.
Nit: use |, like
config::CrateTypeRlib |
config::CrateTypeDylib |
config::CrateTypeCdylib |
config::CrateTypeStaticLib =>
false,
_ => {
err(...)
}
There was a problem hiding this comment.
Are we testing all of these modes?
|
Thanks for the PR @Firstyear! It looks like this also adds staticlib/cdylib support as well, right? Mind adding tests for those as well? |
|
Hey @nikomatsakis and @alexcrichton Thanks for your reviews! I've fixed both nits raised in my branch. I'm running the tests now to be sure they work. With the staticlib test, I can't think of a good way to do this "just" with rust. Would it be acceptable to use a small C program and link with the static lib to check this? Or do you have a better suggestion as to how I could write this test? Thanks again, |
|
Ah yeah having a small C shim is fine. You can poke around some other run-make tests perhaps? There should be some other examples of using |
|
Thanks for that @alexcrichton. I have made the C shim using another test as an example to follow. I have also corrected the nits raised by @nikomatsakis . Looking forward to your review, thanks! |
|
@bors: r+ |
|
📌 Commit 96e8e34 has been approved by |
|
⌛ Testing commit 96e8e34 with merge ccabdd95ea6594dcce40309e83322882fb69cd80... |
|
@Mark-Simulacrum the initial usage in the previous successful build was 13G/30G. Why suddenly bloated by 7G? It seems #43026 failed with the same reason. Edit: the last successful build (13G) runs on "Connie", the following ones run on "Sugilite", which probably explains the 7G increase. There's still no reason why "Sugilite" was chosen. |
|
💔 Test failed - status-travis |
|
@bors retry |
|
⌛ Testing commit 9d339f4 with merge bbcedacb5a75d3388763996040ba9183d433b264... |
|
@Mark-Simulacrum @alexcrichton looks like we need to change |
|
💔 Test failed - status-travis |
|
@kennytm Would you like to file a PR? I will in a few hours when I get around to it if not.. |
|
@Mark-Simulacrum I'm on mobile right now, won't reach a real computer in the next few hours, so no, sorry. |
|
I'm assuming this means I just sit here and wait for the issue to be resolved by @kennytm ? Thanks again, |
|
#43198 has been merged, this PR can be retried now. |
|
@bors retry |
|
⌛ Testing commit 9d339f4 with merge af67663687882a8908095d850d44dcc3318ae451... |
|
@bors: retry
|
|
⌛ Testing commit 9d339f4 with merge 8fb712dedc39c431448817fd6e25640a88bb7c17... |
|
💔 Test failed - status-travis |
|
x86_64-gnu's The command should end with |
…and staticlibs on gnu-linux targets.
|
It worked for me on my system .... sigh, okay, new update with the '--' coming in, which also passes for me on my system. |
Yeah, that wasn't my fault ;) Anyway to make this run again? |
|
@bors: r+ |
|
📌 Commit 0af5c00 has been approved by |
Add support for dylibs with Address Sanitizer Many applications use address sanitizer to assert correct behaviour of their programs. When using Rust with C, it's much more important to assert correct programs with tools like asan/lsan due to the unsafe nature of the access across an ffi boundary. However, previously only rust bin types could use asan. This posed a challenge for existing C applications that link or dlopen .so when the C application is compiled with asan. This PR enables asan to be linked to the dylib and cdylib crate type. We alter the test to check the proc-macro crate does not work with -Z sanitizer=address. Finally, we add a test that compiles a shared object in rust, then another rust program links it and demonstrates a crash through the call to the library. This PR is nearly complete, but I do require advice on the change to fix the -lasan that currently exists in the dylib test. This is required because the link statement is not being added correctly to the rustc build when -Z sanitizer=address is added (and I'm not 100% sure why) Thanks,
|
☀️ Test successful - status-appveyor, status-travis |
Many applications use address sanitizer to assert correct behaviour of their programs. When using Rust with C, it's much more important to assert correct programs with tools like asan/lsan due to the unsafe nature of the access across an ffi boundary. However, previously only rust bin types could use asan. This posed a challenge for existing C applications that link or dlopen .so when the C application is compiled with asan.
This PR enables asan to be linked to the dylib and cdylib crate type. We alter the test to check the proc-macro crate does not work with -Z sanitizer=address. Finally, we add a test that compiles a shared object in rust, then another rust program links it and demonstrates a crash through the call to the library.
This PR is nearly complete, but I do require advice on the change to fix the -lasan that currently exists in the dylib test. This is required because the link statement is not being added correctly to the rustc build when -Z sanitizer=address is added (and I'm not 100% sure why)
Thanks,