Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UnsafeCell in Archived type incompatible with miri #303

Closed
sporksmith opened this issue Aug 26, 2022 · 15 comments
Closed

UnsafeCell in Archived type incompatible with miri #303

sporksmith opened this issue Aug 26, 2022 · 15 comments

Comments

@sporksmith
Copy link
Contributor

sporksmith commented Aug 26, 2022

I'm attempting to create a mutex type that can be used in its Archived form, to facilitate safe mutation of archived data mmap'd into multiple processes.

I have it implemented, and some basic tests working, but noticed that rkyv::archived_root results in an error in miri. rkyv::archived_root_mut works ok. Maybe there's a fundamental reason for that, and the answer is "just use rkyv::archived_root_mut", but I think it might be worth digging into a bit more.

I created a smaller test case that seems to show the problem is more fundamentally around UnsafeCell, which would be needed for interior mutability in any Archive type.

#![deny(unsafe_op_in_unsafe_fn)]

use std::cell::UnsafeCell;

#[repr(transparent)]
struct UnsafeU32 { 
    val: UnsafeCell<u32>,
}

impl<S> rkyv::Serialize<S> for UnsafeU32 
where 
    S: rkyv::Fallible + ?Sized,
{
    fn serialize(&self, serializer: &mut S) -> Result<Self::Resolver, S::Error> {
        let val = unsafe { &*self.val.get() };
        val.serialize(serializer)
    }
}

impl rkyv::Archive for UnsafeU32 {
    type Archived = Self;
    type Resolver = <u32 as rkyv::Archive>::Resolver;

    unsafe fn resolve(&self, pos: usize, resolver: Self::Resolver, out: *mut Self::Archived) {
        unsafe { (&*self.val.get()).resolve(pos, resolver, out as *mut u32) };
    }
}

#[test]
fn test_rkyv_miri() {
    type T = UnsafeU32;
    let original: T = UnsafeU32 {
        val: UnsafeCell::new(10),
    };
    let bytes = rkyv::to_bytes::<_, 256>(&original).unwrap();

    // The archived mutex can be used to mutate the data in place.
    let archived = unsafe { rkyv::archived_root::<T>(&bytes[..]) };
    assert_eq!(unsafe { *archived.val.get() }, 10);
}

The test passes natively, but under miri fails:

$ MIRIFLAGS=-Zmiri-backtrace=full 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/repro-982c58b2cf8c397a)

running 1 test
test test_rkyv_miri ... error: Undefined Behavior: trying to reborrow from <185489> for SharedReadWrite permission at alloc77718[0x0], but that tag only grants SharedReadOnly permission for this location
  --> /home/jnewsome/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.39/src/util/mod.rs:67:5
   |
67 |     &*bytes.as_ptr().add(pos).cast()
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |     |
   |     trying to reborrow from <185489> for SharedReadWrite permission at alloc77718[0x0], but that tag only grants SharedReadOnly permission for this location
   |     this error occurs as part of a reborrow at alloc77718[0x0..0x4]
   |
   = 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: <185489> was created by a retag at offsets [0x0..0x4]
  --> src/lib.rs:38:29
   |
38 |     let archived = unsafe { rkyv::archived_root::<T>(&bytes[..]) };
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: backtrace:
   = note: inside `rkyv::archived_value::<UnsafeU32>` at /home/jnewsome/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.39/src/util/mod.rs:67:5
   = note: inside `rkyv::archived_root::<UnsafeU32>` at /home/jnewsome/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.39/src/util/mod.rs:148:5
note: inside `test_rkyv_miri` at src/lib.rs:38:29
  --> src/lib.rs:38:29
   |
38 |     let archived = unsafe { rkyv::archived_root::<T>(&bytes[..]) };
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at src/lib.rs:30:1
  --> src/lib.rs:30:1
   |
29 |   #[test]
   |   ------- in this procedural macro expansion
