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

ReadFileEx incorrectly takes a Span<byte> #1244

Closed
colejohnson66 opened this issue Jul 23, 2024 · 1 comment
Closed

ReadFileEx incorrectly takes a Span<byte> #1244

colejohnson66 opened this issue Jul 23, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@colejohnson66
Copy link
Contributor

colejohnson66 commented Jul 23, 2024

CsWin32 generates a ReadFileEx and WriteFileEx that take Span<byte>. The generated methods increment the reference count on the handle, pin the span, then invoke the native routines. For asynchronous I/O (which these are designed for), this is incorrect. When the native method returns, two issues occur:

  1. Decrementing the reference count could inadvertently dispose the safe handle
  2. Unpinning the span leaves a GC hold that the underlying I/O driver will be reading or writing against

Generated code

[SupportedOSPlatform("windows5.1.2600")]
internal static unsafe winmdroot.Foundation.BOOL ReadFileEx(SafeHandle hFile, Span<byte> lpBuffer, ref global::System.Threading.NativeOverlapped lpOverlapped, delegate *unmanaged[Stdcall]<uint,uint,global::System.Threading.NativeOverlapped*,void> lpCompletionRoutine)
{
    bool hFileAddRef = false;
    try
    {
        fixed (global::System.Threading.NativeOverlapped* lpOverlappedLocal = &lpOverlapped)
        {
            fixed (byte* lpBufferLocal = lpBuffer)
            {
                winmdroot.Foundation.HANDLE hFileLocal;
                if (hFile is object)
                {
                    hFile.DangerousAddRef(ref hFileAddRef);
                    hFileLocal = (winmdroot.Foundation.HANDLE)hFile.DangerousGetHandle();
                }
                else
                    throw new ArgumentNullException(nameof(hFile));
                winmdroot.Foundation.BOOL __result = PInvoke.ReadFileEx(hFileLocal, lpBufferLocal, (uint )lpBuffer.Length, lpOverlappedLocal, lpCompletionRoutine);
                return __result;
            }
        }
    }
    finally
    {
        if (hFileAddRef)
            hFile.DangerousRelease();
    }
}

Expected behavior

CsWin32 should at least not generate "nice" Span<byte> overloads for ReadFileEx and WriteFileEx.

If you want to go further, you could provide a Memory<byte> version that handles pinning, and a completion routine that wraps the users' to unpin it. This is what one has to do anyways to properly use these APIs.

Repro steps

NativeMethods.txt content:

ReadFileEx
WriteFileEx

Context

  • CsWin32 version: 0.3.106
  • Target Framework: net8.0
  • LangVersion: Latest
@AArnott
Copy link
Member

AArnott commented Sep 10, 2024

This sounds like a dupe (in area at least) of #1066.
But the two functions you mention wouldn't be included in the fix for that because they are missing an attribute in the metadata. I filed microsoft/win32metadata#1994 to track that.

@AArnott AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2024
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

2 participants