Ensure stdio handles are never null when converting to HandleRef

[ChrisDenton]

This fixes this test case, which means env_logger will no longer panic on stable. I'm not totally happy with this change because it's a workaround rather than a real fix. But I think something like this is the best the can be done without changing the API or how people use it.

[BurntSushi]

I don't love it either, but I think this at least re-capitulates the state of things from before, so I'm inclined to move forward. The thing that especially bugs me about this is the silent failure mode.

I guess we should eventually make the stdin/stdout/stderr APIs return a Result, but that really just passes the buck on up.

[BurntSushi]

My guess is that trying to write to such a handle will just result in nothing happening, right? And I think that's what the behavior was before this change, so I guess this doesn't make anything worse.

[ChrisDenton]

It should be an Err when used rather than a panic when created, which was the status quo before the standard library added the assert.

[BurntSushi]

Oh! That's great then. Non-silent failure mode. That makes me happier.

[BurntSushi]

Can we include a note in the stdin/stdout/stderr docs here that say what the behavior here is when stdin/stdout/stderr aren't available? I think we should also specifically mention the Windows GUI subsystem thing, to make it particularly concrete.

[ChrisDenton]

I've added some documentation. I hedged slightly, saying using such a handle is "likely" to fail because I'm not sure I can guarantee the behaviour of every file function in the Windows API, now and in the future.

[sunfishcode]

Could you comment on how this fits into the big picture? The linked testcase is now fixed with a patch that is now in Rust nightly, and we expect to backport it to beta and possibly stable. Are you expecting to revert this patch when that fix reaches Rust stable, or are you expecting to keep it?

The issues around NULL and INVALID_HANDLE_VALUE in the Windows API are confusing as it is, and I'm a little nervous about this patch silently translating from NULL to INVALID_HANDLE_VALUE, introducing another potential moving part into that system.

[ChrisDenton]

Sure. As I said in the OP I'm unhappy with this PR so I'll start by explaining why before explaining my reasoning for doing it. This is likely not anything new to you but I want to be clear to everyone.

A File is a "reference to an open file on the filesystem" and from_raw_handle states its contract is that "they are the sole owner of the file descriptor they are wrapping". winapi-util breaks this by directly converting a raw stdio handle value into a &File without checks. The I/O unsafety is somewhat mitigated by not dropping the resulting File but it's still not particularly good. A panic in this situation is arguably a good thing.

Ultimately the fix for this is to make a breaking change to the API of winapi-util so that it can return an error at some point. Maybe it should use DuplicateHandle to create an owned handle? This would also ensure the handle is valid. Whatever the case, if this PR is merged I do not intend it to outlive any future API changes.

So given all that, why did I submit this PR? Because this appears to be affecting numerous applications right now. And this comment indicated that the upstream patch may not be backported to stable. I'd also note that neither this PR nor the upstream patch prevents the I/O unsafety in this specific case, it just removes the panic.

The point about translating null to INVALID_HANDLE_VALUE is a good one. I chose that value because:

• It's already a value people should expect for stdio.
• Rust itself currently does something very similar when spawning processes (see here and here).

[sunfishcode]

Brainstorming a little here: if it makes sense for winapi-util to translate NULL to INVALID_HANDLE_VALUE here, would it make sense to go even further and make io::Stdout::as_raw_handle etc. translate NULL into INVALID_HANDLE_VALUE? That way, it'd help other libraries besides winapi-util here. And, as you note, it is what Rust already does when spawning processes. And that also might allow us to go back to using NonNull inside OwnedHandle and BorrowedHandle, and eliminate HandleOrNull in favor of Option<OwnedHandle> again.

A downside might be backwards compatibility. Any code expecting to be able to detect "windows" mode or detached consoles by checking for NULL would no longer be able to distinguish that case.

A File is a "reference to an open file on the filesystem" and from_raw_handle states its contract is that "they are the sole owner of the file descriptor they are wrapping". winapi-util breaks this by directly converting a raw stdio handle value into a &File without checks. The I/O unsafety is somewhat mitigated by not dropping the resulting File but it's still not particularly good. A panic in this situation is arguably a good thing.

