Skip to content

Reimplement source assembly-reference check without ObjectModel (Phase 6e-4c2) #9631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Amaury Levé (Evangelink) merged 1 commit into from
Jul 5, 2026
Merged

Reimplement source assembly-reference check without ObjectModel (Phase 6e-4c2) #9631

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 68 additions & 4 deletions src/Adapter/MSTestAdapter.PlatformServices/Services/TestSourceHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.AppContainer;
#endif
using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.Interface;
#if NETFRAMEWORK
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities;
#endif

namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices;

Expand Down Expand Up @@ -79,7 +76,7 @@ public bool IsAssemblyReferenced(AssemblyName assemblyName, string source)
{
#if NETFRAMEWORK
// This loads the dll in a different app domain. We can optimize this to load in the current domain since this code could be run in a new app domain anyway.

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR — Stale Comment] The comment says "This loads the dll in a different app domain", but the new implementation (DoesSourceReferenceAssembly) uses Assembly.ReflectionOnlyLoadFrom in the current AppDomain — no child domain is created. Both sentences are now inaccurate (the second was an optimisation note that is now moot since the domain split was intentionally dropped).

Suggested replacement:

// Reflection-only loads the assembly to inspect its references without executing any code from it.

bool? utfReference = AssemblyHelper.DoesReferencesAssembly(source, assemblyName);
bool? utfReference = DoesSourceReferenceAssembly(source, assemblyName);

// If no reference to UTF don't run discovery. Take conservative approach. If not able to find proceed with discovery.
return !utfReference.HasValue || utfReference.Value;
Expand All @@ -94,6 +91,73 @@ public bool IsAssemblyReferenced(AssemblyName assemblyName, string source)
#endif
}

#if NETFRAMEWORK
/// <summary>
/// Checks whether the source assembly directly references the given assembly.
/// Only the assembly simple name and public key token are matched; version is ignored.
/// Returns <see langword="null"/> if the reference could not be determined.
/// </summary>
/// <param name="source"> The path to the source assembly to inspect. </param>
/// <param name="referenceAssembly"> The assembly to look for in the source's references. </param>
/// <returns> <see langword="true"/> if referenced, <see langword="false"/> if not, <see langword="null"/> if undeterminable. </returns>
private static bool? DoesSourceReferenceAssembly(string source, AssemblyName referenceAssembly)
{
if (string.IsNullOrEmpty(source) || referenceAssembly is null)
{
return null;
}

try
{
string? referenceAssemblyName = referenceAssembly.Name;
byte[] referenceAssemblyPublicKeyToken = referenceAssembly.GetPublicKeyToken();

// ReflectionOnlyLoadFrom loads from the specified path only (no probing) and does not
// execute any code from the loaded assembly.
var assembly = Assembly.ReflectionOnlyLoadFrom(source);

foreach (AssemblyName referencedAssembly in assembly.GetReferencedAssemblies())
{
// Match without version: only the simple name and public key token.
if (!string.Equals(referencedAssembly.Name, referenceAssemblyName, StringComparison.OrdinalIgnoreCase))
{
continue;
}

if (ArePublicKeyTokensEqual(referencedAssembly.GetPublicKeyToken(), referenceAssemblyPublicKeyToken))
{
return true;
}
}

return false;
}
catch

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR — Bare Catch / No Diagnostics] The catch block silently swallows every exception — file-not-found, BadImageFormatException, security exceptions, and the null-deref discussed on ArePublicKeyTokensEqual all land here invisibly. The conservative null return is intentional and correct, but without any trace output it is very hard to tell the difference between "file legitimately couldn't be opened" and "code bug".

Suggestion — at minimum emit a Debug.WriteLine (no user-visible surface, zero overhead in production):

catch (Exception ex)
{
    Debug.WriteLine($"[MSTest] DoesSourceReferenceAssembly could not inspect '{source}': {ex.Message}");
    return null;
}

{
// Return null if we are not able to check.
return null;
}
}

private static bool ArePublicKeyTokensEqual(byte[] left, byte[] right)
{
if (left.Length != right.Length)

@github-actions github-actions Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR — Null Safety] GetPublicKeyToken() returns null on .NET Framework for unsigned assemblies (those with no public key token). Both left (from referencedAssembly.GetPublicKeyToken()) and right (from referenceAssembly.GetPublicKeyToken()) can therefore be null, so left.Length on this line throws NullReferenceException. The outer catch absorbs that and returns null (→ proceed with discovery), so the process doesn't crash; but there are two problems:

  1. For two assemblies that both carry no public-key token, the semantically correct result is true (name match + neither is signed = same unsigned identity). Instead the caller gets a swallowed exception and falls through to "proceed" via null, which is accidentally correct today but for the wrong reason.
  2. referenceAssemblyPublicKeyToken on line 113 is declared byte[] (non-nullable), masking the actual nullable return of GetPublicKeyToken(). It should be byte[]?.

Suggested fix:

private static bool ArePublicKeyTokensEqual(byte[]? left, byte[]? right)
{
    if (left is null && right is null) return true;
    if (left is null || right is null) return false;
    if (left.Length != right.Length) return false;
    for (int i = 0; i < left.Length; ++i)
    {
        if (left[i] != right[i]) return false;
    }
    return true;
}

Also update the assignment on line 113:

byte[]? referenceAssemblyPublicKeyToken = referenceAssembly.GetPublicKeyToken();

{
return false;
}

for (int i = 0; i < left.Length; ++i)
{
if (left[i] != right[i])
{
return false;
}
}

return true;
}
#endif

/// <summary>
/// Gets the set of sources (dll's/exe's) that contain tests. If a source is a package (appx), return the file (dll/exe) that contains tests from it.
/// </summary>
Expand Down
Loading