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

Fix double drop of allocator in IntoIter impl of Vec #95298

Merged
merged 4 commits into from
Mar 31, 2022
Merged

Fix double drop of allocator in IntoIter impl of Vec #95298

merged 4 commits into from
Mar 31, 2022

Conversation

jhorstmann
Copy link
Contributor

Fixes #95269

The drop impl of IntoIter reconstructs a RawVec from buf, cap and alloc, when that RawVec is dropped it also drops the allocator. To avoid dropping the allocator twice we wrap it in ManuallyDrop in the InttoIter struct.

Note this is my first contribution to the standard library, so I might be missing some details or a better way to solve this.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2022
@workingjubilee
Copy link
Member

Can this include a regression test, perhaps based on orlp's MCVE?

danielhenrymantilla
danielhenrymantilla approved these changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM ✅

  • (regarding the regression test —which I agree ought to be added—, you can use the drop glue of, for instance, an Rc, to know how many times something has been dropped)

library/alloc/src/vec/into_iter.rs Outdated Show resolved Hide resolved
jhorstmann and others added 2 commits March 25, 2022 13:27
@jhorstmann @danielhenrymantilla
Use ManuallyDrop::take instead of into_inner
d14c0d2 
Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
@jhorstmann
Add a test verifying the number of drop calls
4b53f56
@jhorstmann
Copy link
Contributor Author

Thanks for the quick review! I added a test using the existing DropCounter impl and verified it was failing before the fix.

Github actions have a scheduled maintenance until 16:30 UTC, will check back later for CI results.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

r? @oli-obk

@bors r+

1 similar comment
@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

r? @oli-obk

@bors r+

@bors
Copy link
Contributor

bors commented Mar 30, 2022

📌 Commit d9a438d has been approved by oli-obk

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 30, 2022

📌 Commit d9a438d has been approved by oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned kennytm Mar 30, 2022
@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 Mar 30, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 30, 2022
@Dylan-DPC
Rollup merge of rust-lang#95298 - jhorstmann:fix-double-drop-of-alloc…
e5c6d84 
…ator-in-vec-into-iter, r=oli-obk

Fix double drop of allocator in IntoIter impl of Vec

Fixes rust-lang#95269

The `drop` impl of `IntoIter` reconstructs a `RawVec` from `buf`, `cap` and `alloc`, when that `RawVec` is dropped it also drops the allocator. To avoid dropping the allocator twice we wrap it in `ManuallyDrop` in the `InttoIter` struct.

Note this is my first contribution to the standard library, so I might be missing some details or a better way to solve this.
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 30, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 30, 2022
@Dylan-DPC
Rollup merge of rust-lang#95298 - jhorstmann:fix-double-drop-of-alloc…
0e31fd1 
…ator-in-vec-into-iter, r=oli-obk

Fix double drop of allocator in IntoIter impl of Vec

Fixes rust-lang#95269

The `drop` impl of `IntoIter` reconstructs a `RawVec` from `buf`, `cap` and `alloc`, when that `RawVec` is dropped it also drops the allocator. To avoid dropping the allocator twice we wrap it in `ManuallyDrop` in the `InttoIter` struct.

Note this is my first contribution to the standard library, so I might be missing some details or a better way to solve this.
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 30, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 30, 2022
@Dylan-DPC
Rollup merge of rust-lang#95298 - jhorstmann:fix-double-drop-of-alloc…
d7b0032 
…ator-in-vec-into-iter, r=oli-obk

Fix double drop of allocator in IntoIter impl of Vec

Fixes rust-lang#95269

The `drop` impl of `IntoIter` reconstructs a `RawVec` from `buf`, `cap` and `alloc`, when that `RawVec` is dropped it also drops the allocator. To avoid dropping the allocator twice we wrap it in `ManuallyDrop` in the `InttoIter` struct.

Note this is my first contribution to the standard library, so I might be missing some details or a better way to solve this.
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 30, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 30, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2022
@bors
Auto merge of rust-lang#95501 - Dylan-DPC:rollup-arx6sdc, r=Dylan-DPC
a39ac5a 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93901 (Stabilize native library modifier syntax and the `whole-archive` modifier specifically)
 - rust-lang#94806 (Fix `cargo run tidy`)
 - rust-lang#94869 (Add the generic_associated_types_extended feature)
 - rust-lang#95011 (async: Give predictable name to binding generated from .await expressions.)
 - rust-lang#95251 (Reduce max hash in raw strings from u16 to u8)
 - rust-lang#95298 (Fix double drop of allocator in IntoIter impl of Vec)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d6c959c into rust-lang:master Mar 31, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 31, 2022
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 7, 2022
@bors
Auto merge of rust-lang#95501 - Dylan-DPC:rollup-arx6sdc, r=Dylan-DPC
bb5b250 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93901 (Stabilize native library modifier syntax and the `whole-archive` modifier specifically)
 - rust-lang#94806 (Fix `cargo run tidy`)
 - rust-lang#94869 (Add the generic_associated_types_extended feature)
 - rust-lang#95011 (async: Give predictable name to binding generated from .await expressions.)
 - rust-lang#95251 (Reduce max hash in raw strings from u16 to u8)
 - rust-lang#95298 (Fix double drop of allocator in IntoIter impl of Vec)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielhenrymantilla danielhenrymantilla danielhenrymantilla approved these changes

Assignees

@oli-obk oli-obk

Labels
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.61.0
Development

Successfully merging this pull request may close these issues.

Pointer dereference after free with allocator containing shared state
8 participants
@jhorstmann @rust-highfive @workingjubilee @oli-obk @bors @danielhenrymantilla @kennytm @rustbot