Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic #83629

the8472 · 2021-03-29T03:00:14Z

This fixes the double-drop but it leaves a behavioral difference compared to the default implementation intact: In the default implementation the source and the destination vec are separate objects, so they get dropped separately. Here they share an allocation and the latter only exists as a pointer into the former. So if dropping the former panics then this fix will leak more items than the default implementation would. Is this acceptable or should the specialization also mimic the default implementation's drops-during-panic behavior?

Fixes #83618

@rustbot label T-libs-impl

rust-highfive · 2021-03-29T03:00:17Z

r? @m-ou-se

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

m-ou-se · 2021-04-01T14:30:24Z

Is this acceptable

Yes.

@bors r+

Thanks!

bors · 2021-04-01T14:30:25Z

📌 Commit 421f5d2 has been approved by m-ou-se

…r=m-ou-se Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic This fixes the double-drop but it leaves a behavioral difference compared to the default implementation intact: In the default implementation the source and the destination vec are separate objects, so they get dropped separately. Here they share an allocation and the latter only exists as a pointer into the former. So if dropping the former panics then this fix will leak more items than the default implementation would. Is this acceptable or should the specialization also mimic the default implementation's drops-during-panic behavior? Fixes rust-lang#83618 `@rustbot` label T-libs-impl

the8472 · 2021-04-01T15:40:52Z

Since the associated issue is labeled critical, should this go into beta too?

…r=m-ou-se Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic This fixes the double-drop but it leaves a behavioral difference compared to the default implementation intact: In the default implementation the source and the destination vec are separate objects, so they get dropped separately. Here they share an allocation and the latter only exists as a pointer into the former. So if dropping the former panics then this fix will leak more items than the default implementation would. Is this acceptable or should the specialization also mimic the default implementation's drops-during-panic behavior? Fixes rust-lang#83618 `@rustbot` label T-libs-impl

RalfJung · 2021-04-02T17:34:41Z

library/alloc/tests/vec.rs

+        let _ = v.into_iter().take(0).collect::<Vec<_>>();
+    }));
+
+    assert_eq!(unsafe { DROP_COUNTER }, 1);


Does this test ensure to eventually actually free the memory that was "leaked" as part of the "panic during drop"?

If not, this would be a problem for running the test in Miri -- to ensure that the standard library is memory-leak-free, Miri errors. That's why other tests like this that already exist explicit free the "leaked" memory after the test is done.

No, it doesn't free. Can you point to a test that does the explicit free so I can steal from that?

I think most tests just avoid having a Box, so even if desturctors don't get called, there's no memory leak -- the DROP_COUNTER is usually enough to check that everything is all right.

I thought there were also tests that used manual "late" deallocation of the Box, but cannot find any... do you think checking DROP_COUNTER is sufficient?

That should be possible, yeah. I took the reproducer from #83629 which relied on a double-free to trigger malloc errors and converted it into a test-case.

Actually, that won't do since the vec's storage itself will also get leaked.

Ah, good point!

…r=m-ou-se Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic This fixes the double-drop but it leaves a behavioral difference compared to the default implementation intact: In the default implementation the source and the destination vec are separate objects, so they get dropped separately. Here they share an allocation and the latter only exists as a pointer into the former. So if dropping the former panics then this fix will leak more items than the default implementation would. Is this acceptable or should the specialization also mimic the default implementation's drops-during-panic behavior? Fixes rust-lang#83618 `@rustbot` label T-libs-impl

Rollup of 7 pull requests Successful merges: - rust-lang#83065 (Rework `std::sys::windows::alloc`) - rust-lang#83478 (rustdoc: Add unstable option to only emit shared/crate-specific files) - rust-lang#83629 (Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic) - rust-lang#83673 (give full path of constraint in suggest_constraining_type_param) - rust-lang#83755 (Simplify coverage tests) - rust-lang#83757 (2229: Support migration via rustfix) - rust-lang#83771 (Fix stack overflow detection on FreeBSD 11.1+) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

the8472 · 2021-04-02T22:56:24Z

This needs another approval, I added one more commit to address the miri issue while the rollup was in flight.

@rustbot label -S-waiting-on-bors +S-waiting-on-review

RalfJung · 2021-04-03T08:49:50Z

I'm afraid changing a PR while a rollup is in flight will confuse things a bit.^^ Most of the commits of this one have landed as part of #83790, so I'll close this PR. It would be better to open a new PR, I think.

RalfJung · 2021-04-03T09:01:08Z

FWIW, here is the anticipated leak -- looks like only one allocation actually leaked?

0.213298   The following memory was leaked: alloc10449836 (Rust heap, size: 16, align: 8) {
0.000075       ╾a10449880[<24771093>]╼ 00 00 00 00 00 00 00 00 │ ╾──────╼........
0.000012   }
0.000007   alloc10449880 (deallocated)

the8472 · 2021-04-03T16:39:47Z

FWIW, here is the anticipated leak -- looks like only one allocation actually leaked?

0.213298   The following memory was leaked: alloc10449836 (Rust heap, size: 16, align: 8) {
0.000075       ╾a10449880[<24771093>]╼ 00 00 00 00 00 00 00 00 │ ╾──────╼........
0.000012   }
0.000007   alloc10449880 (deallocated)

That should actually be correct. Without the fix the Box got dropped twice and the vec got dropped once. Now the box gets dropped once and the vec gets leaked.

I'm afraid changing a PR while a rollup is in flight will confuse things a bit.^^ Most of the commits of this one have landed as part of #83790, so I'll close this PR. It would be better to open a new PR, I think.

Eh, git can deal with merging the same branch twice. But ok, I'll make a new PR. I can also add the box again.

RalfJung · 2021-04-03T16:58:44Z

