Repository: capnproto/capnproto-rust (language: rust)

Issue:
Miri test failures
It seems most tests are not passing under miri.
At least it fails on the very first test:
```
$ cargo +nightly miri test                                                                                                                                                                                                       
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done                                                                                                                                                                                       
    Finished test [unoptimized + debuginfo] target(s) in 0.01s                                                                                                                                                                                                
     Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/capnp-0b6f161a93e8d3c9)                                                                                                                                                    
                                                                                                                                                                                                                                                              
running 37 tests                                                                                                                                                                                                                                              
test any_pointer::init_clears_value ... error: Undefined Behavior: attempting a write access using <196437> at alloc77913[0x8], but that tag does not exist in the borrow stack for this location                                                             
    --> capnp/src/private/layout.rs:602:67                                                                                                                                                                                                                    
     |                                                                                                                                                                                                                                                        
602  |                   Bit | Byte | TwoBytes | FourBytes | EightBytes => ptr::write_bytes(                                                                                                                                                                  
     |  ___________________________________________________________________^
603  | |                     ptr,
604  | |                     0u8,
605  | |                     BYTES_PER_WORD
...    |
609  | |                         ) as usize,
610  | |                 ),
     | |                 ^
     | |                 |
     | |_________________attempting a write access using <196437> at alloc77913[0x8], but that tag does not exist in the borrow stack for this location
     |                   this error occurs as part of an access at alloc77913[0x8..0x20]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information 
help: <196437> was created by a SharedReadWrite retag at offsets [0x0..0x8]
    --> capnp/src/private/layout.rs:180:34
     |
180  |         let this_addr: *mut u8 = self as *mut _ as *mut _;
     |                                  ^^^^
     = note: BACKTRACE (of the first span):
     = note: inside `private::layout::wire_helpers::zero_object_helper` at capnp/src/private/layout.rs:602:67: 610:18
note: inside `private::layout::wire_helpers::zero_object`
    --> capnp/src/private/layout.rs:549:17
     |
549  |                 zero_object_helper(arena, segment_id, reff, (*reff).mut_target())
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `private::layout::PointerBuilder::<'_>::clear`
    --> capnp/src/private/layout.rs:3327:13
     |
3327 |             wire_helpers::zero_object(self.arena, self.segment_id, self.pointer);
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<any_pointer::Builder<'_> as traits::FromPointerBuilder<'_>>::init_pointer`
    --> capnp/src/any_pointer.rs:181:13
     |
181  |             builder.clear();
     |             ^^^^^^^^^^^^^^^
note: inside `any_pointer::Builder::<'_>::init_as::<any_pointer::Builder<'_>>`
    --> capnp/src/any_pointer.rs:150:9
     |
150  |         FromPointerBuilder::init_pointer(self.builder, 0)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `message::Builder::<message::HeapAllocator>::init_root::<'_, any_pointer::Builder<'_>>`
    --> capnp/src/message.rs:443:9
     |
443  |         root.init_as()
     |         ^^^^^^^^^^^^^^
note: inside `any_pointer::init_clears_value`
    --> capnp/src/any_pointer.rs:260:49
     |
260  |         let root: crate::any_pointer::Builder = message.init_root();
     |                                                 ^^^^^^^^^^^^^^^^^^^
note: inside closure
    --> capnp/src/any_pointer.rs:249:24
     |
248  | #[test]
     | ------- in this procedural macro expansion
249  | fn init_clears_value() {
     |                        ^
     = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info) 
```

For everyone piece of mind, I think it would be really great if capnproto-rust would not exploit UB and if all tests could pass with miri.
If there is a relatively trivial fix, with some guidance, I am willing to work on it :)


