microsoft / win32metadata

Tooling to generate metadata for Win32 APIs in the Windows SDK.
Other
1.32k stars 113 forks source link

`ACCESS_MASK` is missing #1948

Open 0x5bfa opened 1 month ago

0x5bfa commented 1 month ago

I found it is missing when I added it to CsWin32 'NativeMethods.txt'. And I looked into this repo to find that definition but I couldn't.

If I'm not wrong, I should add it to enums.json according to the contributing guidelines.

{
  "namespace": "Windows.Win32.Security"
  "name": "ACCESS_MASK",
  "flags": true,
  "type": "uint"
  "autoPopulate": {
    "header": "WinNt.h"
  },
  "members": [
      {
        "name": "DELETE",
        "value": "0x00010000"
      },
...
],
  "uses": [
    {
      "method": "AddAccessAllowedAce",
      "parameter": "AccessMask"
    },
...
  ]
}
0x5bfa commented 1 month ago

I'd line to contribute to this repo for the first time. Would it be possible to do with the definition above?

mikebattista commented 1 month ago

I believe you want Windows.Win32.Storage.FileSystem.FILE_ACCESS_RIGHTS. In CsWin32, I believe if you request the specific enum member you need (like DELETE), it will import the containing enum. @AArnott?

AArnott commented 1 month ago

@mikebattista is correct. If you ask for DELETE in the NativeMethods.txt file and build, CsWin32 will produce the enum that defines it, and emit this warning:

warning PInvoke004: Use the name of the enum that declares this constant: FILE_ACCESS_RIGHTS

So then if you replace DELETE with FILE_ACCESS_RIGHTS, you'll get the enum you need, without a warning.

AArnott commented 1 month ago

@riverar why did you reactivate this? There isn't any missing API here.

riverar commented 1 month ago

ACCESS_MASK is a packed set of flags (i.e. a bitfield). The flags are used to describe rights to various types of objects, such as files and registry keys. So it's not really appropriate to force developers to use file access rights constants with this structure.

To set the GENERIC_[...] bits, one should use the GENERIC_ACCESS_RIGHTS enum / GENERIC_[...] constants.

To set MAXIMUM_ALLOWED bit, we have a naked MAXIMUM_ALLOWED constant.

To set the ACCESS_SYSTEM_SECURITY bit, we have a naked ACCESS_SYSTEM_SECURITY constant.

To set the standard rights bits (16-23), things get a little muddy. For example, we have the FILE_ACCESS_RIGHTS::SYNCHRONIZE enum value, which doesn't quite make sense to developers not working with files. The same applies to WRITE_OWNER, WRITE_DACL, READ_CONTROL, and DELETE.

And finally, the specific object rights (0-15) are implementation specific.

We should probably:

  1. create a neutral synthetic enumeration (e.g. STANDARD_RIGHTS) (prior art)
  2. move SYNCHRONIZE, WRITE_OWNER, WRITE_DACL, READ_CONTROL, and DELETE to this enumeration and fix up references
  3. create a synthetic enumeration ACCESS_MASK ([Flags]) that decomposes to a DWORD with references to the correct enumerations? (Do we handle this today with other bitfields?)
ChrisDenton commented 1 month ago

I don't think the metadata does anything for complex bitfield types other than providing the same constants the C headers do. And even types like HRESULT and NTSTATUS are actually a kind of packed structure but the metadata has no awareness of this.

AArnott commented 1 month ago

1457 is probably relevant to this discussion.

riverar commented 1 month ago

I'm cool with leaving it a naked DWORD, but what does everyone think about item 2 (moving the values into a new standard rights enum)? I think that'll make populating ACCESS_MASK a little less weird for folks manipulating object access rights?

(Oh, @aarnott points to a highly relevant similar discussion we all had, ha! https://github.com/microsoft/win32metadata/issues/1457)

kennykerr commented 1 month ago

From a Rust perspective, if the caller is using sys-style bindings it works fine. But if they're using non-sys-style - great name 😭 - bindings then the different enums have different types and it doesn't work unless its convertible in metadata.

0x5bfa commented 1 month ago

How come the name is different? I think this flag is also used for named pipe, registry keys and more (all securable objects) My problem was when I add AddAccessAlloedAce or any functions that require access mask flags don't produce this (I thought ACCESS_MASK so maybe do). Is that possible to solve by adding them into enums.json?

moving the values into a new standard rights enum

I'm not in favor of this. My use case is often using all masks.

Convert non-specific types like uint that use constants into explicit enums to improve usability and discoverability.

It would be nice to make those DWORD values into ACCESS_MASK. How come @riverar?