numtoa icon indicating copy to clipboard operation
numtoa copied to clipboard

Buffer size check is too aggressive

Open dtolnay opened this issue 8 years ago • 6 comments

If I know for sure that I have numbers in the range 0 .. 100_000_000, I want to be able to use an 8-byte buffer rather than needing 20 bytes in debug mode. Maybe an unsafe way to skip the check? This is why https://github.com/mmstick/numtoa/issues/5#issuecomment-274347968 panics in debug mode even though the code is correct.

dtolnay avatar Jan 22 '17 18:01 dtolnay

I could probably make an _unchecked variant that doesn't have the checks in debug mode and have that denoted with the unsafe keyword with a note that it's basically identical to the checked function, minus the debug assertion checks on debug mode, so inherently less safe.

mmstick avatar Jan 22 '17 18:01 mmstick

That would work for me. It's not actually unsafe right? All that the implementation is doing is indexing into the buffer which is checked even in release mode.

extern crate numtoa;
use numtoa::NumToA;

fn main() {
    let mut buf = [0u8; 0];
    0.numtoa(10, &mut buf);
}
$ cargo run --release
    Finished release [optimized] target(s) in 0.0 secs
     Running `target/release/testing`
thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 18446744073709551615', /home/david/.cargo/registry/src/github.com-1ecc6299db9ec823/numtoa-0.0.6/src/lib.rs:217

dtolnay avatar Jan 22 '17 18:01 dtolnay

The index value wraps around so you will have 'out of bounds' access errors if your supplied buffer was too small for the output, which does panic in release mode, unless I were to switch to unchecked indexing and it then becomes truly unsafe.

mmstick avatar Jan 22 '17 18:01 mmstick

Cool so it sounds like both the checked and unchecked versions are 100% safe as long as usize is more than 4 bits (at which point it would wrap before 20 characters), and the checked version will give better error messages but is slower and requires an oversized buffer in many cases.

dtolnay avatar Jan 22 '17 19:01 dtolnay

If I were to use the unsafe keyword to perform get_unchecked_mut and get_unchecked of slices of the array and lookup tables though, it could be theoretically make it faster. Then the debug checks would be vital to ensure the safety of the underlying unsafe actions.

mmstick avatar Jan 22 '17 19:01 mmstick

shouldn't the asserts speed the code up? https://gist.github.com/jFransham/369a86eff00e5f280ed25121454acec1#assert-conditions-beforehand

edit: ok.. ok... na, this can only work for statically known indices.

chpio avatar Jun 01 '17 22:06 chpio