Fix OverflowException for sign-extended handles in SafeHandle conversions

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

@@ -92,14 +92,17 @@ private bool HasAmbiguousCrefParameterTypes(ParameterListSyntax parameterList)
     private ExpressionSyntax GetIntPtrFromTypeDef(ExpressionSyntax typedefValue, TypeHandleInfo typeDefTypeInfo)
     {
         ExpressionSyntax intPtrValue = typedefValue;
+
+        // Pointer-to-nint conversions must be unchecked: handles such as INVALID_HANDLE_VALUE, pseudo-handles,
+        // and sign-extended GDI/USER handles have the high bit set and would throw under CheckForOverflowUnderflow.
         if (typeDefTypeInfo is PointerTypeHandleInfo)
         {
-            return CastExpression(IntPtrTypeSyntax, typedefValue);
+            return UncheckedExpression(CastExpression(IntPtrTypeSyntax, typedefValue));
         }
 
         if (this.TryGetTypeDefFieldType(typeDefTypeInfo, out TypeHandleInfo? backingType) && backingType is PointerTypeHandleInfo)
         {
-            return CastExpression(IntPtrTypeSyntax, MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, typedefValue, IdentifierName("Value")));
+            return UncheckedExpression(CastExpression(IntPtrTypeSyntax, MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, typedefValue, IdentifierName("Value"))));
         }
 
         if (this.TryGetTypeDefFieldType(typeDefTypeInfo, out TypeHandleInfo? returnTypeField) && returnTypeField is PrimitiveTypeHandleInfo primitiveReturnField)

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

