Skip to content

Commit

Permalink
Merge pull request #576 from microsoft/fix292
Browse files Browse the repository at this point in the history 
Never use marshaled types in a union struct
  • Loading branch information
@AArnott
AArnott committed Jun 4, 2022
2 parents 180c8a3 + 3836495 commit be5ffb3
Show file tree
Hide file tree
Showing 9 changed files with 313 additions and 89 deletions.
13 changes: 10 additions & 3 deletions src/Microsoft.Windows.CsWin32/ArrayTypeHandleInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,15 @@ internal record ArrayTypeHandleInfo(TypeHandleInfo ElementType, ArrayShape Shape
internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs, CustomAttributeHandleCollection? customAttributes, ParameterAttributes parameterAttributes)
{
TypeSyntaxAndMarshaling element = this.ElementType.ToTypeSyntax(inputs, customAttributes);
ArrayTypeSyntax arrayType = ArrayType(element.Type, SingletonList(ArrayRankSpecifier().AddSizes(this.Shape.Sizes.Select(size => LiteralExpression(SyntaxKind.NumericLiteralExpression, Literal(size))).ToArray<ExpressionSyntax>())));
MarshalAsAttribute? marshalAs = element.MarshalAsAttribute is object ? new MarshalAsAttribute(UnmanagedType.LPArray) { ArraySubType = element.MarshalAsAttribute.Value } : null;
return new TypeSyntaxAndMarshaling(arrayType, marshalAs, element.NativeArrayInfo);
if (inputs.AllowMarshaling)
{
ArrayTypeSyntax arrayType = ArrayType(element.Type, SingletonList(ArrayRankSpecifier().AddSizes(this.Shape.Sizes.Select(size => LiteralExpression(SyntaxKind.NumericLiteralExpression, Literal(size))).ToArray<ExpressionSyntax>())));
MarshalAsAttribute? marshalAs = element.MarshalAsAttribute is object ? new MarshalAsAttribute(UnmanagedType.LPArray) { ArraySubType = element.MarshalAsAttribute.Value } : null;
return new TypeSyntaxAndMarshaling(arrayType, marshalAs, element.NativeArrayInfo);
}
else
{
return new TypeSyntaxAndMarshaling(PointerType(element.Type));
}
}
}
260 changes: 199 additions & 61 deletions src/Microsoft.Windows.CsWin32/Generator.cs
View file

Large diffs are not rendered by default.

45 changes: 30 additions & 15 deletions src/Microsoft.Windows.CsWin32/HandleTypeHandleInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,21 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
NameSyntax? nameSyntax;
bool isInterface;
bool isNonCOMConformingInterface;
bool isManagedType = inputs.Generator?.IsManagedType(this) ?? false;
bool hasUnmanagedSuffix = inputs.Generator?.HasUnmanagedSuffix(inputs.AllowMarshaling, isManagedType) ?? false;
string simpleNameSuffix = hasUnmanagedSuffix ? Generator.UnmanagedInteropSuffix : string.Empty;
switch (this.Handle.Kind)
{
case HandleKind.TypeDefinition:
TypeDefinition td = this.reader.GetTypeDefinition((TypeDefinitionHandle)this.Handle);
nameSyntax = inputs.QualifyNames ? GetNestingQualifiedName(inputs.Generator, this.reader, td) : IdentifierName(this.reader.GetString(td.Name));
nameSyntax = inputs.QualifyNames ? GetNestingQualifiedName(inputs.Generator, this.reader, td, hasUnmanagedSuffix) : IdentifierName(this.reader.GetString(td.Name) + simpleNameSuffix);
isInterface = (td.Attributes & TypeAttributes.Interface) == TypeAttributes.Interface;
isNonCOMConformingInterface = isInterface && inputs.Generator?.IsNonCOMInterface(td) is true;
break;
case HandleKind.TypeReference:
var trh = (TypeReferenceHandle)this.Handle;
TypeReference tr = this.reader.GetTypeReference(trh);
nameSyntax = inputs.QualifyNames ? GetNestingQualifiedName(inputs.Generator, this.reader, tr) : IdentifierName(this.reader.GetString(tr.Name));
nameSyntax = inputs.QualifyNames ? GetNestingQualifiedName(inputs.Generator, this.reader, tr, hasUnmanagedSuffix) : IdentifierName(this.reader.GetString(tr.Name) + simpleNameSuffix);
isInterface = inputs.Generator?.IsInterface(trh) is true;
isNonCOMConformingInterface = isInterface && inputs.Generator?.IsNonCOMInterface(trh) is true;
break;
Expand Down Expand Up @@ -97,7 +100,7 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
}
else
{
this.RequestTypeGeneration(inputs.Generator);
this.RequestTypeGeneration(inputs.Generator, this.GetContext(inputs));
}

TypeSyntax syntax = isInterface && (!inputs.AllowMarshaling || isNonCOMConformingInterface)
Expand All @@ -114,7 +117,7 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
// We only want to marshal WinRT objects, not interfaces. We don't have a good way of knowing
// whether it's an interface or an object. "isInterface" comes back as false for a WinRT interface,
// so that doesn't help. Looking at the name should be good enough, but if we needed to, the
// Win32 projection could give us an attribute to make sure
// Win32 projection could give us an attribute to make sure.
string? objName = qualifiedName.Right.ToString();
bool isInterfaceName = InterfaceNameMatcher.IsMatch(objName);
if (!isInterfaceName)
Expand Down Expand Up @@ -155,23 +158,35 @@ private static bool TryMarshalAsObject(TypeSyntaxSettings inputs, string name, [
return false;
}

private static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeDefinitionHandle handle) => GetNestingQualifiedName(generator, reader, reader.GetTypeDefinition(handle));
private static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeDefinitionHandle handle, bool hasUnmanagedSuffix) => GetNestingQualifiedName(generator, reader, reader.GetTypeDefinition(handle), hasUnmanagedSuffix);

internal static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeDefinition td)
internal static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeDefinition td, bool hasUnmanagedSuffix)
{
IdentifierNameSyntax name = IdentifierName(reader.GetString(td.Name));
string simpleName = reader.GetString(td.Name);
if (hasUnmanagedSuffix)
{
simpleName += Generator.UnmanagedInteropSuffix;
}

IdentifierNameSyntax name = IdentifierName(simpleName);
return td.GetDeclaringType() is { IsNil: false } nestingType
? QualifiedName(GetNestingQualifiedName(generator, reader, nestingType), name)
? QualifiedName(GetNestingQualifiedName(generator, reader, nestingType, hasUnmanagedSuffix), name)
: QualifiedName(ParseName(Generator.ReplaceCommonNamespaceWithAlias(generator, reader.GetString(td.Namespace))), name);
}

private static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeReferenceHandle handle) => GetNestingQualifiedName(generator, reader, reader.GetTypeReference(handle));
private static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeReferenceHandle handle, bool hasUnmanagedSuffix) => GetNestingQualifiedName(generator, reader, reader.GetTypeReference(handle), hasUnmanagedSuffix);

private static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeReference tr)
private static NameSyntax GetNestingQualifiedName(Generator? generator, MetadataReader reader, TypeReference tr, bool hasUnmanagedSuffix)
{
SimpleNameSyntax typeName = IdentifierName(reader.GetString(tr.Name));
string simpleName = reader.GetString(tr.Name);
if (hasUnmanagedSuffix)
{
simpleName += Generator.UnmanagedInteropSuffix;
}

SimpleNameSyntax typeName = IdentifierName(simpleName);
return tr.ResolutionScope.Kind == HandleKind.TypeReference
? QualifiedName(GetNestingQualifiedName(generator, reader, (TypeReferenceHandle)tr.ResolutionScope), typeName)
? QualifiedName(GetNestingQualifiedName(generator, reader, (TypeReferenceHandle)tr.ResolutionScope, hasUnmanagedSuffix), typeName)
: QualifiedName(ParseName(Generator.ReplaceCommonNamespaceWithAlias(generator, reader.GetString(tr.Namespace))), typeName);
}

Expand Down Expand Up @@ -210,15 +225,15 @@ private bool IsDelegate(TypeSyntaxSettings inputs, out TypeDefinition delegateTy
return false;
}

private void RequestTypeGeneration(Generator? generator)
private void RequestTypeGeneration(Generator? generator, Generator.Context context)
{
if (this.Handle.Kind == HandleKind.TypeDefinition)
{
generator?.RequestInteropType((TypeDefinitionHandle)this.Handle);
generator?.RequestInteropType((TypeDefinitionHandle)this.Handle, context);
}
else if (this.Handle.Kind == HandleKind.TypeReference)
{
generator?.RequestInteropType((TypeReferenceHandle)this.Handle);
generator?.RequestInteropType((TypeReferenceHandle)this.Handle, context);
}
}
}
24 changes: 23 additions & 1 deletion src/Microsoft.Windows.CsWin32/MetadataIndex.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,15 +279,30 @@ internal bool TryGetTypeDefHandle(MetadataReader reader, TypeReferenceHandle typ
TypeReference typeRef = reader.GetTypeReference(typeRefHandle);
if (typeRef.ResolutionScope.Kind != HandleKind.AssemblyReference)
{
TypeDefinitionHandle expectedNestingTypeDef = default;
bool foundNestingTypeDef = false;
if (typeRef.ResolutionScope.Kind == HandleKind.TypeReference)
{
foundNestingTypeDef = this.TryGetTypeDefHandle(reader, (TypeReferenceHandle)typeRef.ResolutionScope, out expectedNestingTypeDef);
}

bool foundPlatformIncompatibleMatch = false;
foreach (TypeDefinitionHandle tdh in reader.TypeDefinitions)
{
TypeDefinition typeDef = reader.GetTypeDefinition(tdh);
if (typeDef.Name == typeRef.Name && typeDef.Namespace == typeRef.Namespace)
{
if (!MetadataUtilities.IsCompatibleWithPlatform(reader, this, this.platform, typeDef.GetCustomAttributes()))
{
foundPlatformIncompatibleMatch = true;
continue;
}

if (typeRef.ResolutionScope.Kind == HandleKind.TypeReference)
{
// The ref is nested. Verify that the type we found is nested in the same type as well.
if (this.TryGetTypeDefHandle(reader, (TypeReferenceHandle)typeRef.ResolutionScope, out TypeDefinitionHandle nestingTypeDef) && nestingTypeDef == typeDef.GetDeclaringType())
TypeDefinitionHandle actualNestingType = typeDef.GetDeclaringType();
if (foundNestingTypeDef && expectedNestingTypeDef == actualNestingType)
{
typeDefHandle = tdh;
break;
Expand All @@ -304,6 +319,13 @@ internal bool TryGetTypeDefHandle(MetadataReader reader, TypeReferenceHandle typ
}
}
}

if (foundPlatformIncompatibleMatch && typeDefHandle.IsNil)
{
string ns = reader.GetString(typeRef.Namespace);
string name = reader.GetString(typeRef.Name);
throw new PlatformIncompatibleException($"{ns}.{name} is not declared for this platform.");
}
}

this.refToDefCache.TryAdd(typeRefHandle, typeDefHandle);
Expand Down
11 changes: 9 additions & 2 deletions src/Microsoft.Windows.CsWin32/PointerTypeHandleInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
bool xOut = (parameterAttributes & ParameterAttributes.Out) == ParameterAttributes.Out;

// A pointer to a marshaled object is not allowed.
if (customAttributes.HasValue && inputs.Generator?.FindNativeArrayInfoAttribute(customAttributes.Value) is { } nativeArrayInfo)
if (inputs.AllowMarshaling && customAttributes.HasValue && inputs.Generator?.FindNativeArrayInfoAttribute(customAttributes.Value) is { } nativeArrayInfo)
{
// But this pointer represents an array, so type as an array.
MarshalAsAttribute marshalAsAttribute = new MarshalAsAttribute(UnmanagedType.LPArray);
Expand Down Expand Up @@ -50,7 +50,7 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
{
return new TypeSyntaxAndMarshaling(inputs.Generator.FunctionPointer(elementTypeDef));
}
else
else if (inputs.AllowMarshaling)
{
// We can replace a pointer to a struct with a managed equivalent by changing the pointer to an array.
// We only want to enter this branch for struct fields, since method parameters can use in/out/ref modifiers.
Expand All @@ -66,6 +66,13 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
return new TypeSyntaxAndMarshaling(PredefinedType(Token(SyntaxKind.ObjectKeyword)), new MarshalAsAttribute(UnmanagedType.IUnknown), null);
}

// Since we'll be using pointers, we have to ensure the element type does not require any marshaling.
if (inputs.AllowMarshaling)
{
// Evidently all tests pass without actually doing this, so we'll leave it out for now.
////elementTypeDetails = this.ElementType.ToTypeSyntax(inputs with { AllowMarshaling = false }, customAttributes);
}

return new TypeSyntaxAndMarshaling(PointerType(elementTypeDetails.Type));
}

Expand Down
5 changes: 3 additions & 2 deletions src/Microsoft.Windows.CsWin32/SuperGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,9 @@ internal bool TryGetTypeDefinitionHandle(QualifiedTypeReferenceHandle typeRefHan
/// Requests generation of a type referenced across metadata files.
/// </summary>
/// <param name="typeRef">The referenced type, with generator.</param>
/// <param name="context">The generation context.</param>
/// <returns><see langword="true" /> if a matching generator was found; <see langword="false" /> otherwise.</returns>
internal bool TryRequestInteropType(QualifiedTypeReference typeRef)
internal bool TryRequestInteropType(QualifiedTypeReference typeRef, Generator.Context context)
{
if (typeRef.Reference.ResolutionScope.Kind != HandleKind.AssemblyReference)
{
Expand All @@ -116,7 +117,7 @@ internal bool TryRequestInteropType(QualifiedTypeReference typeRef)
{
string ns = typeRef.Generator.Reader.GetString(typeRef.Reference.Namespace);
string name = typeRef.Generator.Reader.GetString(typeRef.Reference.Name);
generator.RequestInteropType(ns, name);
generator.RequestInteropType(ns, name, context);
return true;
}

Expand Down
4 changes: 4 additions & 0 deletions src/Microsoft.Windows.CsWin32/TypeHandleInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,8 @@ protected static bool TryGetSimpleName(TypeSyntax nameSyntax, [NotNullWhen(true)
}

protected TypeSyntax ToTypeSyntaxForDisplay() => this.ToTypeSyntax(DebuggerDisplaySettings, null).Type;

protected Generator.Context GetContext(TypeSyntaxSettings inputs) => inputs.Generator is not null
? inputs.Generator.DefaultContext with { AllowMarshaling = inputs.AllowMarshaling }
: new() { AllowMarshaling = inputs.AllowMarshaling };
}
7 changes: 4 additions & 3 deletions test/GenerationSandbox.Tests/BasicTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Windows.Win32.System.Console;
using Windows.Win32.System.ErrorReporting;
using Windows.Win32.UI.Shell;
using VARDESC = Windows.Win32.System.Com.VARDESC;

[Trait("WindowsOnly", "true")]
public class BasicTests
Expand Down Expand Up @@ -481,10 +482,10 @@ public void ZeroOrMinusOneIsInvalidSafeHandle()
/// <remarks>
/// This demonstrates the problem tracked by <see href="https://github.com/microsoft/CsWin32/issues/292">this bug</see>.
/// </remarks>
[Fact(Skip = "Failure tracked by #292")]
public void LoadProblematicTypes()
[Fact]
public void LoadTypeWithOverlappedRefAndValueTypes_VARDESC()
{
Windows.Win32.System.Com.VARDESC d = new()
VARDESC d = new()
{
Anonymous =
{
Expand Down
33 changes: 31 additions & 2 deletions test/Microsoft.Windows.CsWin32.Tests/GeneratorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ public void COMInterfaceWithSupportedOSPlatform(bool net60, bool allowMarshaling
"CreateFile", // built-in SafeHandle use
"CreateCursor", // 0 or -1 invalid SafeHandle generated
"PlaySound", // 0 invalid SafeHandle generated
"SLIST_HEADER", // Union struct that defined uniquely for each CPU architecture
"PROFILER_HEAP_OBJECT_OPTIONAL_INFO",
"MI_Instance", // recursive type where managed testing gets particularly tricky
"CertFreeCertificateChainList", // double pointer extern method
"D3DGetTraceInstructionOffsets", // SizeParamIndex
"PlgBlt", // SizeConst
"ENABLE_TRACE_PARAMETERS_V1", // bad xml created at some point.
Expand Down Expand Up @@ -354,6 +358,31 @@ public void WildcardForConstants_NoMatch()
Assert.Empty(preciseApi);
}

[Theory, PairwiseData]
public void UnionWithRefAndValueTypeFields(
[CombinatorialValues("VARDESC", "VARIANT")] string typeName,
[CombinatorialValues("net6.0", "net472", "netstandard2.0")] string tfm)
{
this.compilation = this.starterCompilations[tfm];
this.generator = this.CreateGenerator();
Assert.True(this.generator.TryGenerate(typeName, CancellationToken.None));
this.CollectGeneratedCode(this.generator);
this.AssertNoDiagnostics();
}

/// <summary>
/// Generate APIs that depend on common APIs but in both marshalable and non-marshalable contexts.
/// </summary>
[Fact]
public void UnionWithRefAndValueTypeFields2()
{
this.generator = this.CreateGenerator();
Assert.True(this.generator.TryGenerate("MI_MethodDecl", CancellationToken.None));
Assert.True(this.generator.TryGenerate("MI_Value", CancellationToken.None));
this.CollectGeneratedCode(this.generator);
this.AssertNoDiagnostics();
}

[Fact]
public void TypeDefConstantsDeclaredWithinTypeDef()
{
Expand Down Expand Up @@ -1380,7 +1409,7 @@ public void FullGeneration(bool allowMarshaling, [CombinatorialMemberData(nameof
this.generator = this.CreateGenerator(generatorOptions);
this.generator.GenerateAll(CancellationToken.None);
this.CollectGeneratedCode(this.generator);
this.AssertNoDiagnostics(logGeneratedCode: false);
this.AssertNoDiagnostics(logAllGeneratedCode: false);
}

[Theory, PairwiseData]
Expand Down Expand Up @@ -2771,7 +2800,7 @@ private CSharpCompilation AddGeneratedCode(CSharpCompilation compilation, Genera

private bool IsMethodGenerated(string name) => this.FindGeneratedMethod(name).Any();

private void AssertNoDiagnostics(bool logGeneratedCode = true) => this.AssertNoDiagnostics(this.compilation, logGeneratedCode);
private void AssertNoDiagnostics(bool logAllGeneratedCode = true) => this.AssertNoDiagnostics(this.compilation, logAllGeneratedCode);

private void AssertNoDiagnostics(CSharpCompilation compilation, bool logAllGeneratedCode = true)
{
Expand Down

0 comments on commit be5ffb3

Please sign in to comment.