Discussion / hints:
Hm... maybe the [`WirePointer` methods](https://github.com/capnproto/capnproto-rust/blob/be0c63b28fffea4a74776be278e27060a53a03f0/capnp/src/private/layout.rs#L145-L336) need to take `self` by raw pointer rather than reference?
Fixed in https://github.com/capnproto/capnproto-rust/pull/383.

Fix patch:
diff --git a/capnp/src/private/layout.rs b/capnp/src/private/layout.rs
index 10ea5fa5e..53005ad1f 100644
--- a/capnp/src/private/layout.rs
+++ b/capnp/src/private/layout.rs
@@ -164,23 +164,32 @@ impl WirePointer {
         unsafe { this_addr.offset(8 * (1 + ((self.offset_and_kind.get() as i32) >> 2)) as isize) }
     }
 
+    // At one point, we had `&self` here instead of `ptr: *const Self`, but miri
+    // flagged that as running afoul of "stacked borrow" rules.
     #[inline]
-    pub fn target_from_segment(
-        &self,
+    fn target_from_segment(
+        ptr: *const Self,
         arena: &dyn ReaderArena,
         segment_id: u32,
     ) -> Result<*const u8> {
-        let this_addr: *const u8 = self as *const _ as *const _;
-        let offset = 1 + ((self.offset_and_kind.get() as i32) >> 2);
-        unsafe { arena.check_offset(segment_id, this_addr, offset) }
+        let this_addr: *const u8 = ptr as *const _;
+        unsafe {
+            let offset = 1 + (((*ptr).offset_and_kind.get() as i32) >> 2);
+            arena.check_offset(segment_id, this_addr, offset)
+        }
     }
 
+    // At one point, we had `&mut self` here instead of `ptr: *mut Self`, but miri
+    // flagged that as running afoul of "stacked borrow" rules.
     #[inline]
-    pub fn mut_target(&mut self) -> *mut u8 {
-        let this_addr: *mut u8 = self as *mut _ as *mut _;
-        this_addr.wrapping_offset(
-            BYTES_PER_WORD as isize * (1 + ((self.offset_and_kind.get() as i32) >> 2)) as isize,
-        )
+    fn mut_target(ptr: *mut Self) -> *mut u8 {
+        let this_addr: *mut u8 = ptr as *mut _;
+        unsafe {
+            this_addr.wrapping_offset(
+                BYTES_PER_WORD as isize
+                    * (1 + (((*ptr).offset_and_kind.get() as i32) >> 2)) as isize,
+            )
+        }
     }
 
     #[inline]
@@ -470,7 +479,7 @@ mod wire_helpers {
             let pad: *mut WirePointer =
                 (seg_start as *mut WirePointer).offset((*reff).far_position_in_segment() as isize);
             if !(*reff).is_double_far() {
-                Ok(((*pad).mut_target(), pad, segment_id))
+                Ok((WirePointer::mut_target(pad), pad, segment_id))
             } else {
                 //# Landing pad is another far pointer. It is followed by a
                 //# tag describing the pointed-to object.
@@ -511,7 +520,7 @@ mod wire_helpers {
 
             if !(*reff).is_double_far() {
                 Ok((
-                    (*pad).target_from_segment(arena, far_segment_id)?,
+                    WirePointer::target_from_segment(pad, arena, far_segment_id)?,
                     pad,
                     far_segment_id,
                 ))
@@ -528,7 +537,7 @@ mod wire_helpers {
             }
         } else {
             Ok((
-                (*reff).target_from_segment(arena, segment_id)?,
+                WirePointer::target_from_segment(reff, arena, segment_id)?,
                 reff,
                 segment_id,
             ))
@@ -546,7 +555,7 @@ mod wire_helpers {
 
         match (*reff).kind() {
             WirePointerKind::Struct | WirePointerKind::List | WirePointerKind::Other => {
-                zero_object_helper(arena, segment_id, reff, (*reff).mut_target())
+                zero_object_helper(arena, segment_id, reff, WirePointer::mut_target(reff))
             }
             WirePointerKind::Far => {
                 let segment_id = (*reff).far_segment_id();
@@ -1016,7 +1025,7 @@ mod wire_helpers {
                 dst,
                 src_segment_id,
                 src,
-                (*src).mut_target(),
+                WirePointer::mut_target(src),
             );
         } else {
             ptr::copy_nonoverlapping(src, dst, 1);
@@ -1133,7 +1142,7 @@ mod wire_helpers {
         size: StructSize,
         default: Option<&'a [crate::Word]>,
     ) -> Result<StructBuilder<'a>> {
-        let mut ref_target = (*reff).mut_target();
+        let mut ref_target = WirePointer::mut_target(reff);
 
         if (*reff).is_null() {
             match default {
@@ -1335,7 +1344,7 @@ mod wire_helpers {
             "Use get_writable_struct_list_pointer() for struct lists"
         );
 
-        let mut orig_ref_target = (*orig_ref).mut_target();
+        let mut orig_ref_target = WirePointer::mut_target(orig_ref);
 
         if (*orig_ref).is_null() {
             if default_value.is_null() || (*(default_value as *const WirePointer)).is_null() {
@@ -1468,7 +1477,7 @@ mod wire_helpers {
         element_size: StructSize,
         default_value: *const u8,
     ) -> Result<ListBuilder<'_>> {
-        let mut orig_ref_target = (*orig_ref).mut_target();
+        let mut orig_ref_target = WirePointer::mut_target(orig_ref);
 
         if (*orig_ref).is_null() {
             if default_value.is_null() || (*(default_value as *const WirePointer)).is_null() {
@@ -1772,7 +1781,7 @@ mod wire_helpers {
                 }
             }
         } else {
-            (*reff).mut_target()
+            WirePointer::mut_target(reff)
         };
 
         let (ptr, reff, _segment_id) = follow_builder_fars(arena, reff, ref_target, segment_id)?;
@@ -1865,7 +1874,7 @@ mod wire_helpers {
                 }
             }
         } else {
-            (*reff).mut_target()
+            WirePointer::mut_target(reff)
         };
 
         let (ptr, reff, _segment_id) = follow_builder_fars(arena, reff, ref_target, segment_id)?;
diff --git a/capnp/src/serialize/no_alloc_slice_segments.rs b/capnp/src/serialize/no_alloc_slice_segments.rs
index 23f9678d7..b98da67e4 100644
--- a/capnp/src/serialize/no_alloc_slice_segments.rs
+++ b/capnp/src/serialize/no_alloc_slice_segments.rs
@@ -396,6 +396,7 @@ mod tests {
     }
 
     quickcheck! {
+        #[cfg_attr(miri, ignore)] // miri takes a long time with quickcheck
         fn test_no_alloc_buffer_segments_single_segment_optimization(
             segment_0 : Vec<Word>) -> TestResult
         {
@@ -420,6 +421,7 @@ mod tests {
             TestResult::from_bool(true)
         }
 
+        #[cfg_attr(miri, ignore)] // miri takes a long time with quickcheck
         fn test_no_alloc_buffer_segments_multiple_segments(segments_vec: Vec<Vec<Word>>) -> TestResult {
             if segments_vec.is_empty() { return TestResult::discard() };
 
@@ -486,6 +488,7 @@ mod tests {
     }
 
     quickcheck! {
+        #[cfg_attr(miri, ignore)] // miri takes a long time with quickcheck
         fn test_no_alloc_buffer_segments_message_truncated(segments_vec: Vec<Vec<Word>>) -> TestResult {
             if segments_vec.is_empty() { return TestResult::discard() }
 
@@ -508,6 +511,7 @@ mod tests {
             TestResult::from_bool(true)
         }
 
+        #[cfg_attr(miri, ignore)] // miri takes a long time with quickcheck
         fn test_no_alloc_buffer_segments_message_options_limit(
             segments_vec: Vec<Vec<Word>>) -> TestResult
         {
@@ -541,6 +545,7 @@ mod tests {
             TestResult::from_bool(true)
         }
 
+        #[cfg_attr(miri, ignore)] // miri takes a long time with quickcheck
         fn test_no_alloc_buffer_segments_bad_alignment(segment_0: Vec<Word>) -> TestResult {
             if segment_0.is_empty() { return TestResult::discard(); }
             let output_segments = OutputSegments::SingleSegment([Word::words_to_bytes(&segment_0)]);
diff --git a/capnp/src/serialize_packed.rs b/capnp/src/serialize_packed.rs
index 44cba08d0..698d97d50 100644
--- a/capnp/src/serialize_packed.rs
+++ b/capnp/src/serialize_packed.rs
@@ -87,8 +87,9 @@ where
         assert!(len % 8 == 0, "PackedRead reads must be word-aligned.");
 
         unsafe {
-            let mut out = out_buf.as_mut_ptr();
-            let out_end: *mut u8 = out_buf.as_mut_ptr().wrapping_add(len);
+            let out_buf_start = out_buf.as_mut_ptr();
+            let mut out = out_buf_start;
+            let out_end: *mut u8 = out.wrapping_add(len);
 
             let (mut in_ptr, mut in_end) = self.get_read_buffer()?;
             let mut buffer_begin = in_ptr;
@@ -101,7 +102,7 @@ where
                 let tag: u8;
 
                 assert_eq!(
-                    ptr_sub(out, out_buf.as_mut_ptr()) % 8,
+                    ptr_sub(out, out_buf_start) % 8,
                     0,
                     "Output pointer should always be aligned here."
                 );
@@ -538,6 +539,7 @@ mod tests {
             }))
         }
 
+        #[cfg_attr(miri, ignore)] // miri takes a long time with quickcheck
         fn test_unpack(packed: Vec<u8>) -> TestResult {
             let len = packed.len();
             let mut packed_read = PackedRead { inner: &packed[..] };


Test patch: