From 4ce68c13bfbd88df8d93da676e8424544c27321e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 May 2022 21:20:34 -0700 Subject: [PATCH 1/6] Clarify what values `BorrowedHandle`, `OwnedHandle` etc. can hold. Clarify that when `BorrowedHandle`, `OwnedHandle`, or `HandleOrNull` hold the value `-1`, it always means the current process handle, and not `INVALID_HANDLE_VALUE`. --- library/std/src/os/windows/io/handle.rs | 29 +++++++++++++++---------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 290b7f0d08a72..b80ea0ac5eeb3 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -22,8 +22,9 @@ use crate::sys_common::{AsInner, FromInner, IntoInner}; /// so it can be used in FFI in places where a handle is passed as an argument, /// it is not captured or consumed. /// -/// Note that it *may* have the value `INVALID_HANDLE_VALUE` (-1), which is -/// sometimes a valid handle value. See [here] for the full story. +/// Note that it *may* have the value `-1`, which in `BorrowedFd` always +/// represents the current process handle, and not `INVALID_HANDLE_VALUE`, +/// despite the two having the same value. See [here] for the full story. /// /// And, it *may* have the value `NULL` (0), which can occur when consoles are /// detached from processes, or when `windows_subsystem` is used. @@ -45,8 +46,9 @@ pub struct BorrowedHandle<'handle> { /// /// This closes the handle on drop. /// -/// Note that it *may* have the value `INVALID_HANDLE_VALUE` (-1), which is -/// sometimes a valid handle value. See [here] for the full story. +/// Note that it *may* have the value `-1`, which in `OwnedFd` always +/// represents the current process handle, and not `INVALID_HANDLE_VALUE`, +/// despite the two having the same value. See [here] for the full story. /// /// And, it *may* have the value `NULL` (0), which can occur when consoles are /// detached from processes, or when `windows_subsystem` is used. @@ -75,11 +77,11 @@ pub struct OwnedHandle { /// `NULL`. This ensures that such FFI calls cannot start using the handle without /// checking for `NULL` first. /// -/// This type considers any value other than `NULL` to be valid, including `INVALID_HANDLE_VALUE`. -/// This is because APIs that use `NULL` as their sentry value don't treat `INVALID_HANDLE_VALUE` -/// as special. +/// This type may hold any handle value that [`OwnedFd`] may hold, except `NULL`. It may +/// hold `-1`, even though `-1` has the same value as `INVALID_HANDLE_VALUE`, because in +/// `HandleOrNull`, `-1` is interpreted to mean the current process handle. /// -/// If this holds a valid handle, it will close the handle on drop. +/// If this holds a non-null handle, it will close the handle on drop. #[repr(transparent)] #[unstable(feature = "io_safety", issue = "87074")] #[derive(Debug)] @@ -95,11 +97,14 @@ pub struct HandleOrNull(OwnedHandle); /// `INVALID_HANDLE_VALUE`. This ensures that such FFI calls cannot start using the handle without /// checking for `INVALID_HANDLE_VALUE` first. /// -/// This type considers any value other than `INVALID_HANDLE_VALUE` to be valid, including `NULL`. -/// This is because APIs that use `INVALID_HANDLE_VALUE` as their sentry value may return `NULL` -/// under `windows_subsystem = "windows"` or other situations where I/O devices are detached. +/// This type may hold any handle value that [`OwnedFd`] may hold, except `-1`. It must not +/// hold `-1`, because `-1` in `HandleOrInvalid` is interpreted to mean `INVALID_HANDLE_VALUE`. /// -/// If this holds a valid handle, it will close the handle on drop. +/// This type may hold `NULL`, because APIs that use `INVALID_HANDLE_VALUE` as their sentry value +/// may return `NULL` under `windows_subsystem = "windows"` or other situations where I/O devices +/// are detached. +/// +/// If holds a handle other than `INVALID_HANDLE_VALUE`, it will close the handle on drop. #[repr(transparent)] #[unstable(feature = "io_safety", issue = "87074")] #[derive(Debug)] From 2bb7fdb8e16788367ea4a3403ac405b8bc614cc6 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 May 2022 21:42:30 -0700 Subject: [PATCH 2/6] Also document that `as_raw_handle` may return NULL. --- library/std/src/os/windows/io/raw.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/std/src/os/windows/io/raw.rs b/library/std/src/os/windows/io/raw.rs index 68fa8918a56a0..49e4f304f5dba 100644 --- a/library/std/src/os/windows/io/raw.rs +++ b/library/std/src/os/windows/io/raw.rs @@ -32,8 +32,15 @@ pub trait AsRawHandle { /// raw handle to the caller, and the handle is only guaranteed /// to be valid while the original object has not yet been destroyed. /// + /// This function may return null, such as when called on [`Stdin`], + /// [`Stdout`], or [`Stderr`] when the console is detached. + /// /// However, borrowing is not strictly required. See [`AsHandle::as_handle`] /// for an API which strictly borrows a handle. + /// + /// [`Stdin`]: io::Stdin + /// [`Stdout`]: io::Stdout + /// [`Stderr`]: io::Stderr #[stable(feature = "rust1", since = "1.0.0")] fn as_raw_handle(&self) -> RawHandle; } From 0a39e5ad36419c2b257a91705ac1337f18a5e08f Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 May 2022 21:56:51 -0700 Subject: [PATCH 3/6] Fix incorrect mentions of `OwnedFd` and `BorrowedFd` in Windows docs. --- library/std/src/os/windows/io/handle.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index b80ea0ac5eeb3..ef2ef10e05fea 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -22,7 +22,7 @@ use crate::sys_common::{AsInner, FromInner, IntoInner}; /// so it can be used in FFI in places where a handle is passed as an argument, /// it is not captured or consumed. /// -/// Note that it *may* have the value `-1`, which in `BorrowedFd` always +/// Note that it *may* have the value `-1`, which in `BorrowedHandle` always /// represents the current process handle, and not `INVALID_HANDLE_VALUE`, /// despite the two having the same value. See [here] for the full story. /// @@ -46,7 +46,7 @@ pub struct BorrowedHandle<'handle> { /// /// This closes the handle on drop. /// -/// Note that it *may* have the value `-1`, which in `OwnedFd` always +/// Note that it *may* have the value `-1`, which in `OwnedHandle` always /// represents the current process handle, and not `INVALID_HANDLE_VALUE`, /// despite the two having the same value. See [here] for the full story. /// @@ -77,7 +77,7 @@ pub struct OwnedHandle { /// `NULL`. This ensures that such FFI calls cannot start using the handle without /// checking for `NULL` first. /// -/// This type may hold any handle value that [`OwnedFd`] may hold, except `NULL`. It may +/// This type may hold any handle value that [`OwnedHandle`] may hold, except `NULL`. It may /// hold `-1`, even though `-1` has the same value as `INVALID_HANDLE_VALUE`, because in /// `HandleOrNull`, `-1` is interpreted to mean the current process handle. /// @@ -97,7 +97,7 @@ pub struct HandleOrNull(OwnedHandle); /// `INVALID_HANDLE_VALUE`. This ensures that such FFI calls cannot start using the handle without /// checking for `INVALID_HANDLE_VALUE` first. /// -/// This type may hold any handle value that [`OwnedFd`] may hold, except `-1`. It must not +/// This type may hold any handle value that [`OwnedHandle`] may hold, except `-1`. It must not /// hold `-1`, because `-1` in `HandleOrInvalid` is interpreted to mean `INVALID_HANDLE_VALUE`. /// /// This type may hold `NULL`, because APIs that use `INVALID_HANDLE_VALUE` as their sentry value From 2f75b4aaa64fb8bfb9635a879c1af52a5d68f662 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 May 2022 06:36:54 -0700 Subject: [PATCH 4/6] HandleOrNull can hold null, and HandleOrInvalid can hold INVALID_HANDLE_VALUE. --- library/std/src/os/windows/io/handle.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index ef2ef10e05fea..81ae1539acb65 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -77,9 +77,9 @@ pub struct OwnedHandle { /// `NULL`. This ensures that such FFI calls cannot start using the handle without /// checking for `NULL` first. /// -/// This type may hold any handle value that [`OwnedHandle`] may hold, except `NULL`. It may -/// hold `-1`, even though `-1` has the same value as `INVALID_HANDLE_VALUE`, because in -/// `HandleOrNull`, `-1` is interpreted to mean the current process handle. +/// This type may hold any handle value that [`OwnedHandle`] may hold. As with `OwnedHandle`, when +/// it holds `-1`, that value is interpreted as the current process handle, and not +/// `INVALID_HANDLE_VALUE`. /// /// If this holds a non-null handle, it will close the handle on drop. #[repr(transparent)] @@ -97,12 +97,8 @@ pub struct HandleOrNull(OwnedHandle); /// `INVALID_HANDLE_VALUE`. This ensures that such FFI calls cannot start using the handle without /// checking for `INVALID_HANDLE_VALUE` first. /// -/// This type may hold any handle value that [`OwnedHandle`] may hold, except `-1`. It must not -/// hold `-1`, because `-1` in `HandleOrInvalid` is interpreted to mean `INVALID_HANDLE_VALUE`. -/// -/// This type may hold `NULL`, because APIs that use `INVALID_HANDLE_VALUE` as their sentry value -/// may return `NULL` under `windows_subsystem = "windows"` or other situations where I/O devices -/// are detached. +/// This type may hold any handle value that [`OwnedHandle`] may hold, except that when it holds +/// `-1`, that value is interpreted to mean `INVALID_HANDLE_VALUE`. /// /// If holds a handle other than `INVALID_HANDLE_VALUE`, it will close the handle on drop. #[repr(transparent)] From 516a7fa6934c561e9fc1b357a57eb5f0adb89739 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 May 2022 20:50:07 -0700 Subject: [PATCH 5/6] Relax the wording about the meaning of -1. --- library/std/src/os/windows/io/handle.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 81ae1539acb65..e473fac351825 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -23,8 +23,9 @@ use crate::sys_common::{AsInner, FromInner, IntoInner}; /// it is not captured or consumed. /// /// Note that it *may* have the value `-1`, which in `BorrowedHandle` always -/// represents the current process handle, and not `INVALID_HANDLE_VALUE`, -/// despite the two having the same value. See [here] for the full story. +/// represents a valid handle value, such as [the current process handle], and +/// not `INVALID_HANDLE_VALUE`, despite the two having the same value. See +/// [here] for the full story. /// /// And, it *may* have the value `NULL` (0), which can occur when consoles are /// detached from processes, or when `windows_subsystem` is used. @@ -34,6 +35,7 @@ use crate::sys_common::{AsInner, FromInner, IntoInner}; /// handle, which is then borrowed under the same lifetime. /// /// [here]: https://devblogs.microsoft.com/oldnewthing/20040302-00/?p=40443 +/// [the current process handle]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getcurrentprocess#remarks #[derive(Copy, Clone)] #[repr(transparent)] #[unstable(feature = "io_safety", issue = "87074")] @@ -47,8 +49,9 @@ pub struct BorrowedHandle<'handle> { /// This closes the handle on drop. /// /// Note that it *may* have the value `-1`, which in `OwnedHandle` always -/// represents the current process handle, and not `INVALID_HANDLE_VALUE`, -/// despite the two having the same value. See [here] for the full story. +/// represents a valid handle value, such as [the current process handle], and +/// not `INVALID_HANDLE_VALUE`, despite the two having the same value. See +/// [here] for the full story. /// /// And, it *may* have the value `NULL` (0), which can occur when consoles are /// detached from processes, or when `windows_subsystem` is used. @@ -61,6 +64,7 @@ pub struct BorrowedHandle<'handle> { /// [`RegCloseKey`]: https://docs.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regclosekey /// /// [here]: https://devblogs.microsoft.com/oldnewthing/20040302-00/?p=40443 +/// [the current process handle]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getcurrentprocess#remarks #[repr(transparent)] #[unstable(feature = "io_safety", issue = "87074")] pub struct OwnedHandle { @@ -78,10 +82,11 @@ pub struct OwnedHandle { /// checking for `NULL` first. /// /// This type may hold any handle value that [`OwnedHandle`] may hold. As with `OwnedHandle`, when -/// it holds `-1`, that value is interpreted as the current process handle, and not -/// `INVALID_HANDLE_VALUE`. +/// it holds `-1`, that value is interpreted as a valid handle value, such as +/// [the current process handle], and not `INVALID_HANDLE_VALUE`. /// /// If this holds a non-null handle, it will close the handle on drop. +/// [the current process handle]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getcurrentprocess#remarks #[repr(transparent)] #[unstable(feature = "io_safety", issue = "87074")] #[derive(Debug)] From 275812ad2cb0a5a9a8de88151438d1319a6f0704 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 May 2022 21:11:49 -0700 Subject: [PATCH 6/6] Fix comment syntax. --- library/std/src/os/windows/io/handle.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index e473fac351825..f27970eaaf122 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -86,6 +86,7 @@ pub struct OwnedHandle { /// [the current process handle], and not `INVALID_HANDLE_VALUE`. /// /// If this holds a non-null handle, it will close the handle on drop. +/// /// [the current process handle]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getcurrentprocess#remarks #[repr(transparent)] #[unstable(feature = "io_safety", issue = "87074")]