Is this code sound?

Hello all. The title says it all. Curious to know your thoughts on whether the following is sound. Tried to simplify the problem down to the essentials. Miri doesn't complain.

mod imp {
    use std::cell::UnsafeCell;
    use std::sync::Once;

    pub struct RuntimePlusOne {
        once: Once,
        cell: UnsafeCell<&'static mut u8>,
    }

    impl RuntimePlusOne {
        pub fn new(value: &'static mut u8) -> Self {
            Self {
                once: Once::new(),
                cell: UnsafeCell::new(value),
            }
        }
        
        pub fn value(&self) -> &'static u8 {
            self.once.call_once(|| {
                let value = unsafe { &mut *self.cell.get() };
                **value += 1;
            });
            unsafe { &*self.cell.get() }
        }
    }
}

fn main() {
    let bar = {
        static mut INNER: u8 = 1;
        imp::RuntimePlusOne::new(unsafe { &mut INNER })
    };
    println!("{:?}",bar.value());
        
}

playground

1 Like

If the RuntimePlusOne is moved, then it will reassert unique ownership of its value reference.

// MIRIFLAGS=-Zmiri-retag-fields cargo +nightly miri run

mod imp {
    use std::cell::UnsafeCell;
    use std::sync::Once;

    pub struct RuntimePlusOne {
        once: Once,
        cell: UnsafeCell<&'static mut u8>,
    }

    impl RuntimePlusOne {
        pub fn new(value: &'static mut u8) -> Self {
            Self {
                once: Once::new(),
                cell: UnsafeCell::new(value),
            }
        }

        pub fn value(&self) -> &'static u8 {
            self.once.call_once(|| {
                let value = unsafe { &mut *self.cell.get() };
                **value += 1;
            });
            unsafe { &*self.cell.get() }
        }
    }
}

fn main() {
    let inner = Box::leak(Box::new(1));
    let bar = imp::RuntimePlusOne::new(inner);
    let value = bar.value();
    let _bar = bar;
    println!("{:?}", *value);
}
error: Undefined Behavior: trying to retag from <3192> for SharedReadOnly permission at alloc1480[0x0], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:35:22
   |
35 |     println!("{:?}", *value);
   |                      ^^^^^^
   |                      |
   |                      trying to retag from <3192> for SharedReadOnly permission at alloc1480[0x0], but that tag does not exist in the borrow stack for this location
   |                      this error occurs as part of retag at alloc1480[0x0..0x1]
   |
   = 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: <3192> was created by a SharedReadOnly retag at offsets [0x0..0x1]
  --> src/main.rs:33:17
   |
33 |     let value = bar.value();
   |                 ^^^^^^^^^^^
help: <3192> was later invalidated at offsets [0x0..0x1] by a Unique retag
  --> src/main.rs:34:16
   |
34 |     let _bar = bar;
   |                ^^^
   = note: BACKTRACE (of the first span):
   = note: inside `main` at src/main.rs:35:22: 35:28
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (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

The fix is to replace it with a *mut u8 pointer internally, which also lets you discard the UnsafeCell wrapper (since the pointer itself is never modified). Otherwise, the code appears sound: the 'static requirement on RuntimePlusOne::new() prevents the value reference from ever being invalidated, and the properties of Once::call_once() (as well as RuntimePlusOne being !Sync) prevent a &'static u8 reference being created while the value is being modified on another thread.

Also, on the off-chance that you don't need RuntimePlusOne to be Sync, it can be implemented entirely in safe code with an ordinary Cell:

mod imp {
    use std::cell::Cell;

    enum RuntimePlusOneInner {
        Uninit(&'static mut u8),
        Init(&'static u8),
    }

    pub struct RuntimePlusOne {
        inner: Cell<RuntimePlusOneInner>,
    }

    impl RuntimePlusOne {
        pub fn new(value: &'static mut u8) -> Self {
            Self {
                inner: Cell::new(RuntimePlusOneInner::Uninit(value)),
            }
        }

        pub fn value(&self) -> &'static u8 {
            let dummy = RuntimePlusOneInner::Init(&0);
            let value = match self.inner.replace(dummy) {
                RuntimePlusOneInner::Uninit(value) => {
                    *value += 1;
                    value
                }
                RuntimePlusOneInner::Init(value) => value,
            };
            self.inner.set(RuntimePlusOneInner::Init(value));
            value
        }
    }
}
8 Likes

Thanks for taking a look at it @LegionMammal978. Your comments are much appreciated.

Hi all,

I've just read the mentioned article about Stacked Borrows from Ralf Jung and I find it very interesting.

For my understanding when you use *mut u8 instead of UnsafeCell<&'static mut u8> as @LegionMammal978 said the algorithm places a block with a SharedReadWrite tag on the stack for the memory location at the assignment let value = bar.value();. SharedReadWrite gets according to the description of the algorithm not popped on the stack at let _bar = bar; when bar gets disabled and therefore everything should be fine. Did I understand it correctly?

Regards
keks

With UnsafeCell<&'static mut u8>, bar has a Unique item granting access via its reference. let value = bar.value(); creates a new SharedReadOnly item on top of the stack. Then, let _bar = bar; retags the reference in bar, which performs a Unique reborrow. Before pushing the new Unique item to the top of the stack, it performs a write access, using the old Unique item as the granting item. This write access invalidates the SharedReadOnly item, resulting in an error the next time we try to read from value.

With *mut u8, bar has a SharedReadWrite item granting access via its pointer. let value = bar.value(); still creates a SharedReadOnly item, but let _bar = bar; no longer retags the underlying pointer. Therefore, value's corresponding item is no longer invalidated, and the reference can be soundly read from.

3 Likes

Thanks for your explanation! :slight_smile: Right, let value = bar.value(); does of course not place a SharedReadWrite on the stack when value() returns a &'static u8.^^

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.