It's my impression that this kind of "temporary File" idiom isn't specific to winapi-util. As such, I'm expecting we'll revise the contract for from_raw_handle to make this idiom valid. It'll still be unsafe, and require the programmer take responsibility for not closing the handle twice, but there would be a way to do it. I'm curious what you think about this approach.

[ChrisDenton]

if it makes sense for winapi-util to translate NULL to INVALID_HANDLE_VALUE here, would it make sense to go even further and make io::Stdout::as_raw_handle etc. translate NULL into INVALID_HANDLE_VALUE

Perhaps! To be honest though I'm not totally on board with Rust's process spawning behaviour either. I do think it should pass on NULL values because that can have a different meaning to INVALID_HANDLE_VALUE (the first is presumably intentional whereas the latter is likely the result of an error somewhere). Admittedly, I'm not sure if the difference matters too much to the subprocess. Another gotcha (which has be brought up a few times before) is that INVALID_HANDLE_VALUE just so happens to share a value with the current process pseudo handle. In the context of stdio it should be obvious what was meant but when used from a generic as_raw_handle method I could see how it might potentially be misused.

Hm, I think in general a generic as_raw_handle is a bit of a footgun because it's so reliant on context for meaning. Maybe fn try_as_io_handle() -> io::Result<IoHandle> might be better for general purpose use? That makes it clear that, if successful, you've actually got a file-like handle and not some pseudo handle or null result.

It's my impression that this kind of "temporary File" idiom isn't specific to winapi-util. As such, I'm expecting we'll revise the contract for from_raw_handle to make this idiom valid.

I would be on board with that in principle. And, yeah, I suspect it's a common enough pattern. Of course there might be something I'm not thinking of here so I don't want to say "yes definitely" until I'm more certain there are no hidden gotchas.

As mentioned, a safer way to share handles is using DuplicateHandle. The kernel does reference counting for file objects so it should be safe to create duplicate Files this way. Admittedly this is slightly more heavy weight then just copying a handle around but I could imagine a safe clone_file_handle would be useful.

Edit: Well the std actually does have try_clone specifically for File handles but I think not for other I/O handles.

[ChrisDenton]

So to avoid going too far off the topic of winapi-util, I imagine the lowest level of the API might look something like this:

pub trait AsIoHandle {
    /// Returns a raw handle to an I/O object such as a file or pipe.
    ///
    /// For types like [`File`] that are always valid file-like objects, this
    /// is simply a wrapper for [`AsRawHandle`]. However stdio types (such as
    /// [`io::Stdout`]) may fail if there is no handle or the handle is invalid.
    /// Most commonly this will occur when an application is using the Windows
    /// GUI subsystem.
    fn as_io_handle(&self) -> io::Result<RawHandle>;
}

impl AsIoHandle for io::Stdout {
    fn as_io_handle(&self) -> io::Result<RawHandle> {
        validate_stdio_handle(self.as_raw_handle())
    }
}

fn validate_stdio_handle(handle: RawHandle) -> io::Result<RawHandle> {
    // Handle values with the high bit set (i.e. `< 0`) are pseudo handles.
    // These are magic values whose meaning is highly context dependent; each
    // function may interpret them differently or not at all. It doesn't make
    // sense to own, borrow or use these handles in a generic I/O context.
    // 
    // A null handle is never valid.
    if (handle as isize) <= 0 {
        Err(io::Error::new(io::ErrorKind::Other, "the handle is invalid"))
    } else {
        Ok(handle)
    }
}

Where AsIoHandle can be implemented for File, the stdio types and the pipe types used with Command. Other types could then build on this.

[sunfishcode]

@ChrisDenton The original bug here was fixed on Rust stable in Rust 1.57, so I believe we can close this issue now.

And the discussion of NULL handles moved here rust-lang/rust#90964, which led to rust-lang/rust#93263.