search

rust-lang / rust

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

Fixing `unsafe_op_in_unsafe_fn` for `std::{os, sys}` #127747

Open workingjubilee opened 2 months ago

workingjubilee commented 2 months ago

In https://github.com/rust-lang/rust/pull/127744 we begin to deny(unsafe_op_in_unsafe_fn) in the standard library, but there are... many... platforms which all use unsafe code inside unsafe functions. It is unreasonable to fix them all in one step.

Even if it is technically possible to do this in one push that "fixes" it for all the platforms we run tier 1/2 CI on, that would be extremely rude. That would then immediately nuke all tier 3 platforms that aren't a variation on the tier 2 ones. We should at least try to allow people to respond before we do so. After all, we will eventually update the standard library to edition 2024 anyway.

To fix each platform, one at a time, deny(unsafe_op_in_unsafe_fn) should be moved into that platform's support modules (and even individual functions, if necessary). As we can see from the following test sample, you can have a deny, then an allow, then a deny, and it still works correctly. So we will keep the allows at the top level of std::os and std::sys in place.

For now.

In order to not have one giant table of targets that make my eyes blur, I have split the categories in somewhat arbitrary ways. This is primarily based on the heuristic of "if other examples in this category are fixed, are the rest likely to also be fixed?" except for the "uniques" set, which I expect will almost all require individual attention.

berkeley software descendants

darwinian

linuxen

unixen

embedded unix + newlib

Unix-ish impls mostly intended for embedded-ish usage, all using newlib.

windows-y

uniques

We have a grab bag of others here, some of which are "Unix family" but are... estranged cousins, if so.

workingjubilee commented 2 months ago

My current belief is that, like with every edition update so far, we will eventually fix the lint in all relevant places. By "fixed" I mean actually fixing it. However, it is fine if a platform adds more instances of allow for this lint, but I assume that we will eventually remove such allowances.

Pinging first everyone who has their own std::sys::pal submodule, since those are easiest to add #![deny(unsafe_op_in_unsafe_fn)] into their actual submodule, at the moment. You may have nothing to do! Just confirm that and we can mark it off... but do please actually check. At least one platform has started on using deny(unsafe_op_in_unsafe_fn) in some of its submodules, but has at least one instance of code that should fail that lint, yet is neither specifically denied nor allowed.

Also remember to check std::sys::sync and the other "top level" modules of sys for your platform's code in its submodules.

xobs commented 2 months ago

Xous already has #![deny(unsafe_op_in_unsafe_fn)] in sys/pal/xous/mod.rs, and adding it to os/xous/mod.rs does not cause any failures.

Adding it to the top of sys/mod.rs causes failures in:

But otherwise there are no issues for Xous.

joboet commented 2 months ago

I think we should add #[forbid]s to leaf modules instead and slowly move them up the module tree until we reach the root. Just #[deny]ing the lint stills allows adding new cases of this to modules that have already been fixed.

Ayush1325 commented 2 months ago

Adding #![deny(unsafe_op_in_unsafe_fn)] in sys/pal/uefi/mod.rs and os/uefi/mod.rs cause no failures.

SchmErik commented 2 months ago

Thanks for @-ing us @workingjubilee! Adding #![deny(unsafe_op_in_unsafe_fn)] in sys/pal/zkvm/mod.rs resulted in one failure. Here's a small PR to address this: https://github.com/rust-lang/rust/pull/127833

workingjubilee commented 2 months ago

Identified and sifted out some targets into their own set.

Hi, you all are currently listed as maintainers for a target that

Unfortunately, the way the std::sys::pal::unix logic currently is written, it is very deeply entangled between different Unix targets. I am pinging you all as a group, ahead of most other "Unix" targets, because your targets have no CI coverage (so if someone breaks your target, it won't be noticed) and share some of the same architectural considerations. Even if you have little to do, it might be a good idea to factor out some of your implementations so that later passes by other maintainers don't affect your support code.

kawadakk commented 2 months ago

kmc-solid already has #![deny(unsafe_op_in_unsafe_fn)] in std/src/sys/pal/solid/mod.rs and std/src/os/solid/io.rs, and adding it to these target-specific modules causes no failures.

Table | Module path | Source file | | | --- | --- | --- | | `std::sys::pal::solid` | `std/src/sys/pal/solid/mod.rs` | Has `#![deny(unsafe_op_in_unsafe_fn)]` | | `std::sys::pal::solid::itron` | `std/src/sys/pal/solid/mod.rs`¹ | N/A - submodule of `std::sys::pal::solid` | | `std::sys::path::unsupported_backslash` | `std/src/sys/path/unsupported_backslash.rs`² | `#![deny(unsafe_op_in_unsafe_fn)]` can be added without causing compilation failure | | `std::sys::sync::rwlock::solid` | `std/src/sys/sync/rwlock/solid.rs` | `#![deny(unsafe_op_in_unsafe_fn)]` can be added without causing compilation failure | | `std::sys::sync::mutex::itron` | `std/src/sys/sync/mutex/itron.rs` | `#![deny(unsafe_op_in_unsafe_fn)]` can be added without causing compilation failure | | `std::os::solid` | `std/src/os/solid/mod.rs` | `#![deny(unsafe_op_in_unsafe_fn)]` can be added without causing compilation failure | | `std::os::solid::io` | `std/src/os/solid/io.rs` | Has `#![deny(unsafe_op_in_unsafe_fn)]` | | `std::os::solid::ffi` | `std/src/os/solid/ffi.rs` | `#![deny(unsafe_op_in_unsafe_fn)]` can be added without causing compilation failure | ¹ Inline module. Its submodules are implemented in `std/src/sys/pal/itron/*.rs`. ² Shared with `cfg(target_os = "uefi")` (Tier 2 or lower).
workingjubilee commented 2 months ago

@ChrisDenton has been working on this for Windows, but also pinging @roblabla as a heads up: this issue combines with the fact that our Windows support is, for a lot of cases, rotating out its current impls, but keeping the old impls for Windows 7 support. So you won't have quite as much CI coverage, soon.

workingjubilee commented 2 months ago

@biabbas Thank you for https://github.com/rust-lang/rust/pull/127480 and also you might want to be aware that this issue is relevant to the VxWorks targets continuing to build.

workingjubilee commented 1 month ago

Thank you for everyone that has responded and gotten fixes in!

It looks like wasm-?-? and wasm-wasi are already deny(unsafe_op_in_unsafe_fn) for their specific code, or obviously conformant, so that's just upgrading a few annotations...

...expect maybe the... threads code?

cc @g0djan @abrown @loganek can you take a look at https://github.com/rust-lang/rust/blob/006c8df322e55c14d845e1fe317ca1445c2f8e6b/library/std/src/sys/pal/wasi/thread.rs and the other threads-on-wasi files and make sure they all conform to forbid(unsafe_op_in_unsafe_fn)?

workingjubilee commented 1 month ago

emscripten is littered throughout the "unix" code, like many others.

workingjubilee commented 1 month ago

Pinging the remaining tier 3 Unix maintainers for this. Please be aware that when this lint is enabled, all code which is only compiled for tier 3 targets, but nonetheless violates this lint, will cease compiling without any check from CI.

As these are all considered "Unix" implementations, it is possible the majority or even all of your unsafe fn are shared with tier 2+ targets, so you may not necessarily have to do anything. However, please also be aware that as this issue is fixed, for the Unix code in particular, code that is deemed not-unsafe will be made into safe functions.

This entire exercise is really a warm-up for actually having the (un)safety in the standard library's platform interfaces properly documented in the actual code and cross-referencing external sources as appropriate.

ecnelises commented 1 month ago

Thanks for reminding. AIX does not have specific parts to change.

sthibaul commented 1 month ago

Thanks for the heads-up. I added #![deny(unsafe_op_in_unsafe_fn)] to library/std/src/os/hurd/{fs,mod,raw}.rs and it built fine.

semarie commented 1 month ago

library/std/src/os/openbsd/*.rs are fine too. but they will be specific changes need for openbsd in unix parts

g0djan commented 1 month ago

cc @g0djan @abrown @loganek can you take a look at https://github.com/rust-lang/rust/blob/006c8df322e55c14d845e1fe317ca1445c2f8e6b/library/std/src/sys/pal/wasi/thread.rs and the other threads-on-wasi files and make sure they all conform to forbid(unsafe_op_in_unsafe_fn)?

Hey, thanks for reaching out. I see that in this file there're 8 lines which don't compile for wasm32-wasip1-threads when I add #![deny(unsafe_op_in_unsafe_fn)] to it. I'll take a look at the other files and send a PR

ChrisDenton commented 1 month ago

All the tier 1 Windows targets have the Windows specific stuff taken care of. There are still some shared code that needs handling in library\std\src\sys\sync\condvar\futex.rs.

I've not checked tier 3 Windows targets.

ivmarkov commented 1 month ago

Identified and sifted out some targets into their own set.

  • espressif-espidf: @ivmarkov @MabezDev @SergioGasquez
  • nintendo-3ds ("horizon"): @Meziu @AzureMarker @ian-h-chamberlain
  • sony-vita: @nikarh @pheki @zetanumbers

Hi, you all are currently listed as maintainers for a target that

  • is target_family = "unix"
  • is target_env = "newlib"
  • uses the std::sys support for a "unix" target
  • supports only 32-bit systems
  • is officially tier 3 for all variants of the target

Unfortunately, the way the std::sys::pal::unix logic currently is written, it is very deeply entangled between different Unix targets. I am pinging you all as a group, ahead of most other "Unix" targets, because your targets have no CI coverage (so if someone breaks your target, it won't be noticed) and share some of the same architectural considerations. Even if you have little to do, it might be a good idea to factor out some of your implementations so that later passes by other maintainers don't affect your support code.

@workingjubilee Regarding the ESP-IDF targets:

Factoring out I wouldn't consider ATM, as it would be a gigantic effort, and so far the "intermingling" had worked well for us actually. We occasionally get a build failure once per a couple of months or so, but since we run a cargo -Z build-std nightly CI against downstream ESP-IDF crates, this CI is also catching issues in the STD for our Tier-3 targets "next day" so to say. (We might consider putting a proper ./x.py build library/std CI in-place for our targets in future, because - as it turns out - cargo -Z build-std is hiding the STD build warnings from us - and in the meantime - building our targets spits out some unused/deprecation wanings I did not realize are in there.)

So I would say I prefer to "take the hit", wait until the Tier-1/2 target maintainers fix the #[cfg(unix)] space, and if there are regressions specific to ESP-IDF after that, we'll submit PRs in a day or two after the regressions pop up in our nightly CI. Given that the "fixes" are basically putting extra unsafe {} blocks here and there, I think we should be able to react very fast. :)

workingjubilee commented 1 month ago

As you wish. :^)

biabbas commented 1 month ago

As seen in pr #128539, deny_unsafeop_in_unsafe_fn in VxWorks specific files does not affect the build( No code where this lint can be applied is present in VxWorks specific files). VxWorks uses most of Unix library code.