Remove the #[no_sanitize] attribute in favor of #[sanitize(xyz = "on|off")]

[1c3t3a]

This came up during the sanitizer stabilization (#123617). Instead of a #[no_sanitize(xyz)] attribute, we would like to have a #[sanitize(xyz = "on|off")] attribute, which is more powerful and allows to be extended in the future (instead
of just focusing on turning sanitizers off). The implementation is done according to what was discussed on Zulip).

The new attribute also works on modules, traits and impl items and thus enables usage as the following:

#[sanitize(address = "off")]
mod foo {
    fn unsanitized(..) {}

    #[sanitize(address = "on")]
    fn sanitized(..) {}
}

trait MyTrait {
  #[sanitize(address = "off")]
  fn unsanitized_default(..) {}
}

#[sanitize(thread = "off")]
impl MyTrait for () {
    ...
}

r? @rcvalle

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

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

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

Some changes occurred in src/doc/unstable-book/src/language-features/no-sanitize.md

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

Some changes occurred in tests/codegen/sanitizer

cc @rcvalle

Some changes occurred in tests/ui/sanitizer

cc @rcvalle

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

[rcvalle]

Thank you very much for your time and work on this, @1c3t3a! Much appreciated.

[rcvalle]

cc @Darksonn @wesleywiser @traviscross

[Darksonn]

It doesn't matter for me, but should we make this change gradually by supporting both for some time?

[1c3t3a]

It doesn't matter for me, but should we make this change gradually by supporting both for some time?

That's possible for sure, I put stuff into different commits for that reason and I can just drop the middle commit to have both exist in parallel.

What's the reason for having both at the same time? An easier migration period?

[traviscross]

We generally do not make such efforts to migrate usage of nightly features without a specific known good reason to do so. (Let us know of course if there is one, e.g. if this affects RfL in some way.)

[bors]

☔ The latest upstream changes (presumably #142770) made this pull request unmergeable. Please resolve the merge conflicts.

[ojeda]

e.g. if this affects RfL in some way.

We don't use #[no_sanitize] in mainline (nor in -next) at the moment.

[1c3t3a]

I rebased the change to make review easier.

[jieyouxu]

Note that if this is a built-in attribute, then IIUC #[sanitize] (even if unstably gated) could introduce new name resolution ambiguities with user macros or derive macro attributes and thus technically break existing user code. See for instance #143834. cf. #134963.

// tests/ui/whatever_subdir/test.rs

//@ proc-macro: my_derive.rs
//@ edition: 2024

use my_derive::MyDerive;

#[derive(MyDerive)]
#[sanitize]
//~^ ERROR `sanitize` is ambiguous
pub struct Foo;

fn main() {}

// tests/ui/whatever_subdir/auxiliary/my_derive.rs

//@ edition: 2024

extern crate proc_macro;
use proc_macro::{Span, TokenStream};

#[proc_macro_derive(
    MyDerive,
    attributes(
        sanitize
    )
)]
pub fn derive_custom(_item: TokenStream) -> TokenStream {
    TokenStream::new()
}

You can still observe this for #[no_sanitize] too, if you switch sanitize for no_sanitize in the above test case.

error: malformed `no_sanitize` attribute input
  --> /home/joe/repos/rust/tests/ui/bitbuffer_derive/test.rs:7:1
   |
LL | #[no_sanitize]
   | ^^^^^^^^^^^^^^ help: must be of the form: `#[no_sanitize(address, kcfi, memory, thread)]`

error[E0659]: `no_sanitize` is ambiguous
  --> /home/joe/repos/rust/tests/ui/bitbuffer_derive/test.rs:7:3
   |
LL | #[no_sanitize]
   |   ^^^^^^^^^^^ ambiguous name
   |
   = note: ambiguous because of a name conflict with a builtin attribute
   = note: `no_sanitize` could refer to a built-in attribute

error[E0658]: the `#[no_sanitize]` attribute is an experimental feature
  --> /home/joe/repos/rust/tests/ui/bitbuffer_derive/test.rs:7:1
   |
LL | #[no_sanitize]
   | ^^^^^^^^^^^^^^
   |
   = note: see issue #39699 <https://github.com/rust-lang/rust/issues/39699> for more information
   = help: add `#![feature(no_sanitize)]` to the crate attributes to enable
   = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0658, E0659.
For more information about an error, try `rustc --explain E0658`.

Or just

macro_rules! sanitize {
    () => {
        /* .. */
    };
}

pub(crate) use sanitize; // `use` here becomes ambiguous

fn main() {}

I imagine no_sanitize is much less likely to collide with something a user would write (not an argument against this change, just an observation re. technically a breaking change).

I don't know how much of a concern for breakage this is in practice, a crater run may or may not be a good idea. If the breakage is deemed acceptable, then this should get relnotes.

[bors]

☔ The latest upstream changes (presumably #143919) made this pull request unmergeable. Please resolve the merge conflicts.

[traviscross]

@rustbot labels +T-lang +needs-fcp
cc @rust-lang/lang

Given that this is breaking, we should review and FCP. Let's do that crater run, since we'll need it for our review.

@bors2 try

[Zalathar]

Bors hasn't noticed that this was merged.

@bors r- retry

[Zalathar]

Perf run from rollup: #145589 (comment)

Looks like this PR is linked to some regressions in incremental builds of externs and match-stress.

[Zalathar]

I suspect the problem is that (from what I can tell) this PR introduces a walk up the HIR tree to check for sanitizer attributes for every function, even when no attributes are used and no sanitizers are enabled.

(The somewhat similar coverage attribute doesn't have this problem, because the walk only happens when coverage is enabled.)

[zmodem]

Here is some code that's using no_sanitize that no longer compiles after this: https://github.com/webmproject/CrabbyAvif/blob/b44369d0ab22c898e7e7619bacfdb5e04786ecb0/src/lib.rs#L16

Do you have any guidance on how to change the code so that it works both before and after this change?

[cuviper]

@zmodem you will need some kind of feature detection to control cfg/cfg_attr for which version to use, before or after. For example, you could use autocfg's probe_raw in a build script.

[nico]

Any chance you could support both spellings for a while, for a smoother transition path?