Skip to content

Prevent specialized ZipImpl from calling __iterator_get_unchecked twice with the same index #82292

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
Merged
merged 2 commits into from
Mar 7, 2021
Merged

Prevent specialized ZipImpl from calling __iterator_get_unchecked twice with the same index #82292

merged 2 commits into from
Mar 7, 2021

Conversation

SkiFire13
Copy link
Contributor

Fixes #82291

It's open for review, but conflicts with #82289, wait before merging. The conflict involves only the new test, so it should be rather trivial to fix.

@rust-highfive
Copy link
Contributor

r? @m-ou-se

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2021
@SkiFire13 SkiFire13 mentioned this pull request Feb 19, 2021
Merged
@m-ou-se m-ou-se added the A-iterators Area: Iterators label Mar 3, 2021
m-ou-se
m-ou-se reviewed Mar 3, 2021
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you resolve the merge conflict?

@m-ou-se m-ou-se 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 Mar 3, 2021
@SkiFire13
Copy link
Contributor Author

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

@rustbot 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 Mar 3, 2021
@SkiFire13
Copy link
Contributor Author

I marked this PR as "waiting-on-review" but I forgot that it conflicts with #82289, so this should wait until #82289 is fully merged and this is rebased.

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

@rustbot 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 Mar 4, 2021
@m-ou-se m-ou-se added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2021
@bors
Copy link
Collaborator

bors commented Mar 5, 2021

☔ The latest upstream changes (presumably #82795) made this pull request unmergeable. Please resolve the merge conflicts.

@SkiFire13
Copy link
Contributor Author

Merge conflict solved, this should be ready for review/merge now. TIL there's an S-blocked label.

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 5, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Mar 5, 2021

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

📌 Commit c1bfb9a has been approved by m-ou-se

@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 5, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 6, 2021
@JohnTitor
Rollup merge of rust-lang#82292 - SkiFire13:fix-issue-82291, r=m-ou-se
fbdc900 
Prevent specialized ZipImpl from calling `__iterator_get_unchecked` twice with the same index

Fixes rust-lang#82291

It's open for review, but conflicts with rust-lang#82289, wait before merging. The conflict involves only the new test, so it should be rather trivial to fix.
@JohnTitor JohnTitor mentioned this pull request Mar 6, 2021
Closed
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 6, 2021
@JohnTitor
Rollup merge of rust-lang#82292 - SkiFire13:fix-issue-82291, r=m-ou-se
3242730 
Prevent specialized ZipImpl from calling `__iterator_get_unchecked` twice with the same index

Fixes rust-lang#82291

It's open for review, but conflicts with rust-lang#82289, wait before merging. The conflict involves only the new test, so it should be rather trivial to fix.
@JohnTitor JohnTitor mentioned this pull request Mar 6, 2021
Closed
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 6, 2021
@m-ou-se
Rollup merge of rust-lang#82292 - SkiFire13:fix-issue-82291, r=m-ou-se
5e91bab 
Prevent specialized ZipImpl from calling `__iterator_get_unchecked` twice with the same index

Fixes rust-lang#82291

It's open for review, but conflicts with rust-lang#82289, wait before merging. The conflict involves only the new test, so it should be rather trivial to fix.
@m-ou-se m-ou-se mentioned this pull request Mar 6, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 6, 2021
@Dylan-DPC
Rollup merge of rust-lang#82292 - SkiFire13:fix-issue-82291, r=m-ou-se
4c7e3ce 
Prevent specialized ZipImpl from calling `__iterator_get_unchecked` twice with the same index

Fixes rust-lang#82291

It's open for review, but conflicts with rust-lang#82289, wait before merging. The conflict involves only the new test, so it should be rather trivial to fix.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 6, 2021
Closed
@JohnTitor JohnTitor mentioned this pull request Mar 7, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2021
@bors
Auto merge of rust-lang#82851 - JohnTitor:rollup-me5ko8g, r=JohnTitor
66ec64c 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#77916 (Change built-in kernel targets to be os = none throughout)
 - rust-lang#82130 (Make some Option, Result methods unstably const)
 - rust-lang#82292 (Prevent specialized ZipImpl from calling `__iterator_get_unchecked` twice with the same index)
 - rust-lang#82402 (Remove RefCell around `module_trait_cache`)
 - rust-lang#82592 (Improve transmute docs with further clarifications)
 - rust-lang#82651 (Cleanup rustdoc warnings)
 - rust-lang#82720 (Fix diagnostic suggests adding type `[type error]`)
 - rust-lang#82751 (improve offset_from docs)
 - rust-lang#82793 (Move some tests to more suitable subdirs)
 - rust-lang#82803 (rustdoc: Add an unstable option to print all unversioned files)
 - rust-lang#82808 (Sync rustc_codegen_cranelift)
 - rust-lang#82822 (Fix typo)
 - rust-lang#82837 (tweak MaybeUninit docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1d5b2dc into rust-lang:master Mar 7, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 7, 2021
@SkiFire13 SkiFire13 deleted the fix-issue-82291 branch March 7, 2021 10:12
@the8472 the8472 mentioned this pull request Mar 12, 2025
Open
the8472 added a commit to the8472/rust that referenced this pull request May 16, 2025
@the8472
fix Zip unsoundness (again)
1f7a9f5 
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 the8472 mentioned this pull request May 16, 2025
Open
the8472 added a commit to the8472/rust that referenced this pull request May 16, 2025
@the8472
fix Zip unsoundness (again)
41aca57 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

Assignees

@m-ou-se m-ou-se

Labels
A-iterators Area: Iterators 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.52.0
Development

Successfully merging this pull request may close these issues.

Zip may call __iterator_get_unchecked twice with the same index
5 participants
@SkiFire13 @rust-highfive @bors @m-ou-se @rustbot