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 underflow in specialized ZipImpl::size_hint #82289

Merged
merged 3 commits into from
Mar 5, 2021
Merged

Fix underflow in specialized ZipImpl::size_hint #82289

merged 3 commits into from
Mar 5, 2021

Conversation

SkiFire13
Copy link
Contributor

Fixes #82282

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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
@SkiFire13
Copy link
Contributor Author

@kennytm is that supposed to be the same as an r+? Sorry, but this is my first time seeing that in this repository

@Qwaz
Copy link
Contributor

Qwaz commented Feb 19, 2021

I'm not sure if this fix interacts well with next_back()'s length handling. Say we are using iterators with length 1 for both A and B.

  1. Initialize zip
  • index = 0, len = 1
  1. Call next_back()
  • index = 0, len = 0
  • get_unchecked(0) is called on both A and B
  1. Call next()
  • Since self.index < self.a.size(), get_unchecked(0) is called again on A
  • index = 1, len = 1

Calling get_unchecked() on the same index twice is a violation of the second safety invariant of TrustedRandomAccess and can lead to duplicated elements.

@Qwaz
Copy link
Contributor

Qwaz commented Feb 19, 2021

Oh, was this already possible even before the fix?

@SkiFire13
Copy link
Contributor Author

Oh, was this already possible even before the fix?

Yes, it was and this PR wasn't aimed to fix it. I've already opened #82291 a couple of hours ago showing how it can be exploited and #82292 to fix it, unfortunately it conflicts with this PR (both add a test at the end of the same file).

@bluss
Copy link
Member

bluss commented Feb 20, 2021 •
edited
Loading

It's worth considering if the fix that moved the increment of index can be reverted.

Alternatively - would it be better to rewrite the whole zip specialization in terms of a "next_unchecked" method instead, and would that be more robust?

@SkiFire13
Copy link
Contributor Author

It's worth considering if the fix that moved the increment of index can be reverted.

What does that have to do with the bug this PR should fix?

Alternatively - would it be better to rewrite the whole zip specialization in terms of a "next_unchecked" method instead, and would that be more robust?

A discussion about that started here, then we moved on zulip (here). Anyway I thought you already ruled out a next_unchecked approach because LLVM had more trouble to optimize it (and I think it still has).

@bluss
Copy link
Member

bluss commented Feb 20, 2021

What does that have to do with the bug this PR should fix?

Hm, oh I thought this issue was directly caused by that, but on new reading you're right, it's not related, sorry about that.

Anything from 2016 could of course be reevaluated 🙂

@sdroege sdroege mentioned this pull request Mar 1, 2021
Merged
@m-ou-se m-ou-se added A-iterators Area: Iterators T-libs Relevant to the library team, which will review and decide on the PR/issue. 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
@m-ou-se
Copy link
Member

m-ou-se commented Mar 3, 2021

@SkiFire13 Can you resolve the merge conflict?

@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
@m-ou-se
Copy link
Member

m-ou-se commented Mar 4, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2021

📌 Commit aeb4ea7 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 4, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 5, 2021
@m-ou-se
Rollup merge of rust-lang#82289 - SkiFire13:fix-issue-82282, r=m-ou-se
1533a5a 
Fix underflow in specialized ZipImpl::size_hint

Fixes rust-lang#82282
This was referenced Mar 5, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2021
@bors
Auto merge of rust-lang#82795 - m-ou-se:rollup-uzx0b92, r=m-ou-se
8fd946c 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#80723 (Implement NOOP_METHOD_CALL lint)
 - rust-lang#80763 (resolve: Reduce scope of `pub_use_of_private_extern_crate` deprecation lint)
 - rust-lang#81136 (Improved IO Bytes Size Hint)
 - rust-lang#81939 (Add suggestion `.collect()` for iterators in iterators)
 - rust-lang#82289 (Fix underflow in specialized ZipImpl::size_hint)
 - rust-lang#82728 (Avoid unnecessary Vec construction in BufReader)
 - rust-lang#82764 (Add {BTreeMap,HashMap}::try_insert)
 - rust-lang#82770 (Add assert_matches macro.)
 - rust-lang#82773 (Add diagnostic item to `Default` trait)
 - rust-lang#82787 (Remove unused code from main.js)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ee796c6 into rust-lang:master Mar 5, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 5, 2021
@SkiFire13 SkiFire13 deleted the fix-issue-82282 branch March 5, 2021 18:00
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 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.
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.
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.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 7, 2021
@JohnTitor
Rollup merge of rust-lang#82292 - SkiFire13:fix-issue-82291, r=m-ou-se
1d5b2dc 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdroege sdroege sdroege left review comments

@kennytm kennytm kennytm approved these changes

Assignees

@kennytm kennytm

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

Side effect handling in specialized zip implementation causes buffer overflow
9 participants
@SkiFire13 @rust-highfive @Qwaz @bluss @m-ou-se @bors @kennytm @sdroege @rustbot