Add allow-by-default lints for all attributes with unsafe behaviors

[tarcieri]

In #72188, an allow-by-default lint was added against #[no_mangle], which previously permitted unsafety even in the event that #![forbid(unsafe_code)] was used.

I think it'd be a good idea to collect a list of all such "unsafe attributes" and try to add similar lints for them on a case-by-case basis. This shouldn't be terribly difficult to do by extracting them from the Built-in attributes index from the Rust Reference.

I've started a list below. Please leave comments and I'll update it (or anyone else who has the power to do so, feel free). Items in the list with strikethrough are deemed out-of-scope.

ABI, linking, symbols, and FFI

• link
• link_ordinal
• link_args (see Tracking issue for link_args stabilization #29596)
• link_name (see this comment)
• repr (see this comment)
  • repr(packed)
• export_name (Add an allow-by-default lint against #[no_mangle] #72188, Add checking for no_mangle to unsafe_code lint #72209)
• link_section (Report unsafe for overriding link sections #97086)
• no_mangle (Add an allow-by-default lint against #[no_mangle] #72188, Add checking for no_mangle to unsafe_code lint #72209)
• used (see this comment)

Others?

Please leave comments with suggestions or corrections.

[jyn514]

Tangentially related: macros that "hide" the unsafe keyword behind a macro call: https://rustsec.org/advisories/RUSTSEC-2020-0011.html

[jyn514]

I think all of these should be part of unsafe_code, the same way that no_mangle is. I don't think they differ significantly from no_mangle.

[Nemo157]

#[export_name] was also rolled into the same #[no_mangle] PR, since it's basically the same thing: #72209.

Given that #[link_name] can only be applied to an extern { fn } it seems that it would already be covered by the unsafety inherent in extern { fn }.

Can repr cause UB on its own? The direct UB from it I know of is repr(packed) related, but I think that's all checked at the use sites?

[nagisa]

repr(packed) unsafety already has a forward-compat lint for it.

[joshtriplett]

I don't see any obvious way that used could do anything unsafe.

Likewise repr (other than repr(packed) which is already covered).

#[link] can pull in a library, but I don't think it has any more inherent danger than extern, or a build.rs script.

I'd expect many uses of link_section to be innocuous, but it's probably possible to cause UB with it, so sure, let's add it to unsafe_code (after a crater run).

[nagisa]

#[link] can pull in a library, but I don't think it has any more inherent danger than extern, or a build.rs script.

extern by itself does not link any libraries at all, so its fairly orthogonal and does not introduce any soundness concerns as far as I'm aware.

Linking libraries (even if not otherwise used) can cause implicit invocation of FFI calls through the constructors/destructors that library defines. Those can cause similar problems as calling any other FFI function, though there's nothing you can do about them either, other than not linking to the library. There was a recent issue in libloading related to this.

[tarcieri]

I'd expect many uses of link_section to be innocuous, but it's probably possible to cause UB with it, so sure, let's add it to unsafe_code (after a crater run).

link_section unsafety example I've seen:

use core::sync::atomic::{AtomicUsize, Ordering};

#[link_section = ".rodata"]
static BAD: AtomicUsize = AtomicUsize::new(0);

fn main() {
    BAD.store(42, Ordering::SeqCst);
}

[jyn514]

#[link_section = ".rodata"]
static BAD: AtomicUsize = AtomicUsize::new(0);

That uses interior mutability - could we add a more targeted lint that warns against putting anything with interior mutability (or any static mut) in .rodata?

[jonas-schievink]

#[link_section = ".init_array"]
static BAD: usize = 0;

fn main() {}

[Lokathor]

yeah it's easier to just forbid link_section than try to carve out exactly how it might be misused. You're never required to forbid unsafe_code anyway, so we can be overly conservative about the lint.

[chorman0773]

#[link_section = ".rodata"]
static BAD: AtomicUsize = AtomicUsize::new(0);

That uses interior mutability - could we add a more targeted lint that warns against putting anything with interior mutability (or any static mut) in .rodata?

You can also use link_section to put functions into .rodata, or .data, or any section other than a .text section

[Lokathor]

that's only usually UB

[chorman0773]

I mean, I'd presume it must be at least conditionally-supported. Otherwise, it wouldn't be allowed to segfault on properly secured systems that don't allow executing .data.

[Lokathor]

Right. Some embedded systems allow it, but you can generally assume that a "modern" system with an OS and all that won't let you play fast and loose with the program counter.

[tarcieri]

It seems like we have rough consensus that link, link_args, and link_section should get these lints.

I suppose the next step is to open an individual issue for each of them? (and at that point, perhaps this issue can be closed out unless anyone knows of any others)

[joshtriplett]

Given that all three of those are link-related, I think it'd be reasonable to use one issue for all three.

The next step would likely be a PR adding them to unsafe_code, and then a crater run for that PR, to gauge impact.

[workingjubilee]

#[link_args] appears to have vanished in a puff of logic: #29596

[5225225]

Created #97086 for link_section

[RalfJung]

Complementing the efforts tracked here, I think we should move towards the more principled solution of having a concept of 'unsafe attributes'.