Rust: `unsafe` block propagates to closures

[ChayimFriedman2]

I tried this code:

fn main() {
    let a = 1;
    let p: *const i32 = &a;
    unsafe {
        let closure = || { unsafe { *p } };
        println!("*p={}", *p);
        println!("closure()={}", closure());
    }
}

Playground

I expected to see this happen:
Compile without warnings: the two unsafe blocks are needed since the closure can be called somewhere else.

Instead, this happened:

warning: unnecessary `unsafe` block
 --> src/main.rs:5:28
  |
4 |     unsafe {
  |     ------ because it's nested under this `unsafe` block
5 |         let closure = || { unsafe { *p } };
  |                            ^^^^^^ unnecessary `unsafe` block
  |
  = note: `#[warn(unused_unsafe)]` on by default

Meta

Tested on the playground using all channels.

[jyn514]

the two unsafe blocks are needed since the closure can be called somewhere else.

Wow, yeah that is really surprising behavior. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f6c163cffefe319bcfb74565d09f9f91

[Patryk27]

Even though the closure can be called somewhere else, you're constructing it inside an unsafe block, so it's up to you to ensure the closure cannot be abused in creative ways IMO.

[ChayimFriedman2]

But I cannot promise that it'll not violate safety when called, simply because I don't know where it'll be called. In the example above, I can't promise that when the closure is called the dereference is valid.

Closures have two stages, like normal functions: construction and invocation. The difference is that for normal functions the constructing stage occurs at compile time. But anyway, the unsafe is a promise for invocation time, not for construction time. It's not unsafe to construct an unsafe closure; it's unsafe just to invoke it.

[Patryk27]

But I cannot promise that it'll not violate safety when called, simply because I don't know where it'll be called.

This problem is not unique to closures - the same thing can be said about any other kind of value returned from inside the unsafe block; say:

fn read_string<'a>(str: *const str) -> &'a str {
    unsafe { &*str }
}

This functions returns &str, but it does not _guarantee_ you cannot abuse it to get user-after-free, simply because read_string() doesn't know where you'll use that reference:

fn main()  {
    let str = read_string(String::from("asdf").as_str());
    println!("{}", str);
}

So my question is: what makes closures _more special_ than other kind of values returned from inside the unsafe blocks?

[ChayimFriedman2]

Because as I said, in the unsafe block you just construct the closure. On the other hand, in your example, in the unsafe block you actually execute the potentially harmful code.

[ssomers]

I had the same disappointment encountering this. But I don't understand your argument at all:

But I cannot promise that it'll not violate safety when called, simply because I don't know where it'll be called. I can't promise that when the closure is called the dereference is valid.

Yet that's what you're asking to do: to explicitly make a promise through an unsafe block in the closure's body?

Closures have two stages, like normal functions: construction and invocation.

For the point you're making, they have (at least) three stages: construction (AKA compilation), instantiation, and invocation. The unsafe keyword is about the first stage, even though the promise made is about what will occur at invocation time.

That aside, I do agree it's counter-intuitive that unsafety propagates to closures, nested code on a different level, i.e., not just to a nested block:

    unsafe {
        if some_unsafe_function() {
            println!("*p={}", *p);
       }
    }

In particular because the code I work in is under #![deny(unsafe_op_in_unsafe_fn)], which means the body of an unsafe function needs an explicit unsafe block to do unsafe things. But that's not a strong argument either.

Let's compare with local functions: why does the body of the function y here need an unsafe block, while it is in one already?

unsafe fn x() {}

unsafe fn z() {
    fn y() {
        unsafe { x() }
    }
    y()
}

fn main() {
    unsafe { z() }
}

playground

[jyn514]

why does the body of the function y here need an unsafe block, while it is in one already?

Because functions aren't affected by the type context of their environment, unlike closures. You couldn't e.g. use a generic parameter from the outer function in the inner one: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=10d9cff0f7d495b5210027b9d47b8a81

[ChayimFriedman2]

For the point you're making, they have (at least) three stages: construction (AKA compilation), instantiation, and invocation. The > unsafe keyword is about the first stage, even though the promise made is about what will occur at invocation time.

If you want to split this into 3 stages, then I'm talking about instantiation. For normal functions, the first steps occurs at compile time. For closure, the second happens at runtime.

[Patryk27]

in your example, in the unsafe block you actually execute the potentially harmful code

I disagree; my example's read_string() doesn't execute harmful code on itself - the harm _might_ (but does not have to) happen after the function has completed executing.

In other words:

fn everything_alright()  {
    let str = String::from("asdf");
    read_string(str.as_str());
}

fn dragons_ahead() {
    let str = read_string(String::from("asdf").as_str());
    println!("{}", str);
}

As you can see, the issue happens outside the unsafe block - and it's the same with your example: the harm happens _outside_ the unsafe block, because of insufficient "due diligence" around working on values returned from unsafe blocks.

What I'm trying to say is that closures aren't special - any value returned from unsafe block must be scrutinized by the programmer: be it a reference (as in my example), a struct or a closure (as in your example), so I don't quite get why you'd like to make special case just for closures.

I don't want to diminish your idea / bug report, of course - I'm just curious :-)

