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

sanitizers: Fix tests/ui/sanitize/leak.rs fails on #129621

Closed
wants to merge 1 commit into from

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Aug 26, 2024

Fix #111073 by checking if -Zexport-executable-symbols is passed when LeakSanitizer is enabled.

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 26, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2024

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rcvalle
Copy link
Member Author

rcvalle commented Aug 26, 2024

r? @tmiasko

@rustbot rustbot assigned tmiasko and unassigned TaKO8Ki Aug 26, 2024
@rcvalle rcvalle force-pushed the rust-sanitizers-fix-111073 branch from b88d09e to cdb1e11 Compare August 26, 2024 19:25
@rust-log-analyzer

This comment has been minimized.

Fix rust-lang#111073 by checking if `-Zexport-executable-symbols` is passed when
LeakSanitizer is enabled.
@rcvalle rcvalle force-pushed the rust-sanitizers-fix-111073 branch from cdb1e11 to 1b04bb8 Compare August 26, 2024 20:56
@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2024

Some changes occurred in tests/codegen/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rcvalle
Copy link
Member Author

rcvalle commented Aug 27, 2024

cc @compiler-errors

@rcvalle
Copy link
Member Author

rcvalle commented Aug 27, 2024

@compiler-errors for context: #123617 (comment)

@tmiasko
Copy link
Contributor

tmiasko commented Aug 30, 2024

Can you describe the motivation behind this change?

Is it just a workaround for #111073? I think the actual issue is in sanitizer runtime and should be fixed there. In particular the interceptor for free works incorrectly when invoked with free(NULL) during initialization:

https://github.com/llvm/llvm-project/blob/332e6f86c50218ce60cafc9bf6d38d907da535ea/compiler-rt/lib/lsan/lsan_interceptors.cpp#L79-L84

For some reason, dynamic loader happens to call free(NULL) in one scenario but not in the other ...

@tmiasko tmiasko added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2024
@rcvalle
Copy link
Member Author

rcvalle commented Sep 10, 2024

This would be a fix for Clang linking the sanitizer runtimes with --dynamic-list=... or --export-dynamic. Is this related to the free(NULL) interception during initialization? Is the later caused by the former?

@tmiasko
Copy link
Contributor

tmiasko commented Sep 12, 2024

With regards to #111073, while --export-dynamic and -Zexport-executable-symbols avoids the issue for some reason, the interaction is unrelated to the root cause of the problem. Anyway, this should be already fixed in lsan runtime.

From your answer it is still not clear for me if the motivation for this change is limited to #111073 or attempts to replicate clang behavior more generally. If the latter can you describe why is it necessary? What is the problem and how does this change address it?

Additionally, this approach introduces dependency on another unstable feature which would block proposed stabilization.

@rcvalle
Copy link
Member Author

rcvalle commented Sep 12, 2024

If the Clang linking the sanitizer runtimes with --dynamic-list=... or --export-dynamic isn't a separate issue, and just worked around the free(NULL) interception during initialization for unknown reasons, I think we can close this if it's not needed.

@rcvalle rcvalle closed this Sep 12, 2024
@tmiasko
Copy link
Contributor

tmiasko commented Sep 24, 2024

clang exporting sanitizer interface with --dynamic-list makes it possible to load dynamic objects using dlopen that refer to sanitizer interface. The fact that rustc doesn't do that is definitely an issue.

At the moment, -Zexport-executable-symbols is no solution for this problem. The current implementation is incomplete - it doesn't add any symbols to dynamic symbol table. It is also unaware of sanitizer interface and so wouldn't export it.

To address that problem I would suggest following clang strategy. Add necessary linker flags automatically when sanitizers are enabled (as opposed to requiring end user to enable additional options manually as proposed here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests/ui/sanitize/leak.rs fails on Linux with sanitizers = true
5 participants