Skip to content

Panic safety issue in Zip::next_back() TrustedRandomAccess specialization #86443

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Qwaz opened this issue Jun 18, 2021 · 3 comments · Fixed by #86452
Closed

Panic safety issue in Zip::next_back() TrustedRandomAccess specialization #86443

Qwaz opened this issue Jun 18, 2021 · 3 comments · Fixed by #86452
Assignees
Labels
A-iterators Area: Iterators C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Qwaz
Copy link
Contributor

Qwaz commented Jun 18, 2021

if A::MAY_HAVE_SIDE_EFFECT && sz_a > self.len {
for _ in 0..sz_a - self.len {
self.a.next_back();
}
self.a_len = self.len;
}

} else if A::MAY_HAVE_SIDE_EFFECT && self.index < self.a_len {
let i = self.index;
self.index += 1;
self.len += 1;
// match the base implementation's potential side effects
// SAFETY: we just checked that `i` < `self.a.len()`
unsafe {
self.a.__iterator_get_unchecked(i);
}
None

Yet another soundness bug in Zip's TRA specialization. Line 300 is not called when line 298 panics. This leaves self.a_len outdated, which results in calling __iterator_get_unchecked() with an invalid index in line 242.

Here is a playground link that demonstrates creating two mutable references to the same memory location without unsafe code.

@Qwaz Qwaz added the C-bug Category: This is a bug. label Jun 18, 2021
@jonas-schievink jonas-schievink added A-iterators Area: Iterators I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 18, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 18, 2021
@apiraino
Copy link
Contributor

How close is this to #85873?

@the8472
Copy link
Member

the8472 commented Jun 18, 2021

The same functions are involved but the aspects that interact to cause unsafety are quite different.

@rustbot claim

@Qwaz
Copy link
Contributor Author

Qwaz commented Jun 18, 2021

I agree, this is much closer to #81740.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 21, 2021
fix panic-safety in specialized Zip::next_back

This was unsound since a panic in a.next_back() would result in the
length not being updated which would then lead to the same element
being revisited in the side-effect preserving code.

fixes rust-lang#86443
@bors bors closed this as completed in 504c378 Jun 21, 2021
@JohnTitor JohnTitor removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 21, 2021
the8472 added a commit to the8472/rust that referenced this issue May 16, 2025
Some history: The Zip TrustedRandomAccess specialization has tried
to emulate the side-effects of the naive implementation for a long time,
including backwards iteration. rust-lang#82292 tried to fix unsoundness (rust-lang#82291) in that
side-effect-preservation code, but this introduced some panic-safety
unsoundness (rust-lang#86443), but the fix rust-lang#86452 didn't fix it for nested Zip
iterators (rust-lang#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 rust-lang#83791 but we haven't made use of that
so far.
the8472 added a commit to the8472/rust that referenced this issue May 16, 2025
Some history: The Zip TrustedRandomAccess specialization has tried
to emulate the side-effects of the naive implementation for a long time,
including backwards iteration. rust-lang#82292 tried to fix unsoundness (rust-lang#82291) in that
side-effect-preservation code, but this introduced some panic-safety
unsoundness (rust-lang#86443), but the fix rust-lang#86452 didn't fix it for nested Zip
iterators (rust-lang#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 rust-lang#83791 but we haven't made use of that
so far.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-iterators Area: Iterators C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants