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

ACCESS_MASK is missing #1948

Open
0x5bfa opened this issue Jul 16, 2024 · 10 comments
Open

ACCESS_MASK is missing #1948

0x5bfa opened this issue Jul 16, 2024 · 10 comments

Comments

@0x5bfa
Copy link

0x5bfa commented Jul 16, 2024

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
Copy link
Author

0x5bfa commented Jul 16, 2024

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

@mikebattista
Copy link
Contributor

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
Copy link
Member

AArnott commented Jul 16, 2024

@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 AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Jul 16, 2024
@riverar riverar reopened this Jul 16, 2024
@AArnott
Copy link
Member

AArnott commented Jul 16, 2024

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

@riverar
Copy link
Collaborator

riverar commented Jul 16, 2024

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
Copy link
Contributor

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
Copy link
Member

AArnott commented Jul 16, 2024

#1457 is probably relevant to this discussion.

@riverar
Copy link
Collaborator

riverar commented Jul 16, 2024

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! #1457)

@kennykerr
Copy link
Contributor

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
Copy link
Author

0x5bfa commented Jul 16, 2024

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?

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

No branches or pull requests

6 participants