Hidden unsafe due to unintentionally abusable macros and include

language design Unsafe Code Guidelines
43

Personally, I think macro hygiene is the wrong tool to handle this. The rules around hygiene can be very subtle, and proc-macros have powerful APIs available to manipulate hygiene. This is all very well and good when dealing with identifier resolution and span backtraces. However, adding support for unsafe hygiene would be introducting an enourmous amount of complexity around the code that's most important to get right.

I really like the idea of #[safe] or safe { }. Since it can only restrict the scope of existing unsafe blocks, incorrectly using #[safe] will just lead to compile-time errors, not unsoundness. With full-blown unsafe hygiene, combining two proc-macros could very easily lead to an incorrect unsafe hygiene being applied.

Also, full unsafe hygiene would mean that in general, determining the full 'scope' of an unsafe block requires examining an arbitrary Rust program (a proc-macro). This seems really undesirable.

1 Like
44

Note that there is a long-standing bug where the compiler loses spans, and consequently hygiene.

4 Likes
45

Would this prevent a macro from performing an unsafe operation an externally-provided ident, even inside an internal unsafe block? That seems undesirable: Authors may wish to use macros to provide a safe abstraction over unsafe operations, much like using unsafe blocks inside a safe function.

2 Likes
46

How about a compiler warning when an expr is expanded in an unsafe block in a declarative macro, and suggest to move the expression out of the unsafe block like here?

#[macro_export]
macro_rules! alloc_pages {
    ($length:expr) => (
        let x = $length;
        unsafe {
            mmap(0 as *mut c_void, x, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0)
        }
    );
}

The warning can't catch every single problematic case, but I believe the vast majority of them would be detected. If the current, unhygienic behaviour is desired, the warning can be silenced. If it is accidental, it's usually easy to fix.

8 Likes
47

This suggestion doesn't work for even moderately complicated cases though:

unsafe {
    if $a {
        mmap(0 as *mut c_void, $length, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0)
    } else {
        mmap(0 as *mut c_void, 42, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0)
    }
}

Or if the evaluation is in a loop. So #[safe] { ... } makes a lot of sense as something that can be suggested as an automated edit, or used in a pinch, even if in most cases the code can be rewritten to not use it.

1 Like
48

I mean, there's always a way to do it with just unsafe{} blocks if you're careful.

if $a {
    match $length { length => {
        unsafe { mmap(0, length, PROT, MAP, -1, 0)
    }}
else {
    unsafe { mmap(0, 42, PROT, MAP, -1, 0)
}

And even loops can be restructured to unsafe{} every expression rather than the whole block, to allow lifting of expr capture evaluation out of unsafe contexts.

But I agree that automatic application of such lifting is difficult, and a #[safe] {} would be much more convenient.

I still think unsafe hygiene would be safer, but I agree that in combination with wider hygiene control with (nested) proc macros this turns something traditionally not a safety issue into a safety issue, so is probably not a tenable approach.

It would prevent a macro from calling an unsafe operation identified by the user without the user writing unsafe. This is a good thing; the macro has no way to know the unsafe preconditions of the function identified by the user-specified identifier.

But this does allow me to construct one use case unsupportable under my definition of unsafe hygiene:

macro foo($ident) {
    unsafe fn $ident() {}
    unsafe { $ident() };
}

Here the macro does know the preconditions, but the identifier is necessary to the functions use and in the caller's hygiene, and the macro should not require unsafe as it is sound under all inputs.

So yeah, unsafe hygiene would both be problematically tricky for proc macros and block usecases that are supported today, so probably a nonstarter. (You could add on e.g. $unsafe:ident to add unsafe hygiene on, but it's starting to get messy, and #[safe]{} isn't and is obvious.)

6 Likes
49

The obvious downside of #[safe]{}, though, is that it isn't, um, safe by default. Macro authors have to remember to wrap all uses of all arguments within an unsafe block in #[safe]{}, except when they do want to extend unsafety to cover the contents of the argument. This is much easier to get wrong, and simultaneously harder to catch in code review, than if we could somehow have it be the other way around.

(This isn't an objection to #[safe]{} itself, I can see that being handy for complicated unsafe blocks with code that doesn't need unsafe mode in the middle.)

9 Likes
50

One thing that could help here is a macro attribute to automatically shim #[safe] blocks around block and expr arguments at macro-expansion time. At an edition boundary, we could switch this behavior from opt-in to opt-out.

It isn’t perfect, and doesn’t help procedural macros at all, but would probably catch the majority of accidental unsafe leakage.

51

A directive applied at top level to the whole of source file that uses macros?

52

My thought was a directive applied to the macro_rules statement that defines the macro.

53

But wouldn't it be the macro

  • user who would be impacted
  • callsite where the extra unsafe may need to be added?

I find your suggestion appealing but it seems it's the macro user who'd need to opt-in

I wish there was a comparable solution for proc macros..

54

The shim happens at the boundary between the two authors’ code, so it could plausibly be considered as belonging to either side. To my mind, it’s the macro author that has the most relevant information: Does the macro intend for its internal unsafe context to be extended to its arguments or not?

But I don’t have the time to push this through the RFC process right now, and I wouldn’t be upset if someone picked this up and decided to go the other way with it.

55

One could argue that automatic safe-wrapping of args during macro_rules expansion

  • doesn't require any change from macro author
  • does require macro user to try compiling and
  • to add explicit unsafe macro arg wrapping at those macro call-sites where compilation breaks

If you look at it this way it does seem that it's the macro user who needs to opt-in until an edition makes this the default compilation mode.

This still looks like only 1/2 of the solution though without a plan for proc macros doesn't it?

56

I'm surprised that no one has mentioned yet that rustc already supports #[safe] { ... } blocks internally, via BlockSafety::PopUnsafe. Unfortunately I believe this is dead code ATM, as it was introduced for feature(pushpop_unsafe), that has since been removed. See for example pushpop-unsafe-rejects.rs, which uses macros push_unsafe! and pop_unsafe! as one might expect unsafe { ... } and #[safe] { ... }. (I heard about this from Niko here.)

8 Likes
57

Unpopular opinion: use of #![forbid(unsafe)] should generate a warning saying "This is not a security feature and must not be relied on!"

There's so many different ways to hide backdoors that forbidding unsafe is just pointless. Abuse of macros, build scripts, /proc/self/mem, calls into C library, less-known Rust features/gotchas... I could probably spend months inventing new ways of abuse.

58

I agree that #![forbid(unsafe)] is not a security feature, but I don't think it is pointless. It tells contributors, "please don't use unsafe code". They can remove the attribute or abuse macros such as plutonium::safe, but obviously I won't accept PRs with changes like that.

59

Maybe something to put into CI instead of into the code?

60

I'd almost say !#[deny(unsafe)] is better at the task, to be honest. (And yes, I know cargo-geiger doesn't like it.) Specifically, because there's the option of #[allow(unsafe)], people who want unsafe can use that (and the code review can say "this isn't warranted") rather than encouraging some workaround.

The same way #![forbid(warnings)] is a horrible idea, you can make the same argument for #![forbid(unsafe)]. Having a warn-by-default lint explaining how/why #![forbid(unsafe)] isn't "perfect" seems reasonable.

2 Likes