Windows io::Error: also format NTSTATUS error codes #41684

jethrogb · 2017-05-01T21:23:12Z

NTSTATUS errors may be encoded as HRESULT, see [MS-ERREF]. These error codes can still be formatted using FormatMessageW but require some different parameters to be passed in.

I wasn't sure if this needed a test and if so, how to test it. Presumably we wouldn't want to make our tests dependent on localization-dependent strings returned from FormatMessageW.

Users that get an err: NTSTATUS will need to do io::Error::from_raw_os_error(err|0x1000_0000) (the equivalent of HRESULT_FROM_NT)

rust-highfive · 2017-05-01T21:23:26Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

mzji · 2017-05-01T22:00:33Z

src/libstd/sys/windows/os.rs

+            // format according to https://support.microsoft.com/en-us/help/259693
+            const NTDLL_DLL: &'static [u16] = &['N' as _, 'T' as _, 'D' as _, 'L' as _, 'L' as _,
+                                                '.' as _, 'D' as _, 'L' as _, 'L' as _, 0];
+            let module = c::LoadLibraryW(NTDLL_DLL.as_ptr());


Since on Windows, the ntdll.dll is always loaded, we could use GetModuleHandle() to get a handle of ntdll.dll. And since the ntdll.dll is not loaded by our code (it's loaded by Windows itself), we could just drop the handle after using it.

mzji · 2017-05-01T22:21:59Z

src/libstd/sys/windows/os.rs

+                               errnum, error_string(errno()));
+            }
+
+            errnum ^= NTSTATUS_BIT;


Why unset the NTSTATUS_BIT? Does this bit stop FormatMessage() to work?

Oh, OK then.

jethrogb · 2017-05-02T00:22:10Z

Build failure looks spurious

Mark-Simulacrum · 2017-05-02T00:39:01Z

Restarted Travis job, and logged.

alexcrichton

Neat! Would it be possible to add a test for this as well? Can you also expand on the contexts in which this would want to be called? I'm not personally 100% familiar with the exhausive set of error codes in Windows, but should we be using this in libstd somehow?

What kinds of functions return NTSTATUS?

alexcrichton · 2017-05-02T14:44:40Z

src/libstd/sys/windows/os.rs

