Improper use could cause undefined behaviour about command-fds HOT 6 CLOSED

Thanks for the detailed bug report!

Thinking things through a bit more, it looks like I was wrong about there being an issue with running the same command more than once after calling fd_mappings. I was thinking that map_fds would end up modifying the same instance of mappings, but actually each fork has its own copy (along with the rest of the process address space) so this should be fine. I'll remove the warning about that.

I don't remember exactly why I added the warning about calling fd_mappings twice, but as far as I can see the only issue is that they might have mappings with conflicting child_fds which we can't detect. This will result in undesirable behaviour (one of them will be lost) but not undefined behaviour, so I don't think this method needs to be unsafe.

As for using RawFd in FdMappings, OwnedFd and BorrowedFd didn't exist in stable when I originally wrote this, but it does look like they could be useful. That's probably going to require more of a redesign unfortunately but I'll look into it. If you have ideas feel free to send a PR.

from command-fds.

I think FdMapping.parent_fd would ideally be a BorrowedFd, and CommandFdExt::preserved_fds should also take BorrowedFds. The problem is that they should have a lifetime that lasts at least as long as the Command, but there is no way of expressing this. Requiring them to have a 'static lifetime would be correct but not useful, as most BorrowedFds are constructed from an OwnedFd with a finite lifetime.

Taking OwnedFd instead would avoid the lifetime issues, but may require the caller to duplicate the file descriptor first, which is not ideal.

from command-fds.

On further reflection I think OwnedFd is reasonable, as it's unlikely that the parent process would want to use the file descriptors further after spawning a child with them, and if it really wants to it can dup them. Please take a look at #23 and #25.

from command-fds.

On further reflection I think OwnedFd is reasonable, as it's unlikely that the parent process would want to use the file descriptors further after spawning a child with them, and if it really wants to it can dup them. Please take a look at #23 and #25.

I also think taking OwnedFd is the right solution in this case, due to the lack of a lifetime annotation on the Command struct. I would just suggest documenting when the OwnedFd is dropped (and so when is the file descriptor closed).

Consider this example, which uses the os_pipe crate:

fn get_fd3_output() -> Vec<u8> {
    let (mut reader, writer) = os_pipe::pipe().unwrap();
    
    // Let's assume we have a "my_bin" executable which doesn't take any inputs
    // and it "prints" it's output to FD3.
    // Let's also assume the output is very long and will not fit in the pipe buffer.
    let command = Command::new("my_bin")
        .fd_mappings(vec![FdMapping {
            parent_fd: writer.into(),
            child_fd: 3
        }]).expect("FD mappings should be valid")
        .stdout(Stdio::null())
        .stderr(Stdio::null())
        .stdin(Stdio::null());
    
    let mut child = command.spawn().unwrap();
    
    let mut buf: Vec<u8> = vec![];
    // The .read_to_end method will block until it receives an EOF
    // But we will not see EOF until all FDs to the writeable end of the pipe are closed
    // If the OwnedFd is not dropped at this point, a deadlock will occur!
    reader.read_to_end(&mut buf);
    
    child.wait().unwrap();
    buf
}

Would the behaviour be any different if we were to do drop(command) earlier?

fn get_fd3_output_v2() -> Vec<u8> {
    let (mut reader, writer) = os_pipe::pipe().unwrap();

    let command = Command::new("my_bin")
        .fd_mappings(vec![FdMapping {
            parent_fd: writer.into(),
            child_fd: 3
        }]).expect("FD mappings should be valid")
        .stdout(Stdio::null())
        .stderr(Stdio::null())
        .stdin(Stdio::null());
    
    let mut child = command.spawn().unwrap();
    drop(command); // <-- will this drop the inner OwnedFd?
    
    let mut buf: Vec<u8> = vec![];
    // If the OwnedFd is not dropped at this point, a deadlock will occur!
    reader.read_to_end(&mut buf);
    
    child.wait().unwrap();
    buf
}

from command-fds.

Good point. Added documentation of this to #25.

from command-fds.

Fixed by #23 and #25.

from command-fds.