Closed standard file descriptors don't generate errors anymore on Unix

[Alphare]

I've found that #75295 introduced a regression in the "Rust first then fallback to Python" version of the Mercurial test suite.

I was testing if going from 1.41.1 (previous Debian stable version) to 1.48.0 broke anything, and it appears it did. This test checks that closed file descriptors raise an acceptable error, and this does not happen anymore. Here is the fallback code for rhg, running the Python version of hg as a subprocess.

Here is the (somewhat convoluted) reproduction, which requires python be a valid Python 3 interpreter that I used with cargo bisect-rust (thanks @SimonSapin for the pointer on that tool btw):

//! Run the executable (not with `cargo play`, it'll eat the redirect)
//!     `myexecutable -c 'import sys; sys.stdout.write("hello")' 1>&-`
//! If compiled with 1.41.1, it will output (with an error code of 1):
//!
//! Traceback (most recent call last):
//!  File "<string>", line 1, in <module>
//!  AttributeError: 'NoneType' object has no attribute 'write'
//!
//! But with 1.48.0, it doesn't output anything and silently eats the error code,
//! exiting with 0.

use std::process::Command;

fn main() {
    let mut args_os = std::env::args_os();
    args_os.next().expect("expected argv[0] to exist");
    let mut c = Command::new("python");
    c.args(args_os);
    let status = c.status();
    match status {
        Err(_) => (),
        Ok(s) =>  {
            if s.success() {
                std::process::exit(1);
            }
        }
    }
}

I'm not 100% sure what to think of it yet, but my gut feeling is that eating errors is bad; I'm reporting this now since it's fresh in my mind.

[the8472]

I wonder why bash even lets you close the output descriptors when spawning a child, that seems like a direct violation of posix requirements that spawned processes must have fds 0-2 open at program startup.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/stdout.html

[SimonSapin]

I think this falls in this drawback noted in #75295:

The standard descriptors might have been closed intentionally.

It’s intentional in this test case, but the point of that test is ensuring Mercurial reports an error for unintentional closing instead of silently losing output.

[Alphare]

Some more background on why the test was introduced to Mercurial in the first place: https://www.mercurial-scm.org/repo/hg/rev/a04c03b0678e

[Alphare]

I wonder why bash even lets you close the output descriptors when spawning a child, that seems like a direct violation of posix requirements that spawned processes must have fds 0-2 open at program startup.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/stdout.html

This is actually valid in POSIX shell: https://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_07_05. This may constitute a contradiction, but there appears to be no explicit mention of standard fds.

[tmiasko]

With changes from #75295 the standard file descriptors are now reopened to
/dev/null if not present. Standard descriptors are reopened without close on
exec flag, so in this scenario they will be inherited by the spawned process.
Consequently, the spawned process will be able to successfully complete write
operation and with status code of zero. No errors ever arise.

Generally the fact that they are reopened and inherited is expected outcome.
Although expectation was that in most cases you wouldn't be able to tell the
difference between closed file descriptors and reopened to /dev/null, given
that as specified in RFC 1014, the Rust standard streams ignore the EBADF
error. Hence extra setup with Command necessary to observe this.

Additionally, I think it might be a bad idea to rely on being able to detect
closed standard streams, since for example glibc will reopen standard file
descriptors when running as privileged process anyway.

I will also note that Command in the Rust standard library works incorrectly
when standard file descriptors are closed. In particular, the implementation
based on fork crates an internal pipe for convening errors after fork. If one
those descriptors will be assigned to descriptors 0, 1 or 2, they might get
clobbered and process will appear to spawn successfully when it in fact failed.

[the8472]

Maybe CommandExt::pre_exec can be used to work around this by closing the file descriptors after the fork.

[Alphare]

This is actually basically the same issue as #47271, just with a more recent development.

[Alphare]

Maybe CommandExt::pre_exec can be used to work around this by closing the file descriptors after the fork.

It's not possible to detect closed file descriptors from Rust since the /dev/null fix is applied before main is executed. I'm sending a patch to Mercurial to basically say "we can't do anything about it so it's going to differ in the two versions unless Rust changes its mind about the issue". Interestingly, Python 2 went from silently eating the issue as well to (in Python 3) setting the relevant io stream to None, which breaks everything that is relying on it not being None. I'm still of the opinion that we shouldn't re-open the fds in Rust or in glibc, but that's probably dead in the water.

[the8472]

I wonder why bash even lets you close the output descriptors when spawning a child, that seems like a direct violation of posix requirements that spawned processes must have fds 0-2 open at program startup.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/stdout.html

This is actually valid in POSIX shell: https://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_07_05. This may constitute a contradiction, but there appears to be no explicit mention of standard fds.

The 2017 version resolves this contradiction

If the utility would be executed with file descriptor 0, 1, or 2 closed, implementations may execute the utility with the file descriptor open to an unspecified file. If a standard utility or a conforming application is executed with file descriptor 0 not open for reading or with file descriptor 1 or 2 not open for writing, the environment in which the utility or application is executed shall be deemed non-conforming