Rust: `#[allow_internal_unsafe]` evades `#![forbid(unsafe_code)]`

[WildCryptoFox]

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b137113f78a25cec34a258505cf41e3f

I'd like the ability to forbid the use of unsafe_code over all dependencies with at least a per-crate whitelist. For a quick PoC I've wrapped rustc to add --cap-lints=forbid before cargo's arguments and use RUSTFLAGS=-Funsafe-code to forbid unsafe code. No per-crate (or better) whitelist there.

While trying to write some async code I noticed that await!() wasn't in core #56767, so I checked the sources to find it is in std but uses this scary attribute.

I agree #[allow_internal_unsafe] should allow crates that forbid unsafe code to call the macro, but a crate with unsafe code forbidden shouldn't be able to use this attribute on its macros.

[Nemo157]

#[allow_internal_unsafe] is not intended to be used outside the standard library, as noted on its page in the unstable book it has no tracking issue so will never be stabilized.

[WildCryptoFox]

@Nemo157 My concern is intended or not, a crate can use it, and that evades the global forbid unsafe-code method I'd like to use. Aside form this, the other issues are generated code and build.rs.

I'm requesting for #[allow_internal_unsafe] to be treated as any other unsafe { .. } with respect to the unsafe_code lint.

[bjorn3]

My concern is intended or not, a crate can use it [...]

Not on the stable or beta channel. (you need #![feature(allow_internal_unsafe)])

Calling a function containing unsafe code is not linted by unsafe_code, so preventing macros from containing unsafe code doesn't change anything.

fn contains_unsafe_code() {
    unsafe { *(0 as *mut u8) = 42; }
}

#[forbid(unsafe_code)]
pub mod safe {
    pub fn foo() {
        crate::contains_unsafe_code();
    }
}

is just like

#![feature(allow_internal_unsafe)]

#[allow_internal_unsafe]
macro_rules! contains_unsafe_code {
    () => {
        unsafe { *(0 as *mut u8) = 42; }
    }
}

pub mod safe {
    pub fn foo() {
        contains_unsafe_code!();
    }
}

Also the purpose of #[allow_internal_unsafe] is to allow macros containg unsafe code with the unsafe_code lint denied or forbidden:

error[E0658]: allow_internal_unsafe side-steps the unsafe_code lint
 --> src/lib.rs:1:1
  |
1 | #[allow_internal_unsafe]
  | ^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: add #![feature(allow_internal_unsafe)] to the crate attributes to enable

error: aborting due to previous error

[WildCryptoFox]

@bjorn3 I do recognise the hard requirement on nightly, but many crates do target nightly features (optionally with feature gates).

As for your analogy to functions, I already said this:

I agree #[allow_internal_unsafe] should allow crates that forbid unsafe code to call the macro, but a crate with unsafe code forbidden shouldn't be able to use this attribute on its macros.

To clarify, a crate that uses the #[allow_internal_unsafe] must already allow unsafe code to use it. A crate that calls the macro (not defining it) should continue to inherit the unsafe code despite the activated lint.

I'm requesting for this constraint to be added:

• #[allow_internal_unsafe] can only be used on a macro defined in a crate where unsafe_code is allowed.

[WildCryptoFox]

Maybe the following will be a better example.

A crate can use this unstable feature to write unsafe code despite forbidding unsafe code.

#![forbid(unsafe_code)]
#![feature(allow_internal_unsafe)]

#[allow_internal_unsafe]
macro_rules! evil {
    ($e:expr) => {
        unsafe {
            $e
        }
    }
}

fn main() {
    println!("Hello, world! {}", evil!(*(0 as *const u8)));
}

Another option would be to consider #![feature(allow_internal_unsafe)] as unsafe itself, not just unstable; such that the unsafe_code lint would catch the attempt to use the unsafe feature.

Edit: This would be weaker as the crate may allow unsafe code to enable the feature while a module forbids unsafe code - but would still be able to use the attribute; so I'd rather to be safe to make #[allow_internal_unsafe] unsafe, not just the feature.

[ExpHP]

I do not understand the cause for concern. Look at the first two lines of that snippet:

#![forbid(unsafe_code)]
#![feature(allow_internal_unsafe)]

I struggle to picture any form of scenario in which these two clearly contradictory attributes could get added to a crate, go unnoticed, and cause trouble. The feature attribute must be at the root of the crate. The worst you could do is to put the forbid in a submodule so that they're in different files; but I'm not sure who you'd be trying to fool, or how!

A crate willing to rely on internal details of the compiler probably has a short shelf life, anyways.

[Centril]

@ExpHP

A crate willing to rely on internal details of the compiler probably has a short shelf life, anyways.

Ehhehehe... well; https://rocket.rs/ had relied on internal details of the compiler for quite some time ;)

[WildCryptoFox]

@ExpHP I'd like a tool that adds the --cap-lints=forbid -Funsafe-code at build time to forbid unsafe code on a per-crate basis. The crate may not opt for #![forbid(unsafe_code)], I am.

Even if not a crate-wide forbid, it should still be treated exactly as how the #[allow(unsafe_code)] is treated in the defining crate, it is just for macros. If forbidden, you can't allow it. If denied, you can still allow it. I do not see any reason for even an internal feature to evade this rule.

The alternative for me is to scan through the source code and look for these things myself, or add my own linter ala clippy; but IMHO this lint should be as official as the internal feature - IOW it isn't official, it is internal, and the compiler should be responsible.

[oli-obk]

Steps for implementing this:

1. implement the check_attribute for https://github.com/rust-lang/rust/blob/286dc37d1bd30ecd419e889c7f3888575deac5fc/src/librustc_lint/builtin.rs#L239
2. use attr.check_name("allow_internal_unsafe") to detect the attribute
3. report the lint on attr.span
4. add some tests

[WildCryptoFox]

@oli-obk Thanks. I don't currently have (nor want) a rustc-etc build environment. I don't have a priority for this, but if it isn't resolved when I build the per-crate unsafe whitelist tool, then I'll reconsider the resource costs of a rustc build env.

[JohnTitor]

I wanna work on this, okay?

[oli-obk]

Great! It's all yours.

[JohnTitor]

@oli-obk Thank you! I'm a newbie and have two question.
Here's what I came up with.

fn check_attribute(&mut self, cx: &LateContext, attr: hoge) {
    if attr.check_name("allow_internal_unsafe") {
        self.report_unsafe(cx, attr.span, "description");
    }
}

1. What should add after attr:?
2. What is the best description?

Can you give me advice please?

[oli-obk]

For 1. you can look at the LateLintPass trait's check_attribute method. When implementing a trait you need to replicate all types.

For 2. I don't know, I haven't looked at the other calls to report_unsafe. I'd presume it's something like

"`allow_internal_unsafe` allows defining macros using unsafe without triggering the `unsafe_code` lint at their call site"

[JohnTitor]

@oli-obk Thanks for answering! Sorry to bother you again but what do you think this?

fn check_attribute(&mut self, cx: &LateContext, attr: &ast::Attribute) {
    if attr.check_name("allow_internal_unsafe") {
        if let Some(items) = attr.meta_item_list() {
            for item in items {
                self.report_unsafe(cx, item.span, "description");
            }
        }
    }
}

[oli-obk]

Why are you iterating over the items? Is reporting on attr.span causing weird diagnostics? Do you have an example that shows the new diagnostic output?

[JohnTitor]

Oops, I made a mistake, sorry. Is this good?

fn check_attribute(&mut self, cx: &LateContext, attr: &ast::Attribute) {
    if attr.check_name("allow_internal_unsafe") {
        self.report_unsafe(cx, attr.span, "description");
    }
}

[oli-obk]

Probably ;) Don't hesitate to open a PR, then I can comment on the code directly.

You can run all the tests and see if any need ajusting (./x.py test --stage 1 src/test/ui --bless should tell you if anything needs changing).

[JohnTitor]

@oli-obk OK! Could I ask you to review?

[oli-obk]

Jup