Potential unsoundnesses (not yet determined) with use of unsafe

[Manishearth]

Hi!

In the process of unsafe-reviewing this crate, we discovered some issues. It's as of yet unclear if there is any unsoundness here, however the invariants are sufficiently nonlocal that it warrants further review (which I may not have time for at the moment)

Most of these are of the form "unsafe precondition checked by debug assertion; and may be exposed to the user through many layers of API"

The first one is this code:

fast-float-rust/src/binary.rs

Lines 75 to 85 in 83a49b8

	debug_assert!(q >= SMALLEST_POWER_OF_FIVE as i64);
	debug_assert!(q <= LARGEST_POWER_OF_FIVE as i64);
	debug_assert!(precision <= 64);
	let mask = if precision < 64 {
	0xFFFF_FFFF_FFFF_FFFF_u64 >> precision
	} else {
	0xFFFF_FFFF_FFFF_FFFF_u64
	};
	let index = (q - SMALLEST_POWER_OF_FIVE as i64) as usize;
	let (lo5, hi5) = unsafe { *POWER_OF_FIVE_128.get_unchecked(index) };

The safety invariants are upheld by a debug assertion, but it's not guarded in release mode. Ultimately, the q value comes from a parsed exponent, which seems quite sketchy: you really do not want safety invariants relying on outputs of your parsers: indeed: this is one of the most common ways for there to be exploitable undefined behavior in C code.

fast-float-rust/src/common.rs

Lines 22 to 23 in 83a49b8

	pub fn step_by(&mut self, n: usize) -> &mut Self {
	unsafe { self.ptr = self.ptr.add(n) };

This doesn't do any bounds checking. It's possible the many uses of step_by() and step() in this codebase are correct, but it will take time to verify.

There are a couple ways to use typestate to write munchers that can avoid duplicating bounds checks here so that the code can be written with the same performance. Happy to talk about that in more depth.

fast-float-rust/src/common.rs

Line 39 in 83a49b8

unsafe { *self.ptr }

This also doesn't check invariants. Unclear if upheld at the call site.

fast-float-rust/src/common.rs

Lines 158 to 169 in 83a49b8

	fn read_u64(&self) -> u64 {
	debug_assert!(self.as_ref().len() >= 8);
	let src = self.as_ref().as_ptr() as *const u64;
	u64::from_le(unsafe { ptr::read_unaligned(src) })
	}
	#[inline]
	fn write_u64(&mut self, value: u64) {
	debug_assert!(self.as_ref().len() >= 8);
	let dst = self.as_mut().as_mut_ptr() as *mut u64;
	unsafe { ptr::write_unaligned(dst, u64::to_le(value)) };
	}

Only checks invariants in debug mode. Unclear if upheld at the call sites.