[ChayimFriedman2]

OK. I'll try to be more precise and explanatory.

When you write something like:

fn read_string<'a>(str: *const str) -> &'a str {
    unsafe { &*str }
}

You say to the compiler: "Dear compiler, I know you cannot verify this operation is safe. Trust me, it is." (note that this function is semantically incorrect because the compiler is right: it can't be 100% sure that the operator will be unsafe).

So, when you do:

let closure = || *p;

This operation is very safe. Nothing bad can happen just because I created a closure.

But things really bad can happen if I invoke it. Whether I return it or not, that doesn't matter: in read_string() above, the unsafe operation is the dereference. So you need to mark it unsafe. With the closure, the unsafe operation is again, the deref: you need to mark _it_ as unsafe.

Now that I think about that, the fundamental problem is that closures cannot be marked unsafe (see https://stackoverflow.com/questions/27746662/can-i-create-an-unsafe-closure. I think it would be good to change this, but as mentioned in the SO answer, the problem is caused by the Fn traits, so I don't see a way to bypass it). Rust safety rules requires that if a function cannot guarantee that it will always be safe, it should be marked unsafe. Closures that operates unsafely on mutable upvalues (captures variables) can _never_ guarantee that, since they don't control the variable value.

[jyn514]

@ChayimFriedman2 I agree with a lot of the things you're saying here, but you're suggesting changes to the language itself; this is not an implementation issue. If you think this is a good change for Rust, consider opening an RFC at https://github.com/rust-lang/rfcs/ or going through an internals thread at https://internals.rust-lang.org/. Right now I don't think this is actionable.

[ChayimFriedman2]

I don't think this is actionable, too, and this is the reason I'm not opening an RFC 😞

[jyn514]

Should the issue be closed, then?

[ChayimFriedman2]

I don't know. Do you think we should at least not propagate unsafe blocks into closures, as sort of minimal effort?

[jyn514]

That's a language change, not a lint, and so all the stuff I said about an RFC still applies. Rust has a strong backwards-compatibility guarantee, so at a minimum this would need a new edition.

[ChayimFriedman2]

OK. I'm closing this now.

[ssomers]

so at a minimum this would need a new edition.

Well that's not quite true? unsafe_op_in_unsafe_fn is an attribute that opts in to a change in rules.

But I now strongly doubt it's worth any effort. In the original example above, the workaround is obviously to move the let closure = line up. In code like this:

    unsafe { some_unsafe_fn_taking_fn_argument(|| *p) }

the way to avoid accidental unsafe calls is:

    let closure = unsafe { || *p};
    unsafe { unsafe_fn_taking_fn_argument(closure) }

I can't come up with code where you can't move the closure out of the unsafe-block, perhaps chopping up the block. For instance, this:

    let p: *const i32 = &a;
    unsafe {
        let q = unsafe_fn_taking_fn_argument(|| *p);
        unsafe_fn_taking_fn_argument(|| *p + q);
    }

becomes:

    let p: *const i32 = &a;
    let closure = || unsafe { *p };
    let q = unsafe { unsafe_fn_taking_fn_argument(closure) };
    let closure = unsafe { || *p + q }; // entire closure in block, works too
    unsafe { unsafe_fn_taking_fn_argument(closure) };

playground