fix Zip unsoundness (again) #141076

the8472 · 2025-05-16T10:11:18Z

Some history: The Zip TrustedRandomAccess specialization has tried to emulate the side-effects of the naive implementation for a long time, including backwards iteration. #82292 tried to fix unsoundness (#82291) in that side-effect-preservation code, but this introduced some panic-safety unsoundness (#86443), but the fix #86452 didn't fix it for nested Zip iterators (#137255).

Rather than piling yet another fix ontop of this heap of fixes this PR reduces the number of cases in which side-effects will be preserved; the necessary API guarantee change was approved in #83791 but we haven't made use of that so far.

fixes #137255

rustbot · 2025-05-16T10:11:23Z

r? @workingjubilee

rustbot has assigned @workingjubilee.
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

the8472 · 2025-05-16T10:11:37Z

CC @steffahn

workingjubilee · 2025-05-17T05:14:06Z

exciting!

workingjubilee

Could you either commit these comments or, if they're wrong (and that seems reasonably likely), correct them? The description of the issue so far is... a bit in-the-weeds. Tests are a valuable opportunity to spend time describing the actual issues we are trying to avoid.

workingjubilee · 2025-06-03T23:30:39Z

library/coretests/tests/iter/adapters/zip.rs

+    });
+    let b = [1, 2, 3, 4].as_slice().iter().copied();
+    let c = [5, 6, 7].as_slice().iter().copied();
+    let ab = zip(a, b);


Suggested change

let ab = zip(a, b);

// The actual case we are testing: this Zip gets nested in the next, and

// `a` forces a premature unwind from Zip's iterator-driving code.

let ab = zip(a, b);

library/coretests/tests/iter/adapters/zip.rs

Some history: The Zip TrustedRandomAccess specialization has tried to emulate the side-effects of the naive implementation for a long time, including backwards iteration. 82292¹ tried to fix unsoundness (82291¹) in that side-effect-preservation code, but this introduced some panic-safety unsoundness (86443¹), but the fix 86452¹ didn't fix it for nested Zip iterators (137255¹). Rather than piling yet another fix ontop of this heap of fixes this PR reduces the number of cases in which side-effects will be preserved; the necessary API guarantee change was approved in 83791¹ but we haven't made use of that so far. ¹ see merge commit for linkfied issues.

the8472 · 2025-06-04T23:30:23Z

@rustbot ready

The issue is complicated, so I don't think those few suggested comments would a good job, so I tried to add some more instead.

the8472 · 2025-07-15T22:07:18Z

@workingjubilee will you take another look at it or should I reroll?

workingjubilee · 2025-07-17T06:48:27Z

library/core/src/iter/adapters/zip.rs

