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

Safe version of the EnumProcessModulesEx function seems to be incorrect #1266

Open
lowleveldesign opened this issue Jul 7, 2024 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@lowleveldesign
Copy link

Actual behavior

I think that the safe version of the EnumProcessModulesEx has an invalid type for the lphModule parameter:

static unsafe winmdroot.Foundation.BOOL EnumProcessModulesEx(SafeHandle hProcess, out FreeLibrarySafeHandle lphModule, uint cb, out uint lpcbNeeded, winmdroot.System.ProcessStatus.ENUM_PROCESS_MODULES_EX_FLAGS dwFilterFlag)

The unsafe version:

static extern unsafe winmdroot.Foundation.BOOL EnumProcessModulesEx(winmdroot.Foundation.HANDLE hProcess, winmdroot.Foundation.HMODULE* lphModule, uint cb, uint* lpcbNeeded, winmdroot.System.ProcessStatus.ENUM_PROCESS_MODULES_EX_FLAGS dwFilterFlag);

Expected behavior

I would expect lphModule to be an out array or a HMODULE pointer (like in the unsafe version).

Repro steps

  1. NativeMethods.txt content:
EnumProcessModulesEx
  1. NativeMethods.json content (if present):
  1. Any of your own code that should be shared?

Context

  • CsWin32 version: 0.3.106
  • Target Framework: net8.0
@lowleveldesign lowleveldesign added the bug Something isn't working label Jul 7, 2024
@lowleveldesign
Copy link
Author

I observed a similar error (one element instead of array) for the StartServiceCtrlDispatcher method:

static extern winmdroot.Foundation.BOOL StartServiceCtrlDispatcher(in winmdroot.System.Services.SERVICE_TABLE_ENTRYW lpServiceStartTable);

lpServiceStartTable should be a pointer or an array of SERVICE_TABLE_ENTRYW.

@AArnott
Copy link
Member

AArnott commented Jul 15, 2024

Agreed. Thank you for reporting.
I believe this is a bug in the metadata, where these two function should have NativeArrayInfoAttribute applied to the pointer parameters in question. I'll move this issue to the right repo.

@AArnott AArnott transferred this issue from microsoft/CsWin32 Jul 15, 2024
@mikebattista
Copy link
Contributor

EnumProcessModulesEx has a [MemorySize] attribute on lphModule. Do you have support for that attribute?

[Out][MemorySize(BytesParamIndex = 2)] HMODULE* lphModule, [In] uint cb

@mikebattista
Copy link
Contributor

Reactivate if you don't have what you need already for EnumProcessModulesEx.

@AArnott
Copy link
Member

AArnott commented Aug 21, 2024

Is MemorySize meant to imply an array? I thought that was just for buffers. How do you decide whether to use NativeArrayInfo vs. MemorySize?

@mikebattista
Copy link
Contributor

@AArnott
Copy link
Member

AArnott commented Aug 23, 2024

Ok. I'll see what I can do in CsWin32 then.

@AArnott AArnott reopened this Aug 23, 2024
@AArnott AArnott transferred this issue from microsoft/win32metadata Aug 23, 2024
@AArnott
Copy link
Member

AArnott commented Sep 10, 2024

@mikebattista I've been researching from your links, and I believe NativeArrayInfo is still warranted on this function.
As precedent, I call on:

Function Parameter Array? Attributes
VirtualQueryEx lpBuffer No MemorySize
StartServiceCtrlDispatcherW lpServiceStartTable Yes NativeArrayInfo (you recently added)
EnumProcessModules lphModule Yes MemorySize
EnumProcessModulesEx lphModule Yes MemorySize
WriteFile lpBuffer Yes MemorySize

Having MemorySize on the EnumProcessModulesEx makes sense because the size is controlled by byte count rather than array element count. But having a NativeArrayElement attribute as well (even without any properties set) is important so that cswin32 can distinguish between a pointer to one element vs. an array of elements.

Should we move this issue back to win32metadata?

@AArnott
Copy link
Member

AArnott commented Sep 30, 2024

WriteFile is one that was somewhat problematic for CsWin32 in the past. Per microsoft/win32metadata#1555 (comment), @mikebattista suggested that MemorySize should not be construed to mean an array. So I believe when a parameter is an array, we ought to apply the NativeArrayInfoAttribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants