Selectively disable sanitizer instrumentation

[tmiasko]

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

[rust-highfive]

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]

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]

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]

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]

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

[nikomatsakis]

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]

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]

Would be good to extract this to a function.

[pnkfelix]

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]

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]

• 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.

[nikomatsakis]

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

[davidtwco]

@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]

@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]

@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]

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]

@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]

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]

#[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]

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]

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]

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)], notnever.

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]

@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]

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]

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]

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

[nikomatsakis]

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

[nikomatsakis]

(I am doing so)

[nikomatsakis]

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

[nikomatsakis]

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

[tmiasko]

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.

[nikomatsakis]

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

[nikomatsakis]

@bors r+

[bors]

📌 Commit 80adde2 has been approved by nikomatsakis