+        // cases the unsafe code has to handle.
+        // See #137255 for a case where where too many epicycles lead to unsoundness.
+        if self.index < self.len {
+            let old_len = self.len;


We record the original length we are working with...

workingjubilee · 2025-07-17T06:48:29Z

library/core/src/iter/adapters/zip.rs

+            // which can panic we decrement the counter beforehand
+            // so that the same index won't be accessed twice, as required by TrustedRandomAccess.
+            // Additionally this will ensure that the side-effects code won't run a second time.
+            self.len -= 1;


So now, if we somehow arrive in this code again, old_len will be the lower self.len value, preventing the rerun...

workingjubilee · 2025-07-17T07:02:20Z

library/core/src/iter/adapters/zip.rs

+                // This condition can and must only be true on the first `next_back` call,
+                // otherwise we will break the restriction on calls to `self.next_back()`
+                // after calling `get_unchecked()`.
+                if sz_a != sz_b && (old_len == sz_a || old_len == sz_b) {
+                    if A::MAY_HAVE_SIDE_EFFECT && sz_a > old_len {
+                        for _ in 0..sz_a - old_len {
+                            self.a.next_back();
+                        }
                    }
+                    if B::MAY_HAVE_SIDE_EFFECT && sz_b > old_len {
+                        for _ in 0..sz_b - old_len {
+                            self.b.next_back();
+                        }
+                    }
+                    debug_assert_eq!(self.a.size(), self.b.size());


So we're calling next_back a bunch of times just to run side-effects of the closure, which becomes observable. Right?

It feels kinda weird that we potentially next_back a bunch of times here. This allows using next_back to force running side effects that would not be encountered if we only drove using next, right?

The regular, non-TrustedRandomAccess-specialized next_back naturally performs this tail-trimming because it needs to bring the two inner iterators down to the same length (known through ExactSizeIterator) before it can pop off a tail element at the same index.

It's the side-effects of the trimming that we try to emulate here because otherwise it'd look like we're jumping into the middle of the iterator.

Personally I'd prefer if we just did the jumping, but some people like their cute liddle closure side-effects and are sad if they don't get to run.

Hm. It's not obvious to me that this sort of behavior is guaranteed or even expected according to the contract of core::iter::zip. Nonetheless, I suppose this is fine for now, since this is the current behavior.

There's just no stable way skip in the middle of an iterator and elide its side-effects. So people might try to reason from its public interface as-if things were implemented by next().

workingjubilee · 2025-07-17T07:03:21Z

library/core/src/iter/adapters/zip.rs

+                if sz_a != sz_b && (old_len == sz_a || old_len == sz_b) {
+                    if A::MAY_HAVE_SIDE_EFFECT && sz_a > old_len {
+                        for _ in 0..sz_a - old_len {
+                            self.a.next_back();
+                        }
                    }
+                    if B::MAY_HAVE_SIDE_EFFECT && sz_b > old_len {
+                        for _ in 0..sz_b - old_len {


The subtlety here that we are catching is that the len of the zip-iter (zipter?) is the lesser of the two sizes.

It's hopefully less surprising if one starts with the regular implementation and only then looks at the specialized one.

you mean the default fn implementation?

workingjubilee · 2025-07-18T18:59:16Z

Alright, sorry for taking so long to review this. It's quite painful to reason about. I'm satisfied with it.

@bors r+

bors · 2025-07-18T18:59:19Z

📌 Commit 61c2c12 has been approved by workingjubilee

It is now in the queue for this repository.

Rollup of 10 pull requests Successful merges: - #141076 (fix Zip unsoundness (again)) - #142444 (adding run-make test to autodiff) - #143704 (Be a bit more careful around exotic cycles in in the inliner) - #144073 (Don't test panic=unwind in panic_main.rs on Fuchsia) - #144083 (miri sleep tests: increase slack) - #144092 (bootstrap: Detect musl hosts) - #144098 (Do not lint private-in-public for RPITIT) - #144103 (Rename `emit_unless` to `emit_unless_delay`) - #144108 (Ignore tests/run-make/link-eh-frame-terminator/rmake.rs when cross-compiling) - #144115 (fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #141076 - the8472:fix-zip-panic-safety2, r=workingjubilee fix Zip unsoundness (again) Some history: The Zip TrustedRandomAccess specialization has tried to emulate the side-effects of the naive implementation for a long time, including backwards iteration. #82292 tried to fix unsoundness (#82291) in that side-effect-preservation code, but this introduced some panic-safety unsoundness (#86443), but the fix #86452 didn't fix it for nested Zip iterators (#137255). Rather than piling yet another fix ontop of this heap of fixes this PR reduces the number of cases in which side-effects will be preserved; the necessary API guarantee change was approved in #83791 but we haven't made use of that so far. fixes #137255

rustbot assigned workingjubilee May 16, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 16, 2025

This comment has been minimized.

Sign in to view

the8472 force-pushed the fix-zip-panic-safety2 branch from 1f7a9f5 to 41aca57 Compare May 16, 2025 12:20

workingjubilee requested changes Jun 3, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2025

the8472 force-pushed the fix-zip-panic-safety2 branch from 41aca57 to ec51514 Compare June 4, 2025 23:25

This comment has been minimized.

Sign in to view

the8472 added 2 commits June 5, 2025 01:27

add comments to Zip unsoundness regression test

61c2c12

the8472 force-pushed the fix-zip-panic-safety2 branch from ec51514 to 61c2c12 Compare June 4, 2025 23:29

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2025

the8472 requested a review from workingjubilee June 4, 2025 23:30

workingjubilee reviewed Jul 17, 2025

View reviewed changes

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 Jul 18, 2025

matthiaskrgr mentioned this pull request Jul 19, 2025

Rollup of 10 pull requests #144166

Merged

bors merged commit fff53f1 into rust-lang:master Jul 19, 2025
10 checks passed

rustbot added this to the 1.90.0 milestone Jul 19, 2025

fix Zip unsoundness (again) #141076

fix Zip unsoundness (again) #141076

Conversation

the8472 commented May 16, 2025

Uh oh!

rustbot commented May 16, 2025

Uh oh!

the8472 commented May 16, 2025

Uh oh!

This comment has been minimized.

workingjubilee commented May 17, 2025

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

the8472 commented Jun 4, 2025

Uh oh!

the8472 commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the8472 Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Jul 18, 2025

Uh oh!

bors commented Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

the8472 Jul 17, 2025 •

edited

Loading