capnproto / capnproto-rust

Cap'n Proto for Rust
MIT License
2.06k stars 222 forks source link

Fix possible undefined behavior in primitive list as_slice #496

Closed as-com closed 6 months ago

as-com commented 6 months ago

At least on my machine, calling .as_slice() on an empty primitive list with elements u16 or larger in Rust nightly in debug mode results in a panic like this:

thread 'primitive_list_as_slice' panicked at library\core\src\panicking.rs:219:5:
unsafe precondition(s) violated: slice::from_raw_parts_mut requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`
stack backtrace:
   0:     0x7ff759add69d - std::backtrace_rs::backtrace::dbghelp64::trace
                               at /rustc/e82c861d7e5ecd766cb0dab0bf622445dec999dc/library\std\src\..\..\backtrace\src\backtrace\dbghelp64.rs:91
[...]
  19:     0x7ff759b04be3 - core::panicking::panic_nounwind
                               at /rustc/e82c861d7e5ecd766cb0dab0bf622445dec999dc/library\core\src\panicking.rs:219
  20:     0x7ff759a77259 - core::slice::raw::from_raw_parts_mut::precondition_check
                               at /rustc/e82c861d7e5ecd766cb0dab0bf622445dec999dc\library\core\src\ub_checks.rs:68
  21:     0x7ff759a78c50 - core::slice::raw::from_raw_parts_mut<u16>
                               at /rustc/e82c861d7e5ecd766cb0dab0bf622445dec999dc\library\core\src\ub_checks.rs:75
  22:     0x7ff759a7b276 - capnp::primitive_list::Builder<u16>::as_slice<u16>
                               at F:\capnproto-rust\capnp\src\primitive_list.rs:187
  23:     0x7ff759a73768 - primitive_list_as_slice::primitive_list_as_slice
                               at F:\capnproto-rust\capnp\tests\primitive_list_as_slice.rs:40
  24:     0x7ff759a71458 - primitive_list_as_slice::primitive_list_as_slice::closure$0
                               at F:\capnproto-rust\capnp\tests\primitive_list_as_slice.rs:6
[...]
thread caused non-unwinding panic. aborting.

This is because ListReader::into_raw_bytes and ListBuilder::as_raw_bytes return an empty &[u8] when the list is empty, which has a pointer value of 1 in this version of Rust.

The documentation for std::slice::from_raw_parts notes:

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().

This PR adds a check to avoid creating an invalid empty slice and and adds some test cases for empty primitive lists of larger types.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 51.71%. Comparing base (ab342b3) to head (2861b2c). Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #496 +/- ## ========================================== + Coverage 51.64% 51.71% +0.06% ========================================== Files 69 69 Lines 33735 33782 +47 ========================================== + Hits 17422 17469 +47 Misses 16313 16313 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dwrensha commented 6 months ago

Thanks!

dwrensha commented 6 months ago

I moved the empty list case outside of the unsafe block: https://github.com/capnproto/capnproto-rust/commit/edac9159fbd7006378b74f06188d3d3ac591da78