System V shared memory APIs (2nd draft)

[IzawGithub]

trafficstars

What does this PR do

Checklist:

• [X] I have read CONTRIBUTING.md
• [x] I have written necessary tests and rustdoc comments
• [X] A change log has been added if this PR modifies nix's API

I've seen issue #1718 and the pull request #1989 made by @arpankapoor from a year ago, that added some support for the System V API.

This is a continuation of this PR, where I tried to implement most of the change suggested by @VorpalBlade.

What's missing

I haven't added any unit test yet, as somehow I'm getting a lot of compilation error on the test, about some missing symbol, but I've got a closed source project where I'm using this draft as a "eat your own dog food", with unit test, and it seems to work fine.

Some of the bitflags, 2 in ShmgetFlag and a lot in SemctlCmd do not exist in libc, even though they are documented. So to merge this, there is a need to create a PR for libc that add those.

What can be better

Permissions: Could maybe be moved elsewhere? I haven't found any struct already implemented that wrap the octal permission system unix use, so I threw this in 5m.

shmat: I'm really not a fan of returning a void ptr. I've tried to wrap it in a Weak<RefCell<T>> but I get a segfault when it gets dropped. What would be ideal is for the user to give a generic type T that could be wrapped in a non owning smart pointer that could also be null. That's why I though Weak would be the answer, but I'm not good enough to find the problem. Currently, the user has to cast it in an unsafe block, to be able to use the value stored, which is awkward and error prone.

I'm not too sure about the linux only bitflags compilation. We should test it for android too, but I don't know how to do that.

[SteveLauC]

I am trying to add support for System V message queue in #2318

[IzawGithub]

Hello @SteveLauC,

I think this PR is pretty good now. I've added some basic test, with around 30% coverage of the new function. What's missing is testing shmdt, which I don't really know how I could do that, and all of the different bitflags, which is a bit overwhelming.

I also didn't implement the Semop function because I have no clue at what it's even supposed to do. If anyone got an idea about how to implement it, it would be more than welcome.

I don't understand why BSD isn't able to build successfully, when it should be working fine.

Is there anything left to be done before a review and an eventual merge?

[SteveLauC]

Hi @IzawGithub, sorry for the late reply, I plan to take a look at this PR this weekend :)

[SteveLauC]

Some thoughts about these APIs:

1. Module names

   I kinda think we should separate semaphore set and shared memory segments into 2 modules, following the names of their C header files, i.e, sys/sem.rs for semaphore set and sys/shm.rs for shared memory segments.

2. For those APIs, do you think it will be better if we do something like:

   Take semaphore set as an example:

   /// A System V semaphore set.
   pub struct SemaphoreSet(libc::c_int);
   
   pub enum SemCmd {
       IPC_STAT(&mut libc::semid_ds),
       IPC_SET(&libc::semid_ds),
   }
   
   impl SemaphoreSet {
       pub fn new(key: key_t, n_sem: usize, semflg: SemFlg) -> Result<Self>;
       pub fn op<O: AsRef<[Sembuf]>>(&self, ops: O) -> Result<()>;
       pub fn op_timeout<O: AsRef<[Sembuf]>>(&self, ops: O, timeout: TimeSepc) -> Result<()>;
       pub fn ctl(&self, sem_num: usize, cmd: SemCmd) -> Result<Option<libc::c_int>>;
   }
   

   And to remove a semaphore set, I am thinking about making our wrapper type non-Copy, and do this:

   #[allow(missing_copy_implementations)]
   pub struct SemaphoreSet(libc::c_int);
   
   impl SemaphoreSet {
       pub fn remove(self) -> Result<(), (Errno, Self)> {
          let res = unsafe {
              libc::semctl(self.0, /*whatever*/, IPC_RMID)
          };
   
          match Errno::result(res) {
              Ok(_) => Ok(()),
              Err(errno) => Err((errno, self))
          }
       }
   }
   

[IzawGithub]

Hello,

Thanks for the comprehensive review! Those are all fair issue, I'll work on them this week.

For your last question about wrapping all the functions, this is certainly doable, but I wasn't sure if it was prefered to match libc as closely as possible or not. I can definitely try to make something more Rusty. Worst case scenario, if it's not ok we could always revert to what's currently there.

Also, once the PR is done, should I clean the history with a rebase before a merge or not?

[SteveLauC]

Also, once the PR is done, should I clean the history with a rebase before a merge or not?

Nix uses squash merge, so it's fine to leave the commit history here:)

[IzawGithub]

