search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
95.32k stars 12.28k forks source link

Tracking issue for invalid_reference_casting #124951

Open saethlin opened 2 months ago

saethlin commented 2 months ago

This is a tracking issue for the lint invalid_reference_casting, which was uplifted from Clippy's cast_ref_to_mut lint in https://github.com/rust-lang/rust/pull/111567.

This lint is deny-by-default, i.e. #![deny(invalid_reference_casting)].

Status

This lint was uplifted directly from Clippy, but shortly after that a bug in the lint was reported: https://github.com/rust-lang/rust/issues/124685. Currently we are working on eliminating all forms of this issue.

Steps

Open Questions

The lint currently hunts for an expression that represents the backing allocation, then if it encounters a pattern known to produce false positives, we punt on reporting that case. Should we invert the logic and be silent by default instead?

Implementation history

https://github.com/rust-lang/rust/pull/111567 https://github.com/rust-lang/rust/pull/124761 https://github.com/rust-lang/rust/pull/124908 https://github.com/rust-lang/rust/pull/124978

saethlin commented 2 months ago

Doing some crater analysis by extracting crates that trip this lint in the crater runs for 1.79: https://github.com/rust-lang/rust/issues/124770 then re-running those locally with a compiler that contains the above fixes.

These are the GitHub crates which trip the lint in the crater run:

acfoltzer/wasm-async-main/9b99bd278e13fbb79c681bb8f9ea075308a3a1ac
poodlenoodle42/Pipeline-Simulator/7fd823be7c35b40f3aa85b40fd46440ed02cda0a
matklad/rowan/2e75f11a6b98ec35749ae26574a8b552e9fb2261
yuyuyuyurisuki/os_work/fa824c66bd7d181832c344b40512c82236613881
szeweq/aoc2023/2e5f7f50f7c7a2779b585e5e17f5e9d1e31864f2
dxtn89/solana_staking_pool_deployer/da1bffc83a14c568828037cc292c0e46700b4dd4
pjtatlow/jammdb/f406a0d80f12dcc062152c3fb3c85459151dfbc9
biojet1/librsvg/8e3dc21639753f8401a3e1fb1525a7401cb97319
guster32/glplay-rs/bced2f5df1bac66244ee5feaeb02458661817e10
Apochens/mucfuzzer/a0473cf58a2c19c89f92558e753f03b37d01ad50
botnana/botnana-api-rs/29c6f383d8a463233d978cb89feb99ccee0daef8
cgrewon/sol-token-demo-amm/94b68529cd290346d1ab6c84dd6fdca4b974ed98
Tobeyw/solana-neoburger/680eb73ad752752ed9e8a1c2438d7053c31b0c1f
victor-teles/biome-testing/cfa8938952900a697e7f1a9cdd437a7a93af1706
AtosLab/rust-con-tools-base/69aab49c406fa8004394c59eef73d340809a3dd4

afcoltzer/wasm-async-main does not build anymore, guster32/glplay-rs does not build on my system, cgrewon/sol-token-demo-amm and AtosLab/rust-con-tools-base run into some kind of issue with proc_macro2. The rest build successfully and no longer encounter the lint. So at most we could have those 4 still tripping the lint, but there's nothing we can analyze there.

The crates encountered on crates.io are:

any-box/0.0.1
biome_rowan/0.3.1
blazesym/0.2.0-alpha.8
blend2d/0.3.0
drm/0.11.0
hexe_core/0.0.5
jammdb/0.11.0
lv2_raw/0.2.0
memac/0.5.3
mmtk/0.21.0
nut/0.1.1
ozone/0.1.0
particles/0.1.0
region/3.0.0
rips-packets/0.1.0
rome_rowan/0.0.1
rowan/0.15.13
rust-rsm/0.3.2
sgxs/0.7.4
tci/0.1.0
tree-sitter-c2rust/0.20.10
tree-sitter-c2rust-core/0.20.9
vst/0.4.0
winit/0.29.4

