Close file descriptors of pipes after use

[blyxxyz]

nix::unistd::pipe returns RawFds which have to be closed at some point, otherwise they are leaked. Wrapping them in Files does the trick.

The leaked file descriptors can be observed by running uucat /dev/null /dev/null /dev/null - in one terminal (the - ensures it doesn't exit immediately) and then running lsof -p $(pidof uucat) in another.

It's possible to implement this without the unsafe keyword by writing a wrapper that calls nix::unistd::close when it's dropped, but that function is technically also unsafe and probably will be marked as such in the future, so it's better to be upfront. See nix-rust/nix#1421.

[miDeb]

Thanks, this looks good! Could you add a test? I fixed a similar bug some time ago (#2330), and the test I added looked like this:

#[cfg(target_os = "linux")]
#[test]
fn test_closes_file_descriptors() {
    new_ucmd!()
        .arg("-r")
        .arg("--reflink=auto")
        .arg("dir_with_10_files/")
        .arg("dir_with_10_files_new/")
        .with_limit(Resource::NOFILE, 9, 9)
        .succeeds();
}

The important part is .with_limit(Resource::NOFILE, 9, 9), which limits the utility to max. 9 file descriptors. The test then tries to do something with a directory of 10 files, which can only succeed if file descriptors are closed regularly.

Would it be possible to add a similar test for the bugs you fixed? It's not extremely important, but it's nice to have regression tests, especially for subtle bugs like this.

[blyxxyz]

Good idea, I hadn't thought of resource limits.

Both cat and wc were actually set up to fall back if pipe() fails, which makes this hard to test. I think it's ok to change that, pipe() only fails in exceptional circumstances. But in the case of wc it'd require overhauling the control flow, and I don't know if that's worth it.

This is the test I'd write otherwise, for posterity:

#[test]
#[cfg(target_os = "linux")]
fn test_closes_file_descriptors() {
    new_ucmd!()
        .args(&[
            "-c",
            "/dev/stdin",
            "/dev/stdin",
            "/dev/stdin",
            "/dev/stdin",
            "/dev/stdin",
        ])
        .with_limit(Resource::NOFILE, 9, 9)
        .succeeds();
}

[miDeb]

Thanks!