Hello @SteveLauC, sorry for not answering sooner, I was pretty sick.

I have refactored the shared memory segment portion of this PR, to fit more with Rust style of doing things. If you feel that this is the right direction to go, I'll work on making something similar for the semaphore portion.

This also resolve the issue of returning a raw pointer. Before, the end user had to use unsafe for deref. This is now fixed because SharedMemory acts like a smart pointer to the non owning data. This makes for a much nicer interface.

[SteveLauC]

Hello @SteveLauC, sorry for not answering sooner, I was pretty sick.

Hi, no need to be sorry about that! I hope you are doing well:)

SharedMemory acts like a smart pointer to the non owning data.

I really like this smart pointer and that auto-detach-on-drop thing, thanks for this!

One thing I found while reviewing this PR is that with this new smart pointer interface, you can attach a SharedMemory<Foo> to a memory segment of type Bar even though the segment is created by the current process, as shown in the below program:

// create a new segment
let bar_mem_segment = SharedMemory::<Bar>::shmget(..);

// attach to it, with a different underlying type
let mut shared_mem = SharedMemory::<Foo>::new(..);

// write to it, which can cause UB
*shared_mem = Foo;

I think this can be resolved via a Shm<T> wrapper type:

// create a new segment
let bar_mem_segment: Shm<Bar> = SharedMemory::<Bar>::shmget(..);

// attach to it, with a different underlying type
let mut shared_mem: SharedMemory<Bar> = bar_mem_segment.new(..); // or rename `.new()` to `.attach()`

// Error: mismatched types!
*shared_mem = Foo; 

[IzawGithub]

Hello, all of the issue listed above should be fixed, expect for the c_void where I don't really know how to go forward with.

Nice catch on the UB! To resolve it, I had to make shmget an unsafe function. This is because, if the user is connecting to a shared memory zone that we haven't created ourselves, then there is no way to make sure the types match. If we're always creating the new memory zone, then we're sure the type match, so I've added a safe create_and_connect for that.

I'll work on the semaphores during the coming week.

[SteveLauC]

Hi, I just finished another round of review, and realized we can still have UB even with create_and_connect():

use nix::sys::{
    stat::Mode,
    system_v::shm::{Shm, ShmatFlag},
};

#[tokio::main(flavor = "current_thread")]
async fn main() {
    let mode = Mode::S_IRWXU;

    let shm = Shm::<&()>::create_and_connect(65536, mode).unwrap();
    let mem_segment = shm.attach(ShmatFlag::empty()).unwrap();
    println!("reference = {:p}", *mem_segment);
}

In the above code snippet, we create a new shared memory segment and attach to it, then print the value stored there, a Rust reference can never be NULL, i.e, 0, but with the above code, I got:

$ cargo r -q
reference = 0x0

With the current impl, there is no guarantee that the value stored in a segment is valid for T, perhaps we should add create 2 types here:

1. UnsafeSharedMemory
2. SharedMemory

A SharedMemory can ONLY be created via unsafe fn UnsafeShardMemory::assume_init(), thoughts?

[IzawGithub]

Hello, this is harder than I though!

For your example, after a bit of debugging, I think that pointing to 0x0 is because we're trying to store a type that's a ref, when SharedMemory actually expect a non ref. T being a generic, I would like to restrict it to only owned data, which would resolve the issue. However, I don't really see a way to do that. I didn't see any "not a pointer" trait, maybe Default? But that's annoying for the end user if their data struct does not implement default.

The rest of the issue are pretty simple fix I'll work on right away.

[SteveLauC]

Hi, sorry for the late response, been busy with my work and can barely do open source things on work days.

---

For your example, after a bit of debugging, I think that pointing to 0x0 is because we're trying to store a type that's a ref, when SharedMemory actually expect a non ref. T being a generic, I would like to restrict it to only owned data, which would resolve the issue.

Even if we restrict T to be owned types, it is still not guaranteed that the value stored in that shared memory is valid for T, so the problem still exists.

From my observation, that memory segment will be zero-initialized, 0 is a valid type for std::num::NonZeroU8:

use nix::sys::{
    stat::Mode,
    system_v::shm::{Shm, ShmatFlag},
};

fn main() {
    let mode = Mode::S_IRWXU;

    let shm = Shm::<std::num::NonZeroU8>::create_and_connect(65536, mode).unwrap();
    let mem_segment = shm.attach(ShmatFlag::empty()).unwrap();
    println!("number = {}", *mem_segment);
}

$ cargo r                      
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/rust`
number = 0