[BUG] Unsoundness: try_parse_{4,8}digits appear to advance iterators out of bounds

[Ralith]

BytesIter::read is documented as advancing the iterator's state:

rust-lexical/lexical-util/src/iterator.rs

Lines 123 to 125 in 09c686b

	/// Try to read a value of a different type from the iterator.
	/// This advances the internal state of the iterator.
	fn read<V>(&self) -> Option<V>;

However, the try_parse_4digits and try_parse_8digits methods advance the iterator's state manually after reading, without any other apparent length checks:

rust-lexical/lexical-parse-integer/src/algorithm.rs

Lines 220 to 223 in 09c686b

	let bytes = u32::from_le(iter.read::<u32>()?);
	if is_4digits::<FORMAT>(bytes) {
	// SAFETY: safe since we have at least 4 bytes in the buffer.
	unsafe { iter.step_by_unchecked(4) };

rust-lexical/lexical-parse-integer/src/algorithm.rs

Lines 293 to 295 in 09c686b

	if is_8digits::<FORMAT>(bytes) {
	// SAFETY: safe since we have at least 8 bytes in the buffer.
	unsafe { iter.step_by_unchecked(8) };

This is unsound according to the docs of step_by_unchecked:

rust-lexical/lexical-util/src/iterator.rs

Lines 131 to 133 in 09c686b

	/// As long as the iterator is at least `N` elements, this
	/// is safe.
	unsafe fn step_by_unchecked(&mut self, count: usize);

Skimming around for actual implementations, it appears that read may not actually advance the state as advertised, which could explain how this has gone unnoticed so far. Note the absence of any mutation of self.index, contrary to the doc comment:

rust-lexical/lexical-util/src/noskip.rs

Lines 102 to 118 in 09c686b

	/// Read a value of a difference type from the iterator.
	/// This advances the internal state of the iterator.
	///
	/// # Safety
	///
	/// Safe as long as the number of the buffer is contains as least as
	/// many bytes as the size of V.
	#[inline]
	#[allow(clippy::assertions_on_constants)]
	pub unsafe fn read_unchecked<V>(&self) -> V {
	debug_assert!(Self::IS_CONTIGUOUS);
	debug_assert!(self.as_slice().len() >= mem::size_of::<V>());
	let slc = self.as_slice();
	// SAFETY: safe as long as the slice has at least count elements.
	unsafe { ptr::read_unaligned::<V>(slc.as_ptr() as *const _) }
	}

[Alexhuszagh]

This was resolved as of commit #103. If I'm wrong about this, please re-open this or create another issue. All checks now should be done without any debug asserts, using:

    /// Try to read a the next four bytes as a u32.
    /// This advances the internal state of the iterator.
    #[inline(always)]
    pub fn read_u32(&self) -> Option<u32> {
        if Self::IS_CONTIGUOUS && self.as_slice().len() >= mem::size_of::<u32>() {
            // SAFETY: safe since we've guaranteed the buffer is greater than
            // the number of elements read. u32 is valid for all bit patterns
            unsafe { Some(self.read_unchecked()) }
        } else {
            None
        }
    }

    /// Try to read the next eight bytes as a u64
    /// This advances the internal state of the iterator.
    #[inline(always)]
    pub fn read_u64(&self) -> Option<u64> {
        if Self::IS_CONTIGUOUS && self.as_slice().len() >= mem::size_of::<u64>() {
            // SAFETY: safe since we've guaranteed the buffer is greater than
            // the number of elements read. u64 is valid for all bit patterns
            unsafe { Some(self.read_unchecked()) }
        } else {
            None
        }
    }