Transform NTSTATUS functions into Result<()>

[kennykerr]

This provides minimal support for transforming functions returning NTSTATUS values into Result<()> so that they can more easily be used with other Windows functions, providing uniform error propagation.

@tim-weis what do you think of this as a solution for #983 - it avoids complicating the core windows crate with knowledge of NTSTATUS. Since most APIs will never need it there seems to be a case for keeping it specific to such APIs and out of the base crate.

Here's a BCrypt example:

fn main() -> Result<()> {
    unsafe {
        let mut provider = std::ptr::null_mut();
        BCryptOpenAlgorithmProvider(&mut provider, "RNG", None, Default::default())?;

        for _ in 0..10 {
            let mut random = Guid::zeroed();

            BCryptGenRandom(
                provider,
                &mut random as *mut _ as _,
                std::mem::size_of::<Guid>() as _,
                0,
            )?;

            println!("{:?}", random);
        }

        Ok(())
    }
}

And the output:

C:\git\scratch>cargo run
    Blocking waiting for file lock on build directory
   Compiling scratch v0.1.0 (C:\git\scratch)
    Finished dev [unoptimized + debuginfo] target(s) in 1.17s
     Running `target\debug\scratch.exe`
F491F17A-C15E-18E2-90A4-482D791CC9E1
68D30109-3589-EEA5-3FB2-DBE83A76E699
A53E772F-9812-09BA-9E5E-F3BF38FF520F
05F8A51E-966B-3AE4-E534-7147E45FCF1F
9484C386-A2C4-6A41-A14D-A163EA77010A
374C9C8A-6978-4983-CE5F-5BE7B1F54D64
8A6E0402-63AE-48F4-98AD-88DABF0DA315
4A9738D4-22B6-B908-133D-2FE015C6911D
C22311A1-0F95-75E7-258D-CFDFCF0F543F
796CF67B-D56C-77B3-946E-DB43A65305CF

I also filed a bug for the BCrypt handle types: microsoft/win32metadata#585

Fixes #739
Fixes #983

[MarijnS95]

@kennykerr With this adding derive(Eq) to NTSTATUS, should this get "fixes #739"? Not sure if this (should) apply/applies to more simple (enum) types too, though. Being able to match on them is quite nice.

[kennykerr]

@MarijnS95 good catch yes this fixes #739 as well. There is also #656 which is the larger issue of traits for structs. Enums already derive all the usual traits.

[MarijnS95]

@kennykerr Thanks, good to know. Are there many non-enum types that represent constants like NTSTATUS, which should realistically receive the same treatment?

[tim-weis]

This looks better than what I eventually came up with, thanks a lot!

Though I feel that support is a bit too minimal now. It's dropping two pieces of information: A non-zero success code, and the actual NTSTATUS code in case of an error. The latter is useful, for example, when calling BCryptVerifySignature, but the numeric HRESULT value can no longer be compared against the numeric NTSTATUS value.

I'm not aware of a way to implement a custom comparison operator in Rust between different types. Implementing TryFrom on NTSTATUS for HRESULT might work, though, in recovering the actual NTSTATUS code from an HRESULT without library support.

I'm leaving a few comments in the code as well.

[tim-weis]

This deviates from the NT_SUCCESS macro, which essentially does self.0 >= 0. I'm worried that keeping it like this will damage error propagation ergonomics through the ? operator.

[kennykerr]

Yes, good point.

[tim-weis]

How about changing this to return ::windows::Result<NTSTATUS> and Ok(self)? The benefits are that clients can continue to use the ? operator for error propagation in case of true errors, but still get access to a non-zero success code.

[kennykerr]

I wonder how common this is? How often would this end up being used? Is there a representative example?

[tim-weis]

I'm not aware of one. There are about 80 non-zero status codes with severity 0 (success) and around 100 with severity 1 (information) defined in ntstatus.h. While I did recognize some (e.g. STATUS_PENDING) I wouldn't know of a Windows API call where those actually show up as the return value.

Things are different with driver development, though, where functions like IoRegisterDeviceInterface can indeed return values like STATUS_OBJECT_NAME_EXISTS (0x40000000). I don't know whether driver development is in scope for the Windows crate.

[kennykerr]

Hmm, perhaps it would be better to keep the .ok() method but remove the automatic Result transformation so that developers can easily call method().ok() for most cases but deal directly with the error code in other cases.

NTSTATUS is much the same as HRESULT in its structure and usage but it certainly does seem to have a lot more use for success codes. I tried writing a simple wrapper for BCryptVerifySignature but its certainly not convenient.

We'd like to eventually support driver development

[kennykerr]

Perhaps just a to_hresult method on NTSTATUS to make this a littler simpler:

fn verify() -> Result<bool> {
    match BCryptVerifySignature() {
        Err(e) => {
            if e.code() == STATUS_INVALID_SIGNATURE.to_hresult() {
                Ok(false)
            } else {
                Err(e)
            }
        }
        _ => Ok(true),
    }
}

[kennykerr]

And ironically the docs state that it could return NTE_NO_MEMORY, which is actually an HRESULT. 😉

[kennykerr]

#997

[MarijnS95]

I'm not aware of a way to implement a custom comparison operator in Rust between different types.

This is what Rhs is for in pub trait PartialEq<Rhs = Self>. You could have something along the lines of:

impl NTSTATUS {
    #[inline]
    pub const fn hresult(self) -> ::windows::HRESULT {
        ::windows::HRESULT(self.0 | 0x10000000)
    }
}


impl PartialEq<::windows::HRESULT> for NTSTATUS {
    fn eq(&self, hresult: &::windows::HRESULT) -> bool {
        self.hresult() == *hresult
    }
}

And the other way around (PartialEq<NTSTATUS> for ::windows::HRESULT) forwarding to this implementation.

[tim-weis]

Thanks, Marijn, that's actually what I asked for, but not what I wanted/meant to ask. I was going for something like

if let Err(e) = BCryptVerifySignature(...) {
    match e {
        STATUS_INVALID_SIGNATURE => /* ... */,
        STATUS_NOT_SUPPORTED => /* ... */,
        _ => return Err(e),
    }
}

As far as I understand, the PartialEq trait isn't used in match expressions. While writing code like that would certainly be great, I'm neither aware of a way to get there, or whether it would be worth the effort, given how rarely the concrete error code is interesting to the program.

[MarijnS95]

@tim-weis It uses Eq, but with the notion that this is required to be implemented through derive(Eq), see also #739 (comment) / https://rust-lang.github.io/rfcs/1445-restrict-constants-in-patterns.html. With those STATUS_* enums being of NTSTATUS type, and that type receiving the appropriate #[derive(PartialEq, Eq)] since #994 (closing #739), it shouldn't be too bothersome to have a helper fn to_ntstatus() -> Option<NTSTATUS> for use in match e.to_nstatus().expect("BCryptVerifySignature error must be NTSTATUS") { ... } especially if that error-code is rarely useful.

Alternatively you could write:

if let Err(e) = BCryptVerifySignature(...) {
    match e.to_ntresult() {
        Some(STATUS_INVALID_SIGNATURE) => /* ... */,
        Some(STATUS_NOT_SUPPORTED) => /* ... */,
        Some(ntresult) => /* ... */,
        None => return Err(e),
    }
}

if the function is expected to return non-NTRESULT HRESULTs too.