microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2.13k stars 94 forks source link

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

Open lowleveldesign opened 4 months ago

lowleveldesign commented 4 months ago

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
  2. NativeMethods.json content (if present):

  3. Any of your own code that should be shared?

Context

lowleveldesign commented 4 months ago

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 commented 4 months ago

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.

mikebattista commented 3 months ago

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

[Out][MemorySize(BytesParamIndex = 2)] HMODULE* lphModule, [In] uint cb
mikebattista commented 3 months ago

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

AArnott commented 3 months ago

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 commented 3 months ago

It's based on the SAL annotations.

https://github.com/microsoft/win32metadata/blob/185c3ef016663ab8201878da7efab0a2cff0bcc7/generation/WinSDK/RecompiledIdlHeaders/um/Psapi.h#L158

Additional context on the attribute is at https://github.com/microsoft/win32metadata/issues/284 as well.

AArnott commented 3 months ago

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

AArnott commented 2 months ago

@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 commented 2 months ago

WriteFile is one that was somewhat problematic for CsWin32 in the past. Per https://github.com/microsoft/win32metadata/issues/1555#issuecomment-1528200787, @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.