cmpute / dashu

A library set of arbitrary precision numbers implemented in Rust.
Apache License 2.0
74 stars 9 forks source link

Fix buffer overflow from off-by-one error #26

Closed saethlin closed 1 year ago

saethlin commented 1 year ago

The original code here reads one byte out-of-bounds because it does the read first and the bounds check second. This is caught by AddressSanitizer. (which IMO should be used through cargo-careful, because the flags to get doctests to work are annoying).

There are multiple ways to prevent the out-of-bounds read, but this code also runs afoul of the requirements of pointer::sub: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.sub because it does the decrement of self.len to 0 and the decrement of the pointer on the same iteration. If self.len == 0, then tail_ptr = self.ptr - 1, which is not in-bounds of the allocated object or one past the end.

There are other ways to the ptr::sub issue as well, such as using wrapping_sub. The test I'm adding here, if run under Miri (rustup component add miri, cargo +nightly miri test pop_all_zeros) should catch any subtly-incorrect attempt to fix the underlying problem. Or at least, it catches all of my incorrect attempts at fixing the problem.

Please feel free to substitute or request whatever alternative soundness fix for this function you'd prefer.

cmpute commented 1 year ago

Thanks for the fix! I think the current fix is good.

BTW I haven't tried cargo-careful before, I may try it and see how it can be integrated with the CI.