diff --git a/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs b/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs index 53b38799..7621afe7 100644 --- a/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs +++ b/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) diff --git a/src/Microsoft.Windows.CsWin32/Generator.SafeHandleLease.cs b/src/Microsoft.Windows.CsWin32/Generator.SafeHandleLease.cs index 8d2b6a14..7a4f77d6 100644 --- a/src/Microsoft.Windows.CsWin32/Generator.SafeHandleLease.cs +++ b/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))))), diff --git a/test/GenerationSandbox.Tests/HandleOwnershipTests.cs b/test/GenerationSandbox.Tests/HandleOwnershipTests.cs new file mode 100644 index 00000000..b7196801 --- /dev/null +++ b/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; + +/// +/// Calls real Win32 APIs through their SafeHandle-returning friendly overloads and verifies whether the returned +/// SafeHandle owns (will release) the native handle. +/// +/// +/// +/// Ownership comes from win32metadata: [DoNotRelease], [RAIIFree], 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. +/// +/// +/// 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. +/// +/// +[Trait("WindowsOnly", "true")] +public unsafe class HandleOwnershipTests +{ + /// + /// A string resource name that user32 interprets as the integer resource 32512 (IDC_ARROW and IDI_APPLICATION). + /// + private const string SystemResource32512 = "#32512"; + + /// + /// The private field behind the ownership flag passed to the constructor. + /// There is no public API to read it. The name is the same on .NET Framework and .NET. + /// + 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)!; + + /// + /// Asserts that will not release the native handle it wraps, then marks it invalid + /// so that it cannot release the handle even if the assertion fails. + /// + /// The SafeHandle returned by the API under test. + 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(); + } + } + + /// + /// Asserts that will release the native handle it wraps. + /// + /// The SafeHandle returned by the API under test. + private static void AssertOwned(SafeHandle handle) + => Assert.True(OwnsHandle(handle), $"{handle.GetType().Name} does not own a handle that the caller must release."); +} diff --git a/test/GenerationSandbox.Tests/NativeMethods.txt b/test/GenerationSandbox.Tests/NativeMethods.txt index 84a6cba0..c49c625f 100644 --- a/test/GenerationSandbox.Tests/NativeMethods.txt +++ b/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 diff --git a/test/GenerationSandbox.Tests/SignExtendedHandleTests.cs b/test/GenerationSandbox.Tests/SignExtendedHandleTests.cs new file mode 100644 index 00000000..6f2fff6d --- /dev/null +++ b/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; + +/// +/// Calls real Win32 APIs whose handle values have the high bit set, through the SafeHandle-returning +/// friendly overloads, in a project that has CheckForOverflowUnderflow enabled. +/// +/// +/// +/// Native handles are pointer-sized values, not addresses. Several common ones have the high bit set when viewed +/// as a native integer: INVALID_HANDLE_VALUE (-1), the GetCurrentProcess (-1) and +/// GetCurrentThread (-2) pseudo-handles, and 32-bit GDI/USER handles that are sign-extended to 64 bits. +/// Converting such a value between void* and with a checked C# cast throws +/// , so all generated conversions must be unchecked. +/// See https://github.com/microsoft/CsWin32/issues/1412. +/// +/// +[Trait("WindowsOnly", "true")] +public class SignExtendedHandleTests +{ + /// + /// 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. + /// + 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); + } + } + + /// + /// Exercises the generated Marshal.InitHandle path, DangerousValue, Lease, a + /// parameter, and ReleaseHandle for a bitmap. + /// + /// The device context the bitmap is compatible with. + /// if the bitmap's handle was sign-extended. + 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; + } +}