Skip to content
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.

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

Bug: LookupAccountNameW does not use Option for optional arguments #2111

Closed
fgimian opened this issue Oct 22, 2022 · 5 comments
Closed

Bug: LookupAccountNameW does not use Option for optional arguments #2111

fgimian opened this issue Oct 22, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@fgimian
Copy link

fgimian commented Oct 22, 2022

Which crate is this about?

windows

Crate version

0.42.0

Summary

As per the docs, this function has various optional arguments:

BOOL LookupAccountNameW(
  [in, optional]  LPCWSTR       lpSystemName,
  [in]            LPCWSTR       lpAccountName,
  [out, optional] PSID          Sid,
  [in, out]       LPDWORD       cbSid,
  [out, optional] LPWSTR        ReferencedDomainName,
  [in, out]       LPDWORD       cchReferencedDomainName,
  [out]           PSID_NAME_USE peUse
);

However, these are not marked as such in the Rust crate:

pub unsafe fn LookupAccountNameW<'a, P0, P1>(
    lpsystemname: P0,
    lpaccountname: P1,
    sid: super::Foundation::PSID,
    cbsid: *mut u32, referenceddomainname: ::windows::core::PWSTR,
    cchreferenceddomainname: *mut u32, peuse: *mut SID_NAME_USE) -> super::Foundation::BOOL
where
    P0: ::std::convert::Into<::windows::core::PCWSTR>,
    P1: ::std::convert::Into<::windows::core::PCWSTR>,
{
    #[cfg_attr(windows, link(name = "windows"))]
    extern "system" {
        fn LookupAccountNameW(lpsystemname: ::windows::core::PCWSTR, lpaccountname: ::windows::core::PCWSTR, sid: super::Foundation::PSID, cbsid: *mut u32, referenceddomainname: ::windows::core::PWSTR, cchreferenceddomainname: *mut u32, peuse: *mut SID_NAME_USE) -> super::Foundation::BOOL;
    }
    LookupAccountNameW(lpsystemname.into(), lpaccountname.into(), ::core::mem::transmute(sid), ::core::mem::transmute(cbsid), ::core::mem::transmute(referenceddomainname), ::core::mem::transmute(cchreferenceddomainname), ::core::mem::transmute(peuse))
}

Furthermore, PWSTR doesn't have a From implemntation from Option like PCWSTR so I'm left with something like this:

        let result = LookupAccountNameW(
            None, // Local Computer (this is technically a PCWSTR but is using the From trait defined)
            owner_name,
            PSID::default(),
            &mut owner_sid_size,
            PWSTR::null(),
            &mut domain_name_length,
            &mut sid_type,
        );

I would love to be able to call this function as follows instead (similar to various others):

        let result = LookupAccountNameW(
            None, // Local Computer
            owner_name,
            None,
            &mut owner_sid_size,
            None,
            &mut domain_name_length,
            &mut sid_type,
        );

Toolchain version/configuration

Default host: x86_64-pc-windows-gnu
rustup home:  C:\Users\Fots\scoop\persist\rustup\.rustup

installed toolchains
--------------------

stable-x86_64-pc-windows-gnu (default)
stable-x86_64-pc-windows-msvc

active toolchain
----------------

stable-x86_64-pc-windows-gnu (default)
rustc 1.64.0 (a55dd71d5 2022-09-19)

Reproducible example

N/A

Crate manifest

[package]
name = "file-owners"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
widestring = "1.0.2"

[dependencies.windows]
version = "0.42.0"
features = [
    "Win32_Foundation",
    "Win32_Security",
    "Win32_Security_Authorization",
    "Win32_Storage_FileSystem",
    "Win32_System_Threading",
    "Win32_System_SystemServices",
]

Expected behavior

lpSystemName, Sid and ReferencedDomainName should all be Option<T>s 😄

Actual behavior

Currently, all parameters are marked as required even though several of them are optional.

Additional comments

I'm struggling to know where such issues originate (e.g. win32metadata vs windows-rs). I searched win32metadata for the definition of this function and others that have optional arguments but couldn't see a distinction, so I'm guessing this is handled within windows-rs? 😄

@fgimian fgimian added the bug Something isn't working label Oct 22, 2022
@fgimian
Copy link
Author

fgimian commented Oct 22, 2022

Similar problem with RegQueryInfoKeyW for the lpClass parameter which is optional 😄

pub unsafe fn RegQueryInfoKeyW<'a, P0>(hkey: P0, lpclass: ::windows::core::PWSTR,...

@kennykerr
Copy link
Collaborator

This is a limitation where non-pointer parameters don't consider the optional metadata flag, but it probably should do so for handle types (they're not pointers but often contain pointers).

@kennykerr kennykerr added enhancement New feature or request and removed bug Something isn't working labels Oct 24, 2022
@fgimian
Copy link
Author

fgimian commented Oct 24, 2022

This is a limitation where non-pointer parameters don't consider the optional metadata flag, but it probably should do so for handle types (they're not pointers but often contain pointers).

Thanks a lot Kenny. Another idea that may help is also implementing the From trait as you have done for PCWSTR for both PWSTR and PSTR too:

e.g.

impl From<Option<PCWSTR>> for PCWSTR {
    fn from(from: Option<PCWSTR>) -> Self {
        from.unwrap_or_else(Self::null)
    }
}

Thanks heaps
Fotis

@Enyium
Copy link
Contributor

Enyium commented Jan 28, 2023

Same for AssocQueryStringW(), which has optional LPCWSTR and LPWSTR.

@kennykerr
Copy link
Collaborator

All these parameters should now support None thanks to #2360 - let me know if you have any further issues. Enjoy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants