Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: unsafe precondition(s) violated: slice::get_unchecked_mut requires that the index is within the slice #123285

Closed
Mark-Simulacrum opened this issue Mar 31, 2024 · 6 comments
Labels
C-gub Category: the reverse of a compiler bug is generally UB regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Mar 31, 2024

[INFO] [stderr] error: failed to run custom build command for `libsodium-sys v0.2.4 (https://github.com/Dhole/sodiumoxidez?branch=extra#53c0fb16)`
[INFO] [stderr] note: To improve backtraces for build dependencies, set the CARGO_PROFILE_DEV_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug information generation.
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   process didn't exit successfully: `/opt/rustwide/target/debug/build/libsodium-sys-06e88a69c64e58f8/build-script-build` (signal: 6, SIGABRT: process abort signal)
[INFO] [stderr]   --- stdout
[INFO] [stderr]   cargo:rerun-if-env-changed=SODIUM_LIB_DIR
[INFO] [stderr]   cargo:rerun-if-env-changed=SODIUM_SHARED
[INFO] [stderr]   cargo:rerun-if-env-changed=SODIUM_USE_PKG_CONFIG
[INFO] [stderr]   cargo:rerun-if-env-changed=SODIUM_DISABLE_PIE
[INFO] [stderr] 
[INFO] [stderr]   --- stderr
[INFO] [stderr]   thread 'main' panicked at library/core/src/panicking.rs:156:5:
[INFO] [stderr]   unsafe precondition(s) violated: slice::get_unchecked_mut requires that the index is within the slice
[INFO] [stderr]   stack backtrace:

reverse-deps of libsodium-sys 0.2.5, pnet_packet 0.26:

"unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed isize::MAX"

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 31, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.78.0 milestone Mar 31, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 31, 2024
@Mark-Simulacrum
Copy link
Member Author

Most likely we will just close this as won't fix, but this has a lot of breakage -- probably ~300-400 crates in total across the Crater run (tests or build scripts). At minimum it'll be in release notes and possibly blog post as "hey, we're doing new checking"...

@Noratrieb
Copy link
Member

@saethlin

@saethlin
Copy link
Member

saethlin commented Mar 31, 2024

This is about what I expected 🤷

One of the biggest offenders is that we didn't have any documentation for the preconditions of slice::get_unchecked{_mut} at 1.0 so a lot of people who learned unsafe Rust around that era and didn't look back have published crates with increasingly dangerous code in them, as we did add documentation for what the precondition is (it should have raised a lot of eyebrows that we had unsafe code with no documented preconditions but I guess there wasn't such rigor back then) and last release we told LLVM about the UB we're checking for in slice::get_unchecked.

The PR that originally added these checks already has the relnotes label, it's #120594. The implementation has evolved a bit since that PR, but the observable behavior for most users is the same as in that PR.

@saethlin saethlin added T-opsem Relevant to the opsem team and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 1, 2024
@Amanieu
Copy link
Member

Amanieu commented Apr 3, 2024

We discussed this in the libs meeting and agree that there's probably nothing to be done on our end. Most of these seem to be from very outdated dependencies, so a fix in most cases would be to simply update dependencies.

@scottmcm
Copy link
Member

scottmcm commented Apr 3, 2024

If you need something else to link, #117039 was the libs-api FCP that get_unchecked(_mut) like this has always been UB.

Those doc clarifications landed in 1.75, then (as was mentioned above) it's #116915 in 1.76 that actually started directly exploiting this UB, so the fact that we're actually detecting violations now seems like pure win.

@workingjubilee
Copy link
Member

Seems to have been resolved as "tell people as clearly as possible to experience bij". Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-gub Category: the reverse of a compiler bug is generally UB regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

No branches or pull requests

7 participants