Warn on references casting to bigger memory layout

[Urgau]

This PR extends the invalid_reference_casting lint (deny-by-default) which currently lint on &T -> &mut T casting to also lint on &(mut) A -> &(mut) B where size_of::<B>() > size_of::<A>() (bigger memory layout requirement).

The goal is to detect such cases:

let u8_ref: &u8 = &0u8;
let u64_ref: &u64 = unsafe { &*(u8_ref as *const u8 as *const u64) };
//~^ ERROR casting references to a bigger memory layout is undefined behavior

let mat3 = Mat3 { a: Vec3(0i32, 0, 0), b: Vec3(0, 0, 0), c: Vec3(0, 0, 0) };
let mat3 = unsafe { &*(&mat3 as *const _ as *const [[i64; 3]; 3]) };
//~^ ERROR casting references to a bigger memory layout is undefined behavior

This is added to help people who write unsafe code, especially when people have matrix struct that they cast to simple array of arrays.

EDIT: One caveat, due to the &Header uncertainty the lint only fires when it can find the underline allocation.

I have manually tested all the new expressions that warn against Miri, and they all report immediate UB.

r? @est31

[Noratrieb]

Whether this is actually always UB is subject to debate, also called the "&Header issue".
rust-lang/unsafe-code-guidelines#256. SB forbids it, TB allows it, but from what I've gathered, consensus seems to be that it should be allowed, probably.
It is or course UB to cast a &u8 to a &u32 if the backing allocation is only 1 byte, but this lint cannot know how big the backing allocation is. Given that it's a deny by default lint, we probably want to be careful with false positives.
I'm not sure what to do though, just commenting that this pattern may not always be UB.

[Urgau]

Hum, this is surprising. This may not be as obviously UB (or not) as I though and expected.

If this happens to not be (for now) undecided, I can probably change the implementation to find the backing allocation, if possible of course.

cc @RalfJung, to have an opsem expert opinion on this.

[Urgau]

@Nilstrieb rust-lang/unsafe-code-guidelines#256 seems to be about the size component (read layout size) only? What about the alignment? Casting to reference to reference that requires more alignment, is this UB?

[Noratrieb]

Creating an unaligned reference is always UB, but you only know at runtime whether the source reference will be aligned or not.

[RalfJung]

@Nilstrieb is right. Such a cast is certainly highly suspect, but may or may not always be UB. Tree Borrows allows such casts under certain conditions.

[Urgau]

@RalfJung I suspect that none of those cases can be trivial determined? And that in it's current form this PR cannot make progress until we have an approved/official aliasing model?

[Noratrieb]

These cases can indeed not be determined trivially, knowing whether the allocation is bigger than the referent is very non-local.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[Urgau]

I've updated the implementation to only lint when we can find the allocation, that means that it only works locally. The implementation tries to follow the reference at the start of the casting chain to find those patterns, let a = 12; or let a = foo();.

This should resolve the concern from @Nilstrieb and @RalfJung.

@est31 this is ready for review

[RalfJung]

Is rustc the right place for such a specific lint or should this go to clippy?

[asquared31415]

Tests that make sure the lint is conservative with regards to certain patterns that may be accepted in the future would be nice to have.

[Urgau]

Is rustc the right place for such a specific lint or should this go to clippy?

I think so. We should warn/deny code that is UB as soon as possible, not wait for people to write their code, clean it up and then run clippy to find out that it is actually not OK.

(Not to mention that many people don't use clippy).

EDIT: Note that this PR does not add any new lint, it just extends the invalid_reference_casting lint that already lints on &T to &mut T casting.

Tests that make sure the lint is conservative with regards to certain patterns that may be accepted in the future would be nice to have.

Do you have such pattern in mind? I've already restricted the scope to exclude the &Header issue (rust-lang/unsafe-code-guidelines#256).

[est31]

In general, I think there is a place for lints that find instances of code that is obvious true undefined behaviour. The emphasis here lies on obvious: there have to be zero false positives. That's a much higher bar than what many clippy lints need to pass.

If the compiler tells the user that some pattern is UB, then the user will believe that much more likely than if clippy said it.

That doesn't mean that this particular PR is automatically a good idea. There need to be thorough tests to ensure that there is no false positives. Also, there is many ways that code can be UB, and we cannot write and maintain heuristics for all possible ways: we need to pick the instances which we think are easiest to maintain and that catch the most UB "in the wild".

[oli-obk]

@bors r+

[bors]

📌 Commit e08c9d1 has been approved by oli-obk

It is now in the queue for this repository.

[saethlin]

Why was this FCP approved? The wording in the lint as reported in #124685 is misleading at best. @traviscross are there any notes from the meeting that I can review? I can't really figure out what lang approved or discussed for this change.

[Urgau]

@saethlin It was discussed on 2024-01-03: https://hackmd.io/@rust-lang-team/Hk4z917da#Warn-on-references-casting-to-bigger-memory-layout-rust118983