From 9c17029702621f3e0c22cdbdb512c30159d39df3 Mon Sep 17 00:00:00 2001 From: Zachary Northrup Date: Fri, 4 Mar 2022 19:01:08 -0800 Subject: [PATCH 1/4] Fix the ordering of COM interface methods and properties to appear in their originally defined order. This was resulting in fatal crashes while running decompiled COM interop code. --- .../CSharp/CSharpDecompiler.cs | 114 +++++++++++++----- 1 file changed, 84 insertions(+), 30 deletions(-) diff --git a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs index 8c2e659f1c..e8f6767aa8 100644 --- a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs +++ b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs @@ -429,6 +429,47 @@ internal static bool IsTransparentIdentifier(string identifier) } #endregion + #region NativeOrdering + + /// + /// Determines whether a given type requires that its methods be ordered precisely as they were originally defined. + /// + /// The type whose members may need native ordering. + internal bool RequiresNativeOrdering(ITypeDefinition typeDef) + { + // The main scenario for requiring the native method ordering is COM interop, where the V-table is fixed by the ABI + return ComHelper.IsComImport(typeDef); + } + + /// + /// Compare handles with the method definition ordering intact by using the underlying method's MetadataToken, + /// which is defined as the index into a given metadata table. This should equate to the original order that + /// methods and properties were defined by the author. + /// + /// The type whose members to order using their method's MetadataToken + /// A sequence of all members ordered by MetadataToken + internal IEnumerable GetMembersWithNativeOrdering(ITypeDefinition typeDef) + { + EntityHandle GetOrderingHandle(IMember member) + { + // Note! Technically COM interfaces could define property getters and setters out of order or interleaved with other + // methods, but C# doesn't support this so we can't define it that way. + + if (member is IMethod) + return member.MetadataToken; + else if (member is IProperty property) + return property.Getter?.MetadataToken ?? property.Setter?.MetadataToken ?? property.MetadataToken; + else if (member is IEvent @event) + return @event.AddAccessor?.MetadataToken ?? @event.RemoveAccessor?.MetadataToken ?? @event.InvokeAccessor?.MetadataToken ?? @event.MetadataToken; + else + return member.MetadataToken; + } + + return typeDef.Fields.Concat(typeDef.Properties).Concat(typeDef.Methods).Concat(typeDef.Events).OrderBy((member) => GetOrderingHandle(member), HandleComparer.Default); + } + + #endregion + static PEFile LoadPEFile(string fileName, DecompilerSettings settings) { settings.LoadInMemory = true; @@ -1264,48 +1305,61 @@ EntityDeclaration DoDecompile(ITypeDefinition typeDef, DecompileRun decompileRun // With C# 9 records, the relative order of fields and properties matters: IEnumerable fieldsAndProperties = recordDecompiler?.FieldsAndProperties ?? typeDef.Fields.Concat(typeDef.Properties); - foreach (var fieldOrProperty in fieldsAndProperties) + + // For COM interop scenarios, the relative order of virtual functions/properties matters: + IEnumerable allOrderedMembers = RequiresNativeOrdering(typeDef) ? GetMembersWithNativeOrdering(typeDef) : + fieldsAndProperties.Concat(typeDef.Events).Concat(typeDef.Methods); + + foreach (var member in allOrderedMembers) { - if (fieldOrProperty.MetadataToken.IsNil || MemberIsHidden(module.PEFile, fieldOrProperty.MetadataToken, settings)) - { - continue; - } - if (fieldOrProperty is IField field) + if (member is IField || member is IProperty) { - if (typeDef.Kind == TypeKind.Enum && !field.IsConst) + var fieldOrProperty = member; + if (fieldOrProperty.MetadataToken.IsNil || MemberIsHidden(module.PEFile, fieldOrProperty.MetadataToken, settings)) + { continue; - var memberDecl = DoDecompile(field, decompileRun, decompilationContext.WithCurrentMember(field)); - typeDecl.Members.Add(memberDecl); + } + if (fieldOrProperty is IField field) + { + if (typeDef.Kind == TypeKind.Enum && !field.IsConst) + continue; + var memberDecl = DoDecompile(field, decompileRun, decompilationContext.WithCurrentMember(field)); + typeDecl.Members.Add(memberDecl); + } + else if (fieldOrProperty is IProperty property) + { + if (recordDecompiler?.PropertyIsGenerated(property) == true) + { + continue; + } + var propDecl = DoDecompile(property, decompileRun, decompilationContext.WithCurrentMember(property)); + typeDecl.Members.Add(propDecl); + } } - else if (fieldOrProperty is IProperty property) + else if (member is IMethod method) { - if (recordDecompiler?.PropertyIsGenerated(property) == true) + if (recordDecompiler?.MethodIsGenerated(method) == true) { continue; } - var propDecl = DoDecompile(property, decompileRun, decompilationContext.WithCurrentMember(property)); - typeDecl.Members.Add(propDecl); - } - } - foreach (var @event in typeDef.Events) - { - if (!@event.MetadataToken.IsNil && !MemberIsHidden(module.PEFile, @event.MetadataToken, settings)) - { - var eventDecl = DoDecompile(@event, decompileRun, decompilationContext.WithCurrentMember(@event)); - typeDecl.Members.Add(eventDecl); + if (!method.MetadataToken.IsNil && !MemberIsHidden(module.PEFile, method.MetadataToken, settings)) + { + var memberDecl = DoDecompile(method, decompileRun, decompilationContext.WithCurrentMember(method)); + typeDecl.Members.Add(memberDecl); + typeDecl.Members.AddRange(AddInterfaceImplHelpers(memberDecl, method, typeSystemAstBuilder)); + } } - } - foreach (var method in typeDef.Methods) - { - if (recordDecompiler?.MethodIsGenerated(method) == true) + else if (member is IEvent @event) { - continue; + if (!@event.MetadataToken.IsNil && !MemberIsHidden(module.PEFile, @event.MetadataToken, settings)) + { + var eventDecl = DoDecompile(@event, decompileRun, decompilationContext.WithCurrentMember(@event)); + typeDecl.Members.Add(eventDecl); + } } - if (!method.MetadataToken.IsNil && !MemberIsHidden(module.PEFile, method.MetadataToken, settings)) + else { - var memberDecl = DoDecompile(method, decompileRun, decompilationContext.WithCurrentMember(method)); - typeDecl.Members.Add(memberDecl); - typeDecl.Members.AddRange(AddInterfaceImplHelpers(memberDecl, method, typeSystemAstBuilder)); + throw new ArgumentOutOfRangeException("Unexpected member type"); } } if (typeDecl.Members.OfType().Any(idx => idx.PrivateImplementationType.IsNull)) From abf36b0f43fbdd0f949ee3ae90e04312dc381326 Mon Sep 17 00:00:00 2001 From: Zachary Northrup Date: Sat, 5 Mar 2022 00:02:57 -0800 Subject: [PATCH 2/4] Add a unit test validating that COM V-table ordering doesn't change with mixed properties, methods, and events --- .../CorrectnessTestRunner.cs | 6 +++ .../ICSharpCode.Decompiler.Tests.csproj | 1 + .../TestCases/Correctness/ComInterop.cs | 42 +++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs diff --git a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs index 3054a9a666..c8a77eafdc 100644 --- a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs @@ -391,6 +391,12 @@ public async Task MiniJSON([ValueSource(nameof(defaultOptions))] CompilerOptions await RunCS(options: options); } + [Test] + public async Task ComInterop([ValueSource(nameof(noMonoOptions))] CompilerOptions options) + { + await RunCS(options: options); + } + async Task RunCS([CallerMemberName] string testName = null, CompilerOptions options = CompilerOptions.UseDebug) { if ((options & CompilerOptions.UseRoslynMask) != 0 && (options & CompilerOptions.TargetNet40) == 0) diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 7fb5f6753f..3c116d3e54 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -106,6 +106,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs new file mode 100644 index 0000000000..df17c55234 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs @@ -0,0 +1,42 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Runtime.InteropServices; +using System.Text; +using System.Threading.Tasks; + +namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness +{ + public class ComInterop + { + public static void Main() + { + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetMethod("MyMethod1"))); + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetProperty("MyProperty1").GetMethod)); + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetMethod("MyMethod2"))); + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetProperty("MyProperty2").GetMethod)); + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetProperty("MyProperty2").SetMethod)); + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetEvent("MyEvent1").AddMethod)); + Console.WriteLine(Marshal.GetComSlotForMethodInfo(typeof(IMixedPropsAndMethods).GetEvent("MyEvent1").RemoveMethod)); + } + + + [Guid("761618B8-3994-449A-A96B-F1FF2795EA85")] + [ComImport] + [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)] + internal interface IMixedPropsAndMethods + { + int MyMethod1(); + + int MyProperty1 { get; } + + int MyMethod2(); + + int MyProperty2 { get; set; } + + event EventHandler MyEvent1; + + int MyMethod3(); + } + } +} From 0921d83e0dc3ed3fca827214c4dcec4d7eec5663 Mon Sep 17 00:00:00 2001 From: Zachary Northrup Date: Sat, 5 Mar 2022 00:19:01 -0800 Subject: [PATCH 3/4] Add testing of overloaded methods to COM V-table test --- .../TestCases/Correctness/ComInterop.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs index df17c55234..cea92ef071 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs @@ -29,14 +29,18 @@ internal interface IMixedPropsAndMethods int MyMethod1(); int MyProperty1 { get; } + int MyOverload(); int MyMethod2(); int MyProperty2 { get; set; } + int MyOverload(int x); + event EventHandler MyEvent1; int MyMethod3(); + } } } From 006bc18a9689c77d294959ccc685754ddb0fcd57 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Tue, 12 Apr 2022 15:23:37 +0200 Subject: [PATCH 4/4] Run ComInterop test case only on .NET 4.0. --- .../CorrectnessTestRunner.cs | 16 +++++++++++++++- .../ICSharpCode.Decompiler.Tests.csproj | 2 +- .../TestCases/Correctness/ComInterop.cs | 4 ---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs index c8a77eafdc..499cc16d51 100644 --- a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs @@ -72,6 +72,20 @@ public void AllFilesHaveTests() CompilerOptions.Optimize | CompilerOptions.UseRoslynLatest, }; + static readonly CompilerOptions[] net40OnlyOptions = + { + CompilerOptions.None, + CompilerOptions.Optimize, + CompilerOptions.UseRoslyn1_3_2 | CompilerOptions.TargetNet40, + CompilerOptions.Optimize | CompilerOptions.UseRoslyn1_3_2 | CompilerOptions.TargetNet40, + CompilerOptions.UseRoslyn2_10_0 | CompilerOptions.TargetNet40, + CompilerOptions.Optimize | CompilerOptions.UseRoslyn2_10_0 | CompilerOptions.TargetNet40, + CompilerOptions.UseRoslyn3_11_0 | CompilerOptions.TargetNet40, + CompilerOptions.Optimize | CompilerOptions.UseRoslyn3_11_0 | CompilerOptions.TargetNet40, + CompilerOptions.UseRoslynLatest | CompilerOptions.TargetNet40, + CompilerOptions.Optimize | CompilerOptions.UseRoslynLatest | CompilerOptions.TargetNet40 + }; + static readonly CompilerOptions[] defaultOptions = { CompilerOptions.None, @@ -392,7 +406,7 @@ public async Task MiniJSON([ValueSource(nameof(defaultOptions))] CompilerOptions } [Test] - public async Task ComInterop([ValueSource(nameof(noMonoOptions))] CompilerOptions options) + public async Task ComInterop([ValueSource(nameof(net40OnlyOptions))] CompilerOptions options) { await RunCS(options: options); } diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 3c116d3e54..8d1dfa60b0 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -106,7 +106,7 @@ - + diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs index cea92ef071..0baf4bfcfc 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/ComInterop.cs @@ -1,9 +1,5 @@ using System; -using System.Collections.Generic; -using System.Linq; using System.Runtime.InteropServices; -using System.Text; -using System.Threading.Tasks; namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness {