Is it safe to implement SharedMemCast for bool?

[asajeffrey]

One of the requirements for bool is that it only has the values 0x00 and 0x01, so there is a possible safety issue around having bool implement SharedMemCast.

[asajeffrey]

cc @nox

[elast0ny]

Regular use of a bool that lives in the shared memory should be safe (ie: manipulated/initialized through the structs traits)

In contrast for example, the regular use of Vec in the shmem is unsafe as some Vec traits manipulate the backing allocation (alloc/realloc/etc...)

[nox]

Regular use of a bool that lives in the shared memory should be safe (ie: manipulated/initialized through the structs traits)

How? Anything that crosses the process boundary should be untrusted, there is no guarantee that the value is actually a bool as Rust means it on the other side.

[nox]

This is UB and does not use any unsafe code. Please reopen this issue.

use ::shared_memory::*;
use std::ffi::{CStr, CString};

static GLOBAL_LOCK_ID: usize = 0;

fn main() -> Result<(), SharedMemError> {
    let mut shmem = match SharedMem::create_linked("shared_mem.link", LockType::Mutex, 4096) {
        // We created and own this mapping
        Ok(v) => v,
        // Link file already exists
        Err(SharedMemError::LinkExists) => SharedMem::open_linked("shared_mem.link")?,
        Err(e) => return Err(e),
    };

    {
        let mut shared_state = shmem.wlock::<u8>(GLOBAL_LOCK_ID)?;
        **shared_state = 2;
    }

    println!("{:?}", *shmem.rlock::<bool>(GLOBAL_LOCK_ID)?);
    Ok(())
}

[nox]

You can also do println!("{:?}", **shmem.rlock::<bool>(GLOBAL_LOCK_ID)? as u8); and see that it will print 2.

[nox]

Also note that for similar reasons, the implementations for char and str are also unsound.

[asajeffrey]

+1 on reopening.

[nox]

I just realised that it also makes the impls for AtomicBool, AtomicPtr, AtomicUsize and AtomicIsize unsound, for the same reason that you can also read the bytes as a different type than you wrote them.

[asajeffrey]

Not sure about that last comment, though it depends on what you think the contract for AtomicBool is. Why is AtomicUsize unsound?

[asajeffrey]

Ah, it's because you can use a read-write lock to access the same shared memory both at type &usize and at type &AtomicUsize, then use the &AtomicUsize to write to the memory, which makes the &usize UB.

[asajeffrey]

That raises a related issue, which is whether the implementation of Deref for ReadLockGuard is safe. I'll open a separate issue for that.

[elast0ny]

I see what you're saying now. I will re-open the issue.

If I look at shared_memory's behavior with your point of view, then the whole crate is essentially unsafe. This is simply the inherent unsafety of sharing memory between two processes (regardless of languages)... Both processes have to agree exactly on what the memory represents or nothing will work, there is simply no way around this.

This can be clearly highlighted when using shared_memory in process1 in safe Rust and process2 in C. Process 2 can :

• Mangle the metadata header values
• Cast the shared memory to the wrong types
• Ignore synchronization for data access
• etc...

As highligted in this issue, some of these can be done with "safe" usage of this crate... This is also ignoring the fact that the rust ABI is not stable which could lead to even more subtle issues.

Im open to suggestions on how to fix this but unfortunately, i believe the only way to get the message across to the users of the crate is to mark essentially 90% of the API unsafe, eg :

• Openning an existing mapping relies on the metadata being valid, this can crash a program if not
• Any operations on locks/events are inherently unsafe as their state might be corrupted
• Every read will be marked unsafe as the contents might be invalid for the chosen data type
• Every write will be marked unsafe as there is no guarante another process isnt writing at the same place corrupting data

Unforunately, this would essentialy render the API a pain in the @$* to use...

Thoughts ?

[asajeffrey]

Thanks for re-opening!

One way of dealing this would be to be explicit about the attacker model: is the crate intended to be safe against all attackers, or just ones written in Rust? Defending against all attackers is the goal of shared-data, which I think I can see how to do apart from truncation :( https://github.com/asajeffrey/shared-data/issues/7

I think a fair bit of the API for shared_memory is safe, it's the implementations of Deref and DerefMut that look dubious. At the expense of extra copying, I think it's safe (apart from issues with truncation) to use read_volatile to copy data out of shared memory, it's the access via &T that's an issue.

[nox]

I feel like many parts of the crate could be safe and sound, and that maybe its scope should be reduced while we look for the perfect set of safe primitives, for example, what would you think of removing the casting infrastructure to make the crate only be about reading and writing bytes in shared memory?

I’ll file another issue for the usage on non-anonymous memory-mapped files, because that also leads to soundness issues.

Thanks for reopening btw!

[asajeffrey]

I guess even read_volatile will be UB if the shared memory is truncated in the middle of a read. Sigh, it would be nice if the OS would provide protection against file truncation.

[elast0ny]

Yeap good point, I could strip the whole Deref/Casting feature and simply return raw pointers to the shared memory and let the callers handle those the way they want.

In addition, I would have to strip the lock/events out of the crate as they make a lot of assumptions about the shmem contents being valid but I was thinking of doing so anyways for https://github.com/elast0ny/shared_memory-rs/issues/29

Essentialy, this would simplify the shared_memory crate to a thin wrapper for creating/openning shared memory mappings. Safely manipulating the shared memory would be 100% offloaded on the callers of this crate. Afterall, they are the only ones who can enforce proper initialization, sychronization and what specific offsets in the memory represents.

[asajeffrey]

@elast0ny I can make a home for SharedMemCast in shared-data.

There's still the issue of truncation for anyone who ever wants to use one of the raw pointers, but that would then be my issue :)

[mitsuhiko]

I was playing around with this crate a bit to make it work with procspawn / mitosis. I think what would be possible to make safe is a pattern like this:

#[derive(SharedMemCast)]
pub struct SharedState {
    did_something: bool,
}

fn main() {
    procspawn::init();

    let region = shared_memory::Region::new(SharedState {
        did_something: false,
    });

    procspawn::spawn(region.clone(), |region| {
        let state = region.wlock().unwrap();
        state.did_something = true;
    }).join().unwrap();

    let state = region.rlock().unwrap();
    assert_eq!(state.did_something, true);
}

This way you can't unsafely have the wrong type on the subprocess. It's already easily possible to implement this type of thing on top of the systems that exist.

[nox]

What's shared_memory::Region?

[mitsuhiko]

@nox see #39 (renamed region to handle)

[elast0ny]

This issue can finally be closed.

I have decided to remove all shared memory casting and exposed an Shmem::as_ptr() -> *mut u8 method on the mapping which callers can now cast to whatever they please.

This now removes all the safety issues highlighted in this thread from the crate.