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 in-place collection leak when remaining element destructor panic #101642

Merged
merged 5 commits into from
Oct 4, 2022
Merged

Fix in-place collection leak when remaining element destructor panic #101642

merged 5 commits into from
Oct 4, 2022

Conversation

SkiFire13
Copy link
Contributor

Fixes #101628

cc @the8472

I went for the drop guard route, placing it immediately before the forget_allocation_drop_remaining call and after the comment, as to signal they are closely related.

I also updated the test to check for the leak, though the only change really needed was removing the leak clean up for miri since now that's no longer leaked.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 10, 2022
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2022
the8472
the8472 reviewed Sep 10, 2022
View reviewed changes
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

I assume this passes miri?

library/alloc/tests/vec.rs Outdated Show resolved Hide resolved
library/alloc/src/vec/in_place_collect.rs Outdated Show resolved Hide resolved
library/alloc/tests/vec.rs Outdated Show resolved Hide resolved
@the8472
Copy link
Member

the8472 commented Sep 10, 2022 •
edited
Loading

You should also update the "Drop- and panic-safety" section in the module docs.

@SkiFire13 SkiFire13 force-pushed the fix-inplace-collection-leak branch from 85d7c44 to dad049c Compare September 10, 2022 11:14
@SkiFire13
Copy link
Contributor Author

I assume this passes miri?

I'm having a bit of trouble running miri tests with the modified stdlib since I'm on windows, but I think I'm getting there.

@SkiFire13
Copy link
Contributor Author

Ok I managed to run it after some problems with invalid language items but in the end it worked and the test passes.

Voultapher
Voultapher suggested changes Sep 10, 2022
View reviewed changes
library/alloc/tests/vec.rs Show resolved Hide resolved
library/alloc/tests/vec.rs Show resolved Hide resolved
library/alloc/src/vec/in_place_collect.rs Show resolved Hide resolved
@Voultapher
Copy link
Contributor

I'm not too familiar with the PR process for this repository. I marked the changes as approved, because from my perspective they are good and complete.

@the8472 the8472 assigned the8472 and unassigned joshtriplett Sep 28, 2022
the8472
the8472 reviewed Oct 2, 2022
View reviewed changes
Copy link
Member

@the8472 the8472 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.

I ran some vec benches locally and didn't see any changes, so this should be good. Plus there are some codegen tests covering primitives (but not U: Drop) that ensure it optimizes in the ideal case so from a perspective this should be fine.

With the test and miri passing it should be fine too from the safety perspective.
It seems pretty straight-forward.

  1. pass ownership from IntoIter to InPlaceDstBufDrop
  2. drop src (can panic)
  3. drop sink (on panic) (can panic)
  4. if not panic: pass ownership to final vec

So at no time there's duplicate ownership while things can panic.

So there are only a few minor things left about the comments

library/alloc/src/vec/in_place_collect.rs Outdated Show resolved Hide resolved
library/alloc/src/vec/in_place_collect.rs Outdated Show resolved Hide resolved
@SkiFire13 SkiFire13 force-pushed the fix-inplace-collection-leak branch from fb01af3 to 1750c7b Compare October 3, 2022 18:24
@the8472
Copy link
Member

the8472 commented Oct 3, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2022

📌 Commit 1750c7b has been approved by the8472

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 Oct 3, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Oct 4, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2022
@bors
Auto merge of rust-lang#102652 - Dylan-DPC:rollup-6ff8ct8, r=Dylan-DPC
02cd79a 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#101189 (Implement `Ready::into_inner()`)
 - rust-lang#101642 (Fix in-place collection leak when remaining element destructor panic)
 - rust-lang#102489 (Normalize substs before resolving instance in `NoopMethodCall` lint)
 - rust-lang#102559 (Don't ICE when trying to copy unsized value in const prop)
 - rust-lang#102568 (Lint against nested opaque types that don't satisfy associated type bounds)
 - rust-lang#102633 (Fix rustdoc ICE in invalid_rust_codeblocks lint)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f24d00d into rust-lang:master Oct 4, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the8472 the8472 the8472 left review comments

@Voultapher Voultapher Voultapher approved these changes

Assignees

@the8472 the8472

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

Successfully merging this pull request may close these issues.

In-place optimisation in IntoIterator can lead to memory leak
7 participants
@SkiFire13 @rust-highfive @rustbot @the8472 @Voultapher @bors @joshtriplett