UB behaviour from Stacked Borrow violation

[mkatychev]

Calling clone on an object holding a sophia_inmem::graph::LightGraph after cloning the object and dropping the original owner oftentimes results in undefined behaviour when attempting decode a MownStr back into utf-8:

thread 'test::case_1' panicked at ~/.cargo/git/checkouts/sophia_rs-4223a9188f8617e1/064b6dc/api/src/term/bnode_id.rs:43:1:
called `Result::unwrap()` on an `Err` value: InvalidBnodeId("\0@Ҷm\u{600}\0\u{2}\0\0\0")

Running cargo +nightly miri test -- no_double_free claims there is an invalid retag that violates stacked borrow rules

    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.09s
     Running unittests src/lib.rs (target/miri/aarch64-apple-darwin/debug/deps/mownstr-63ff6766c76f206c)

running 1 test
test test::no_double_free ... error: Undefined Behavior: trying to retag from <213316> for SharedReadOnly permission at alloc70220[0x0], but that tag does not exist in the borrow stack for this location
   --> /Users/mkatychev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/slice/raw.rs:140:9
    |
140 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <213316> for SharedReadOnly permission at alloc70220[0x0], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of retag at alloc70220[0x0..0xb]
    |
    = 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: <213316> was created by a SharedReadWrite retag at offsets [0x0..0xb]
   --> src/lib.rs:146:20
    |
146 |         let addr = other.as_mut_ptr();
    |                    ^^^^^^^^^^^^^^^^^^
help: <213316> was later invalidated at offsets [0x0..0xb] by a Unique retag
   --> src/lib.rs:152:26
    |
152 |         std::mem::forget(other);
    |                          ^^^^^
    = note: BACKTRACE (of the first span) on thread `test::no_double_free`:
    = note: inside `std::slice::from_raw_parts::<'_, u8>` at /Users/mkatychev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/slice/raw.rs:140:9: 140:47
note: inside `<MownStr<'_> as std::ops::Deref>::deref`
   --> src/lib.rs:184:25
    |
184 |             let slice = slice::from_raw_parts(ptr, len);
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `test::no_double_free`
   --> src/lib.rs:470:25
    |
470 |             assert_eq!(&mown[..4], "hell");
    |                         ^^^^
note: inside closure
   --> src/lib.rs:467:24
    |
466 |     #[test]
    |     ------- in this procedural macro expansion
467 |     fn no_double_free() {
    |                        ^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error; 2 warnings emitted

error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/Users/mkatychev/.rustup/toolchains/nightly-aarch64-apple-darwin/bin/cargo-miri runner /Users/mkatychev/Documents/rust/mownstr/target/miri/aarch64-apple-darwin/debug/deps/mownstr-63ff6766c76f206c no_double_free` (exit status: 1)
note: test exited abnormally; to see the full output pass --nocapture to the harness.

Attempts to reproduce this in a mownstr crate unit tests are ongoing.

[pchampin]

Thanks for spotting and analyzing this. As I wrote in my comment on #7, the design of MownStr was supposed to avoid double-free, so hope we can isolate this issue an better understand it.

[pchampin]

Could this issue be related to this: https://doc.rust-lang.org/std/mem/fn.forget.html#relationship-with-manuallydrop ? Using ManuallyDrop or Box::leak might be preferable.

[mkatychev]

UPDATE: wasn't able to produce a minimally viable example for the UB behaviour that is somewhat readable, for now miri checks on the tests should suffice. Box::leak + miri coverage should suffice for now.

I would like to follow up with the clippy lints for the naming schema in a breaking: MownStr::from_str and MownStr::borrowed are too similar to stdlib methods and made debugging a crate that borrows from strings quite confusing.

[pchampin]

Now that #8 has been merged (despite github thinking otherwise), can we close this issue?