From 814f5f0d49f616d5c34b51518aab7137303afbf5 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Wed, 9 Dec 2020 09:05:42 +0100 Subject: [PATCH 1/4] Remove unnecessary work and refactor CompareAssemblyIdentity (#3930) --- .../AssemblyDependency/ReferenceTable.cs | 6 +-- src/Tasks/NativeMethods.cs | 51 +++++-------------- 2 files changed, 14 insertions(+), 43 deletions(-) diff --git a/src/Tasks/AssemblyDependency/ReferenceTable.cs b/src/Tasks/AssemblyDependency/ReferenceTable.cs index a842843a021..791ec22459b 100644 --- a/src/Tasks/AssemblyDependency/ReferenceTable.cs +++ b/src/Tasks/AssemblyDependency/ReferenceTable.cs @@ -2279,14 +2279,12 @@ private static int ResolveAssemblyNameConflict(AssemblyNameReference assemblyRef bool rightConflictLegacyUnified = !isNonUnified && assemblyReference1.reference.IsPrimary; // This is ok here because even if the method says two versions are equivalent the algorithm below will still pick the highest version. - NativeMethods.CompareAssemblyIdentity + bool equivalent = NativeMethods.AreAssembliesEquivalent ( leftConflictFusionName, leftConflictLegacyUnified, rightConflictFusionName, - rightConflictLegacyUnified, - out bool equivalent, - out _ + rightConflictLegacyUnified ); Version leftConflictVersion = assemblyReference0.assemblyName.Version; diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index a4e4fd164a2..55ecb95ecc6 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1108,18 +1108,16 @@ internal static extern int CompareAssemblyIdentityWindows string pwzAssemblyIdentity2, [MarshalAs(UnmanagedType.Bool)] bool fUnified2, [MarshalAs(UnmanagedType.Bool)] out bool pfEquivalent, - out AssemblyComparisonResult pResult + out int pResult ); // TODO: Verify correctness of this implementation and // extend to more cases. - internal static void CompareAssemblyIdentity( + internal static bool AreAssembliesEquivalent( string assemblyIdentity1, bool fUnified1, string assemblyIdentity2, - bool fUnified2, - out bool pfEquivalent, - out AssemblyComparisonResult pResult) + bool fUnified2) { #if FEATURE_FUSION_COMPAREASSEMBLYIDENTITY if (NativeMethodsShared.IsWindows) @@ -1129,46 +1127,38 @@ internal static void CompareAssemblyIdentity( fUnified1, assemblyIdentity2, fUnified2, - out pfEquivalent, - out pResult); + out bool pfEquivalent, + out _); + return pfEquivalent; } #endif AssemblyName an1 = new AssemblyName(assemblyIdentity1); AssemblyName an2 = new AssemblyName(assemblyIdentity2); - //pfEquivalent = AssemblyName.ReferenceMatchesDefinition(an1, an2); - pfEquivalent = RefMatchesDef(an1, an2); - if (pfEquivalent) + if (RefMatchesDef(an1, an2)) { - pResult = AssemblyComparisonResult.ACR_EquivalentFullMatch; - return; + return true; } if (!an1.Name.Equals(an2.Name, StringComparison.OrdinalIgnoreCase)) { - pResult = AssemblyComparisonResult.ACR_NonEquivalent; - pfEquivalent = false; - return; + return false; } var versionCompare = an1.Version.CompareTo(an2.Version); if ((versionCompare < 0 && fUnified2) || (versionCompare > 0 && fUnified1)) { - pResult = AssemblyComparisonResult.ACR_NonEquivalentVersion; - pfEquivalent = true; - return; + return true; } if (versionCompare == 0) { - pResult = AssemblyComparisonResult.ACR_EquivalentFullMatch; - pfEquivalent = true; - return; + return true; } - pResult = pfEquivalent ? AssemblyComparisonResult.ACR_EquivalentFullMatch : AssemblyComparisonResult.ACR_NonEquivalent; + return false; } // Based on coreclr baseassemblyspec.cpp (https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757b2/src/vm/baseassemblyspec.cpp#L330) @@ -1229,23 +1219,6 @@ private static bool CompareRefToDef(AssemblyName @ref, AssemblyName def) return true; } - internal enum AssemblyComparisonResult - { - ACR_Unknown, // Unknown - ACR_EquivalentFullMatch, // all fields match - ACR_EquivalentWeakNamed, // match based on weak-name, version numbers ignored - ACR_EquivalentFXUnified, // match based on FX-unification of version numbers - ACR_EquivalentUnified, // match based on legacy-unification of version numbers - ACR_NonEquivalentVersion, // all fields match except version field - ACR_NonEquivalent, // no match - - ACR_EquivalentPartialMatch, - ACR_EquivalentPartialWeakNamed, - ACR_EquivalentPartialUnified, - ACR_EquivalentPartialFXUnified, - ACR_NonEquivalentPartialVersion - } - //------------------------------------------------------------------------------ // PFXImportCertStore //------------------------------------------------------------------------------ From bbb61cc9341953c410c4d8d659123c851d9ff00c Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 22 Dec 2020 10:14:31 +0100 Subject: [PATCH 2/4] Remove the native call for CompareAssemblyIdentity (fusion.dll) in AreAssembliesEquivalent function --- src/Directory.BeforeCommon.targets | 1 - src/Tasks/NativeMethods.cs | 83 +++--------------------------- 2 files changed, 8 insertions(+), 76 deletions(-) diff --git a/src/Directory.BeforeCommon.targets b/src/Directory.BeforeCommon.targets index 8e63a94f93d..3741fdc7d47 100644 --- a/src/Directory.BeforeCommon.targets +++ b/src/Directory.BeforeCommon.targets @@ -42,7 +42,6 @@ $(DefineConstants);FEATURE_ENCODING_DEFAULT $(DefineConstants);FEATURE_ENVIRONMENT_SYSTEMDIRECTORY $(DefineConstants);FEATURE_FILE_TRACKER - $(DefineConstants);FEATURE_FUSION_COMPAREASSEMBLYIDENTITY $(DefineConstants);FEATURE_GAC $(DefineConstants);FEATURE_GET_COMMANDLINE $(DefineConstants);FEATURE_HANDLE_SAFEWAITHANDLE diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index 55ecb95ecc6..7ea1f7abb3d 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1052,87 +1052,20 @@ internal static extern int CreateAssemblyNameObject( internal static extern int GetCachePath(AssemblyCacheFlags cacheFlags, StringBuilder cachePath, ref int pcchPath); #endif - /*------------------------------------------------------------------------------ - CompareAssemblyIdentity - The Fusion API to compare two assembly identities to determine whether or not they are equivalent is now available. This new API is exported from mscorwks.dll, which you can access via mscoree's GetRealProcAddress. The function prototype is defined in fusion.h as follows: - - STDAPI CompareAssemblyIdentity(LPCWSTR pwzAssemblyIdentity1, - BOOL fUnified1, - LPCWSTR pwzAssemblyIdentity2, - BOOL fUnified2, - BOOL *pfEquivalent, - AssemblyComparisonResult *pResult); - -typedef enum _tagAssemblyComparisonResult -{ - ACR_Unknown, // Unknown - ACR_EquivalentFullMatch, // all fields match - ACR_EquivalentWeakNamed, // match based on weak-name, version numbers ignored - ACR_EquivalentFXUnified, // match based on FX-unification of version numbers - ACR_EquivalentUnified, // match based on legacy-unification of version numbers - ACR_NonEquivalentVersion, // all fields match except version field - ACR_NonEquivalent, // no match - - ACR_EquivalentPartialMatch, - ACR_EquivalentPartialWeakNamed, - ACR_EquivalentPartialUnified, - ACR_EquivalentPartialFXUnified, - ACR_NonEquivalentPartialVersion -} AssemblyComparisonResult; - - Parameters: - [in] LPCWSTR pwzAssemblyIdentity1 : Textual identity of the first assembly to be compared - [in] BOOL fUnified1 : Flag to indicate user-specified unification for pwzAssemblyIdentity1 (see below) - [in] LPCWSTR pwzAssemblyIdentity2 : Textual identity of the second assembly to be compared - [in] BOOL fUnified2 : Flag to inidcate user-specified unification for pwzAssemblyIdentity2 (see below) - [out] BOOL *pfEquivalent : Boolean indicating whether the identities are equivalent - [out] AssemblyComparisonResult *pResult : Contains detailed information about the comparison result - - This API will check whether or not pwzAssemblyIdentity1 and pwzAssemblyIdentity2 are equivalent. Both of these identities must be full-specified (name, version, pkt, culture). The pfEquivalent parameter will be set to TRUE if one (or more) of the following conditions is true: - - a) The assembly identities are equivalent. For strongly-named assemblies this means full match on (name, version, pkt, culture); for simply-named assemblies this means a match on (name, culture) - - b) The assemblies being compared are FX assemblies (even if the version numbers are not the same, these will compare as equivalent by way of unification) - - c) The assemblies are not FX assemblies but are equivalent because fUnified1 and/or fUnified2 were set. - - The fUnified flag is used to indicate that all versions up to the version number of the strongly-named assembly are considered equivalent to itself. For example, if pwzAssemblyIdentity1 is "foo, version=5.0.0.0, culture=neutral, publicKeyToken=...." and fUnified1==TRUE, then this means to treat all versions of the assembly in the range 0.0.0.0-5.0.0.0 to be equivalent to "foo, version=5.0.0.0, culture=neutral, publicKeyToken=...". If pwzAssemblyIdentity2 is the same as pwzAssemblyIdentity1, except has a lower version number (e.g. version range 0.0.0.0-5.0.0.0), then the API will return that the identities are equivalent. If pwzAssemblyIdentity2 is the same as pwzAssemblyIdentity1, but has a greater version number than 5.0.0.0 then the two identities will only be equivalent if fUnified2 is set. - - The AssemblyComparisonResult gives you information about why the identities compared as equal or not equal. The description of the meaning of each ACR_* return value is described in the declaration above. - ------------------------------------------------------------------------------*/ - [DllImport("fusion.dll", CharSet = CharSet.Unicode, EntryPoint = "CompareAssemblyIdentity")] - internal static extern int CompareAssemblyIdentityWindows - ( - string pwzAssemblyIdentity1, - [MarshalAs(UnmanagedType.Bool)] bool fUnified1, - string pwzAssemblyIdentity2, - [MarshalAs(UnmanagedType.Bool)] bool fUnified2, - [MarshalAs(UnmanagedType.Bool)] out bool pfEquivalent, - out int pResult - ); - - // TODO: Verify correctness of this implementation and - // extend to more cases. + //------------------------------------------------------------------------------ + // AreAssembliesEquivalent + //------------------------------------------------------------------------------ + // TODO: Verify correctness of this implementation and extend to more cases. + // Should be consistent with CompareAssemblyIdentity from Fusion API + /// + /// Compares two assembly identities to determine whether or not they are equivalent. + /// internal static bool AreAssembliesEquivalent( string assemblyIdentity1, bool fUnified1, string assemblyIdentity2, bool fUnified2) { -#if FEATURE_FUSION_COMPAREASSEMBLYIDENTITY - if (NativeMethodsShared.IsWindows) - { - CompareAssemblyIdentityWindows( - assemblyIdentity1, - fUnified1, - assemblyIdentity2, - fUnified2, - out bool pfEquivalent, - out _); - return pfEquivalent; - } -#endif - AssemblyName an1 = new AssemblyName(assemblyIdentity1); AssemblyName an2 = new AssemblyName(assemblyIdentity2); From a7563b81c046715eab846ba26ed41ca58408b7af Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 22 Dec 2020 15:29:57 +0100 Subject: [PATCH 3/4] Move AreAssembliesEquivalent from NativeMethods.cs --- .../AssemblyDependency/ReferenceTable.cs | 113 +++++++++++++++++- src/Tasks/NativeMethods.cs | 100 ---------------- 2 files changed, 112 insertions(+), 101 deletions(-) diff --git a/src/Tasks/AssemblyDependency/ReferenceTable.cs b/src/Tasks/AssemblyDependency/ReferenceTable.cs index 791ec22459b..127a1c38f33 100644 --- a/src/Tasks/AssemblyDependency/ReferenceTable.cs +++ b/src/Tasks/AssemblyDependency/ReferenceTable.cs @@ -2236,6 +2236,117 @@ Dictionary> baseNameToReferences } } + // TODO: Verify correctness of this implementation and extend to more cases. + // Should be consistent with CompareAssemblyIdentity from Fusion API. + // Both of these identities must be full-specified (name, version, pkt, culture). The result parameter will TRUE if one (or more) of the following conditions is true: + // a) The assembly identities are equivalent. For strongly-named assemblies this means full match on (name, version, pkt, culture); for simply-named assemblies this means a match on (name, culture) + // b) The assemblies being compared are FX assemblies (even if the version numbers are not the same, these will compare as equivalent by way of unification) + // c) The assemblies are not FX assemblies but are equivalent because fUnified1 and/or fUnified2 were set. + // The fUnified flag is used to indicate that all versions up to the version number of the strongly-named assembly are considered equivalent to itself. + // For example, if assemblyIdentity1 is "foo, version=5.0.0.0, culture=neutral, publicKeyToken=...." and fUnified1==TRUE, then this means to treat all versions of the assembly in the range 0.0.0.0-5.0.0.0 to be equivalent to "foo, version=5.0.0.0, culture=neutral, publicKeyToken=...". + // If assemblyIdentity2 is the same as assemblyIdentity1, except has a lower version number (e.g.version range 0.0.0.0-5.0.0.0), then the function will return that the identities are equivalent.If assemblyIdentity2 is the same as assemblyIdentity1, but has a greater version number than 5.0.0.0 then the two identities will only be equivalent if fUnified2 is set. + /// + /// Compares two assembly identities to determine whether or not they are equivalent. + /// + /// Textual identity of the first assembly to be compared. + /// Flag to indicate user-specified unification for assemblyIdentity1. + /// Textual identity of the second assembly to be compared. + /// Flag to indicate user-specified unification for assemblyIdentity2. + /// + /// Boolean indicating whether the identities are equivalent. + /// + private static bool AreAssembliesEquivalent( + string assemblyIdentity1, + bool fUnified1, + string assemblyIdentity2, + bool fUnified2) + { + AssemblyName an1 = new AssemblyName(assemblyIdentity1); + AssemblyName an2 = new AssemblyName(assemblyIdentity2); + + if (RefMatchesDef(an1, an2)) + { + return true; + } + + if (!an1.Name.Equals(an2.Name, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + var versionCompare = an1.Version.CompareTo(an2.Version); + + if ((versionCompare < 0 && fUnified2) || (versionCompare > 0 && fUnified1)) + { + return true; + } + + if (versionCompare == 0) + { + return true; + } + + return false; + } + + // Based on coreclr baseassemblyspec.cpp (https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757b2/src/vm/baseassemblyspec.cpp#L330) + private static bool RefMatchesDef(AssemblyName @ref, AssemblyName def) + { + if (IsStrongNamed(@ref)) + { + return IsStrongNamed(def) && CompareRefToDef(@ref, def); + } + else + { + return @ref.Name.Equals(def.Name, StringComparison.OrdinalIgnoreCase); + } + } + + // Based on coreclr baseassemblyspec.inl (https://github.com/dotnet/coreclr/blob/32f0f9721afb584b4a14d69135bea7ddc129f755/src/vm/baseassemblyspec.inl#L679-L683) + private static bool IsStrongNamed(AssemblyName assembly) + { + var refPkt = assembly.GetPublicKeyToken(); + return refPkt != null && refPkt.Length != 0; + } + + // Based on https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757b2/src/vm/baseassemblyspec.cpp#L241 + private static bool CompareRefToDef(AssemblyName @ref, AssemblyName def) + { + if (!@ref.Name.Equals(def.Name, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + byte[] rpkt = @ref.GetPublicKeyToken(); + byte[] dpkt = def.GetPublicKeyToken(); + + if (rpkt.Length != dpkt.Length) + { + return false; + } + + for (int i = 0; i < rpkt.Length; i++) + { + if (rpkt[i] != dpkt[i]) + { + return false; + } + } + + if (@ref.Version != def.Version) + { + return false; + } + + if (@ref.CultureName != null && + @ref.CultureName != def.CultureName) + { + return false; + } + + return true; + } + /// /// Given two references along with their fusion names, resolve the filename conflict that they /// would have if both assemblies need to be copied to the same directory. @@ -2279,7 +2390,7 @@ private static int ResolveAssemblyNameConflict(AssemblyNameReference assemblyRef bool rightConflictLegacyUnified = !isNonUnified && assemblyReference1.reference.IsPrimary; // This is ok here because even if the method says two versions are equivalent the algorithm below will still pick the highest version. - bool equivalent = NativeMethods.AreAssembliesEquivalent + bool equivalent = AreAssembliesEquivalent ( leftConflictFusionName, leftConflictLegacyUnified, diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index 7ea1f7abb3d..174dfe25f83 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1052,106 +1052,6 @@ internal static extern int CreateAssemblyNameObject( internal static extern int GetCachePath(AssemblyCacheFlags cacheFlags, StringBuilder cachePath, ref int pcchPath); #endif - //------------------------------------------------------------------------------ - // AreAssembliesEquivalent - //------------------------------------------------------------------------------ - // TODO: Verify correctness of this implementation and extend to more cases. - // Should be consistent with CompareAssemblyIdentity from Fusion API - /// - /// Compares two assembly identities to determine whether or not they are equivalent. - /// - internal static bool AreAssembliesEquivalent( - string assemblyIdentity1, - bool fUnified1, - string assemblyIdentity2, - bool fUnified2) - { - AssemblyName an1 = new AssemblyName(assemblyIdentity1); - AssemblyName an2 = new AssemblyName(assemblyIdentity2); - - if (RefMatchesDef(an1, an2)) - { - return true; - } - - if (!an1.Name.Equals(an2.Name, StringComparison.OrdinalIgnoreCase)) - { - return false; - } - - var versionCompare = an1.Version.CompareTo(an2.Version); - - if ((versionCompare < 0 && fUnified2) || (versionCompare > 0 && fUnified1)) - { - return true; - } - - if (versionCompare == 0) - { - return true; - } - - return false; - } - - // Based on coreclr baseassemblyspec.cpp (https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757b2/src/vm/baseassemblyspec.cpp#L330) - private static bool RefMatchesDef(AssemblyName @ref, AssemblyName def) - { - if (IsStrongNamed(@ref)) - { - return IsStrongNamed(def) && CompareRefToDef(@ref, def); - } - else - { - return @ref.Name.Equals(def.Name, StringComparison.OrdinalIgnoreCase); - } - } - - // Based on coreclr baseassemblyspec.inl (https://github.com/dotnet/coreclr/blob/32f0f9721afb584b4a14d69135bea7ddc129f755/src/vm/baseassemblyspec.inl#L679-L683) - private static bool IsStrongNamed(AssemblyName assembly) - { - var refPkt = assembly.GetPublicKeyToken(); - return refPkt != null && refPkt.Length != 0; - } - - // Based on https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757b2/src/vm/baseassemblyspec.cpp#L241 - private static bool CompareRefToDef(AssemblyName @ref, AssemblyName def) - { - if (!@ref.Name.Equals(def.Name, StringComparison.OrdinalIgnoreCase)) - { - return false; - } - - byte[] rpkt = @ref.GetPublicKeyToken(); - byte[] dpkt = def.GetPublicKeyToken(); - - if (rpkt.Length != dpkt.Length) - { - return false; - } - - for (int i = 0; i < rpkt.Length; i++) - { - if (rpkt[i] != dpkt[i]) - { - return false; - } - } - - if (@ref.Version != def.Version) - { - return false; - } - - if (@ref.CultureName != null && - @ref.CultureName != def.CultureName) - { - return false; - } - - return true; - } - //------------------------------------------------------------------------------ // PFXImportCertStore //------------------------------------------------------------------------------ From b5d46e2dc5ed9a802702fa85e951e99f445d36f1 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 22 Dec 2020 16:11:32 +0100 Subject: [PATCH 4/4] Fix comment wording --- src/Tasks/AssemblyDependency/ReferenceTable.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Tasks/AssemblyDependency/ReferenceTable.cs b/src/Tasks/AssemblyDependency/ReferenceTable.cs index 127a1c38f33..405f8662611 100644 --- a/src/Tasks/AssemblyDependency/ReferenceTable.cs +++ b/src/Tasks/AssemblyDependency/ReferenceTable.cs @@ -2237,14 +2237,15 @@ Dictionary> baseNameToReferences } // TODO: Verify correctness of this implementation and extend to more cases. - // Should be consistent with CompareAssemblyIdentity from Fusion API. - // Both of these identities must be full-specified (name, version, pkt, culture). The result parameter will TRUE if one (or more) of the following conditions is true: + // Should be consistent with CompareAssemblyIdentity from Fusion API: + // The result should be TRUE if one (or more) of the following conditions is true: // a) The assembly identities are equivalent. For strongly-named assemblies this means full match on (name, version, pkt, culture); for simply-named assemblies this means a match on (name, culture) // b) The assemblies being compared are FX assemblies (even if the version numbers are not the same, these will compare as equivalent by way of unification) // c) The assemblies are not FX assemblies but are equivalent because fUnified1 and/or fUnified2 were set. // The fUnified flag is used to indicate that all versions up to the version number of the strongly-named assembly are considered equivalent to itself. // For example, if assemblyIdentity1 is "foo, version=5.0.0.0, culture=neutral, publicKeyToken=...." and fUnified1==TRUE, then this means to treat all versions of the assembly in the range 0.0.0.0-5.0.0.0 to be equivalent to "foo, version=5.0.0.0, culture=neutral, publicKeyToken=...". - // If assemblyIdentity2 is the same as assemblyIdentity1, except has a lower version number (e.g.version range 0.0.0.0-5.0.0.0), then the function will return that the identities are equivalent.If assemblyIdentity2 is the same as assemblyIdentity1, but has a greater version number than 5.0.0.0 then the two identities will only be equivalent if fUnified2 is set. + // If assemblyIdentity2 is the same as assemblyIdentity1, except has a lower version number (e.g.version range 0.0.0.0-5.0.0.0), then the function will return that the identities are equivalent. + // If assemblyIdentity2 is the same as assemblyIdentity1, but has a greater version number than 5.0.0.0 then the two identities will only be equivalent if fUnified2 is set. /// /// Compares two assembly identities to determine whether or not they are equivalent. ///