eclipse-zenoh / zenoh

zenoh unifies data in motion, data in-use, data at rest and computations. It carefully blends traditional pub/sub with geo-distributed storages, queries and computations, while retaining a level of time and space efficiency that is well beyond any of the mainstream stacks.
https://zenoh.io
Other
1.42k stars 149 forks source link

[Bug] `memcpy` source and destination overlap in `zenoh_keyexpr` #651

Closed fuzzypixelz closed 3 days ago

fuzzypixelz commented 8 months ago

Describe the bug

Valgrind reports memcpy source and destination overlap errors in the Canonizable impl of &mut str. I think the following object is the problem:

pub struct Writer {
    pub ptr: *mut u8,
    pub len: usize,
}

impl Writer {
    pub fn write(&mut self, slice: &[u8]) {
        let len = slice.len();
        unsafe { ptr::copy(slice.as_ptr(), self.ptr.add(self.len), len) };
        self.len += len
    }
    pub fn write_byte(&mut self, byte: u8) {
        unsafe { *self.ptr.add(self.len) = byte };
        self.len += 1
    }
}

Writer is used in the Canonizable::canonize implementation of &mut str (and String by extension) to write from one slice of a keyexpr to another with possible overlap, which renders the use of ptr::copy and not ptr::copy_nonoverlapping in Writer::write necessary.

However, because of Rust's strict W^R rules, the compiler is allowed to assume that the &mut self and slice slice arguments of Writer::write point to non overlapping regions of memory. This is because if you take a mutable borrow of a slice in Rust, you can't have another immutable borrow to it (even if they're non-overlapping) using safe code. Thus, the compiler will replace the memmove with the memcpy after optimization passes.

Here's a simple Rust playground example which confirms this: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=c8577ea64c6c1cc514442eb014a77c82 If you select Show ASM with the Debug optim level, you'll see that the ptr::copy compiles to a memmove. But when selecting the Release optim level, it becomes a memcpy and all hell breaks loose.

Also, miri complains about the exact same ptr::copy call, but I'm unfamiliar with their unsafe code rules (they have a model more general than the Rust compiler's):

error: Undefined Behavior: not granting access to tag <107636> because that would remove [SharedReadOnly for <109114>] which is strongly protected because it is an argument of call 48948
   --> commons/zenoh-keyexpr/src/key_expr/utils.rs:24:18
    |
24  |         unsafe { ptr::copy(slice.as_ptr(), self.ptr.add(self.len), len) };
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <107636> because that would remove [SharedReadOnly for <109114>] which is strongly protected because it is an argument of call 48948
    |
    = 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: <107636> was created by a SharedReadWrite retag at offsets [0x0..0x5]

To reproduce

  1. Change the rustup toolchain to nightly in rust-toolchain.toml
  2. Run cargo miri test keyexpr

System info

fuzzypixelz commented 3 days ago

Resolved in https://github.com/eclipse-zenoh/zenoh/pull/1191.