-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Add LLVM realtime sanitizer #147935
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
base: master
Are you sure you want to change the base?
Add LLVM realtime sanitizer #147935
Conversation
|
Some changes occurred in tests/ui/sanitizer cc @rcvalle Some changes occurred in src/tools/compiletest cc @jieyouxu Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_hir/src/attrs These commits modify compiler targets. Some changes occurred in src/doc/unstable-book/src/compiler-flags/sanitizer.md cc @rust-lang/project-exploit-mitigations, @rcvalle Some changes occurred in compiler/rustc_attr_parsing This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
rustbot has assigned @petrochenkov. Use |
|
Looks ok from attr parsing pov :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
|
This change has a lang component (marking functions a blocking or non-blocking), so I'll send this to lang team first, before reviewing the implementation in detail. |
|
Thanks for checking with us. We talked about this in the lang meeting today. We were OK with this going forward experimentally in nightly. We talked about how, ahead of stabilization, we'd like to see the feature flags for the individual sanitizers be separate to facilitate our review, as we're likely to want to stabilize these incrementally. |
| declare_lint! { | ||
| /// The `rtsan_nonblocking_async` lint detects incompatible use of | ||
| /// [`#[sanitize(realtime = "nonblocking")]`][sanitize] on async functions. | ||
| /// | ||
| /// [sanitize]: https://doc.rust-lang.org/nightly/unstable-book/language-features/no-sanitize.html | ||
| /// ### Example | ||
| /// | ||
| /// ```rust | ||
| /// #![feature(sanitize)] | ||
| /// | ||
| /// #[sanitize(realtime = "nonblocking")] | ||
| /// async fn x() {} | ||
| /// | ||
| /// fn main() { | ||
| /// x() | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// {{produces}} | ||
| /// | ||
| /// ### Explanation | ||
| /// | ||
| /// The sanitizer only considers the async function body nonblocking. The executor, which runs on | ||
| /// every `.await` point can run non-realtime code, without the sanitizer catching it. | ||
| pub RTSAN_NONBLOCKING_ASYNC, | ||
| Warn, | ||
| r#"detects incompatible uses of `#[sanitize(realtime = "nonblocking")]` on async functions"#, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the sanitizer should warn on .await from a nonblocking context then. Including in async blocks and closures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds correct. I will try to implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For async fn there are multiple functions being generated. One that constructs a future, second that drops it, and third that polls it. Usually (by default) attributes apply to the first one, which in this case makes the least sense, since a function that constructs the future never blocks.
EDIT: Never mind. Since in effect sanitizer attributes are computed recursively, they are inherited by default unlike all other attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to also catch async blocks and async closures with the warning.
I am unsure on how to deal with the three function split for the "nonblocking" attribute.
I assume it is possible to apply attributes to the other generated functions?
I could apply the attribute to all three functions (or just the polling and drop function), but i don't know if this really makes sense. It still won't behave as expected, as the executor still won't be sanitized, so there is a lot of code between the start and the end of the function that rtsan doesn't "see" (the original reason for adding the warning).
For the "blocking" attribute i will probably apply the attribute to the polling and destructing functions, because these are the only ones that run user defined code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmiasko i have now read your edit.
So i don't need to change anything here, correct? The attributes get applied to all three functions, which is the best that can be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't expect any specific changes. Just a general suggestion to review whether the existing behavior makes sense.
As far as I understand, for a function that constructs a future, sanitizer settings are based on attributes of async fn item (and inherited from a parent). A polling function is logically nested inside, so its settings are are inherited from async fn item. A drop function is an instantiation of drop_in_place from core and therefore uses sanitizer settings of drop_in_place.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Since T-lang discussed this: |
| let sanitizers = tcx.sanitizer_settings_for(did); | ||
| codegen_fn_attrs.sanitizers.disabled |= sanitizers.disabled; | ||
| if codegen_fn_attrs.sanitizers.rtsan_setting == RtsanSetting::default() { | ||
| codegen_fn_attrs.sanitizers.rtsan_setting = sanitizers.rtsan_setting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok here that explicit #[sanitize(realtime = "blocking")] will be equivalent to not specifying the sanitizer attribute at all? It will also be overridden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not completely sure what you mean.
The behaviour for the new sanitizer should be the same as for the other sanitizers. For example if address sanitizer is set in the passed in codegen_fn_attrs it doesn't matter what the value in the tcx.sanitizer_settings_for is, because of binary or.
For rtsan i have done the same: If the setting is non-default in the passed in codegen_fn_attrs it doesn't matter what the setting from tcx.sanitizer_settings_for is.
| let hir_id = tcx.local_def_id_to_hir_id(did); | ||
| tcx.node_span_lint(lint::builtin::RTSAN_NONBLOCKING_ASYNC, hir_id, sanitize_span, |lint| { | ||
| lint.primary_message(r#"the async executor can run blocking code, without realtime sanitizer catching it"#); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: weird formatting, if rustfmt skips this code, then it can be adjusted manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the if condition or the node_span_lint call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some changes. let me know if you are happy with it.
This is a new attempt at adding the LLVM real-time sanitizer to rust.
Previously this was attempted in rust-lang/rfcs#3766.
Since then the
sanitizeattribute was introduced in #142681 and it is a lot more flexible than the oldno_santizeattribute. This allows adding real-time sanitizer without the need for a new attribute, like it was proposed in the RFC. Because i only add a new value to a existing command line flag and to a attribute i don't think an MCP is necessary.Currently real-time santizer is usable in rust code with the rtsan-standalone crate. This downloads or builds the sanitizer runtime and then links it into the rust binary.
The first commit adds support for more detailed sanitizer information.
The second commit then actually adds real-time sanitizer.
The third adds a warning against using real-time sanitizer with async functions, cloures and blocks because it doesn't behave as expected when used with async functions. I am not sure if this is actually wanted, so i kept it in a seperate commit.
The fourth commit adds the documentation for real-time sanitizer.