+
+            if module == ptr::null_mut() {
+                // Ok to call recursively, GetModuleHandleW doesn't return NTSTATUS
+                return format!("OS Error {} (GetModuleHandleW() returned error {})",


This seems like it'd be a confusing error message perhaps? The os error errnum has nothing to do with GetModuleHandleW returning an error?

I copied this from the way FormatMessage errors below are handled. I could rewrite both these messages to include that this is why there is no text?

Sure yeah, but that's an error about how it can't be formatted. This is an error about how an intermediate step failed, so this message could be confusing. I think if the ptr coming out here is null we should just fall through to the same code path as below

jethrogb · 2017-05-02T16:15:31Z

Neat! Would it be possible to add a test for this as well?

As mentioned in the PR, I'm not sure what would be appropriate here. What did you have in mind?

What kinds of functions return NTSTATUS?

Functions that return NTSTATUS directly are mostly lower-level kernel functions. But, as mentioned, any NTSTATUS can be encoded in a regular HRESULT error code.

Can you also expand on the contexts in which this would want to be called? I'm not personally 100% familiar with the exhausive set of error codes in Windows, but should we be using this in libstd somehow?

I linked to the error reference in the PR. Since these errors can be returned from io::Error::last_os_error (which is a wrapper for GetLastError), they should be handled in io::Error too.

I'm not 100% when such codes might be returned from Windows APIs that are called from with std, but since you're dealing with file handles, which can map to various device drivers, there's a real possibility you'll encounter these.

retep998 · 2017-05-03T06:28:42Z

Since when does libstd even work with HRESULT? Practically everywhere we're dealing with error codes returned by GetLastError which are win32 DWORD errors, not HRESULT errors.

jethrogb · 2017-05-03T07:34:27Z

Practically everywhere we're dealing with error codes returned by GetLastError which are win32 DWORD errors, not HRESULT errors.

As far as I can tell, these are practically equivalent. A Win32 error is only 16 bits, according to MS-ERREF. GetLastError may return up to 32 bits. Converting from DWORD errors to HRESULT using HRESULT_FROM_WIN32 leaves HRESULT values (those with the highest bit set) alone. If anything, DWORD error codes are a superset of HRESULT error codes.

Here's an example of GetLastError returning a non-Win32 error code from within std:

Compile this program:

fn main() {
	std::collections::HashMap::<(),()>::new();
}

Now backup, and then delete the registry key HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Cryptography\Defaults\Provider. Then, run the program you just compiled, resulting in:

thread 'main' panicked at 'failed to create an OS RNG: Error { repr: Os { code: -2146893799, message: "The keyset is not defined." } }', C:\bot\slave\stable-dist-rustc-win-msvc-64\build\src\libcore\result.rs:868
note: Run with `RUST_BACKTRACE=1` for a backtrace.

-2146893799 is 0x80090019 (NTE_KEYSET_NOT_DEF). Source of the error is https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/rand.rs#L32.

Remember to restore the registry from your backup.

alexcrichton · 2017-05-03T16:51:30Z

@jethrogb

What did you have in mind?

Just something that executes this code path, I don't particularly mind how it's executed.

I suppose I'm still confused a bit about how this is used. We don't even define NTSTATUS or HRESULT in the standard library, so do we use it? Are these values magically encoded already into the return result of GetLastError and not deciphered by FormatMessageW? (I'm just trying to understand what's going on here)

jethrogb · 2017-05-03T16:55:20Z

Are these values magically encoded already into the return result of GetLastError and not deciphered by FormatMessageW?

Yes, this, exactly.

alexcrichton · 2017-05-03T16:59:28Z

Could you add some more comments in the source about these relationships? Right now there's no mention of how the DWORD from GetLastError relates to an HRESULT or an NTSTATUS

arielb1 · 2017-05-09T09:19:51Z

@alexcrichton looks like your concerns were addressed?

alexcrichton · 2017-05-09T18:48:56Z

@bors: r+

Thanks @jethrogb! Also feel free to ping a PR when it's updated, unfortunately github doesn't sent out notifications for those events :(

bors · 2017-05-09T18:48:57Z

📌 Commit 404ece5 has been approved by alexcrichton

bors · 2017-05-10T07:12:24Z

⌛ Testing commit 404ece5 with merge 0b1c374...

bors · 2017-05-10T08:54:48Z

💔 Test failed - status-appveyor

arielb1 · 2017-05-10T09:18:15Z

  Compiling std v0.0.0 (file:///C:/projects/rust/src/libstd)
error[E0433]: failed to resolve. Use of undeclared type or module `sys`
   --> src\libstd\sys\windows\os.rs:327:71
    |
327 |         assert!(!::io::Error::from_raw_os_error(STATUS_UNSUCCESSFUL | sys::c::FACILITY_NT_BIT as _)
    |                                                                       ^^^^^^^^^^^^^^^^^^^^^^^ Use of undeclared type or module `sys`

jethrogb · 2017-05-10T15:38:30Z

Whoops, fixed.

Does this work?

@bors r=@alexcrichton

alexcrichton · 2017-05-10T18:12:12Z

Oh @bors only accepts commands from reviewers, but it looks like there's a tidy problem? (feel free to just ping me when this is ready to go and I'll r+)

jethrogb · 2017-05-10T23:58:22Z

@alexcrichton updated

alexcrichton · 2017-05-10T23:59:39Z

@bors: r+

bors · 2017-05-10T23:59:40Z

📌 Commit 3305e74 has been approved by alexcrichton

…ichton Windows io::Error: also format NTSTATUS error codes `NTSTATUS` errors may be encoded as `HRESULT`, see [[MS-ERREF]](https://msdn.microsoft.com/en-us/library/cc231198.aspx). These error codes can still be formatted using `FormatMessageW` but require some different parameters to be passed in. I wasn't sure if this needed a test and if so, how to test it. Presumably we wouldn't want to make our tests dependent on localization-dependent strings returned from `FormatMessageW`. Users that get an `err: NTSTATUS` will need to do `io::Error::from_raw_os_error(err|0x1000_0000)` (the equivalent of [`HRESULT_FROM_NT`](https://msdn.microsoft.com/en-us/library/ms693780(VS.85).aspx))

bors · 2017-05-11T09:39:48Z

⌛ Testing commit 3305e74 with merge 4ab969c...

bors · 2017-05-11T11:27:41Z

💔 Test failed - status-appveyor

Mark-Simulacrum · 2017-05-11T12:59:04Z

Legitimate failure:

01:47:11] error: the type of this value must be known in this context
[01:47:11]    --> src\libstd\sys\windows\os.rs:330:65
[01:47:11]     |
[01:47:11] 330 |         assert!(!Error::from_raw_os_error(STATUS_UNSUCCESSFUL | c::FACILITY_NT_BIT as _)
[01:47:11]     |                                                                 ^^^^^^^^^^^^^^^^^^^^^^^
[01:47:11] 
[01:47:11] error: aborting due to previous error

jethrogb · 2017-05-11T16:47:42Z

Third time's the charm, right?

alexcrichton · 2017-05-11T17:36:01Z

@bors: r+

bors · 2017-05-11T17:36:01Z

📌 Commit 71de9db has been approved by alexcrichton

bors · 2017-05-11T22:37:44Z

⌛ Testing commit 71de9db with merge 39bcd6f...

Windows io::Error: also format NTSTATUS error codes `NTSTATUS` errors may be encoded as `HRESULT`, see [[MS-ERREF]](https://msdn.microsoft.com/en-us/library/cc231198.aspx). These error codes can still be formatted using `FormatMessageW` but require some different parameters to be passed in. I wasn't sure if this needed a test and if so, how to test it. Presumably we wouldn't want to make our tests dependent on localization-dependent strings returned from `FormatMessageW`. Users that get an `err: NTSTATUS` will need to do `io::Error::from_raw_os_error(err|0x1000_0000)` (the equivalent of [`HRESULT_FROM_NT`](https://msdn.microsoft.com/en-us/library/ms693780(VS.85).aspx))

…ichton Windows io::Error: also format NTSTATUS error codes `NTSTATUS` errors may be encoded as `HRESULT`, see [[MS-ERREF]](https://msdn.microsoft.com/en-us/library/cc231198.aspx). These error codes can still be formatted using `FormatMessageW` but require some different parameters to be passed in. I wasn't sure if this needed a test and if so, how to test it. Presumably we wouldn't want to make our tests dependent on localization-dependent strings returned from `FormatMessageW`. Users that get an `err: NTSTATUS` will need to do `io::Error::from_raw_os_error(err|0x1000_0000)` (the equivalent of [`HRESULT_FROM_NT`](https://msdn.microsoft.com/en-us/library/ms693780(VS.85).aspx))

bors · 2017-05-12T03:48:58Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 39bcd6f to master...

rust-highfive assigned brson May 1, 2017

jethrogb force-pushed the feature/ntstatus branch 2 times, most recently from d4b03e5 to 1438068 Compare May 1, 2017 21:52

mzji reviewed May 1, 2017

View reviewed changes

Mark-Simulacrum mentioned this pull request May 2, 2017

Tracking issue for spurious network failures on bots #40474

Open

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2017

alexcrichton reviewed May 2, 2017

View reviewed changes

jethrogb force-pushed the feature/ntstatus branch from 90ac070 to af476ff Compare May 3, 2017 17:17

Windows io::Error: also format NTSTATUS error codes

70c267f

jethrogb force-pushed the feature/ntstatus branch 3 times, most recently from 4286ba4 to 404ece5 Compare May 3, 2017 17:25

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2017

arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2017

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 9, 2017

jethrogb force-pushed the feature/ntstatus branch from 404ece5 to b0aab8f Compare May 10, 2017 15:37

jethrogb force-pushed the feature/ntstatus branch from b0aab8f to 3305e74 Compare May 10, 2017 18:35

frewsxcv mentioned this pull request May 11, 2017

Rollup of 10 pull requests #41901

Closed

Add test

71de9db

jethrogb force-pushed the feature/ntstatus branch from 3305e74 to 71de9db Compare May 11, 2017 16:47

Mark-Simulacrum mentioned this pull request May 12, 2017

Rollup of 14 pull requests #41929

Closed

bors merged commit 71de9db into rust-lang:master May 12, 2017

workingjubilee added the O-windows Operating system: Windows label Jul 30, 2023

Windows io::Error: also format NTSTATUS error codes #41684

Windows io::Error: also format NTSTATUS error codes #41684

Conversation

jethrogb commented May 1, 2017 • edited Loading

rust-highfive commented May 1, 2017

mzji May 1, 2017 • edited Loading

Choose a reason for hiding this comment

mzji May 1, 2017 • edited Loading

Choose a reason for hiding this comment

jethrogb May 1, 2017

Choose a reason for hiding this comment

mzji May 1, 2017

Choose a reason for hiding this comment

jethrogb commented May 2, 2017

Mark-Simulacrum commented May 2, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton May 2, 2017

Choose a reason for hiding this comment

jethrogb May 2, 2017 • edited Loading

Choose a reason for hiding this comment

alexcrichton May 3, 2017

Choose a reason for hiding this comment

jethrogb commented May 2, 2017

retep998 commented May 3, 2017

jethrogb commented May 3, 2017 • edited Loading

alexcrichton commented May 3, 2017

jethrogb commented May 3, 2017

alexcrichton commented May 3, 2017

arielb1 commented May 9, 2017

alexcrichton commented May 9, 2017

bors commented May 9, 2017

bors commented May 10, 2017

bors commented May 10, 2017

arielb1 commented May 10, 2017

jethrogb commented May 10, 2017

alexcrichton commented May 10, 2017

jethrogb commented May 10, 2017

alexcrichton commented May 10, 2017

bors commented May 10, 2017

bors commented May 11, 2017

bors commented May 11, 2017

Mark-Simulacrum commented May 11, 2017

jethrogb commented May 11, 2017

alexcrichton commented May 11, 2017

bors commented May 11, 2017

bors commented May 11, 2017

bors commented May 12, 2017

jethrogb commented May 1, 2017 •

edited

Loading

mzji May 1, 2017 •

edited

Loading

mzji May 1, 2017 •

edited

Loading

jethrogb May 2, 2017 •

edited

Loading

jethrogb commented May 3, 2017 •

edited

Loading