Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible undefined behavior in primitive list as_slice #496

Merged
merged 1 commit into from
May 12, 2024

Conversation

as-com
Copy link
Contributor

@as-com as-com commented May 12, 2024

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.

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dwrensha dwrensha merged commit 929cd4e into capnproto:master May 12, 2024
10 checks passed
@dwrensha
Copy link
Member

Thanks!

@dwrensha
Copy link
Member

I moved the empty list case outside of the unsafe block: edac915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants