diff --git a/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs b/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs index 5aa8b160..a2441225 100644 --- a/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs +++ b/src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs @@ -904,17 +904,34 @@ private IEnumerable DeclareFriendlyOverload( signatureChanged = true; parameters[paramIndex] = parameters[paramIndex] .WithType(NullableType(elementType).WithTrailingTrivia(TriviaList(Space))); - leadingStatements.Add( - LocalDeclarationStatement(VariableDeclaration( - elementType, - [ - VariableDeclarator(localName.Identifier, EqualsValueClause( - BinaryExpression(SyntaxKind.CoalesceExpression, origName, DefaultExpression(elementType)))) - ]))); - arguments[paramIndex] = Argument(ConditionalExpression( - MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, origName, IdentifierName("HasValue")), - PrefixUnaryExpression(SyntaxKind.AddressOfExpression, localName), - LiteralExpression(SyntaxKind.NullLiteralExpression))); + if (externParam.Type is ArrayTypeSyntax) + { + // The extern method exposes this [In, Optional] managed (non-blittable) struct parameter as an + // array because a pointer to a managed type is illegal and an `in` modifier cannot represent a + // null pointer (passing null would make the marshaler dereference a null reference). Pass a + // single-element array when a value is provided, or null to omit it. A null array marshals to a + // null pointer. See https://github.com/microsoft/CsWin32/issues/1739. + arguments[paramIndex] = Argument(ConditionalExpression( + MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, origName, IdentifierName(nameof(Nullable.HasValue))), + ImplicitArrayCreationExpression(InitializerExpression( + SyntaxKind.ArrayInitializerExpression, + [MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, origName, IdentifierName(nameof(Nullable.Value)))])), + LiteralExpression(SyntaxKind.NullLiteralExpression))); + } + else + { + leadingStatements.Add( + LocalDeclarationStatement(VariableDeclaration( + elementType, + [ + VariableDeclarator(localName.Identifier, EqualsValueClause( + BinaryExpression(SyntaxKind.CoalesceExpression, origName, DefaultExpression(elementType)))) + ]))); + arguments[paramIndex] = Argument(ConditionalExpression( + MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, origName, IdentifierName("HasValue")), + PrefixUnaryExpression(SyntaxKind.AddressOfExpression, localName), + LiteralExpression(SyntaxKind.NullLiteralExpression))); + } } else if (isIn && isOut) { diff --git a/src/Microsoft.Windows.CsWin32/PointerTypeHandleInfo.cs b/src/Microsoft.Windows.CsWin32/PointerTypeHandleInfo.cs index f100c569..65806895 100644 --- a/src/Microsoft.Windows.CsWin32/PointerTypeHandleInfo.cs +++ b/src/Microsoft.Windows.CsWin32/PointerTypeHandleInfo.cs @@ -51,6 +51,23 @@ elementTypeDetails.MarshalUsingType is string || bool xIn = (parameterAttributes & ParameterAttributes.In) == ParameterAttributes.In; bool xOut = (parameterAttributes & ParameterAttributes.Out) == ParameterAttributes.Out; + // An optional [In] (but not [Out]) parameter that points to a managed (non-blittable) struct cannot be + // exposed as a pointer (pointers to managed types are illegal) and an `in` modifier cannot represent a + // null pointer: passing null makes the marshaler dereference a null reference (NullReferenceException). + // Represent it as an array instead (as we already do for such struct fields below): a null array marshals + // to a null pointer and a single-element array marshals to a pointer to the struct. A friendly overload + // exposes this as a nullable value. See https://github.com/microsoft/CsWin32/issues/1739. + if (xIn && !xOut && xOptional && inputs.AllowMarshaling && nativeArrayInfo is null + && forElement == Generator.GeneratingElement.ExternMethod + && inputs.Generator?.IsManagedType(this.ElementType) is true + && this.ElementType.IsValueType(inputs) is true) + { + return new TypeSyntaxAndMarshaling( + ArrayType(elementTypeDetails.Type, [ArrayRankSpecifier()]), + elementTypeDetails.MarshalAsAttribute is object ? new MarshalAsAttribute(UnmanagedType.LPArray) { ArraySubType = elementTypeDetails.MarshalAsAttribute.Value } : null, + elementTypeDetails.NativeArrayInfo); + } + // A pointer to a marshaled object is not allowed. if (inputs.AllowMarshaling && customAttributes.HasValue && nativeArrayInfo is not null) { diff --git a/test/GenerationSandbox.Tests/NativeMethods.txt b/test/GenerationSandbox.Tests/NativeMethods.txt index d540f0bc..d0e25726 100644 --- a/test/GenerationSandbox.Tests/NativeMethods.txt +++ b/test/GenerationSandbox.Tests/NativeMethods.txt @@ -82,6 +82,7 @@ ShellWindowTypeConstants SHFILEOPSTRUCTW SIZE SYSTEM_INFO +TrySubmitThreadpoolCallback VARDESC WER_REPORT_INFORMATION wglGetProcAddress diff --git a/test/GenerationSandbox.Tests/ThreadpoolCallbackTests.cs b/test/GenerationSandbox.Tests/ThreadpoolCallbackTests.cs new file mode 100644 index 00000000..d163828d --- /dev/null +++ b/test/GenerationSandbox.Tests/ThreadpoolCallbackTests.cs @@ -0,0 +1,52 @@ +// 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.Runtime.InteropServices; +using Windows.Win32; +using Windows.Win32.Foundation; +using Windows.Win32.System.Threading; + +[Trait("WindowsOnly", "true")] +public class ThreadpoolCallbackTests +{ + // Regression tests for https://github.com/microsoft/CsWin32/issues/1739. + // TP_CALLBACK_ENVIRON_V3 is non-blittable (it contains delegate fields), so in the default + // marshaling mode CsWin32 exposes the optional pcbe parameter as a nullable value type and + // forwards it to the native method through an array: a non-null value becomes a single-element + // array and null becomes a null array. These tests exercise both paths end to end by actually + // submitting work to the native threadpool and confirming the callback runs. Passing null must + // marshal to a null pointer rather than dereferencing a null reference (the original bug). + [Fact] + public void NonNullOptionalNonBlittableStructIsMarshaled() + { + TP_CALLBACK_ENVIRON_V3 environment = default; + environment.Version = 3; + environment.CallbackPriority = TP_CALLBACK_PRIORITY.TP_CALLBACK_PRIORITY_NORMAL; + environment.Size = (uint)Marshal.SizeOf(); + + AssertCallbackRuns(environment); + } + + [Fact] + public void NullOptionalNonBlittableStructDoesNotThrow() + { + AssertCallbackRuns(null); + } + + private static unsafe void AssertCallbackRuns(TP_CALLBACK_ENVIRON_V3? environment) + { + using ManualResetEventSlim callbackRan = new(false); + PTP_SIMPLE_CALLBACK callback = (instance, context) => callbackRan.Set(); + + try + { + BOOL submitted = PInvoke.TrySubmitThreadpoolCallback(callback, environment); + Assert.True(submitted, $"TrySubmitThreadpoolCallback failed with error 0x{Marshal.GetLastWin32Error():X}."); + Assert.True(callbackRan.Wait(TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken), "The threadpool callback did not run."); + } + finally + { + GC.KeepAlive(callback); + } + } +}