Skip to content

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

1c3t3a
Copy link
Member

@1c3t3a 1c3t3a commented Jun 18, 2025

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 rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-rustc-dev-guide Area: rustc-dev-guide PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2025

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
Copy link
Member

rcvalle commented Jun 18, 2025

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

@rcvalle
Copy link
Member

rcvalle commented Jun 18, 2025

@rust-log-analyzer

This comment has been minimized.

@1c3t3a 1c3t3a force-pushed the sanitize-off-on branch from 5f8e6ee to 0302657 Compare June 18, 2025 16:03
@rust-log-analyzer

This comment has been minimized.

@1c3t3a 1c3t3a force-pushed the sanitize-off-on branch from 0302657 to a2216db Compare June 18, 2025 17:07
@Darksonn
Copy link
Contributor

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

@1c3t3a
Copy link
Member Author

1c3t3a commented Jun 18, 2025

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
Copy link
Contributor

traviscross commented Jun 19, 2025

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
Copy link
Collaborator

bors commented Jun 20, 2025

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

@ojeda
Copy link
Contributor

ojeda commented Jul 1, 2025

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

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

@1c3t3a
Copy link
Member Author

1c3t3a commented Jul 14, 2025

I rebased the change to make review easier.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

bors commented Jul 14, 2025

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

1c3t3a added 2 commits July 14, 2025 16:37
This change implements the #[sanitize(..)] attribute, which opts to
replace the currently unstable #[no_sanitize]. Essentially the new
attribute works similar as #[no_sanitize], just with more flexible
options regarding where it is applied. E.g. it is possible to turn
a certain sanitizer either on or off:
`#[sanitize(address = "on|off")]`

This attribute now also applies to more places, e.g. it is possible
to turn off a sanitizer for an entire module or impl block:
```rust
\#[sanitize(address = "off")]
mod foo {
    fn unsanitized(..) {}

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

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

This attribute is enabled behind the unstable `sanitize` feature.
This removes the #[no_sanitize] attribute, which was behind an unstable
feature named no_sanitize. Instead, we introduce the sanitize attribute
which is more powerful and allows to be extended in the future (instead
of just focusing on turning sanitizers off).
@1c3t3a 1c3t3a force-pushed the sanitize-off-on branch from bb98641 to a466546 Compare July 14, 2025 16:55
@rust-log-analyzer

This comment has been minimized.

@traviscross
Copy link
Contributor

@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

@rustbot rustbot added needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. T-lang Relevant to the language team labels Jul 14, 2025
@rust-bors
Copy link

rust-bors bot commented Jul 14, 2025

⌛ Trying commit a466546 with merge 4949cf2

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jul 14, 2025
Remove the `#[no_sanitize]` attribute in favor of `#[sanitize(xyz = "on|off")]`

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](https://rust-lang.zulipchat.com/#narrow/channel/343119-project-exploit-mitigations/topic/Stabilize.20the.20.60no_sanitize.60.20attribute/with/495377292)).

The new attribute also works on modules, traits and impl items and thus enables usage as the following:
```rust
#[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`
@traviscross
Copy link
Contributor

Since this produces a user-visible effect on stable Rust, it feels like we should be documenting this somehow in the Reference. We'll talk about how we might want to do that on the lang-docs side.

cc @ehuss

@1c3t3a
Copy link
Member Author

1c3t3a commented Jul 14, 2025

Given that this is breaking, we should review and FCP

Yes that sounds good. The initial idea of @rcvalle and me was to stabilize this together with the sanitizers in #123617. Does that make sense?

technically break existing user code.

Thanks a lot for the great summary @jieyouxu! I did a quick GH codesearch and found e.g. https://crates.io/crates/sanitizer which defines a sanitizer macro. How is the usual way of reacting to such name clashes?

…ress

To do it the same as how clang disables address sanitizer, we now
disable ASAN on sanitize(kernel_address = "off") and KASAN on
sanitize(address = "off").

The same was added to clang in https://reviews.llvm.org/D44981.
@1c3t3a 1c3t3a force-pushed the sanitize-off-on branch from a466546 to 9ee7903 Compare July 14, 2025 19:49
@rust-bors
Copy link

rust-bors bot commented Jul 14, 2025

☀️ Try build successful (CI)
Build commit: 4949cf2 (4949cf2f817beadb9a248d423b6d55d6d2398ffe, parent: cccf075eba88363269e8589ebb8d40874cc542d8)

@traviscross
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-142681 created and queued.
🤖 Automatically detected try build 4949cf2
⚠️ Try build based on commit a466546, but latest commit is 9ee7903. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-rustc-dev-guide Area: rustc-dev-guide needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-crater Status: Waiting on a crater run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants