lint: transmuting known null pointer to ref

[llogiq]

mem::transmute(std::ptr::null()) – this is undefined behavior.

[felix91gr]

I think I can try this one while Oli's busy (I'm atm also working on #3803 under his mentorship). Would you be willing to mentor me here? :)

[flip1995]

This is practically just two matches on methods. First you have to find calls to transmute and then you have to match the first argument of the call, whether it's a call to null. That should get you started. There also may be other methods to create a null pointer like casting 0 as *const _.

If you got any questions, just ask or open a WIP PR. I'm happy to help you from there.

[felix91gr]

Okay I'm now making a first UI test for this. It should show all the cases that we're looking for in this lint.

Which cases are missing?

fn transmute_0() {
    let zero : *const i32 = 0 as *const_;
    let zero_ts : &i32 = std::mem::transmute(zero);
}

fn transmute_null() {
    let nil : *const i32 = std::ptr::null();
    let nil_ts : &i32 = std::mem::transmute(nil);
}

fn main() {
    transmute_0();
    transmute_null();
}

[flip1995]

Maybe you can also add:

pub const ZPTR: *const usize = 0 as *const _;
...
... std::mem::transmute(ZPTR); ...

... std::mem::transmute(0 as *const _); ...

Maybe also some different types, but that shouldn't be an issue on getting this lint working.

[felix91gr]

Thank you!

This should use a LateLintPass, right? Since this is more than just a syntactic analysis.

[felix91gr]

Working on this at #3848

[tesuji]

Could we close this now?

[felix91gr]

Maybe? I'm not sure, @lzutao. The thing is, my PR implemented this but only the trivial bits. There are many cases where one would see that a null pointer was transmuted, but we can't lint those yet because miri isn't yet as complete as we needed.

The idea was that at some point we could do simple constant propagation over a stretch of code using miri, and then check if transmute was being called with any propagated null pointers. This is very hard and not necessary to do by hand, but doing it with miri would be trivial. That's why we chose to wait on it before completing this feature :3

[felix91gr]

Is there anyone working on this at the moment? If I understand correctly, the constant propagation machinery should now be able to propagate the null pointers. Am I right, @oli-obk?

If so, and if no one is working on this, then I'd like to get back on this so we can close this issue :D

[flip1995]

Nope no one is working on this right now.

[felix91gr]

Awesome. If Oliver confirms to me that this can be completed now, I'll tackle it :)

[oli-obk]

Unfortunately not yet, but I don't know why. If you click on the three dots in the top left corner and select MIR https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=907e81a3947910a850f01271a054209e you'll see

bb0: {
        StorageLive(_1);                 // bb0[0]: scope 1 at src/main.rs:3:13: 3:14
        _1 = const 0usize;               // bb0[1]: scope 1 at src/main.rs:3:17: 3:18
                                         // ty::Const
                                         // + ty: usize
                                         // + val: Value(Scalar(0x0000000000000000))
                                         // mir::Constant
                                         // + span: src/main.rs:3:17: 3:18
                                         // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000000)) }
        StorageLive(_2);                 // bb0[2]: scope 2 at src/main.rs:4:9: 4:46
        StorageLive(_3);                 // bb0[3]: scope 2 at src/main.rs:4:44: 4:45
        _3 = _1;                         // bb0[4]: scope 2 at src/main.rs:4:44: 4:45
        _2 = const std::intrinsics::transmute::<usize, &i32>(move _3) -> bb1; // bb0[5]: scope 2 at src/main.rs:4:9: 4:46
                                         // ty::Const
                                         // + ty: unsafe extern "rust-intrinsic" fn(usize) -> &i32 {std::intrinsics::transmute::<usize, &i32>}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: src/main.rs:4:9: 4:43
                                         // + user_ty: UserType(0)
                                         // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(usize) -> &i32 {std::intrinsics::transmute::<usize, &i32>}, val: Value(Scalar(<ZST>)) }
    }

which (ignorign a lot of the comments and StorageLive identifiers, doesn't really have any reason why the `0usize isn't propagated into the function call

_2 = const std::intrinsics::transmute::<usize, &i32>(move _3) -> bb1;

I'd expect the above to be

_2 = const std::intrinsics::transmute::<usize, &i32>(0usize) -> bb1;

which would then be easy to detect. This needs changes in rustc, but if you're interested, I can mentor you for that. I believe it's not very hard to do, but you'll have to learn quite a bit about MIR.

[felix91gr]

Excellent. I accept your mentorship, @oli-obk 🌈

Let's do this.

Also: hey, this will serve as a future test for const propagation :D

[RalfJung]

FWIW, this seems very similar to the existing "invalid_value" lint in rustc (not sure if Clippy also has such a lint), which already detects transmute(0usize) to a reference.

[felix91gr]

Oh, that's cool! I think this is more general maybe? Although with dataflow-aware constprop, the invalid_value lint might become equivalent to this, depending on how it's evaluated :3

[RalfJung]

invalid_value is a HIR lint so I doubt constprop will change anything there. ;)

However, is_zero could be extended to recognize ptr::null.