30 | / fn test_rkyv_miri() {
31 | |     type T = UnsafeU32;
32 | |     let original: T = UnsafeU32 {
33 | |         val: UnsafeCell::new(10),
...  |
39 | |     assert_eq!(unsafe { *archived.val.get() }, 10);
40 | | }
   | |_^
   = note: inside `<[closure@src/lib.rs:30:1: 40:2] as std::ops::FnOnce<()>>::call_once - shim` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:248:5
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:248:5
   = note: inside `test::__rust_begin_short_backtrace::<fn()>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:572:5
   = note: inside closure at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:563:30
   = note: inside `<[closure@test::run_test::{closure#1}] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:248:5
   = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1935:9
   = note: inside `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:271:9
   = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:492:40
   = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:456:19
   = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:137:14
   = note: inside `test::run_test_in_process` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:595:18
   = note: inside closure at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:489:39
   = note: inside `test::run_test::run_test_inner` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:527:13
   = note: inside `test::run_test` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:559:28
   = note: inside `test::run_tests::<[closure@test::run_tests_console::{closure#2}]>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:302:17
   = note: inside `test::run_tests_console` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/console.rs:293:5
   = note: inside `test::test_main` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:113:15
   = note: inside `test::test_main_static` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:132:5
   = note: inside `main`
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:248:5
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:122:18
   = note: inside closure at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:145:18
   = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:280:13
   = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:492:40
   = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:456:19
   = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:137:14
   = note: inside closure at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:48
   = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:492:40
   = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:456:19
   = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:137:14
   = note: inside `std::rt::lang_start_internal` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:20
   = note: inside `std::rt::lang_start::<()>` at /home/jnewsome/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:144:17
   = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)
@sporksmith
Copy link
Contributor Author

I also tried wrapping the serialized buffer in an UnsafeCell, in case that's needed to signal that the raw buffer has interior mutability. It doesn't seem to make a difference, though.

#[test]
fn test_rkyv_miri_unsafecell_wrapper() {
    type T = UnsafeU32;
    let original: T = UnsafeU32 {
        val: UnsafeCell::new(10),
    };
    let bytes = UnsafeCell::new(rkyv::to_bytes::<_, 256>(&original).unwrap());

    // The archived mutex can be used to mutate the data in place.
    let archived = unsafe { rkyv::archived_root::<T>(&(&*bytes.get())[..]) };
    assert_eq!(unsafe { *archived.val.get() }, 10);
}
$ cargo +nightly miri test
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
   Compiling repro v0.1.0 (/home/jnewsome/tmp/repro)
    Finished test [unoptimized + debuginfo] target(s) in 0.05s
     Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/repro-982c58b2cf8c397a)

running 1 test
test test_rkyv_miri_unsafecell_wrapper ... error: Undefined Behavior: trying to reborrow from <185858> for SharedReadWrite permission at alloc77920[0x0], but that tag only grants SharedReadOnly permission for this location
  --> /home/jnewsome/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.39/src/util/mod.rs:67:5
   |
67 |     &*bytes.as_ptr().add(pos).cast()
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |     |
   |     trying to reborrow from <185858> for SharedReadWrite permission at alloc77920[0x0], but that tag only grants SharedReadOnly permission for this location
   |     this error occurs as part of a reborrow at alloc77920[0x0..0x4]
   |
   = 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: <185858> was created by a retag at offsets [0x0..0x4]
  --> src/lib.rs:53:29
   |
53 |     let archived = unsafe { rkyv::archived_root::<T>(&(&*bytes.get())[..]) };
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: backtrace:
   = note: inside `rkyv::archived_value::<UnsafeU32>` at /home/jnewsome/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.39/src/util/mod.rs:67:5
   = note: inside `rkyv::archived_root::<UnsafeU32>` at /home/jnewsome/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.39/src/util/mod.rs:148:5
note: inside `test_rkyv_miri_unsafecell_wrapper` at src/lib.rs:53:29
  --> src/lib.rs:53:29
   |
53 |     let archived = unsafe { rkyv::archived_root::<T>(&(&*bytes.get())[..]) };
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at src/lib.rs:45:1
  --> src/lib.rs:45:1
   |
44 |   #[test]
   |   ------- in this procedural macro expansion
45 | / fn test_rkyv_miri_unsafecell_wrapper() {
46 | |     type T = UnsafeU32;
47 | |     let original: T = UnsafeU32 {
48 | |         val: UnsafeCell::new(10),
...  |
54 | |     assert_eq!(unsafe { *archived.val.get() }, 10);
55 | | }
   | |_^
   = 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

@sporksmith
Copy link
Contributor Author

I have it implemented, and some basic tests working, but noticed that rkyv::archived_root results in an error in miri. rkyv::archived_root_mut works ok. Maybe there's a fundamental reason for that, and the answer is "just use rkyv::archived_root_mut", but I think it might be worth digging into a bit more.

Expanding on this a bit more, I'm hesitant to wrap the mmap'd buffer in a &mut [u8] instead of just a &[u8], because I think that breaks the "A mutable reference cannot be aliased" rule from https://doc.rust-lang.org/nomicon/references.html .

More concretely, I imagine that having the buffer behind a mutable reference signals to the compiler that nothing inside will be mutated behind its back, since it has the only reference. This assumption is false, of course, because the buffer will be mmap'd into other processes, with their own (mutable) references.

@djkoloski
Copy link
Collaborator

I actually think that using an &mut [u8] would resolve this issue since the mutable references have to be simultaneous to violate the aliasing rule. You can still have an &mut of some field reborrowed from an &mut of a larger structure. The compiler should pick up on that.

@sporksmith
Copy link
Contributor Author

In my use case, multiple processes would be mmap'ing the same shared memory pages and deserializing it. If those processes each construct a &mut [u8] from the pointer to that shared memory so that they can call rkyv::archived_root_mut, then there would be simultaneous mutable references to the same memory.

@djkoloski
Copy link
Collaborator

djkoloski commented Aug 31, 2022

Hm, that's true. It seems like this kind of external shared memory is not something Rust was intended to work with natively. I do still think that using a mutable buffer would work, but there is a problem with reads. We would need to treat everything as potentially volatile, which is something that rkyv is not able to do right now.

@sporksmith
Copy link
Contributor Author

I think treating the buffer as a shared/const reference and later using interior mutability ought to be workable. We need to be careful that the Archive types are Sync (e.g. put the whole thing behind a Mutex or the archivable SelfContainedMutex I'm working on), and that the backing mmap has w permission (if the Archive type uses interior mutability), but after that it shouldn't be much different than if the buffer were shared across threads within a single process.

The snag we're hitting here is just how to express it in a way compatible with the Stacked Borrows proposal being checked by miri.

I'm still fuzzy on the details, but I followed a hunch that it didn't like some of the intermediate steps. If I get rid of the intermediate slice and cast the pointer directly, miri seems to think it's ok:

#[test]
fn test_rkyv_miri() {
    type T = UnsafeU32;
    let original: T = UnsafeU32 {
        val: UnsafeCell::new(10),
    };
    let bytes = rkyv::to_bytes::<_, 256>(&original).unwrap();
    let bytes_slice_ptr = bytes.as_ptr();
    let bytes_slice_len = bytes.len();
    let pos = bytes_slice_len - std::mem::size_of::<UnsafeU32>();
    let archived : &T = unsafe { &*bytes_slice_ptr.add(pos).cast() };
    assert_eq!(unsafe { *archived.val.get() }, 10);
    unsafe { archived.val.get().write(11) };
    assert_eq!( unsafe { *archived.val.get() }, 11);
}
$ cargo +nightly miri test
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
   Compiling repro v0.1.0 (/home/jnewsome/tmp/repro)
    Finished test [unoptimized + debuginfo] target(s) in 0.04s
     Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/repro-982c58b2cf8c397a)

running 1 test
test test_rkyv_miri ... ok

@sporksmith
Copy link
Contributor Author

sporksmith commented Aug 31, 2022

If I get rid of the intermediate slice and cast the pointer directly, miri seems to think it's ok:

Actually this is starting to make sense. In the current helpers, there's a point in time where the same underlying data has both a slice of &[u8] and a &T referring to it. In this case, since &T contains UnsafeCell, miri knows that the underlying data has interior mutability, which is incompatible with the &[u8] alias.

@sporksmith
Copy link
Contributor Author

sporksmith commented Aug 31, 2022

I realized another issue when archiving types with interior mutability - rkyv::archived_root doesn't require pinning, unlike rkyv::archived_root_mut.

When T has interior mutability, it's possible to get a mutable reference and move some internals out, which may have contained relative pointers etc.

IDK if this is a bug per se, or covered by the safety requirements already put on the user via the Unsafe wrapper etc. Still, it'd be nice to make it easier to avoid this footgun.

I'm starting to sketch out a new helper for my own use that addresses both issues:

    /// Taking the AlignedVec here instead of a slice avoids a gray safety area
    /// that violates miri's current Stacked Borrows check.
    /// https://github.com/rkyv/rkyv/issues/303
    /// 
    /// The returned value is Pin<T>, because T's that provide interior
    /// mutability could otherwise potentially be incorrectly moved.
    /// 
    /// SAFETY:
    /// * `vec` must contain a value of type `T` at the end.
    unsafe fn archived_root<T>(vec: &rkyv::AlignedVec) -> Pin<&<T as rkyv::Archive>::Archived>
    where T: rkyv::Archive
    {
        let pos = vec.len() - std::mem::size_of::<T>();
        // SAFETY: caller guarantees the cast is valid
        let reference = unsafe { &*vec.as_ptr().add(pos).cast() };
        // SAFETY: the resulting pin has an immutable borrow from `vec`.
        // Mutable methods of `vec` that might cause reallocation etc cannot
        // be called while this reference exists.
        unsafe { pin::Pin::new_unchecked(reference) }
    }

@djkoloski
Copy link
Collaborator

If a type has interior mutability and produces a mutable reference to some internal value, then it must ensure that the reference obeys the typical aliasing rules. For example, RefCell dynamically guarantees that a mutable and shared reference do not exist simultaneously. Cell gets around these rules by never producing a mutable reference, it only ever moves the value around. The core of the problem is not that we can't have mutable references to rkyv types (that's just a symptom), the real issue is that we can't allow rkyv types to be moved. Another way to think about this is that all rkyv types must be treated as pinned, which is not the way that standard library types with interior mutability treat their types by default.

I think the correct way to solve this issue would be to create an archived variant of UnsafeCell that only produces pinned mutable references to the interior type. Implementing Cell with that primitive would only be possible when T: Unpin, which is the bound we needed earlier (rkyv types are !Unpin by default). This seems persuasive to me.

Your idea for returning a pinned shared reference is on the right track, but not quite correct. It's not your fault though, pinning is notoriously difficult to get right. Pinning a shared reference doesn't actually do anything because you can just get the shared reference back out with get_ref. Rather than having some notion of a pinned value, rkyv is working moreso with a system of immovable types if that makes any sense.

@sporksmith
Copy link
Contributor Author

Pinning a shared reference doesn't actually do anything because you can just get the shared reference back out with get_ref

Hah, whoops!

I think the correct way to solve this issue would be to create an archived variant of UnsafeCell that only produces pinned mutable references to the interior type. Implementing Cell with that primitive would only be possible when T: Unpin, which is the bound we needed earlier (rkyv types are !Unpin by default). This seems persuasive to me.

Hmm, yeah I think that makes sense.

@sporksmith
Copy link
Contributor Author

I think I've addressed the pinning issues in my WIP mutex API. I have a PR out now being reviewed by a teammate; would be happy to have your review there too. Otherwise if you're interested I could separately take a stab at merging it into rkyv itself, and you can review there. shadow/shadow#2386

@djkoloski
Copy link
Collaborator

I think the PR looks good in general. Because of the reliance on libc, I think this would be better suited for rkyv_contrib if you wanted to upstream it. I think it would be beneficial to add the pointer-only API for archived_root and friends. Issue filed here.

@sporksmith
Copy link
Contributor Author

Thanks! Yeah, I could drop the nix dependency pretty easily, but dropping libc would be trickier. I'll have a look at adding to rkyv_contrib

@djkoloski
Copy link
Collaborator

Since it seems like this issue has been resolved, I'm going to close this issue. Feel free to reopen if I've missed something!

@sporksmith
Copy link
Contributor Author

I'm still using my own replacement for archived_root to avoid the miri stacked borrows failure. Maybe this or something like it should be added to rkyv:

    /// Taking the AlignedVec here instead of a slice avoids a gray safety area
    /// that violates miri's current Stacked Borrows check.
    /// https://github.com/rkyv/rkyv/issues/303
    ///
    /// SAFETY:
    /// * `vec` must contain a value of type `T` at the end.
    unsafe fn archived_root<T>(vec: &rkyv::AlignedVec) -> &T::Archived
    where
        T: rkyv::Archive,
    {
        let pos = vec.len() - std::mem::size_of::<T::Archived>();
        // SAFETY:
        // * Caller guarantees that this memory contains a valid `T`
        // * The reference borrows from `vec`, ensuring that it can't
        //   be dropped or mutated.
        // * `AlignedVec::as_ptr` explicitly supports this use-case,
        //   including for `T` that has internal mutability via `UnsafeCell`.
        unsafe { &*vec.as_ptr().add(pos).cast() }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants