Tracking Issue for removing unsafe code from bootstrap

[jyn514]

This is a tracking issue for removing unsafe code from bootstrap. This is an implementation detail of the build system and there is no associated RFC.

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

If you are interested in working on this issue, please leave a comment saying which part you are working on. Do not use @rustbot claim; this issue is too large to be done by a single person. Do not claim a checkbox that has already been claimed without first asking the existing assignee.

Note that much of this code is platform-specific. I strongly recommend not working on Windows-specific code if you don't have a computer that can run Windows, and vice-versa for Unix.

You can ask for help in https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap, #dark-arts on the community discord (https://discord.gg/rust-lang-community), or #windows-dev on the community discord. Please do not ask for help on this tracking issue since it will quickly get overwhelming.

Here are the current uses of unsafe:

• [x] Creating a directory junction on Windows: https://github.com/rust-lang/rust/blob/d6f27401f12f02876aa6fe53139b030033f37e5e/src/bootstrap/util.rs#L149-L249 This can use https://docs.rs/junction/latest/junction/fn.create.html instead.
• [x] std::path::absolute, copied into bootstrap: https://github.com/rust-lang/rust/blob/d6f27401f12f02876aa6fe53139b030033f37e5e/src/bootstrap/util.rs#L560-L585 This is blocked on stabilization: https://github.com/rust-lang/rust/issues/92750
• [ ] rusage metadata on unix: https://github.com/rust-lang/rust/blob/d6f27401f12f02876aa6fe53139b030033f37e5e/src/bootstrap/bin/rustc.rs#L354-L413 This can be replaced with https://docs.rs/nix/latest/nix/sys/resource/fn.getrusage.html.
• [ ] rusage metadata on windows: https://github.com/rust-lang/rust/blob/d6f27401f12f02876aa6fe53139b030033f37e5e/src/bootstrap/bin/rustc.rs#L281-L352 Not sure what to replace this with, maybe there's something in docs.rs/windows? https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/System/Threading/fn.GetProcessTimes.html appears to also be unsafe, though.
• [ ] libc::setpriority: https://github.com/rust-lang/rust/blob/249198c1f8ae841a49124729a8a0f6f20a905c9c/src/bootstrap/lib.rs#L76-L78 This does not appear to have a counterpart in nix.
• [ ] libc::getuid: https://github.com/rust-lang/rust/blob/249198c1f8ae841a49124729a8a0f6f20a905c9c/src/bootstrap/lib.rs#L349-L358 This can be replaced with https://docs.rs/nix/latest/nix/unistd/struct.Uid.html#method.current.
• [ ] everything in job.rs, I don't really understand what's going on there: https://github.com/rust-lang/rust/blob/d6f27401f12f02876aa6fe53139b030033f37e5e/src/bootstrap/job.rs#L50-L143
• [ ] all the unsafe code in cache.rs: https://github.com/rust-lang/rust/blob/fe0b0428b89802f02a050eab72373b75709d0bce/src/bootstrap/cache.rs#L63-L103 We can replace this with https://docs.rs/lasso/

cc @rust-lang/bootstrap

Unresolved Questions

Are there any safe wrappers for the following APIs?

• job.rs
• libc::setpriority
• rusage on Windows

Implementation history

[Mark-Simulacrum]

I'm not convinced that replacing trivial unsafe for calling libc or windows APIs with dependencies makes sense. That code is rarely changed and the dependency isn't necessarily better vetted (certainly on updates more work).

IMO unsafe code is fine where there's not much in the way of alternative implementations.

[jyn514]

The symlink code at least I think makes sense to replace, it's quite complicated and I don't know anyone on the team who could maintain it. I could be convinced getuid and setpriority are ok, but the rusage code is also non-trivial. And once we pull in nix for rusage I don't see a reason not to also use it for getuid and setpriority.

What worries me most is that we don't have any sorts of tests for this code :/ @saethlin has already found one unsoundness in the cache code which broke local-rebuild for a stable release until we backported a fix. I'm not convinced there isn't more unsoundness that we just haven't noticed yet.

[Mark-Simulacrum]

Third party code that matches our code is just as likely to have problems, imo, and is harder to patch. So I don't really see soundness or lack thereof as a strong argument in this case.

The majority of this code has worked fine for years at this point; I believe the soundness problem wasn't actually leading to UB? But in any case, certainly if we're changing the code replacing it with a library that does equivalent things is fine. But I wouldn't take a goal of "unsafe is special" here.

[Noratrieb]

cc #109960 :)

[GrigorenkoPV]

The OP is a bit outdated, here's the current state of affairs.

• std::path::absolute, copied into bootstrap

• [x] Fixed in #126488.

• rusage metadata on unix This can be replaced with https://docs.rs/nix/latest/nix/sys/resource/fn.getrusage.html.

• [x] I've opened #128290.

• rusage metadata on windows

• [ ] Nothing has changed, AFAICT.

• • libc::setpriority: https://github.com/rust-lang/rust/blob/249198c1f8ae841a49124729a8a0f6f20a905c9c/src/bootstrap/lib.rs#L76-L78

• [ ] Got moved into utils/jobs.rs (and currently does not have an unsafe block, because unsafe_op_in_unsafe_fn is still allowed in bootstrap).

• libc::getuid: This can be replaced with https://docs.rs/nix/latest/nix/unistd/struct.Uid.html#method.current.

• [x] Adding an entire dependency (which under the hood does essentially the same as the current implementation) was deemed to be too much. A SAFETY comment was added instead: #116577.

• everything in job.rs

• [ ] Still has some unsafe.

• all the unsafe code in cache.rs: We can replace this with https://docs.rs/lasso/

• [ ] I've opened a PR #128289, not with lasso though, but with internment. We'll see how it goes.