clone() is broken and unsafe with CLONE_VM flag

[arcnmx]

clone() has a few issues when used with CLONE_VM, unless it is used together with CLONE_VFORK.

1. it takes a Box<FnMut> by value, which will be moved, dropped, and destroyed when the function returns back to the caller.
   • essentially means it will unconditionally segfault when passed CLONE_VM
2. Even if it did work, the mutable borrow of the stack will end once the clone() call returns, even if the cloned process is still running, meaning you can modify the child process' stack as it runs.
3. There's no way to provide the thread/TLS/etc. parameters for the thread-related flags.

I was actually going to suggest that its prototype be changed to an unboxed FnOnce, since the caller's stack will never fall out from under you unless CLONE_VM is used... but I think it also needs to disallow the CLONE_VM flag entirely (unless CLONE_VM | CLONE_VFORK). Perhaps offer a second unsafe version for that use.

[bugaevc]

clone() also doesn't impose any limitations on the callback -- it can be !Send, it can reference variables that go out of scope as soon as the clone() call is made... basically, clone() enables every unsafety Rust was built to prevent. This is a very serious soundness hole; for a lack of a better solution I propose to make clone() unsafe ASAP.

[Susurrus]

The point of nix is to provide safe wrappers around a lot of these primitives. The final suggestion from @arcnmx seems like it's a safe version, even if it has reduced functionality. I think we should offer that use case instead and punt on a complete but safe version for now, unless someone objects to that.

[Susurrus]

Coming back to this as I'd like to fix up some bugs, I think having both an unsafe and safe-but-restricted clone makes a lot of sense here. @arcnmx it seems like you have an understanding of what the API should be, would you be able to whip up a PR for this?

@bugaevc Is there anything we should do to the existing clone() function besides make it unsafe? I'd also propose a rename to clone_unsafe or something along those lines so that the safe version can be clone() and it will encourage people to use the safe one over the unsafe one.

[bugaevc]

Yes, we could make it accept the same kind of closures thread::spawn accepts: Send + 'static. That itself would be safe, and if anyone wants to use a non-Send or non-'static closure they can do the unsafe dance on top of that (e.g. by casting any reference to and from a *const u8).

But then if a clone() invocation doesn't use CLONE_VM, it can be safely used with a non-Send closure, because memory-wise it's just like fork()+call. So it may indeed be better to provide two versions.

[bugaevc]

Something like this:

extern "C" fn clone_cb<F>(data: *mut libc::c_void) -> *mut libc::c_void
    where F: FnOnce() -> isize
{
    let bf = unsafe {
        Box::from_raw(data as *mut F);
    };
    (*bf)()
    // (boxed) closure data gets freed here
}

pub fn clone<F>(f: F, other_args: OtherArgs) -> Result<Pid>
    where F: FnOnce() -> isize + 'static
{
    // box the closure data
    let bf = Box::new(f);
    // leave it on the heap
    let p = Box::into_raw(bf) as *mut libc::void;
    let res = unsafe {
        libc::clone(clone_cb::<F>, /* data */ p, other_args);
    };
    make_nice_rusty_result(res)
}

pub fn clone_with_VM<F>(f: F, other_args: OtherArgs) -> Result<Pid>
     where F: FnOnce() -> isize + 'static + Send
{
    // same, or wrap clone()
}