libnative: process spawning should not close inherited file descriptors

[ipetkov]

When creating a subprocess via std::io::process::Command it is possible to allow the child to inherit certain open file descriptors. When the process is spawned (in native::io::process), however, the file descriptor is unconditionally closed. This is an error for the following reasons:

1. The caller can safely create a file descriptor via native::io::file::open, which returns a FileDesc on success. Since there is no public interface for specifying that the internal fd should NOT be cleaned up, the FileDesc structure will attempt to close the fd when it is destroyed. Since spawning closes the file descriptor, when the FileDesc structure mentioned above goes out of scope, it will print an error message to screen. This behavior may be unwanted, and there is no way to prevent it without leaking the FileDesc structure (which is not a good alternative).
2. Alternatively, it is possible for another file to be opened whose fd matches that stored by the FileDesc structure returned by native::io::file::open before said structure goes out of scope. Thus when it does, it will incorrectly succeed at closing the file, causing unexpected errors for the task that opened the file.
3. The caller may not wish to close the file descriptor just yet. Imagine a scenario where a lock file is created by a child process and unlinked when it exits. The parent may wish to keep the file on disk until it exits so that any other process looking for that lock file will see it. When it finally does exit and close its handle to the file, the OS will clean it up then.

Thus the caller must be solely responsible for cleaning up (or leaking if so desired) any opened file descriptors, especially since the std::io::process:Command interface for adding inherited file descriptors involves using a literal descriptor and not a wrapper like FileDesc.

[ipetkov]

Sample program that demonstrates scenario 1 above:

extern crate native;
extern crate rustrt;

fn main() {
    let fdes = match native::io::file::open(&"output.txt".to_c_str(), rustrt::rtio::Open, rustrt::rtio::Write) {
        Ok(f) => f,
        Err(_) => fail!("failed to open file"),
    };

    let mut cmd = std::io::process::Command::new("echo");
    cmd.arg("hello");
    cmd.arg("world");
    cmd.stdout(std::io::process::InheritFd(fdes.fd()));

    let mut process = match cmd.spawn() {
        Ok(p) => p,
        Err(e) => fail!("failed to spawn process: {}", e),
    };

    match process.wait() {
        Ok(s) => println!("subprocess exited with status: {}", s),
        Err(e) => fail!("failed to wait on process: {}", e),
    }

    // fdes destroyed here
}

Output:

subprocess exited with status: exit code: 0
error -1 when closing file descriptor 3

[ipetkov]

Perhaps std::io::process::Command could be given an interface to set a file descriptor for a child via a file path, where it would internally create a FileDesc object and close it after the process was spawned, thus safely freeing up resources after spawning.

This approach, however, would require creating the FileDesc object via libnative, causing a nasty circular dependency between the libstd and libnative...

[alexcrichton]

This seems like it can definitely cause badness, nice catch! Just a few requests and this should be good to go:

• Can you update the documentation for InheritFd to explicitly say that it does not take ownership of the file descriptor?
• Can you add a test for this functionality?

[ipetkov]

@alexcrichton done, and done!

I added the test under libnative since I didn't want to add a dependency to it from std, even though it is technically testing commands from std.

Also I apparently messed up rebasing and pushing the branch last night and pushed an existing commit, making bors think the branch was already merged. Since I cant seem to be able to reopen the PR I submitted a new one here: #16421

Sorry about the confusion!