Open Tratcher opened 11 months ago
/cc @jkoritzinsky @jtschuster @agocke
Is this an issue with interpreting annotations?
[in, optional] LPOVERLAPPED Overlapped
vs
[in, out, optional] LPOVERLAPPED lpOverlapped
I understand interpreting [in] as won't be modified, vs. [in, out] where it could be. But doesn't LPOVERLAPPED mean that it's a pointer to an OVERLAPPED, and while the pointer isn't going to be modified, the OVERLAPPED could be?
Somewhere around here? https://github.com/microsoft/CsWin32/blob/baddb6dcfec95703dbbf3061d1e936f5ef3aa661/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs#L341-L345
@tratcher While I look into this, it sounds like just calling the extern
overload would work for you, since it takes a pointer to the overlapped structure. Would that unblock you?
The extern overload doesn't do the SafeHandle management.
I'm not currently blocked, I have the old pinvokes, I'd just like to get rid of them.
The extern overload doesn't do the SafeHandle management.
True. You could do that yourself though by writing your own overload.
I'm trying to get my head around the actual bug here. The annotation on the parameter suggests that the native code will not write at the memory pointed to by Overlapped
. I'll assume that's correct unless someone says otherwise.
I see in the docs that folks shouldn't use the same struct across multiple calls concurrently. Since it's actually a pointer being passed in, I suppose that means that you're not supposed to pass in the same pointer. And if that matters, I'll further suppose that the method retains that pointer after the call is over. This is a deviation from the norm for win32. The norm led CsWin32 to only pin the struct in memory during the call. But if this is an API that needs to pin it beyond that, we need CsWin32 to never use ref
or in
, but rather just always use a pointer so that the caller can manage the lifetime of the pinned memory.
Does that sound right?
I wouldn't classify this as anything out of the norm. See also documented asynchronous I/O procedure.
I think what happens is that the overlapped structure is updated during the initial native call. It's an IN because the pointer isn't modified, but the memory it points to could be. And yes, .NET does manager the lifetime and pinning of this memory.
It's an IN because the pointer isn't modified, but the memory it points to could be
The pointer is passed by value (as all args are). The [in] marker can only meaningfully describe what is done to pointed memory. If it's marked as [in], yet the memory pointed to is mutated, then that's inconsistent and a bug in the declaration.
.NET does manager the lifetime and pinning of this memory.
I don't think it does. The extern overload takes a pointer, which means the memory management is in the hands of the caller (not .NET, as pointers are unmanaged). The 'friendly' overload similarly makes it own pointer, which is to memory that it pins directly. But it unpins it once the call has completed. After that, the memory (which was on the stack because the struct is copied for the method call) is no longer safe to access.
Thank you for your link, @riverar. In that doc, I found this:
Do not deallocate or modify the OVERLAPPED structure or the data buffer until all asynchronous I/O operations to the file object have been completed. If you declare your pointer to the OVERLAPPED structure as a local variable, do not exit the local function until all asynchronous I/O operations to the file object have been completed. If the local function is exited prematurely, the OVERLAPPED structure will go out of scope and it will be inaccessible to any ReadFile or WriteFile functions it encounters outside of that function.
That seems to confirm my hypothesis. Any functions in Win32 that hold onto pointers beyond the life of the call to the function pose a problem for CsWin32. I hope there's a way to quickly and exhaustively identify these.
This is a deviation from the norm for win32. The norm led CsWin32 to only pin the struct in memory during the call.
I wouldn't classify this as anything out of the norm. See also documented asynchronous I/O procedure.
Right. The Win32 Overlapped semantics are pretty clear and in/out semantics are how they work generally.
then that's inconsistent and a bug in the declaration.
Yep. I would argue this is a bug in the declaration. I think CsWin32 is doing the right thing here because it was lied to, not because there is a mistake in the generator.
then that's inconsistent and a bug in the declaration.
Yep. I would argue this is a bug in the declaration. I think CsWin32 is doing the right thing here because it was lied to, not because there is a mistake in the generator.
So you're saying that the OVERLAPPED struct is modified by win32? I couldn't find anything that indicated that in the documentation that @riverar linked to. But maybe I missed it.
As for CsWin32 doing the right thing, I'm not sure I agree. My interpretation of the doc is that the OVERLAPPED*
pointer must remain valid beyond the scope of the call. But CsWin32 doesn't ensure that in its friendly overload. It pins the local struct only as long as the method call. A 'fix' to the metadata to include [out]
wouldn't change CsWin32 behavior here, because for most win32 functions, even [out]
parameters can safely be pinned only around the function call itself.
We need a hint somewhere to CsWin32 that the pin must be long-lived so that CsWin32 does not generate the by-value struct parameter in the friendly overload.
@aarnott Yep, the Internal
and other members are typically updated during the async operation. Check out the docs for the struct itself https://learn.microsoft.com/windows/win32/api/minwinbase/ns-minwinbase-overlapped
@AArnott Yep, the
Internal
and other members are typically updated during the async operation. Check out the docs for the struct itself https://learn.microsoft.com/windows/win32/api/minwinbase/ns-minwinbase-overlapped
Right. The in
notion is a confusion in this case. The semantics of "in" here are invalid because from a use case it has to be a "ref" or else one would never be able to handle the follow up "callback". The OVERLAPPED
struct is capturing the state for the caller to continue from.
The blocked
label should be removed after https://github.com/microsoft/win32metadata/pull/1792 merges and new metadata is pushed to nuget.org.
Actual behavior
Generated APIs that take [In] NativeOverlapped object don't work, causing null refs in the callbacks when trying to retrieve the original state.
Expected behavior
We've (@AaronRobinsonMSFT) found that NativeOverlapped parameters need to be passed by ref (NativeOverlapped*) in both generated signatures so that the native call can correctly update the value.
Repro steps
NativeMethods.txt
content:Generated API:
When I copy and modify the generated code as follows it works:
I found https://github.com/microsoft/CsWin32/issues/545 which first introduced NativeOverlapped support for ReadFile and I see that NativeOverlapped* is generated correctly there.
Context
LangVersion
preview