Windows: add context when opening NUL for child stdio fails#159425
Conversation
A failed open of \\.\NUL for Stdio::Null surfaced as a bare OS error (usually NotFound), which reads as if the spawned program is missing. Wrap it with the operation and stream name, preserving the ErrorKind.
rustbot
added
S-waiting-on-review
|
r? @Darksonn rustbot has assigned @Darksonn. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I would also like to hear an answer to #157049 (comment). I'm concerned that the issue may have been misdiagnosed. |
|
The report does isolate the failing call: it says "After investigation, the missing path was not Environments where the NUL open fails do exist. The device is backed by the Either way the change is scoped to the NUL open itself: it only wraps the error from that one call, so it can't mislabel an unrelated failure, and nothing changes when NUL opens fine. |
| let stream = match stdio_id { | ||
| c::STD_INPUT_HANDLE => "stdin", | ||
| c::STD_OUTPUT_HANDLE => "stdout", | ||
| _ => "stderr", |
There was a problem hiding this comment.
Is there really no possibility of stdio_id being something other than these three values? Perhaps we should handle unknown values by printing the integer value instead of printing stderr when it might not be stderr?
There was a problem hiding this comment.
Today it can't be anything else (the only callers are the three spawn lines passing the constants), but you're right that the arm shouldn't claim stderr for values it doesn't know. Changed it to print the raw id for anything unexpected.
There was a problem hiding this comment.
The original issue still seems weird, but returning a better error in this case is probably a good idea regardless.
@bors r+
rust-bors
Bot
added
S-waiting-on-bors
…uwer Rollup of 8 pull requests Successful merges: - #159425 (Windows: add context when opening NUL for child stdio fails) - #159467 (Add explicit `Iterator::count` impl for `str::EncodeUtf16`) - #157860 (Move rustdoc run-make tests into new `tests/run-make/rustdoc/`) - #158545 (Move `std::io::read_to_string` to `alloc::io`) - #159328 (tests/assembly-llvm: pin frame pointer in issue-141649 aarch64 test) - #159459 (Detect when trait bound requires closure to return itself) - #159470 (Fix string indexing in diagnostic format strings) - #159500 (Move compiletest CLI parsing to `cli.rs`)
Rollup merge of #159425 - Joel-Wwalker:157049-nul-error-context, r=Darksonn Windows: add context when opening NUL for child stdio fails Opening `\\.\NUL` for `Stdio::Null` can fail in some environments, and `Command::spawn`/`output()` passes the raw OS error through. In the reported case that was `NotFound`, which looks exactly like the program itself being missing. Add context saying what failed to open and for which stream: ``` failed to open NUL device for child stdin: The system cannot find the file specified. (os error 2) ``` The `ErrorKind` is kept. `raw_os_error()` on this path becomes `None` since the payload is now a message string. I don't see a way to make opening NUL fail in CI, so no test; verified by hand in a local build by pointing the open at a bogus device name and checking the error out of `Command::output()`. Fixes #157049
…uwer Rollup of 8 pull requests Successful merges: - rust-lang/rust#159425 (Windows: add context when opening NUL for child stdio fails) - rust-lang/rust#159467 (Add explicit `Iterator::count` impl for `str::EncodeUtf16`) - rust-lang/rust#157860 (Move rustdoc run-make tests into new `tests/run-make/rustdoc/`) - rust-lang/rust#158545 (Move `std::io::read_to_string` to `alloc::io`) - rust-lang/rust#159328 (tests/assembly-llvm: pin frame pointer in issue-141649 aarch64 test) - rust-lang/rust#159459 (Detect when trait bound requires closure to return itself) - rust-lang/rust#159470 (Fix string indexing in diagnostic format strings) - rust-lang/rust#159500 (Move compiletest CLI parsing to `cli.rs`)
3 participants
Opening
\\.\NULforStdio::Nullcan fail in some environments, andCommand::spawn/output()passes the raw OS error through. In the reported case that wasNotFound, which looks exactly like the program itself being missing. Add context saying what failed to open and for which stream:The
ErrorKindis kept.raw_os_error()on this path becomesNonesince the payload is now a message string. I don't see a way to make opening NUL fail in CI, so no test; verified by hand in a local build by pointing the open at a bogus device name and checking the error out ofCommand::output().Fixes #157049