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

Prefer signal handlers provided by a sanitizer runtime to those in std. #69540

Closed
wants to merge 2 commits into from

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Feb 28, 2020

Sanitizer runtimes install signal handlers for SIGBUS and SIGSEGV which
generate informative error reports. Use them in preference to those installed
by std.

The first commit introduces the use of cfg_if to select between different
signal handlers implementations, no functional changes are intended there.
The second commit uses empty implementation when sanitizers are enabled.

Helps with #69524.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2020
@jsgf
Copy link
Contributor

jsgf commented Feb 28, 2020

I think this only works if libstd is compiled with a santizer which doesn't happen by default.

I think this check needs to be done at runtime - it should only install the the handler if there isn't one already (or chain to it in the non overflow case).

@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 29, 2020

Yes, those changes require recompilation of standard library, but it is
intentional design choice which avoids changing the current behaviour of
signal handlers more generally.

Additionally, using sanitizer with uninstrumented standard library either
doesn't work at all (memory sanitizer), or produces false positives and false
negatives (thread sanitizer) or false negatives (address sanitizer), and
should be avoided.

@sfackler
Copy link
Member

In what context will these actually be picked up then? Does cargo-fuzz recompile the standard library with sanitizers for example?

@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 29, 2020

When using cargo -Zbuild-std, which is how I would recommend to use them. For further details see https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/sanitizer.html#instrumentation-of-external-dependencies-and-std.

cargo-fuzz does supports the -Zbuild-std, although there are additional trade-offs involved in that specific context.

@jsgf
Copy link
Contributor

jsgf commented Mar 2, 2020

Asan is not ideal with uninstrumented libstd but still completely usable and useful. That's particularly true for catching faults which requires no instrumentation to be useful.

I don't think relying on build-std to get major parts of santizer functionality is necessary or desirable.

Agree about msan and tsan but I think they're a separate case.

@tmiasko
Copy link
Contributor Author

tmiasko commented Mar 2, 2020

Thanks for clarification. If this approach doesn't solve the issue in your context I would suggest proposing the runtime variant independently

@cuviper
Copy link
Member

cuviper commented Mar 4, 2020

See #69685 for a runtime check.

@tmiasko
Copy link
Contributor Author

tmiasko commented Mar 7, 2020

Superseded by changes from #69685. Thanks for creating the pull request @cuviper.

@tmiasko tmiasko closed this Mar 7, 2020
@tmiasko tmiasko deleted the sanitizer-signals branch March 21, 2020 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants