Selectively disable sanitizer instrumentation #68164

tmiasko · 2020-01-12T21:42:29Z

Add no_sanitize attribute that allows to opt out from sanitizer
instrumentation in an annotated function.

rust-highfive · 2020-01-12T21:42:32Z

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

davidtwco · 2020-01-13T13:25:06Z

Is there an RFC for this that is being implemented? I've not been able to find one and this feels like it would be the type of thing that requires one?

cc @rust-lang/lang

nikomatsakis · 2020-01-13T14:21:56Z

I do not believe there is any RFC. This support has been effectively "evolving" in an experimental fashion. I think this is potentially a reasonable thing, but it would be useful at minimum to be tracking the design as it evolves -- @tmiasko has been updating the unstable book, which is great, but it seems like it'd be nice to be collecting the overall progress and (ideally) direction.

(Is there a tracking issue for sanitizer support? There must be but I didn't find it right away...)

tmiasko · 2020-01-14T17:34:41Z

The general tracking issue for sanitizers is #39699. I also used it as a tracking issue here, although I don't have the rights to edit the issue itself.

nikomatsakis · 2020-01-14T20:20:46Z

OK well I at least updated that commit to link to this PR and document the existence of #[no_sanitize]

nikomatsakis · 2020-01-14T20:23:37Z

I guess I would say that I think it's ok to land this as part of that "WIP" work, @davidtwco, so long as it's all unstable, but I do think it'd be nice to decide if we want to carve out a path to stability for this (seems like it'd be good to do so?)

nikomatsakis · 2020-01-14T20:24:34Z

I suppose I can nominate it for lang discussion, since at this point we're modifying the language, even if it's only an attribute. Let me do that.

Centril · 2020-01-16T21:25:15Z

src/librustc_typeck/collect.rs

+            if let Some(list) = attr.meta_item_list() {
+                for item in list.iter() {
+                    if item.check_name(sym::address) {
+                        codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_SANITIZE_ADDRESS;
+                    } else if item.check_name(sym::memory) {
+                        codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_SANITIZE_MEMORY;
+                    } else if item.check_name(sym::thread) {
+                        codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_SANITIZE_THREAD;
+                    } else {
+                        tcx.sess
+                            .struct_span_err(item.span(), "invalid argument for `no_sanitize`")
+                            .note("expected one of: `address`, `memory` or `thread`")
+                            .emit();
+                    }
+                }
+            }


Would be good to extract this to a function.

pnkfelix · 2020-01-17T01:50:03Z

We discussed this at today's lang team meeting.

The main concerns were:

We'd like documentation in the unstable guide. ( think that can be a follow-up work item.)
We do not want uses of sanitize-related attributes to affect the language's static semantics. That means their (mis)use should not generate static semantic-analysis errors that cannot be overridden. More specifically: we understand that the user should not expect useful results from combining #[inline(always)] fn foo() with turning off sanitization of foo; but that should just signal a diagnostic warning.
Bikeshed warning: The specific name #[no_sanitize(..)] may be a mistake.
- I personally suspect we may want a richer set of attributes associated with sanitizers in the long term. (For example, it was pointed out during the meeting that miri is arguably just another sanitizer.)
- We might have more room to grow the set of attributes naturally if we expressed this one as: #[sanitize(never, ...)].
- Or maybe we would write #[sanitize(never(address, memory, thread))]

pnkfelix · 2020-01-17T01:50:58Z

Having said all that, we were basically in favor of moving forward with this, without needing an RFC at this time, as long as you address the static semantics issue (and of course make sure this all continues to be feature-gated).

tmiasko · 2020-01-17T15:53:52Z

Changed inline(always) compatibility diagnostic to emit a warning instead of an error.
Draft of changes to rustc-guide Update sanitizers documentation rustc-dev-guide#562. Please comment there if there is additional information you would like to see included.

src/librustc_typeck/collect.rs

nikomatsakis · 2020-01-21T23:47:15Z

@davidtwco do you feel up for reviewing this, or should we reassign? (I could take it, in theory)

davidtwco · 2020-01-22T07:02:32Z

@davidtwco do you feel up for reviewing this, or should we reassign? (I could take it, in theory)

Oops, I missed the follow up comments after being nominated for discussion.

r? @nikomatsakis

nikomatsakis · 2020-01-23T19:52:08Z

@tmiasko In the lang team meeting, there was general consensus that #[sanitize(never)] would be better than #[no_sanitize], how do you feel about implementing that syntax instead?

tmiasko · 2020-01-23T23:15:49Z

@nikomatsakis I assume that you are referring to the #[sanitize(never(address, memory))] variant.

Did you have any specific (even if hypothetical) extensions in mind when
discussing it? For example, selectively enabling checks for particular function
doesn't strike me as something useful, nor does clang / gcc have any more
elaborate extensions here.

The no_sanitize is already flexible enough to accommodate different kinds of
sanitizers, or more fine grained checks within a single sanitizer. I would find
no_sanitize preferable, although I don't have strong feelings about it if lang
team considers otherwise.

Centril · 2020-01-23T23:32:10Z

Ostensibly one might want to pass configuration parameters to the sanitizer via the attribute. For example: #[sanitize(miri(aggressive))] to opt into extra aggressive Miri checks.

nikomatsakis · 2020-01-24T14:47:33Z

@tmiasko

One thing we were considering is that there are likely to be different kinds of sanitizers, and you might want a function to be sanitized in some cases but not others. I do hope that as we make progress around the unsafe code guidelines, for example, it will be standard practice for unsafe code authors to run their code using a Rust-provided sanitizer of some kind that detects violations of our aliasing rules, and (as @pnkfelix pointed out) miri could be considered a sort of sanitizer (indeed, miri is what we currently use for that purpose, though it's not ideal as it can't handle C code).

The main downside I see to sanitize(never) is that it is more work to write the code to recognize and accept it.

I guess there is also a bikeshed side to it... #[no_sanitize] feels just that little bit more ad-hoc, though I don't have a strong opinion on this point and I suspect we have precedent both ways.

tmiasko · 2020-01-26T12:34:29Z

I feel that a positive variant of no_sanitize will not be a thing. After all
one does not generally know where the problematic code is, and when such a
code is located solution is never going to be: add more sanitizer checks.

I would rather avoid a deeply nested variant as long as there are no use-cases for it.

nikic · 2020-01-27T14:00:12Z

#[sanitize(address(never))] gets awkward with fine-grained sanitizers. While rustc doesn't have UBSan, the C++ analogon would be stuff like #[sanitizer(division_by_zero(never), signed_integer_overflow(never))]. I think that is quite a bit less readable than #[no_sanitize(division_by_zero, signed_integer_overflow)].

nikomatsakis · 2020-01-28T07:15:39Z

I guess the option option would be to push the "no" inwards #[sanitize(no_division_by_zero...)] and so forth, but I don't find that particularly readable.

#[skip_sanitize] and #[no_sanitize] don't seem especially different to me. The precedent I can think of is towards no_ -- e.g., no_mangle, no_std, no_implicit_prelude. There is #[rustfmt::skip], I suppose.

In any case, I think I'm persuaded that something along the lines of #[no_sanitize] is better, unless we have an example of a "positive" use case where we think we might want to say sanitize(foo) for something that is not skipping the sanitizer.

Centril · 2020-01-28T14:17:59Z

Remembering that Rust is sound by default, and thus that using sanitizers are substantially less necessary than in e.g., C++, how often would you need such fine-grained approaches as #[sanitizer(division_by_zero(never), signed_integer_overflow(never))]? Presumably, you can also put each of these on separate lines which in my view would be better for readability:

#[sanitizer(division_by_zero(never)]
#[sanitizer(signed_integer_overflow(never))]
fn stuff() { ... }

@nikomatsakis I gave a positive example in #68164 (comment). It doesn't seem so strange to me that you could pass per-function configuration to some specific sanitizer ("when entering this function, start sanitizing with X configuration").

eddyb · 2020-01-28T17:07:40Z

I'm persuaded by the precedent of #[no_mangle], FWIW, I was hoping we don't have a lot of these laying around but sometimes it's easier to compromise.

The alternative there is probably #[mangling(none)], not never.

Actually, #[inline(never)] is named that because the decision to inline can be made many times (i.e. for each possible call site), so always/never makes sense.
(Similarly, the default for #[inline] is roughly "sometimes", which doesn't apply to mangling or sanitizers, IMO)

So I'd argue #[sanitize(address = no)] is more plausible than #[sanitize(address(never))].

(But this is again bikeshed, and we can do #[no_sanitize] on the precedent of #[no_mangle] and it doesn't feel significantly worse, just wanted to expand my thoughts a bit)

nikomatsakis · 2020-02-04T21:41:14Z

@Centril Ah, the example of #[sanitize(miri(aggressive))] is plausible, I agree, though I'm not sure if any existing sanitizer has such a concept. Still, it doesn't really fit within a #[no_sanitizer] attribute at all.

I still hesitate, though, I guess because @tmiasko makes a good case that #[no_sanitize] seems far more common, and because the alternatives based on never feel clumsy, though I do sort of like @eddyb's proposal of #[sanitize(foo = no)] .

Centril · 2020-02-05T00:06:05Z

I'm not particularly attached to never specifically. miri = no + miri(aggressive) could work together. Mainly, I think that the negation in the attribute name is a mistake as it doesn't leave room for the positive case.

nikic · 2020-02-05T11:23:41Z

It seems pretty clear that there is no consensus on this question. Is it possible to land this as-is and decide the final syntax at a later point in time, prior to stabilization? Maybe by that time it will be clearer whether there are other use-cases for the syntax and what they are?

Centril · 2020-02-05T11:26:58Z

I would be happy to defer the naming to a later point in time.

nikomatsakis · 2020-02-05T22:04:33Z

Yeah, I'm good with that too. It would be worth adding a short summary of the notes here to the tracking issue.

nikomatsakis · 2020-02-05T22:05:21Z

(I am doing so)

nikomatsakis

OK, did a quick review for code quality, left a few nits @tmiasko

nikomatsakis · 2020-02-05T22:09:29Z

src/librustc_typeck/collect.rs

+        | CodegenFnAttrFlags::NO_SANITIZE_MEMORY
+        | CodegenFnAttrFlags::NO_SANITIZE_THREAD;
+    if codegen_fn_attrs.flags.intersects(no_sanitize_flags) {
+        if codegen_fn_attrs.inline == InlineAttr::Always {


We issue this lint for #[inline(always)], but not just a plain #[inline]?

Rustc and LLVM go out of the way to respect #[inline(always)] attribute.
Even ignoring restrictions that would otherwise prevent inlining.
I didn't want to change that behaviour just for no-sanitize.

On the other hand the #[inline] hint is much weaker. In the case that
no-sanitize attribute is present and sanitizer enabled, it takes the
precedence over inlining.

src/librustc_typeck/collect.rs

src/librustc/middle/codegen_fn_attrs.rs

Add `no_sanitize` attribute that allows to opt out from sanitizer instrumentation in an annotated function.

nikomatsakis

OK, seems good. I noted the key elements of the design in #39699.

nikomatsakis · 2020-02-07T11:02:21Z

@bors r+

bors · 2020-02-07T11:02:22Z

📌 Commit 80adde2 has been approved by nikomatsakis

Selectively disable sanitizer instrumentation Add `no_sanitize` attribute that allows to opt out from sanitizer instrumentation in an annotated function.

@ghost

Rollup of 7 pull requests Successful merges: - #68164 (Selectively disable sanitizer instrumentation) - #68413 (Add GitHub issue templates) - #68889 (Move the `hir().krate()` method to a query and remove the `Krate` dep node) - #68909 (Respect --nocapture in panic=abort test mode) - #68910 (Add myself to .mailmap) - #68919 (Remove HashStable impl for ast::Lifetime) - #68928 (clean up E0276 explanation) Failed merges: r? @ghost

rust-highfive assigned davidtwco Jan 12, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 12, 2020

japaric mentioned this pull request Jan 14, 2020

Tracking issue for sanitizer support #39699

Open

5 tasks

nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 14, 2020

Centril reviewed Jan 16, 2020

View reviewed changes

pnkfelix removed the I-nominated label Jan 17, 2020

tmiasko force-pushed the no-sanitize branch from 5cf1f7e to f0d697f Compare January 17, 2020 12:46

Centril reviewed Jan 17, 2020

View reviewed changes

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

tmiasko force-pushed the no-sanitize branch 2 times, most recently from 537aa76 to 50acfa4 Compare January 19, 2020 12:38

rust-highfive assigned nikomatsakis and unassigned davidtwco Jan 22, 2020

tmiasko mentioned this pull request Feb 3, 2020

Update sanitizers documentation rust-lang/rustc-dev-guide#562

Merged

nikomatsakis requested changes Feb 5, 2020

View reviewed changes

tmiasko added 3 commits February 5, 2020 23:30

Selectively disable sanitizer instrumentation

b846b42

Add `no_sanitize` attribute that allows to opt out from sanitizer instrumentation in an annotated function.

Apply LLVM sanitize attributes to generated entry wrapper

1caa875

Add CodegenFnAttrFlags::NO_SANITIZE_ANY

80adde2

tmiasko force-pushed the no-sanitize branch from 50acfa4 to 80adde2 Compare February 6, 2020 08:26

nikomatsakis approved these changes Feb 7, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2020

Dylan-DPC-zz mentioned this pull request Feb 7, 2020

Rollup of 6 pull requests #68927

Closed

Dylan-DPC-zz mentioned this pull request Feb 7, 2020

Rollup of 7 pull requests #68933

Merged

bors merged commit 80adde2 into rust-lang:master Feb 7, 2020

tmiasko deleted the no-sanitize branch February 7, 2020 21:03

steckes mentioned this pull request Dec 5, 2024

Rust RFC cjappl/radsan#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selectively disable sanitizer instrumentation #68164

Selectively disable sanitizer instrumentation #68164

tmiasko commented Jan 12, 2020

rust-highfive commented Jan 12, 2020

davidtwco commented Jan 13, 2020

nikomatsakis commented Jan 13, 2020

tmiasko commented Jan 14, 2020

nikomatsakis commented Jan 14, 2020

nikomatsakis commented Jan 14, 2020 •

edited

Loading

nikomatsakis commented Jan 14, 2020

Centril Jan 16, 2020

pnkfelix commented Jan 17, 2020

pnkfelix commented Jan 17, 2020 •

edited

Loading

tmiasko commented Jan 17, 2020

nikomatsakis commented Jan 21, 2020

davidtwco commented Jan 22, 2020

nikomatsakis commented Jan 23, 2020

tmiasko commented Jan 23, 2020

Centril commented Jan 23, 2020

nikomatsakis commented Jan 24, 2020

tmiasko commented Jan 26, 2020

nikic commented Jan 27, 2020 •

edited

Loading

nikomatsakis commented Jan 28, 2020

Centril commented Jan 28, 2020

eddyb commented Jan 28, 2020

nikomatsakis commented Feb 4, 2020

Centril commented Feb 5, 2020

nikic commented Feb 5, 2020

Centril commented Feb 5, 2020

nikomatsakis commented Feb 5, 2020

nikomatsakis commented Feb 5, 2020

nikomatsakis left a comment

nikomatsakis Feb 5, 2020

tmiasko Feb 6, 2020

nikomatsakis left a comment

nikomatsakis commented Feb 7, 2020

bors commented Feb 7, 2020

Selectively disable sanitizer instrumentation #68164

Selectively disable sanitizer instrumentation #68164

Conversation

tmiasko commented Jan 12, 2020

rust-highfive commented Jan 12, 2020

davidtwco commented Jan 13, 2020

nikomatsakis commented Jan 13, 2020

tmiasko commented Jan 14, 2020

nikomatsakis commented Jan 14, 2020

nikomatsakis commented Jan 14, 2020 • edited Loading

nikomatsakis commented Jan 14, 2020

Centril Jan 16, 2020

Choose a reason for hiding this comment

pnkfelix commented Jan 17, 2020

pnkfelix commented Jan 17, 2020 • edited Loading

tmiasko commented Jan 17, 2020

nikomatsakis commented Jan 21, 2020

davidtwco commented Jan 22, 2020

nikomatsakis commented Jan 23, 2020

tmiasko commented Jan 23, 2020

Centril commented Jan 23, 2020

nikomatsakis commented Jan 24, 2020

tmiasko commented Jan 26, 2020

nikic commented Jan 27, 2020 • edited Loading

nikomatsakis commented Jan 28, 2020

Centril commented Jan 28, 2020

eddyb commented Jan 28, 2020

nikomatsakis commented Feb 4, 2020

Centril commented Feb 5, 2020

nikic commented Feb 5, 2020

Centril commented Feb 5, 2020

nikomatsakis commented Feb 5, 2020

nikomatsakis commented Feb 5, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Feb 5, 2020

Choose a reason for hiding this comment

tmiasko Feb 6, 2020

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Feb 7, 2020

bors commented Feb 7, 2020

nikomatsakis commented Jan 14, 2020 •

edited

Loading

pnkfelix commented Jan 17, 2020 •

edited

Loading

nikic commented Jan 27, 2020 •

edited

Loading