Manishearth/triomphe

UB in `ArcBorrow` implementation due to provenance

steffahn opened this issue · 1 comments

ArcBorrow<'_, T> contains an actual &T reference. Such a reference cannot have provenance over the reference count though, which leads to miri detecting UB if you run cargo +nightly miri test on #64.

git fetch origin bf35d79324665bd4886a4319216a0964a958dda5
git checkout bf35d79324665bd4886a4319216a0964a958dda5
cargo +miri test
running 33 tests
test arc::tests::arc_eq_and_cmp ... ok
test arc::tests::arc_eq_and_partial_cmp ... ok
test arc::tests::from_iterator_exact_size ... ok
test arc::tests::from_iterator_unknown_size ... ok
test arc::tests::maybeuninit ... ok
test arc::tests::maybeuninit_array ... ok
test arc::tests::maybeuninit_slice_ub_to_proceed - should panic ... ok
test arc::tests::maybeuninit_ub_to_proceed - should panic ... ok
test arc::tests::roundtrip ... ok
test arc::tests::roundtrip_slice ... ok
test arc::tests::try_unwrap ... ok
test arc_borrow::clone_arc_borrow ... error: Undefined Behavior: trying to retag from <346713> for SharedReadWrite permission at alloc90475[0x0], but that tag does not exist in the borrow stack for this location
   --> src/arc.rs:230:18
    |
230 |         unsafe { &*self.ptr() }
    |                  ^^^^^^^^^^^^
    |                  |
    |                  trying to retag from <346713> for SharedReadWrite permission at alloc90475[0x0], but that tag does not exist in the borrow stack for this location
    |                  this error occurs as part of retag at alloc90475[0x0..0x10]
    |
    = 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: <346713> was created by a SharedReadOnly retag at offsets [0x8..0xc]
   --> src/arc_borrow.rs:39:42
    |
39  |         let arc = unsafe { Arc::from_raw(self.0) };
    |                                          ^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `arc::Arc::<i32>::inner` at src/arc.rs:230:18: 230:30
note: inside `<arc::Arc<i32> as std::clone::Clone>::clone`
   --> src/arc.rs:423:24
    |
423 |         let old_size = self.inner().count.fetch_add(1, Relaxed);
    |                        ^^^^^^^^^^^^
note: inside `arc_borrow::ArcBorrow::<'_, i32>::clone_arc`
   --> src/arc_borrow.rs:41:21
    |
41  |         mem::forget(arc.clone());
    |                     ^^^^^^^^^^^
note: inside `arc_borrow::clone_arc_borrow`
   --> src/arc_borrow.rs:122:13
    |
122 |     let y = b.clone_arc();
    |             ^^^^^^^^^^^^^
note: inside closure
   --> src/arc_borrow.rs:119:22
    |
118 | #[test]
    | ------- in this procedural macro expansion
119 | fn clone_arc_borrow() {
    |                      ^
    = 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 previous error

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

Caused by:
  process didn't exit successfully: `/home/frank/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner /home/frank/forks/just-a-clone/triomphe/target/miri/x86_64-unknown-linux-gnu/debug/deps/triomphe-70e77d022ca4bab7` (exit status: 1)

I should have time tomorrow to work on a fix, probably comparable to the implementation of rc_borrow::ArcBorrow which uses a raw (NonNull) pointer instead of a reference.

Ah, thanks for going through this. This crate was written ages ago and has some rough edges, very much appreciate your attempts to review and fix it!