crc32c icon indicating copy to clipboard operation
crc32c copied to clipboard

UB when `util::split` returns zero-length slices

Open nissaofthesea opened this issue 2 years ago • 1 comments

util::split documents the following invariants of the returned slices: https://github.com/zowens/crc32c/blob/47c2999907a770daec5444cf9fcb522926d8cfe4/src/util.rs#L3-L6

however, mid doesn't seem to uphold these.

adding the following to the function before it returns (start, mid, end) results in failed assertions.

// manual implementation of unstable `pointer::is_aligned_to`
#[cfg(debug_assertions)]
fn is_aligned_to<T>(ptr: *const T, align: usize) -> bool {
    assert!(align.is_power_of_two());
    ptr as usize % align == 0
}

/* `mid` invariants */
debug_assert!(is_aligned_to(mid.as_ptr(), 8)); // 8-byte aligned
debug_assert!(mid.len() % 8 == 0); // len is multiple of 8

/* `end` invariants */
debug_assert!(is_aligned_to(end.as_ptr(), 8)); // 8-byte aligned
debug_assert!(end.len() < 8); // len is less than 8
$ cargo test
<cut>
running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/simple.rs (target/debug/deps/simple-561134e7e9ea1232)

running 6 tests
test crc ... ok
test crc_append ... ok
test long_string ... FAILED
test crc_combine ... FAILED
test very_small ... FAILED
test very_big ... ok

failures:

---- long_string stdout ----
thread 'long_string' panicked at 'assertion failed: mid.len() % 8 == 0', src/util.rs:48:5

---- crc_combine stdout ----
thread 'crc_combine' panicked at 'assertion failed: is_aligned_to(mid.as_ptr(), 8)', src/util.rs:47:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- very_small stdout ----
thread 'very_small' panicked at 'assertion failed: is_aligned_to(mid.as_ptr(), 8)', src/util.rs:47:5


failures:
    crc_combine
    long_string
    very_small

test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass '--test simple'

if there aren't any issues with my tests, then this may be unsound, since we're creating a &[u64] with unaligned elements.

EDIT: my tests were a little messed up but they're fixed now, assertions still fail

nissaofthesea avatar Oct 23 '22 01:10 nissaofthesea

after some testing, this is only an issue with small input buffers (like in test very_small or crc_combine (which has zero-length slices)). while these invariants may not be upheld when some returned buffers have a length of zero, that could still be okay in the sense of correctness. docs could be changed accordingly, that these invariants are only upheld if the buffer length is nonzero.

however, ive realized that this implementation currently exhibits undefined behavior when calling slice::from_raw_parts for mid.

data must be non-null and aligned even for zero-length slices. One reason for this is that enum layout optimizations may rely on references (including slices of any length) being aligned and non-null to distinguish them from other data. You can obtain a pointer that is usable as data for zero-length slices using NonNull::dangling().

so if split uses NonNull::<u64>::dangling().as_ptr() for mid's pointer if the length is zero, it should be fine.

nissaofthesea avatar Nov 04 '22 03:11 nissaofthesea