Skip to content

Fix off-by-one error causing slice::sort to abort the program #136163

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
Merged
merged 1 commit into from
Feb 1, 2025
Merged

Fix off-by-one error causing slice::sort to abort the program #136163

merged 1 commit into from
Feb 1, 2025

Conversation

uellenberg
Copy link
Contributor

Fixes #136103.
Based on the analysis by @jonathan-gruber-jg and @orlp.

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2025

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 28, 2025
uellenberg
uellenberg commented Jan 28, 2025
View reviewed changes
@workingjubilee
Copy link
Member

r? libs

@orlp
Copy link
Contributor

orlp commented Jan 28, 2025

Looks good to me, @Voultapher any thoughts?

Voultapher
Voultapher suggested changes Jan 28, 2025
View reviewed changes
@Voultapher
Copy link
Contributor

I'm suggesting minor changes, the fix itself looks good.

@uellenberg uellenberg force-pushed the driftsort-off-by-one branch from 8ffa11c to 37abe21 Compare January 28, 2025 22:50
@uellenberg
Copy link
Contributor Author

Some of the comments mention MIN_SMALL_SORT_SCRATCH_LEN but I don't see that actually defined anywhere, nor used in any code. Was this renamed at some point, or am I missing something?

@uellenberg uellenberg force-pushed the driftsort-off-by-one branch from 37abe21 to ea42603 Compare January 29, 2025 00:30
Voultapher
Voultapher approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@Voultapher Voultapher left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix @uellenberg

@Voultapher
Copy link
Contributor

Some of the comments mention MIN_SMALL_SORT_SCRATCH_LEN but I don't see that actually defined anywhere, nor used in any code. Was this renamed at some point, or am I missing something?

In the original driftsort repository this constant was defined as pub const MIN_SMALL_SORT_SCRATCH_LEN: usize = i32::SMALL_SORT_THRESHOLD + 16;, while merging the driftsort and ipnsort code bases it was renamed to SMALL_SORT_GENERAL_SCRATCH_LEN. I think this kind of coment fixup doesn't need a separate PR, so feel free to do it here.

@uellenberg uellenberg force-pushed the driftsort-off-by-one branch from ea42603 to 1565254 Compare January 29, 2025 07:40
@Voultapher
Copy link
Contributor

Could we maybe change the PR title, crash usually suggests UB, here we are just running into assert style function preconditions, in this case not using panics and instead abort to save on binary-size.

@uellenberg uellenberg changed the title Fix off-by-one error causing driftsort to crash Fix off-by-one error causing slice::sort to abort the program Jan 30, 2025
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 31, 2025

📌 Commit 1565254 has been approved by Mark-Simulacrum

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 Jan 31, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jan 31, 2025
@workingjubilee
Rollup merge of rust-lang#136163 - uellenberg:driftsort-off-by-one, r…
21184df 
…=Mark-Simulacrum

Fix off-by-one error causing slice::sort to abort the program

Fixes rust-lang#136103.
Based on the analysis by `@jonathan-gruber-jg` and `@orlp.`
@workingjubilee workingjubilee mentioned this pull request Jan 31, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
@bors
Auto merge of rust-lang#136367 - workingjubilee:rollup-pud5uj0, r=wor…
1bc23f1 
…kingjubilee

Rollup of 16 pull requests

Successful merges:

 - rust-lang#133266 (ci: fix explanation why LLVM download is disabled for windows-gnu)
 - rust-lang#135768 (tests: Port `symbol-mangling-hashed` to rmake.rs)
 - rust-lang#135836 (bootstrap: only build `crt{begin,end}.o` when compiling to MUSL)
 - rust-lang#135840 (omit unused args warnings for intrinsics without body)
 - rust-lang#135900 (Manually walk into WF obligations in `BestObligation` proof tree visitor)
 - rust-lang#136146 (Explicitly choose x86 softfloat/hardfloat ABI)
 - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe})
 - rust-lang#136163 (Fix off-by-one error causing slice::sort to abort the program)
 - rust-lang#136266 (fix broken release notes id)
 - rust-lang#136283 (Update encode_utf16 to mention it is native endian)
 - rust-lang#136309 (set rustc dylib on manually constructed rustc command)
 - rust-lang#136314 (Use proper type when applying deref adjustment in const)
 - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets)
 - rust-lang#136348 (miri: make float min/max non-deterministic)
 - rust-lang#136351 (Add documentation for derive(CoercePointee))
 - rust-lang#136358 (`#[optimize(none)]` implies `#[inline(never)]`)

Failed merges:

 - rust-lang#135994 (Rename rustc_middle::Ty::is_unsafe_ptr to is_raw_ptr)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 1, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2025
@bors
Auto merge of rust-lang#136371 - matthiaskrgr:rollup-0b880v3, r=matth…
9e48dfe 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#135840 (omit unused args warnings for intrinsics without body)
 - rust-lang#135900 (Manually walk into WF obligations in `BestObligation` proof tree visitor)
 - rust-lang#136163 (Fix off-by-one error causing slice::sort to abort the program)
 - rust-lang#136266 (fix broken release notes id)
 - rust-lang#136314 (Use proper type when applying deref adjustment in const)
 - rust-lang#136348 (miri: make float min/max non-deterministic)
 - rust-lang#136351 (Add documentation for derive(CoercePointee))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f90c321 into rust-lang:master Feb 1, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@Voultapher Voultapher Voultapher approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.

Sorting a Slice Raises an Illegal Instruction Signal
8 participants
@uellenberg @rustbot @workingjubilee @orlp @Voultapher @Mark-Simulacrum @bors @chenyukang