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.

Sign up for GitHub

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

Jump to bottom

Clarify UB in get_unchecked(_mut) #117039

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Clarify UB in get_unchecked(_mut) #117039

merged 3 commits into from
Nov 10, 2023

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Oct 22, 2023 •
edited
Loading

Inspired by #116915, it was unclear to me what exactly "out-of-bounds index" means in get_unchecked.

One could potentially interpret it that get_unchecked is just another way to write offset, but I think get_unchecked(len) is supposed to be UB even though .offet(len) is well-defined (as is .get_unchecked(..len)), so write that more directly in the docs.

libs-api folks: Can you confirm whether this is what you expect this to mean? And is the situation any different for <*const [T]>::get_unchecked?

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Oct 22, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 22, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2023
@the8472
Copy link
Member

the8472 commented Oct 22, 2023

And is the situation any different for <*const [T]>::get_unchecked?

To add: One could interpret this as an offset, in which case one-past-the-end would be fine. But if we reserve the right to add intrinsics::assume(idx < len) here then it should also be UB.

alex
alex reviewed Oct 22, 2023
View reviewed changes
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
@asquared31415
Copy link
Contributor

"Out of bounds index" is almost certainly meant to mean exactly the same cases where you get the following panic with [idx]:

thread 'main' panicked at src/main.rs:2:3:
index out of bounds: the len is 1 but the index is 10

This same "out of bounds" phrasing is used all over docs, the Reference, and a bunch of other places, it would be surprising for it to mean anything else.

@scottmcm
Copy link
Member Author

Yeah, I think it makes sense for it to be the same as the other places, but the codegen means that LLVM generally wouldn't have any problems with it today, and it's only caught by stacked borrows checks in MIRI (and it's not caught at all today by MIRI for <*const [T]>::get_unchecked because that doesn't make a ref). So people might be relying on it "working".

Some quick examples: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=e6c38266426650ec506f0a91f32cf264

@asquared31415
Copy link
Contributor

Right, that's library UB not language UB for the raw pointer version. I forgot that that method exists!

@Amanieu
Copy link
Member

Amanieu commented Oct 24, 2023

We discussed this in the libs-api meeting: abusing get_unchecked_mut to get a pointer to the end of a slice is clearly UB since it involves creating an out-of-bounds reference.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 24, 2023 •
edited by BurntSushi
Loading

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 24, 2023
@rfcbot
Copy link

rfcbot commented Oct 25, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 25, 2023
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Oct 31, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 4, 2023
@rfcbot
Copy link

rfcbot commented Nov 4, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
cuviper
cuviper approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bors r+ rollup

@cuviper
Copy link
Member

cuviper commented Nov 9, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 9, 2023

📌 Commit 545175c has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 10, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2023
@bors
Auto merge of rust-lang#117769 - matthiaskrgr:rollup-4efjlg3, r=matth…
d42d73b 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#114191 (Update exploit mitigations documentation)
 - rust-lang#117039 (Clarify UB in `get_unchecked(_mut)`)
 - rust-lang#117730 (Closure-consuming helper functions for `fmt::Debug` helpers)
 - rust-lang#117741 (Fix typo in internal.rs)
 - rust-lang#117743 (Suggest removing `;` for `;` within let-chains)
 - rust-lang#117751 (rustdoc-json: Fix test so it actually checks things)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7096ec3 into rust-lang:master Nov 10, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 10, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2023
@rust-timer
Unrolled build for rust-lang#117039
9b66575 
Rollup merge of rust-lang#117039 - scottmcm:clarify-get-unchecked, r=cuviper

Clarify UB in `get_unchecked(_mut)`

Inspired by rust-lang#116915, it was unclear to me what exactly "out-of-bounds index" means in `get_unchecked`.

One could [potentially](https://rust.godbolt.org/z/hxM764orW) interpret it that `get_unchecked` is just another way to write `offset`, but I think `get_unchecked(len)` is supposed to be UB even though `.offet(len)` is well-defined (as is `.get_unchecked(..len)`), so write that more directly in the docs.

**libs-api folks**: Can you confirm whether this is what you expect this to mean?  And is the situation any different for `<*const [T]>::get_unchecked`?
@scottmcm scottmcm deleted the clarify-get-unchecked branch November 10, 2023 04:19
@scottmcm scottmcm mentioned this pull request Feb 11, 2024
Closed
@workingjubilee workingjubilee mentioned this pull request Mar 10, 2024
Closed
@scottmcm scottmcm mentioned this pull request Apr 3, 2024
Closed
celinval added a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
@github-actions @celinval
Automatic toolchain upgrade to nightly-2023-11-11 (rust-lang#2870)
d1a038f 
Update Rust toolchain from nightly-2023-11-10 to nightly-2023-11-11
without any other source changes.
This is an automatically generated pull request. If any of the CI checks
fail, manual intervention is required. In such a case, review the
changes at https://github.com/rust-lang/rust from
rust-lang@0f44eb3
up to
rust-lang@edf0b1d.
The log for this commit range is:
rust-lang@edf0b1db0a Auto merge of
rust-lang#115229 - iSwapna:issue-115222-fix, r=estebank
rust-lang@56a109d15b Recurse over the
method chain and maintain a stack to peek at previous receiver to align
spans
rust-lang@d4c86cfc49 Auto merge of
rust-lang#117779 - bjorn3:sync_cg_clif-2023-11-10, r=bjorn3
rust-lang@d186b49460 Merge commit
'c84d1871dc4456539b7b578830268ab3539915d0' into sync_cg_clif-2023-11-10
rust-lang@c84d1871dc Rustup to rustc
1.75.0-nightly (0f44eb3 2023-11-09)
rust-lang@6e7961ac5d Sync from rust
0f44eb3
rust-lang@3d0e99d632 Auto merge of
rust-lang#117765 - onur-ozkan:fix-117762, r=clubby789
rust-lang@17d0a45f5d Auto merge of
rust-lang#117572 - RalfJung:addr_of, r=cuviper
rust-lang@e30f8ae867 mention null
explicitly
rust-lang@0a1e5598b0 Auto merge of
rust-lang#117750 - klensy:icu-followup, r=Nilstrieb
rust-lang@d42d73b144 Auto merge of
rust-lang#117769 - matthiaskrgr:rollup-4efjlg3, r=matthiaskrgr
rust-lang@186a3c8c61 Rollup merge of
rust-lang#117751 - aDotInTheVoid:unkind, r=GuillaumeGomez
rust-lang@7607597d3a Rollup merge of
rust-lang#117743 - sjwang05:issue-117720, r=estebank
rust-lang@7fd7719ca1 Rollup merge of
rust-lang#117741 - eltociear:patch-23, r=compiler-errors
rust-lang@0f1da7e682 Rollup merge of
rust-lang#117730 - jmillikin:fmt-debug-helper-fns, r=cuviper
rust-lang@7096ec3e00 Rollup merge of
rust-lang#117039 - scottmcm:clarify-get-unchecked, r=cuviper
rust-lang@9dc022dd80 Rollup merge of
rust-lang#114191 - rcvalle:rust-exploit-mitigations, r=cuviper
rust-lang@82a9f94de5 Closure-consuming
helper functions for `fmt::Debug` helpers
rust-lang@fdb72795d1 enable unstable
feature on `x clean [PATH]`
rust-lang@22e1576a12 rustdoc-json: Fix
test so it actuall checks things
rust-lang@7142c8d83c bump few ICU4X
leftover deps
rust-lang@5693a34db2 Suggest fix for ;
within let-chains
rust-lang@b8648216a5 Fix typo in
internal.rs
rust-lang@7c385f5a03 Update exploit
mitigations documentation
rust-lang@545175ce87 Fix addition
formatting
rust-lang@82487a9447 Merge pull request
rust-lang#1417 from rust-lang/implement_xgetbv
rust-lang@864973135a Implement all vendor
intrinsics used by the simd-json crate
rust-lang@9f426cef38 Merge pull request
rust-lang#1416 from afonso360/aarch64-intrinsics-1
rust-lang@ecf79a304a Implement all vendor
intrinsics used by the fimg crate
rust-lang@0a35232c85 Implement all vendor
intrinsics used by the httparse crate
rust-lang@61e38ceea7 Implement all SSE
intrinsics used by the jpeg-decoder crate
rust-lang@438194980b Implement all avx2
intrinsics used by the image crate
rust-lang@6a53acefd8 Implement
_mm256_permute2f128_ps and _mm256_permute2f128_pd intrinsics
rust-lang@81af5b5031 update and clarify
addr_of docs
rust-lang@209476e33a Only import aarch64
intrinsics on aarch64
rust-lang@f824da66c6 Make neon example
build in all arches
rust-lang@70a6abfd29 Add unsigned
saturating add/sub intrinsics for aarch64
rust-lang@88c2e7896b Implement aarch64
addp intrinsics
rust-lang@1f09bae6a8 Implement min/max
neon intrisics
rust-lang@8eca01f4b6 Remove support for
compiler plugins.
rust-lang@f6a8c3afb5 Add real
implementation of _xgetbv()
rust-lang@909513ef74 Use Value instead of
CValue in CInlineAsmOperand
rust-lang@ef3703694f Disable a couple of
rustc tests which are broken due to a rustc bug
rust-lang@c04ceb4342 Fix workaround for
the `int $$0x29` issue to not crash on empty inline asm
rust-lang@04f1024ecb Rustup to rustc
1.75.0-nightly (75b064d 2023-11-01)
rust-lang@361585e06d Sync from rust
75b064d
rust-lang@03c9acdd8f Support enum
variants in offset_of!
rust-lang@48ca2d9703 Implement
llvm.fma.v* intrinsics
rust-lang@aed0ed2875 Rollup merge of
rust-lang#117317 - RalfJung:track-caller, r=oli-obk
rust-lang@9a33f82140 Remove inline asm
support from the list of limitations
rust-lang@51f6ac7bfc Merge branch
'sync_from_rust'
rust-lang@41dcb52153 Merge commit
'dde58803fd6cbb270c7a437f36a8a3a29fbef679' into sync_cg_clif-2023-10-29
rust-lang@c6f5090294 share the
track_caller handling within a mir::Body
rust-lang@bad4be6e29 interpret: call
caller_location logic the same way codegen does, and share some code
rust-lang@01ca7a0cb0 Add the missing word
rust-lang@2c13ee8970 Clarify UB in
`get_unchecked(_mut)`
rust-lang@40a83be6eb Format exploit
mitigations documentation

Co-authored-by: celinval <celinval@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex alex alex left review comments

@ChayimFriedman2 ChayimFriedman2 ChayimFriedman2 left review comments

@asquared31415 asquared31415 asquared31415 left review comments

@cuviper cuviper cuviper approved these changes

Assignees

@cuviper cuviper

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.75.0
Development

Successfully merging this pull request may close these issues.