Skip to content

BTreeMap: detect bulk_steal's count-1 underflow in release builds too #79987

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
Dec 13, 2020
Merged

BTreeMap: detect bulk_steal's count-1 underflow in release builds too #79987

merged 1 commit into from
Dec 13, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Dec 12, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2020
@Mark-Simulacrum
Copy link
Member

The title here implies that this is happening on release builds, but I presume you just mean "detect it if it happens"?

@ssomers
Copy link
Contributor Author

ssomers commented Dec 13, 2020

The count -1 expression is always evaluated and - as far as I noriced and know - always panics in debug builds when it underflows. Could use count.checked_sub(1) or something instead, but that seemed awkward. Could change its meaning to extra and start from 0, or change the type to NonZeroUsize, but that seemed awkward in the caller.

@Mark-Simulacrum
Copy link
Member

My question is more so - is this PR caused by a suspicion that this actually happens? It seems fine to me, but I would like to know if this is brought on by something or just seemed like a good idea.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 13, 2020

It actually happened to me in experimental code, but it cannot happen in the current code. The callers in split.rs are pretty clear about that.

Which makes me realize now that it's also possible to skip the operation if count == 0, which allows taking self by value and getting close to the other steal functions (which are really the same written out for count == 1).

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Dec 13, 2020

📌 Commit ad75a96 has been approved by Mark-Simulacrum

@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 Dec 13, 2020
@bors
Copy link
Collaborator

bors commented Dec 13, 2020

⌛ Testing commit ad75a96 with merge 69ff39e...

@bors
Copy link
Collaborator

bors commented Dec 13, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 69ff39e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2020
@bors bors merged commit 69ff39e into rust-lang:master Dec 13, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 13, 2020
@ssomers ssomers deleted the btree_cleanup_4 branch December 13, 2020 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ssomers @Mark-Simulacrum @bors @rust-highfive @rustbot