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
1.99k stars 84 forks source link

CsWin32 generates `SafeHandle` parameter for `LPPROC_THREAD_ATTRIBUTE_LIST` in `CreateRemoteThreadEx` #1180

Closed alexrp closed 1 month ago

alexrp commented 1 month ago

Actual behavior

image image

Expected behavior

Probably no SafeHandle-ification for this parameter.

Repro steps

  1. NativeMethods.txt content:

    CreateRemoteThreadEx
  2. NativeMethods.json content (if present):

    {
    "$schema": "https://raw.githubusercontent.com/microsoft/CsWin32/main/src/Microsoft.Windows.CsWin32/settings.schema.json",
    "className": "WindowsPInvoke",
    "allowMarshaling": false,
    "wideCharOnly": false,
    "emitSingleFile": true
    }

Context

alexrp commented 1 month ago

Additionally, the generated overload throws ArgumentNullException if a null SafeHamdle is passed for lpAttributeList, even though this is perfectly valid according to the native signature.

Maybe this is just a Win32Metadata bug?

AArnott commented 1 month ago

I'm on 0.3.106 and don't see the problem with offering a SafeHandle overload. A non-SafeHandle overload is also offered. This is typical behavior.

As for throwing ArgumentNullException, that does look like a bug, since the metadata attributes that parameter with [Optional].

alexrp commented 1 month ago

I'm on 0.3.106 and don't see the problem with offering a SafeHandle overload. A non-SafeHandle overload is also offered. This is typical behavior.

Just to clarify: Are you saying that you do see the SafeHandle lpAttributeList overload, but you don't see why offering it would be a problem?

Assuming I understood you correctly: The problem is that it just isn't a handle. Calling CloseHandle() on it will not work; you're supposed to use DeleteProcThreadAttributeList(). So representing it as a handle ironically ends up being very error-prone and unsafe. I should probably have clarified earlier that CsWin32 0.3.49-beta didn't do this, which is why the code in the screenshots above only broke after updating.

AArnott commented 1 month ago

SafeHandle is an abstract class, and we use derived types in .NET for a variety of native resources, where CloseHandle is just one way of releasing them. With CsWin32, we emit custom SafeHandle-derived types for many of these native resources. In this case, the metadata indicates that LPPROC_THREAD_ATTRIBUTE_LIST is a handle to a native resources released by DeleteProcThreadAttributeList, so CsWin32 is emitting the SafeHandle overload in anticipation that a SafeHandle-derived type may exist to track this type and release it when no longer used.

What I don't see here is an actual SafeHandle-derived type being emitted by CsWin32 even when the allocating InitializeProcThreadAttributeList function is generated. This is unexpected, and is probably due to InitializeProcThreadAttributeList using an output parameter to provide the handle to its caller instead of its return value. CsWin32 may not be prepared to trigger its SafeHandle overload and type for allocating functions except by return value. I'll file an issue to track that.

alexrp commented 1 month ago

Hmm. If CsWin32 is going to encourage SafeHandles for non-handle resources like this, I would at least suggest that the generated overloads should be strongly-typed - i.e. take ProcThreadAttributeListSafeHandle (or whatever it'd be called) instead of SafeHandle.

By taking just SafeHandle, type safety is lost in a way not dissimilar to the olden days of .NET interop where everyone would pass IntPtr around due to irrational fear of unsafe, thus having less type safety and more fragile code than they would have had with properly-typed pointers.

AArnott commented 1 month ago

Your concern about the input parameters being typed as SafeHandle instead of the specific derived type is valid, and an ongoing discussion tracking it is in #125.