Of those, only these crates trip the lint in question on the latest compiler (blend2d and rust-rsm didn't build for me):

drm/0.11.0
mmtk/0.21.0
tree-sitter-c2rust/0.20.10
tree-sitter-c2rust-core/0.20.9
vst/0.4.0
winit/0.29.4

I've trimmed the diagnostics a bit and deduplicated them within crates. It's very common to find the same dodgy unsafe code copy-pasted around.

error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
    --> src/control/mod.rs:1133:34
     |
1128 |             let event = unsafe { &*(self.event_buf.as_ptr().add(self.i) as *const ffi::drm_event) };
     |                                   --------------------------------------------------------------- backing allocation comes from here
...
1133 |                         unsafe { &*(event as *const _ as *const ffi::drm_event_vblank) };
     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
   --> src/util/heap/freelistpageresource.rs:193:38
    |
193 |                 vm_map.bind_freelist(&*(&common_flpr as &CommonFreeListPageResource as *const _));
    |                                      ^^^^-----------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                          |
    |                                          backing allocation comes from here
error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
    --> binding_rust/core/subtree.rs:1525:5
     |
1522 |        let mut data: *mut SubtreeHeapData = &mut *((*children).contents)
     |   __________________________________________-____-
     |  |__________________________________________|
     | ||
1523 | ||         .offset((*children).size as isize) as *mut Subtree
     | ||__________________________________________- backing allocation comes from here
1524 | |          as *mut SubtreeHeapData;
     | |________________________________- casting happend here
1525 |  /     *data = {
1526 |  |         let mut init = SubtreeHeapData { visible_named_extra_fragile_left_fragile_right_has_changes_has_external_to...
1527 |  |         init.set_visible(metadata.visible);
1528 |  |         init.set_named(metadata.named);
...     |
1538 |  |         init
1539 |  |     };
     |  |_____^
error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
    --> src/subtree.rs:1518:5
     |
1515 |        let mut data: *mut SubtreeHeapData = &mut *((*children).contents)
     |   __________________________________________-____-
     |  |__________________________________________|
     | ||
1516 | ||         .offset((*children).size as isize) as *mut Subtree
     | ||__________________________________________- backing allocation comes from here
1517 | |          as *mut SubtreeHeapData;
     | |________________________________- casting happend here
1518 |  /     *data = {
1519 |  |         let mut init = SubtreeHeapData { visible_named_extra_fragile_left_fragile_right_has_changes_has_external_to...
1520 |  |         init.set_visible(metadata.visible);
1521 |  |         init.set_named(metadata.named);
...     |
1531 |  |         init
1532 |  |     };
     |  |_____^
error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
   --> src/event.rs:123:51
    |
90  |         let event = &*event;
    |                      ------ backing allocation comes from here
...
123 |                     let event: &api::SysExEvent = &*(event as *const api::Event as *const api::SysExEvent);
    |                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
    --> src/platform_impl/linux/x11/event_processor.rs:1265:33
     |
1261 |                     let xev = unsafe { &*(xev as *const _ as *const ffi::XkbAnyEvent) };
     |                                         --------------------------------------------- backing allocation comes from here
...
1265 |                                 &*(xev as *const _ as *const ffi::XkbNewKeyboardNotifyEvent)
     |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think these are all buggy, because they are incorrectly detecting the backing allocation in &*expr. I've also tried out a patch for this, similar to https://github.com/rust-lang/rust/pull/124908. Adding another case for this would get us to 0 false positives in crater. If we empirically have no false positives, it's tempting to just stay with the strategy we have right now.

@Urgau What do you think?

Urgau commented 2 months ago

I think these are all buggy, because they are incorrectly detecting the backing allocation in &*expr.

I would tend to agree that all of those cases are incorrectly detecting the backing allocation. If your patch fixes those issues, go ahead, I'm find with stay with the strategy we have right now.