Soundness issue in Zip::next() specialization

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

Lines 191 to 211 in e708cbd

    
           #[inline] 
        
           fn next(&mut self) -> Option<(A::Item, B::Item)> { 
        
               if self.index < self.len { 
        
                   let i = self.index; 
        
                   self.index += 1; 
        
                   // SAFETY: `i` is smaller than `self.len`, thus smaller than `self.a.len()` and `self.b.len()` 
        
                   unsafe { 
        
                       Some((self.a.__iterator_get_unchecked(i), self.b.__iterator_get_unchecked(i))) 
        
                   } 
        
               } else if A::may_have_side_effect() && self.index < self.a.size() { 
        
                   // match the base implementation's potential side effects 
        
                   // SAFETY: we just checked that `self.index` < `self.a.len()` 
        
                   unsafe { 
        
                       self.a.__iterator_get_unchecked(self.index); 
        
                   } 
        
                   self.index += 1; 
        
                   None 
        
               } else { 
        
                   None 
        
               } 
        
           }

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

Lines 395 to 396 in e708cbd

    
           /// 2. If `self: !Clone`, then `get_unchecked` is never called with the same 
        
           ///    index on `self` more than once.

There is a panic safety issue in Zip::next() that allows to call __iterator_get_unchecked() to the same index twice. __iterator_get_unchecked() is called at line 204 and the index is updated at line 206. If line 204 panics, the index is not updated and the subsequent next() call will use the same index for __iterator_get_unchecked(). This violates the second safety requirement of TrustedRandomAccess.

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

The bug fixed by this PR seems to have the same consequence, but it didn't get the unsound label.
#80670

Seems like this would be fixed by simply doing the same as in the first branch of the if and incrementing the self.index beforehand. Are you creating a PR for this?

Seems like this would be fixed by simply doing the same as in the first branch of the if and incrementing the self.index beforehand.

Agreed :)

Are you creating a PR for this?

Not in the meantime. Fill free to work on a PR if you are interested.

Not in the meantime. Fill free to work on a PR if you are interested.

Sure, why not :)

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

Rollup merge of rust-lang#81741 - sdroege:zip-trusted-random-access-s…

Navigation Menu

Repository navigation

Soundness issue in `Zip::next()` specialization #81740

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	#[inline]
	fn next(&mut self) -> Option<(A::Item, B::Item)> {
	if self.index < self.len {
	let i = self.index;
	self.index += 1;
	// SAFETY: `i` is smaller than `self.len`, thus smaller than `self.a.len()` and `self.b.len()`
	unsafe {
	Some((self.a.__iterator_get_unchecked(i), self.b.__iterator_get_unchecked(i)))
	}
	} else if A::may_have_side_effect() && self.index < self.a.size() {
	// match the base implementation's potential side effects
	// SAFETY: we just checked that `self.index` < `self.a.len()`
	unsafe {
	self.a.__iterator_get_unchecked(self.index);
	}
	self.index += 1;
	None
	} else {
	None
	}
	}

	/// 2. If `self: !Clone`, then `get_unchecked` is never called with the same
	/// index on `self` more than once.

Soundness issue in Zip::next() specialization #81740

Description

Activity

qwaz commented on Feb 4, 2021

sdroege commented on Feb 4, 2021

qwaz commented on Feb 4, 2021

sdroege commented on Feb 4, 2021

apiraino commented on Feb 5, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Soundness issue in `Zip::next()` specialization #81740