But ok, I'll make a new PR. I can also add the box again.

Thanks a lot!

…r=RalfJung cleanup leak after test to make miri happy Contains changes that were requested in rust-lang#83629 but didn't make it into the rollup. r? `@RalfJung`

…r=RalfJung cleanup leak after test to make miri happy Contains changes that were requested in rust-lang#83629 but didn't make it into the rollup. r? ``@RalfJung``

…r=RalfJung cleanup leak after test to make miri happy Contains changes that were requested in rust-lang#83629 but didn't make it into the rollup. r? ```@RalfJung```

…r=RalfJung cleanup leak after test to make miri happy Contains changes that were requested in rust-lang#83629 but didn't make it into the rollup. r? ````@RalfJung````

…r=RalfJung cleanup leak after test to make miri happy Contains changes that were requested in rust-lang#83629 but didn't make it into the rollup. r? `````@RalfJung`````

cuviper · 2021-04-16T17:54:17Z

This PR was cited as the fix for CVE-2021-31162, so I'm nominating it for beta.

the8472 · 2021-04-16T18:14:07Z

If miri tests are executed for beta builds you'll want #83827 too

RalfJung · 2021-04-16T18:40:09Z

No, Miri is nightly-only.

wesleywiser · 2021-04-22T14:44:26Z

We discussed this in the compiler team triage meeting this morning and decided to approve for backport.

[beta] backports This backports two beta-accepted PRs, fixing CVE-2020-36323 and CVE-2021-31162. - Fixes API soundness issue in `join()` rust-lang#81728 - Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic rust-lang#83629

the8472 added 2 commits March 29, 2021 04:39

add testcase for double-drop during Vec in-place collection

fa89c0f

fix double-drop in in-place collect specialization

421f5d2

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 29, 2021

rust-highfive assigned m-ou-se Mar 29, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2021

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 Apr 1, 2021

Dylan-DPC-zz mentioned this pull request Apr 1, 2021

Rollup of 6 pull requests #83750

Closed

Dylan-DPC-zz mentioned this pull request Apr 2, 2021

Rollup of 7 pull requests #83789

Closed

RalfJung reviewed Apr 2, 2021

View reviewed changes

Dylan-DPC-zz mentioned this pull request Apr 2, 2021

Rollup of 7 pull requests #83790

Merged

This comment has been minimized.

Sign in to view

cleanup leak after test to make miri happy

328a5e0

the8472 force-pushed the fix-inplace-panic-on-drop branch from 72688e5 to 328a5e0 Compare April 2, 2021 21:23

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 2, 2021

RalfJung closed this Apr 3, 2021

the8472 mentioned this pull request Apr 3, 2021

cleanup leak after test to make miri happy #83827

Merged

cuviper added this to the 1.53.0 milestone Apr 16, 2021

cuviper added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 16, 2021

wesleywiser added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Apr 22, 2021

cuviper mentioned this pull request Apr 26, 2021

[beta] backports #84603

Merged

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 27, 2021

Mark-Simulacrum modified the milestones: 1.53.0, 1.52.0 Apr 27, 2021

JohnTitor mentioned this pull request Apr 28, 2021

Add initial 1.52.0 post rust-lang/blog.rust-lang.org#822

Merged

steffahn mentioned this pull request Sep 9, 2022

In-place optimisation in IntoIterator can lead to memory leak #101628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic #83629

Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic #83629

the8472 commented Mar 29, 2021

rust-highfive commented Mar 29, 2021

m-ou-se commented Apr 1, 2021 •

edited

Loading

bors commented Apr 1, 2021

the8472 commented Apr 1, 2021

RalfJung Apr 2, 2021

the8472 Apr 2, 2021

RalfJung Apr 2, 2021

the8472 Apr 2, 2021

the8472 Apr 2, 2021

RalfJung Apr 3, 2021

This comment has been minimized.

the8472 commented Apr 2, 2021

RalfJung commented Apr 3, 2021

RalfJung commented Apr 3, 2021 •

edited

Loading

the8472 commented Apr 3, 2021

RalfJung commented Apr 3, 2021

cuviper commented Apr 16, 2021

the8472 commented Apr 16, 2021

RalfJung commented Apr 16, 2021

wesleywiser commented Apr 22, 2021

Fix double-drop in Vec::from_iter(vec.into_iter()) specialization when items drop during panic #83629

Fix double-drop in Vec::from_iter(vec.into_iter()) specialization when items drop during panic #83629

Conversation

the8472 commented Mar 29, 2021

rust-highfive commented Mar 29, 2021

m-ou-se commented Apr 1, 2021 • edited Loading

bors commented Apr 1, 2021

the8472 commented Apr 1, 2021

RalfJung Apr 2, 2021

Choose a reason for hiding this comment

the8472 Apr 2, 2021

Choose a reason for hiding this comment

RalfJung Apr 2, 2021

Choose a reason for hiding this comment

the8472 Apr 2, 2021

Choose a reason for hiding this comment

the8472 Apr 2, 2021

Choose a reason for hiding this comment

RalfJung Apr 3, 2021

Choose a reason for hiding this comment

This comment has been minimized.

the8472 commented Apr 2, 2021

RalfJung commented Apr 3, 2021

RalfJung commented Apr 3, 2021 • edited Loading

the8472 commented Apr 3, 2021

RalfJung commented Apr 3, 2021

cuviper commented Apr 16, 2021

the8472 commented Apr 16, 2021

RalfJung commented Apr 16, 2021

wesleywiser commented Apr 22, 2021

Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic #83629

Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic #83629

m-ou-se commented Apr 1, 2021 •

edited

Loading

RalfJung commented Apr 3, 2021 •

edited

Loading