@@ -81,7 +81,8 @@ private ExpressionSyntax GetNativeValueFromHandle(ExpressionSyntax handle, TypeH
         TypeSyntax fieldType = backingType.ToTypeSyntax(this.fieldTypeSettings, GeneratingElement.HelperClassMember, null).Type;
         return backingType switch
         {
-            PointerTypeHandleInfo => CastExpression(fieldType, handle),
+            // Handles with the high bit set (e.g. sign-extended GDI handles) must not throw under CheckForOverflowUnderflow.
+            PointerTypeHandleInfo => UncheckedExpression(CastExpression(fieldType, handle)),
             PrimitiveTypeHandleInfo { PrimitiveTypeCode: PrimitiveTypeCode.IntPtr } => handle,
             PrimitiveTypeHandleInfo { PrimitiveTypeCode: PrimitiveTypeCode.UIntPtr } => UncheckedExpression(CastExpression(fieldType, CastExpression(IdentifierName("nint"), handle))),
             PrimitiveTypeHandleInfo { PrimitiveTypeCode: PrimitiveTypeCode.UInt32 } => CheckedExpression(CastExpression(fieldType, UncheckedExpression(CastExpression(IdentifierName("nuint"), CastExpression(IdentifierName("nint"), handle))))),

test/GenerationSandbox.Tests/HandleOwnershipTests.cs

@@ -0,0 +1,128 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Reflection;
+using System.Runtime.InteropServices;
+using Microsoft.Win32.SafeHandles;
+using Windows.Win32;
+using Windows.Win32.Security;
+using Windows.Win32.System.Console;
+
+/// <summary>
+/// Calls real Win32 APIs through their SafeHandle-returning friendly overloads and verifies whether the returned
+/// SafeHandle owns (will release) the native handle.
+/// </summary>
+/// <remarks>
+/// <para>
+/// Ownership comes from win32metadata: <c>[DoNotRelease]</c>, <c>[RAIIFree]</c>, or the default release function of
+/// the handle type. A wrong annotation makes disposing the SafeHandle release a resource the caller was only lent,
+/// or leak one the caller must release. These tests exist to catch metadata regressions, so they call the actual APIs
+/// rather than synthetic metadata.
+/// </para>
+/// <para>
+/// Only APIs whose ownership is already correct in the metadata that CsWin32 consumes belong here, so that fixing
+/// the metadata never requires changing these tests.
+/// </para>
+/// </remarks>
+[Trait("WindowsOnly", "true")]
+public unsafe class HandleOwnershipTests
+{
+    /// <summary>
+    /// A string resource name that user32 interprets as the integer resource 32512 (<c>IDC_ARROW</c> and <c>IDI_APPLICATION</c>).
+    /// </summary>
+    private const string SystemResource32512 = "#32512";
+
+    /// <summary>
+    /// The private field behind the ownership flag passed to the <see cref="SafeHandle"/> constructor.
+    /// There is no public API to read it. The name is the same on .NET Framework and .NET.
+    /// </summary>
+    private static readonly FieldInfo OwnsHandleField = typeof(SafeHandle).GetField("_ownsHandle", BindingFlags.Instance | BindingFlags.NonPublic)
+        ?? throw new MissingFieldException(nameof(SafeHandle), "_ownsHandle");
+
+    [Fact]
+    public void GetCurrentProcess_IsBorrowed() => AssertBorrowed(PInvoke.GetCurrentProcess_SafeHandle());
+
+    [Fact]
+    public void GetStdHandle_IsBorrowed() => AssertBorrowed(PInvoke.GetStdHandle_SafeHandle(STD_HANDLE.STD_OUTPUT_HANDLE));
+
+    [Fact]
+    public void GetProcessHeap_IsBorrowed() => AssertBorrowed(PInvoke.GetProcessHeap_SafeHandle());
+
+    [Fact]
+    public void GetModuleHandle_IsBorrowed()
+    {
+        FreeLibrarySafeHandle kernel32 = PInvoke.GetModuleHandle("kernel32.dll");
+        Assert.False(kernel32.IsInvalid);
+        AssertBorrowed(kernel32);
+    }
+
+    [Fact]
+    public void LoadCursor_SharedCursor_IsBorrowed()
+    {
+        DestroyCursorSafeHandle arrow = PInvoke.LoadCursor(null, SystemResource32512);
+        Assert.False(arrow.IsInvalid);
+        AssertBorrowed(arrow);
+    }
+
+    // The following APIs return handles the caller must release. They guard against over-applying [DoNotRelease].
+    [Fact]
+    public void CreateRectRgn_IsOwned()
+    {
+        using DeleteObjectHRGNSafeHandle region = PInvoke.CreateRectRgn_SafeHandle(0, 0, 1, 1);
+        Assert.False(region.IsInvalid);
+        AssertOwned(region);
+    }
+
+    [Fact]
+    public void CreatePopupMenu_IsOwned()
+    {
+        using DestroyMenuSafeHandle menu = PInvoke.CreatePopupMenu_SafeHandle();
+        Assert.False(menu.IsInvalid);
+        AssertOwned(menu);
+    }
+
+    [Fact]
+    public void OpenProcessToken_IsOwned()
+    {
+        using SafeFileHandle process = PInvoke.GetCurrentProcess_SafeHandle();
+        Assert.True(PInvoke.OpenProcessToken(process, TOKEN_ACCESS_MASK.TOKEN_QUERY, out SafeFileHandle token));
+        using (token)
+        {
+            AssertOwned(token);
+        }
+    }
+
+    [Fact]
+    public void FindFirstFile_IsOwned()
+    {
+        using FindCloseSafeHandle find = PInvoke.FindFirstFile(Path.Combine(Environment.SystemDirectory, "kernel32.dll"), out _);
+        Assert.False(find.IsInvalid);
+        AssertOwned(find);
+    }
+
+    private static bool OwnsHandle(SafeHandle handle) => (bool)OwnsHandleField.GetValue(handle)!;
+
+    /// <summary>
+    /// Asserts that <paramref name="handle"/> will not release the native handle it wraps, then marks it invalid
+    /// so that it cannot release the handle even if the assertion fails.
+    /// </summary>
+    /// <param name="handle">The SafeHandle returned by the API under test.</param>
+    private static void AssertBorrowed(SafeHandle handle)
+    {
+        try
+        {
+            Assert.False(OwnsHandle(handle), $"{handle.GetType().Name} owns a handle that the API only lends to the caller.");
+        }
+        finally
+        {
+            handle.SetHandleAsInvalid();
+        }
+    }
+
+    /// <summary>
+    /// Asserts that <paramref name="handle"/> will release the native handle it wraps.
+    /// </summary>
+    /// <param name="handle">The SafeHandle returned by the API under test.</param>
+    private static void AssertOwned(SafeHandle handle)
+        => Assert.True(OwnsHandle(handle), $"{handle.GetType().Name} does not own a handle that the caller must release.");
+}

test/GenerationSandbox.Tests/NativeMethods.txt

@@ -111,3 +111,22 @@ INetFwMgr
 NetFwMgr
 IEnumVARIANT
 INetFwAuthorizedApplication
+CreateCompatibleBitmap
+CreateCompatibleDC
+CreatePopupMenu
+CreateRectRgn
+FindFirstFile
+GetCurrentProcess
+GetCurrentThread
+GetCurrentThreadId
+GetObjectType
+OBJ_TYPE
+GetProcessHeap
+GetProcessId
+GetStdHandle
+GetThreadId
+LoadCursor
+OpenProcessToken
+DeleteDC
+GetObject
+BITMAP

test/GenerationSandbox.Tests/SignExtendedHandleTests.cs

@@ -0,0 +1,152 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Diagnostics;
+using System.Runtime.InteropServices;
+using Microsoft.Win32.SafeHandles;
+using Windows.Win32;
+using Windows.Win32.Foundation;
+using Windows.Win32.Graphics.Gdi;
+using Windows.Win32.Security;
+using Windows.Win32.Storage.FileSystem;
+
+/// <summary>
+/// Calls real Win32 APIs whose handle values have the high bit set, through the SafeHandle-returning
+/// friendly overloads, in a project that has <c>CheckForOverflowUnderflow</c> enabled.
+/// </summary>
+/// <remarks>
+/// <para>
+/// Native handles are pointer-sized values, not addresses. Several common ones have the high bit set when viewed
+/// as a native integer: <c>INVALID_HANDLE_VALUE</c> (-1), the <c>GetCurrentProcess</c> (-1) and
+/// <c>GetCurrentThread</c> (-2) pseudo-handles, and 32-bit GDI/USER handles that are sign-extended to 64 bits.
+/// Converting such a value between <c>void*</c> and <see cref="IntPtr"/> with a checked C# cast throws
+/// <see cref="OverflowException"/>, so all generated conversions must be unchecked.
+/// See https://github.com/microsoft/CsWin32/issues/1412.
+/// </para>
+/// </remarks>
+[Trait("WindowsOnly", "true")]
+public class SignExtendedHandleTests
+{
+    /// <summary>
+    /// GDI handle values are recycled with an incrementing uniqueness counter in their upper bits, so creating and
+    /// deleting objects in a loop yields a sign-extended handle within a few hundred iterations.
+    /// This cap only bounds the loop if that assumption ever stops holding.
+    /// </summary>
+    private const int MaxGdiAttempts = 10_000;
+
+    [Fact]
+    public void GetCurrentProcess_SafeHandle_PseudoHandleIsUsable()
+    {
+        using SafeFileHandle process = PInvoke.GetCurrentProcess_SafeHandle();
+
+        Assert.Equal(new IntPtr(-1), process.DangerousGetHandle());
+        Assert.Equal((uint)Process.GetCurrentProcess().Id, PInvoke.GetProcessId(process));
+
+        Assert.True(PInvoke.OpenProcessToken(process, TOKEN_ACCESS_MASK.TOKEN_QUERY, out SafeFileHandle token), $"OpenProcessToken failed: {Marshal.GetLastWin32Error()}");
+        using (token)
+        {
+            Assert.False(token.IsInvalid);
+        }
+    }
+
+    [Fact]
+    public void GetCurrentThread_SafeHandle_PseudoHandleIsUsable()
+    {
+        SafeFileHandle thread = PInvoke.GetCurrentThread_SafeHandle();
+        try
+        {
+            Assert.Equal(new IntPtr(-2), thread.DangerousGetHandle());
+            Assert.Equal(PInvoke.GetCurrentThreadId(), PInvoke.GetThreadId(thread));
+        }
+        finally
+        {
+            // The pseudo-handle must never be closed. HandleOwnershipTests verifies the ownership annotation.
+            thread.SetHandleAsInvalid();
+        }
+    }
+
+    [Fact]
+    public void CreateFile_Failure_ReturnsInvalidHandle()
+    {
+        string missingFile = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
+        using SafeFileHandle file = PInvoke.CreateFile(
+            missingFile,
+            (uint)FILE_ACCESS_RIGHTS.FILE_GENERIC_READ,
+            FILE_SHARE_MODE.FILE_SHARE_READ,
+            lpSecurityAttributes: null,
+            FILE_CREATION_DISPOSITION.OPEN_EXISTING,
+            FILE_FLAGS_AND_ATTRIBUTES.FILE_ATTRIBUTE_NORMAL,
+            hTemplateFile: null);
+        int lastError = Marshal.GetLastWin32Error();
+
+        Assert.True(file.IsInvalid);
+        Assert.Equal(new IntPtr(-1), file.DangerousGetHandle());
+        Assert.Equal((int)WIN32_ERROR.ERROR_FILE_NOT_FOUND, lastError);
+    }
+
+    [Fact]
+    public void FindFirstFile_NoMatch_ReturnsInvalidHandle()
+    {
+        string missingDirectory = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
+        using FindCloseSafeHandle find = PInvoke.FindFirstFile(Path.Combine(missingDirectory, "*"), out _);
+
+        Assert.True(find.IsInvalid);
+        Assert.Equal(new IntPtr(-1), find.DangerousGetHandle());
+    }
+
+    [Fact]
+    public void GdiBitmaps_SignExtendedHandles_RoundTripThroughSafeHandles()
+    {
+        HDC memoryDC = PInvoke.CreateCompatibleDC(default);
+        Assert.False(memoryDC.IsNull);
+        try
+        {
+            // GDI handles are 32-bit values that are sign-extended on 64-bit Windows. The upper bits hold a reuse
+            // counter, so creating and deleting objects soon produces one with the high bit set.
+            // Bitmaps are used rather than regions because gdi32 caches deleted regions and hands back the same handle.
+            bool sawSignExtendedBitmap = false;
+            for (int i = 0; i < MaxGdiAttempts && !sawSignExtendedBitmap; i++)
+            {
+                sawSignExtendedBitmap = CreateUseAndDeleteBitmap(memoryDC);
+            }
+
+            // If this fails, the test never exercised a sign-extended handle, so it proves nothing.
+            Assert.True(sawSignExtendedBitmap, "No sign-extended HBITMAP was observed.");
+        }
+        finally
+        {
+            PInvoke.DeleteDC(memoryDC);
+        }
+    }
+
+    /// <summary>
+    /// Exercises the generated <c>Marshal.InitHandle</c> path, <c>DangerousValue</c>, <c>Lease</c>, a
+    /// <see cref="SafeHandle"/> parameter, and <c>ReleaseHandle</c> for a bitmap.
+    /// </summary>
+    /// <param name="memoryDC">The device context the bitmap is compatible with.</param>
+    /// <returns><see langword="true"/> if the bitmap's handle was sign-extended.</returns>
+    private static unsafe bool CreateUseAndDeleteBitmap(HDC memoryDC)
+    {
+        HBITMAP value;
+        BITMAP info;
+        using (DeleteObjectHBITMAPSafeHandle bitmap = PInvoke.CreateCompatibleBitmap_SafeHandle(memoryDC, 1, 1))
+        {
+            Assert.False(bitmap.IsInvalid);
+            value = (HBITMAP)bitmap.DangerousGetHandle();
+            Assert.True(bitmap.DangerousValue == value);
+
+            using (DeleteObjectHBITMAPSafeHandle.LeaseScope lease = bitmap.Lease())
+            {
+                Assert.True(lease.Value == value);
+            }
+
+            Assert.Equal((uint)OBJ_TYPE.OBJ_BITMAP, PInvoke.GetObjectType(bitmap));
+            Assert.Equal(sizeof(BITMAP), PInvoke.GetObject((HGDIOBJ)value, sizeof(BITMAP), &info));
+        }
+
+        // Disposal must have deleted the bitmap, not just returned without throwing.
+        // GetObjectType can still report a type for a deleted handle, but GetObject fails.
+        Assert.Equal(0, PInvoke.GetObject((HGDIOBJ)value, sizeof(BITMAP), &info));
+        return ((IntPtr)value).ToInt64() < 0;
+    }
+}