Friendly overloads for NativeOverlapped not working

[Tratcher]

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

1. NativeMethods.txt content:

HttpReceiveHttpRequest

Generated API:

internal static unsafe uint HttpReceiveHttpRequest(SafeHandle RequestQueueHandle, ulong RequestId, winmdroot.Networking.HttpServer.HTTP_RECEIVE_HTTP_REQUEST_FLAGS Flags, out winmdroot.Networking.HttpServer.HTTP_REQUEST_V2 RequestBuffer, uint RequestBufferLength, uint* BytesReturned, global::System.Threading.NativeOverlapped? Overlapped)
{
	bool RequestQueueHandleAddRef = false;
	try
	{
		fixed (winmdroot.Networking.HttpServer.HTTP_REQUEST_V2* RequestBufferLocal = &RequestBuffer)
		{
			winmdroot.Foundation.HANDLE RequestQueueHandleLocal;
			if (RequestQueueHandle is object)
			{
				RequestQueueHandle.DangerousAddRef(ref RequestQueueHandleAddRef);
				RequestQueueHandleLocal = (winmdroot.Foundation.HANDLE)RequestQueueHandle.DangerousGetHandle();
			}
			else
throw new ArgumentNullException(nameof(RequestQueueHandle));
			global::System.Threading.NativeOverlapped OverlappedLocal = Overlapped ?? default(global::System.Threading.NativeOverlapped);
			uint __result = PInvoke.HttpReceiveHttpRequest(RequestQueueHandleLocal, RequestId, Flags, RequestBufferLocal, RequestBufferLength, BytesReturned, Overlapped.HasValue ? &OverlappedLocal : null);
			return __result;
		}
	}
	finally
	{
		if (RequestQueueHandleAddRef)
			RequestQueueHandle.DangerousRelease();
	}
}

internal static extern unsafe uint HttpReceiveHttpRequest(winmdroot.Foundation.HANDLE RequestQueueHandle, ulong RequestId, winmdroot.Networking.HttpServer.HTTP_RECEIVE_HTTP_REQUEST_FLAGS Flags, winmdroot.Networking.HttpServer.HTTP_REQUEST_V2* RequestBuffer, uint RequestBufferLength, [Optional] uint* BytesReturned, [Optional] global::System.Threading.NativeOverlapped* Overlapped);

// From
HTTPAPI_LINKAGE ULONG HttpReceiveHttpRequest(
  [in]            HANDLE          RequestQueueHandle,
  [in]            HTTP_REQUEST_ID RequestId,
  [in]            ULONG           Flags,
  [out]           PHTTP_REQUEST   RequestBuffer,
  [in]            ULONG           RequestBufferLength,
  [out, optional] PULONG          BytesReturned,
  [in, optional]  LPOVERLAPPED    Overlapped
);

When I copy and modify the generated code as follows it works:

[SupportedOSPlatform("windows6.0.6000")]
internal static unsafe uint HttpReceiveHttpRequest(SafeHandle RequestQueueHandle, ulong RequestId, winmdroot.Networking.HttpServer.HTTP_RECEIVE_HTTP_REQUEST_FLAGS Flags, out winmdroot.Networking.HttpServer.HTTP_REQUEST_V2 RequestBuffer, uint RequestBufferLength, uint* BytesReturned, global::System.Threading.NativeOverlapped* Overlapped)
{
    bool RequestQueueHandleAddRef = false;
    try
    {
        fixed (winmdroot.Networking.HttpServer.HTTP_REQUEST_V2* RequestBufferLocal = &RequestBuffer)
        {
            winmdroot.Foundation.HANDLE RequestQueueHandleLocal;
            if (RequestQueueHandle is object)
            {
                RequestQueueHandle.DangerousAddRef(ref RequestQueueHandleAddRef);
                RequestQueueHandleLocal = (winmdroot.Foundation.HANDLE)RequestQueueHandle.DangerousGetHandle();
            }
            else
                throw new ArgumentNullException(nameof(RequestQueueHandle));
                
            uint __result = PInvoke.HttpReceiveHttpRequest(RequestQueueHandleLocal, RequestId, Flags, RequestBufferLocal, RequestBufferLength, BytesReturned, Overlapped);
            return __result;
        }
    }
    finally
    {
        if (RequestQueueHandleAddRef)
            RequestQueueHandle.DangerousRelease();
    }
}

I found #545 which first introduced NativeOverlapped support for ReadFile and I see that NativeOverlapped* is generated correctly there.

internal static unsafe winmdroot.Foundation.BOOL ReadFile(SafeHandle hFile, Span<byte> lpBuffer, uint* lpNumberOfBytesRead, global::System.Threading.NativeOverlapped* lpOverlapped) { ... }

// From
BOOL ReadFile(
  [in]                HANDLE       hFile,
  [out]               LPVOID       lpBuffer,
  [in]                DWORD        nNumberOfBytesToRead,
  [out, optional]     LPDWORD      lpNumberOfBytesRead,
  [in, out, optional] LPOVERLAPPED lpOverlapped
);

3. Any of your own code that should be shared?
   Use generated PInvokes and exchange types dotnet/aspnetcore#50685

Context

• CsWin32 version: 0.3.18-beta
• Target Framework: net9.0
• LangVersion preview

[AaronRobinsonMSFT]

/cc @jkoritzinsky @jtschuster @agocke

[Tratcher]

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?

CsWin32/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs

Lines 341 to 345 in baddb6d

	else if (isIn && isOptional && !isOut && !isPointerToPointer)
	{
	signatureChanged = true;
	parameters[param.SequenceNumber - 1] = parameters[param.SequenceNumber - 1]
	.WithType(NullableType(elementType).WithTrailingTrivia(TriviaList(Space)));

[AArnott]

@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?

[Tratcher]

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.

[AArnott]

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?

[riverar]

I wouldn't classify this as anything out of the norm. See also documented asynchronous I/O procedure.

[Tratcher]

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.

[AArnott]

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.

[AaronRobinsonMSFT]

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.

[AArnott]

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.

[riverar]

@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

[AaronRobinsonMSFT]

@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.

[AArnott]

The blocked label should be removed after microsoft/win32metadata#1792 merges and new metadata is pushed to nuget.org.