diff --git a/.github/workflows/build-nuget-package.yml b/.github/workflows/build-nuget-package.yml index c28f121f..11248425 100644 --- a/.github/workflows/build-nuget-package.yml +++ b/.github/workflows/build-nuget-package.yml @@ -48,7 +48,7 @@ jobs: - name: Run unit tests shell: pwsh run: | - $testFiles = Get-ChildItem -Path . -Filter "*test.dll" -Recurse | ForEach-Object { $_.FullName } + $testFiles = Get-ChildItem -Path . -Filter "ObfuscarTest*.dll" -Recurse | ForEach-Object { $_.FullName } foreach ($testFile in $testFiles) { dotnet test $testFile --logger trx } diff --git a/Obfuscar/GraphNode.cs b/Obfuscar/GraphNode.cs index 964662a8..3ef207a8 100644 --- a/Obfuscar/GraphNode.cs +++ b/Obfuscar/GraphNode.cs @@ -24,19 +24,19 @@ #endregion +using Mono.Cecil; using System.Collections.Generic; using System.Diagnostics; using System.Linq; -using Mono.Cecil; namespace Obfuscar { [DebuggerDisplay("{type}")] - class GraphNode + internal class GraphNode { - private TypeKey type; - private bool scaned; - private List baseNodes = new List(); + private readonly TypeKey type; + private bool scanned; + private readonly List baseNodes = new List(); public GraphNode(TypeKey type) { @@ -45,7 +45,7 @@ public GraphNode(TypeKey type) internal void Scan(Dictionary nodes, HashSet toRemove, Project project) { - if (scaned) + if (scanned) { return; } @@ -66,7 +66,7 @@ internal void Scan(Dictionary nodes, HashSet toRemo } } - scaned = true; + scanned = true; } public static List GetBaseTypes(Project project, TypeDefinition type) @@ -102,7 +102,7 @@ public static List GetBaseTypes(Project project, TypeDefinition type) return result; } - public void FillMethodGroup(IList groups, Project project) + public void FillMethodGroup(IDictionary groups, Project project) { if (baseNodes.Count == 0) { @@ -132,32 +132,38 @@ public void FillMethodGroup(IList groups, Project project) continue; } - var newGroup = new MethodGroup(); - newGroup.Methods.Add(new MethodKey(method)); + var key = new MethodKey(method); + if (!groups.TryGetValue(key, out var newGroup)) + { + newGroup = new MethodGroup(); + newGroup.Methods.Add(key); + groups[key] = newGroup; + } + MethodDefinition rootMethod = null; if (method.Parameters.Any(p => p.ParameterType is GenericParameter)) { // If the method has generic arguments we need to group it with overloads in the same class so they are renamed the same // way, otherwise the call site updating may fail to choose the right overload - MatchMethodGroup(method, newGroup, project, ref rootMethod); + MatchMethodGroup(method, ref newGroup, groups, project, ref rootMethod); } else { foreach (var baseType in baseNodes) { - baseType.MatchMethodGroup(method, newGroup, project, ref rootMethod); + baseType.MatchMethodGroup(method, ref newGroup, groups, project, ref rootMethod); } } // Mark external methods declared in a base class outside the scanned class hierarchy - if (rootMethod == null && !newGroup.External && method.IsVirtual && method.IsReuseSlot && !method.IsSpecialName) + if (!newGroup.External && rootMethod == null && method.IsVirtual && method.IsReuseSlot && !method.IsSpecialName) { newGroup.External = true; } - if (newGroup.Methods.Count > 1 || newGroup.External) + if (newGroup.Methods.Count < 2 && !newGroup.External) { - groups.Add(newGroup); + groups.Remove(key); } } } @@ -179,22 +185,47 @@ private void FillMethods(IList methods) } } - private void MatchMethodGroup(MethodDefinition method, MethodGroup newGroup, Project project, ref MethodDefinition rootMethod) + private void MatchMethodGroup( + MethodDefinition method, ref MethodGroup newGroup, IDictionary groups, + Project project, ref MethodDefinition rootMethod) { foreach (var baseMethod in type.TypeDefinition.Methods) { if (MethodKey.MethodMatch(baseMethod, method) || MethodKey.MethodMatch(method, baseMethod)) { - newGroup.Methods.Add(new MethodKey(baseMethod)); newGroup.External |= !project.Contains(type); if (baseMethod.IsNewSlot) rootMethod = baseMethod; + + var baseKey = new MethodKey(baseMethod); + if (!groups.TryGetValue(baseKey, out var baseGroup)) + { + // add into the current group + newGroup.Methods.Add(baseKey); + groups[baseKey] = newGroup; + continue; + } + + if (baseGroup == newGroup) + { + // already in the current group + continue; + } + + // save a little time by updating the smaller group + if (baseGroup.Methods.Count > newGroup.Methods.Count) + (newGroup, baseGroup) = (baseGroup, newGroup); + + // merge into an existing group + foreach (var key in baseGroup.Methods) + groups[key] = newGroup; + newGroup.Merge(baseGroup); } } // Add base type methods recursively as the method might override something further up the hierarchy foreach (var baseType in baseNodes) - baseType.MatchMethodGroup(method, newGroup, project, ref rootMethod); + baseType.MatchMethodGroup(method, ref newGroup, groups, project, ref rootMethod); } internal TypeKey[] GetBaseTypes() diff --git a/Obfuscar/InheritMap.cs b/Obfuscar/InheritMap.cs index cb67ccaf..64d750e5 100644 --- a/Obfuscar/InheritMap.cs +++ b/Obfuscar/InheritMap.cs @@ -64,34 +64,13 @@ public InheritMap(Project project) // nodes.Remove(item); //} - var methods = new List(); var properties = new List(); foreach (var node in nodes) { - node.Value.FillMethodGroup(methods, Project); + node.Value.FillMethodGroup(methodGroups, Project); node.Value.FillPropertyGroup(properties, Project); } - // Merge overlapping method groups - foreach (var group in methods) - { - MethodGroup mergeGroup = null; - foreach (var item in group.Methods) - { - if (methodGroups.TryGetValue(item, out mergeGroup)) - { - mergeGroup.Merge(group); - break; - } - } - if (mergeGroup == null) - mergeGroup = group; - foreach (var item in group.Methods) - { - methodGroups[item] = mergeGroup; - } - } - // Merge overlapping property groups foreach (var group in properties) { diff --git a/ObfuscarTestNet/CollectionExpressionTest.cs b/ObfuscarTestNet/CollectionExpressionTest.cs index ad465a0a..2ba30e78 100644 --- a/ObfuscarTestNet/CollectionExpressionTest.cs +++ b/ObfuscarTestNet/CollectionExpressionTest.cs @@ -12,7 +12,7 @@ public class CollectionExpressionTest [TestMethod] public void CheckCollectionExpression() { - string outputPath = TestHelper.OutputPath; + string outputPath = TestHelper.GenerateOutputPath(); string xml = string.Format( @"" + @"" + diff --git a/ObfuscarTestNet/Input/AssemblyWithGenericsHierarchy.cs b/ObfuscarTestNet/Input/AssemblyWithGenericsHierarchy.cs new file mode 100644 index 00000000..469ad2a8 --- /dev/null +++ b/ObfuscarTestNet/Input/AssemblyWithGenericsHierarchy.cs @@ -0,0 +1,56 @@ +using System.Reflection; + +namespace ObfuscarTestNet.Input +{ + /// Interface for testing proper generic method grouping. + public interface IBaseInterface + { + /// This method would normally go into an int param method grouping. + void Method(int index, T value); + + /// This method would normally go into a string param method grouping. + void Method(string key, T value); + } + + /// Class obfuscated by default. + public class BaseClass1 : IBaseInterface + { + /// This method should be named same as base method after obfuscation. + public virtual void Method(int index, T value) + { + } + + /// This method should be named same as base method after obfuscation. + public virtual void Method(string key, T value) + { + } + } + + /// Class excluded from obfuscation. + [Obfuscation(Exclude = true, ApplyToMembers = true)] + public class BaseClass2 : IBaseInterface + { + /// This method would normally cause all int param methods to be skipped. + public virtual void Method(int index, T value) + { + } + + /// This method would normally cause all string param methods to be skipped. + public virtual void Method(string key, T value) + { + } + } + + /// Derived class excluded from obfuscation. + [Obfuscation(Exclude = true, ApplyToMembers = true)] + public class Class2 : BaseClass2 + { + /// Due to this method having two generic arguments it should be renamed same as both methods + /// for proper overload resolution, effectively merging the int and string parameter groups. But due to a bug in ThreeShape.Obfuscar v4.5.0, + /// the group merging fails and as a result one of the overloads in gets renamed anyway + /// which leads to the method implementation being impossible to resolve at runtime. + public void Method(V key, T value) + { + } + } +} diff --git a/ObfuscarTestNet/MethodGroupingTest.cs b/ObfuscarTestNet/MethodGroupingTest.cs new file mode 100644 index 00000000..f1e9ab38 --- /dev/null +++ b/ObfuscarTestNet/MethodGroupingTest.cs @@ -0,0 +1,40 @@ +using Obfuscar; +using System.IO; +using System.Linq; +using System.Reflection; + +namespace ObfuscarTestNet +{ + [TestClass] + public class MethodGroupingTest + { + [TestMethod] + public void CheckGenericMethodGrouping() + { + // Arrange + const string assemblyName = "AssemblyWithGenericsHierarchy"; + const string assemblyDll = $"{assemblyName}.dll"; + var outputPath = TestHelper.GenerateOutputPath(); + var xml = $""" + + + + + + + + + """; + + // Act + var output = TestHelper.BuildAndObfuscate(assemblyName, xml); + var assembly = Assembly.LoadFrom(Path.GetFullPath(Path.Combine(outputPath, assemblyDll))); + + // Assert + + // all methods should have been skipped + Assert.IsTrue(output.Mapping.ClassMap.SelectMany(t => t.Value.Methods.Values).All(t => t.Status == ObfuscationStatus.Skipped)); + Assert.IsTrue(assembly.DefinedTypes.SelectMany(t => t.DeclaredMethods).All(t => t.Name == "Method")); + } + } +} diff --git a/ObfuscarTestNet/SourceBuilder.cs b/ObfuscarTestNet/SourceBuilder.cs index 50b7095a..42e87fa8 100644 --- a/ObfuscarTestNet/SourceBuilder.cs +++ b/ObfuscarTestNet/SourceBuilder.cs @@ -40,9 +40,10 @@ public string Build(string fileName, params string[] references) return outPath; } - public bool AddAssembly(string assemblyDll) + public void AddAssembly(string assemblyDll) { - if (string.IsNullOrEmpty(assemblyDll)) return false; + if (string.IsNullOrEmpty(assemblyDll)) + throw new ArgumentException("Assembly DLL path cannot be null or empty.", nameof(assemblyDll)); var file = Path.GetFullPath(assemblyDll); @@ -52,22 +53,20 @@ public bool AddAssembly(string assemblyDll) var path = Path.GetDirectoryName(typeof(object).Assembly.Location)!; file = Path.Combine(path, assemblyDll); if (!File.Exists(file)) - return false; + Assert.Fail($"Could not find assembly file: {assemblyDll}"); } - if (_references.Any(r => r.FilePath == file)) return true; + if (_references.Any(r => r.FilePath == file)) return; try { var reference = MetadataReference.CreateFromFile(file); _references.Add(reference); } - catch + catch (Exception e) { - return false; + Assert.Fail($"Could not add assembly reference \"{file}\": {e.Message}"); } - - return true; } private void AddAssemblies(params string[] assemblies) @@ -76,22 +75,20 @@ private void AddAssemblies(params string[] assemblies) AddAssembly(assembly); } - public bool AddAssembly(Type type) + public void AddAssembly(Type type) { try { if (_references.Any(r => r.FilePath == type.Assembly.Location)) - return true; + return; var systemReference = MetadataReference.CreateFromFile(type.Assembly.Location); _references.Add(systemReference); } - catch + catch (Exception e) { - return false; + Assert.Fail($"Could not add assembly reference for type {type.FullName}: {e.Message}"); } - - return true; } public void AddNetCoreDefaultReferences() @@ -99,7 +96,7 @@ public void AddNetCoreDefaultReferences() var runtimePath = Path.GetDirectoryName(typeof(object).Assembly.Location) + Path.DirectorySeparatorChar; AddAssemblies( - runtimePath + "System.Private.CoreLib.dll", + runtimePath + "mscorlib.dll", runtimePath + "System.Runtime.dll", runtimePath + "System.Console.dll", runtimePath + "System.Text.RegularExpressions.dll", @@ -108,14 +105,19 @@ public void AddNetCoreDefaultReferences() runtimePath + "System.IO.dll", runtimePath + "System.Net.Primitives.dll", runtimePath + "System.Net.Http.dll", - runtimePath + "System.Private.Uri.dll", runtimePath + "System.Reflection.dll", runtimePath + "System.ComponentModel.Primitives.dll", runtimePath + "System.Globalization.dll", runtimePath + "System.Collections.Concurrent.dll", runtimePath + "System.Collections.NonGeneric.dll", runtimePath + "Microsoft.CSharp.dll", - runtimePath + "netstandard.dll" + runtimePath + "netstandard.dll", +#if NETFRAMEWORK + runtimePath + "System.Core.dll" +#else + runtimePath + "System.Private.CoreLib.dll", + runtimePath + "System.Private.Uri.dll" +#endif ); } } diff --git a/ObfuscarTestNet/TestHelper.cs b/ObfuscarTestNet/TestHelper.cs index dd0baa02..3d475229 100644 --- a/ObfuscarTestNet/TestHelper.cs +++ b/ObfuscarTestNet/TestHelper.cs @@ -1,9 +1,6 @@ -using System; +using System.Collections.Generic; using System.IO; -using System.CodeDom.Compiler; -using System.Text; using System.Linq; -using System.Collections.Generic; namespace ObfuscarTestNet { @@ -13,10 +10,7 @@ static class TestHelper private static int count; - public static string OutputPath - { - get { return Path.Combine("..", "..", "Output", count++.ToString()); } - } + public static string GenerateOutputPath() => Path.Combine("..", "..", "Output", count++.ToString()); public static string BuildAssembly(string name, params string[] references) {