Skip to content

Ensure stdio handles are never null when converting to HandleRef #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Ensure stdio handles are never null when converting to HandleRef #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22ac515
Ensure stdio handles are never null when converting to `HandleRef`
ChrisDenton Nov 12, 2021
df84205
Document behaviour when an stdio handle is missing
ChrisDenton Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ features = [
"consoleapi",
"errhandlingapi",
"fileapi",
"handleapi",
"minwindef",
"processenv",
"winbase",
Expand Down
45 changes: 42 additions & 3 deletions src/win.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::os::windows::io::{
};
use std::path::Path;
use std::process;
use winapi::um::handleapi::INVALID_HANDLE_VALUE;

/// A handle represents an owned and valid Windows handle to a file-like
/// object.
Expand Down Expand Up @@ -117,26 +118,64 @@ impl Clone for HandleRef {
}
}

/// Ensures the handle is never null by converting null values to the
/// pseudo-handle `INVALID_HANDLE_VALUE`.
fn nonnull_handle<T: AsRawHandle>(stdio: T) -> RawHandle {
let handle = stdio.as_raw_handle();
if handle.is_null() {
INVALID_HANDLE_VALUE
} else {
handle
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ChrisDenton ChrisDenton Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


impl HandleRef {
/// Create a borrowed handle to stdin.
///
/// When the returned handle is dropped, stdin is not closed.
///
/// If there is no stdin handle then `INVALID_HANDLE_VALUE` will be used
/// as the underlying file handle. This is likely to cause any use of the
/// handle to fail.
///
/// The handle will most commonly be missing in GUI applications using the
/// ["windows" subsystem][subsystem].
///
/// [subsystem]: https://doc.rust-lang.org/reference/runtime.html?#the-windows_subsystem-attribute
pub fn stdin() -> HandleRef {
unsafe { HandleRef::from_raw_handle(io::stdin().as_raw_handle()) }
unsafe { HandleRef::from_raw_handle(nonnull_handle(io::stdin())) }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// Create a handle to stdout.
///
/// When the returned handle is dropped, stdout is not closed.
///
/// If there is no stdout handle then `INVALID_HANDLE_VALUE` will be used
/// as the underlying file handle. This is likely to cause any use of the
/// handle to fail.
///
/// The handle will most commonly be missing in GUI applications using the
/// ["windows" subsystem][subsystem].
///
/// [subsystem]: https://doc.rust-lang.org/reference/runtime.html?#the-windows_subsystem-attribute
pub fn stdout() -> HandleRef {
unsafe { HandleRef::from_raw_handle(io::stdout().as_raw_handle()) }
unsafe { HandleRef::from_raw_handle(nonnull_handle(io::stdout())) }
}

/// Create a handle to stderr.
///
/// When the returned handle is dropped, stderr is not closed.
///
/// If there is no stderr handle then `INVALID_HANDLE_VALUE` will be used
/// as the underlying file handle. This is likely to cause any use of the
/// handle to fail.
///
/// The handle will most commonly be missing in GUI applications using the
/// ["windows" subsystem][subsystem].
///
/// [subsystem]: https://doc.rust-lang.org/reference/runtime.html?#the-windows_subsystem-attribute
pub fn stderr() -> HandleRef {
unsafe { HandleRef::from_raw_handle(io::stderr().as_raw_handle()) }
unsafe { HandleRef::from_raw_handle(nonnull_handle(io::stderr())) }
}

/// Create a borrowed handle to the given file.
Expand Down