microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.46k stars 494 forks source link

Feature request: Error from Lsa functions should be converted with LsaNtStatusToWinError beforehand #1945

Closed Zerowalker closed 2 years ago

Zerowalker commented 2 years ago

Motivation

When receiving a Result the error should provide useful information that's expected to be obtained if said function fails.

Drawbacks

An extra step behind the scenes, but that's already done for any error as far as I know cause that's just how it works.

Rationale and alternatives

It would fit how Err is currently used in functions. You call a function that returns a Result, if it fails it will display the error message, like say Access Denied. If it were just to give you error code, it makes sense if that's always the case for consistency (unless there's no error message to be retrieved, if that's a thing).

Additional context

Currently one has to do something like this as far as I can tell:

        LsaRegisterLogonProcess(&string, &mut lsa_handle, &mut _sec_mode).or_else(|f| {
            let nt_status = LsaNtStatusToWinError(NTSTATUS(f.code().0));
            WIN32_ERROR(nt_status).ok()
        }).unwrap();
Zerowalker commented 2 years ago

hmm looking at these functions i notice that some expects HANDLE others LsaHandle? Think that's a metadata issue, where HANDLE LsaHandle is treated as LsaHandle, but PHANDLE LsaHandle is treated as HANDLE?

_IRQL_requires_same_
_IRQL_requires_max_(PASSIVE_LEVEL)
NTSTATUS
NTAPI
LsaRegisterLogonProcess (
    _In_ PLSA_STRING LogonProcessName,
    _Out_ PHANDLE LsaHandle,
    _Out_ PLSA_OPERATIONAL_MODE SecurityMode
    );
 _IRQL_requires_same_
_IRQL_requires_max_(PASSIVE_LEVEL)
NTSTATUS
NTAPI
LsaDeregisterLogonProcess (
    _In_ HANDLE LsaHandle
    );

though it does say this in emitter.settings.rsp

LsaRegisterLogonProcess::LsaHandle=LsaHandle*
riverar commented 2 years ago

Can you create a new issue with the second part? Doesn't look related to the feature request. Then I can transfer it over to the right spot.

Zerowalker commented 2 years ago

@riverar will do

kennykerr commented 2 years ago

The windows crate already transforms the LsaRegisterLogonProcess function such that it returns a Result<()> for convenience while the windows-sys crate intentionally returns the underlying error code without any transformation.

This is by design. If you'd prefer to convert the error code into a WIN32_ERROR value then you can always do so yourself.