Pass TPA through host-runtime contract instead of property string

[elinor-fung]

Pass trusted platform assemblies from the host to the runtime through host_runtime_contract callbacks instead of as a large string with path separators.

The host provides the resolved assembly names during initialization and the runtime requests each resolved path when needed. This reduces unnecessary parsing, reduces the amount we allocate as part of startup, and allows running applications with paths including a path separator.

The TRUSTED_PLATFORM_ASSEMBLIES property can still be explicitly queried (reconstructed on demand) and can still be explicitly supplied by custom hosts.

Resolves#3163
Resolves#75882
Resolves#104402

cc @dotnet/appmodel @AaronRobinsonMSFT

Windows x64, basic console app:

Metric	Main	Branch	Delta
Startup, launch-level median	67.684 ms	66.831 ms	-0.853 ms (-1.26%)
Native allocated bytes at live snapshot	10,131.65 KiB	9,593.95 KiB	-537.70 KiB (-5.31%)
Live retained native bytes	1,955.30 KiB	1,871.10 KiB	-84.20 KiB (-4.31%)

Startup was measured with the dotnet/performance TimeToMain2 ETW harness, from process start until a minimal console app emitted its first event. Native allocations were with the minimal app blocked on Console.ReadLine().

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jkotas]

Would it save even more memory allocations and copying if we stopped building the hashtable here and asked the host to resolve the assembly name to a filepath on demand every time?

[elinor-fung]

Good call. But maybe still keep the names only? The runtime does a case-insensitive hash for the names, which I don't know that we want to make any host's responsibility.

[jkotas]

Sounds reasonable

[Copilot]

Copilot review overview

🔵 Needs a closer look

There are a couple of correctness/robustness issues in newly added/updated tests that should be addressed before merge.

Review tier: Lite
Findings: 1

Pre-existing issues (1)

Severity	Finding
	src/​mono/​mono/​mini/​monovm.c — Mono AppContext.TryGetHostPropertyValue currently reconstructs TRUSTED_PLATFORM_ASSEMBLIES only via… View comment

Issues resolved since last review (1)

Severity	Finding
	src/​installer/​tests/​Assets/​Projects/​HostApiInvokerApp/​HostRuntimeContract.cs — HostApiInvokerApp's managed host_runtime_contract struct updates the size by adding… View resolved comment

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/installer/tests/HostActivation.Tests/DependencyResolution/DependencyResolutionCommandResultExtensions.cs:70

• Splitting stderr using Environment.NewLine can miss lines if the captured output uses a different newline convention (e.g., "\n" only). Using a split that handles both "\r\n" and "\n" makes the assertion more robust across platforms and output sources.
  src/tests/Loader/TrustedPlatformAssembliesProperty/TrustedPlatformAssemblies.cs:24
• The test uses StringComparer.OrdinalIgnoreCase unconditionally when checking that the CoreLib path is present in TRUSTED_PLATFORM_ASSEMBLIES. On case-sensitive file systems (typical on Linux), this can hide casing issues and make the assertion weaker than intended. Consider using a case-insensitive comparison only where appropriate (e.g., Windows).

[Copilot]

Copilot review overview

🟡 Changes recommended

Mono’s new AppContext host-property ICALL currently uses a hard g_assert for a reachable condition, which can abort the process in debug builds instead of failing gracefully.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	src/​mono/​mono/​mini/​monovm.c — ves_icall_System_AppContext_TryGetHostPropertyValue uses `g_assert(trusted_platform_assemblies->as…

Issues resolved since last review (1)

Severity	Finding
	src/​mono/​mono/​mini/​monovm.c — Mono AppContext.TryGetHostPropertyValue currently reconstructs TRUSTED_PLATFORM_ASSEMBLIES only via… View resolved comment

[steveisok]

@elinor-fung in case this helps at all. I could never land it - #100503

[Copilot]

Copilot review overview

🟡 Changes recommended

Mono’s new ves_icall_System_AppContext_TryGetHostPropertyValue uses property_value without declaring/initializing it, which is a build-breaking defect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	src/​mono/​mono/​mini/​monovm.c — property_value is used but never declared/initialized in `ves_icall_System_AppContext_TryGetHostPr…

Issues resolved since last review (1)

Severity	Finding
	src/​mono/​mono/​mini/​monovm.c — ves_icall_System_AppContext_TryGetHostPropertyValue uses `g_assert(trusted_platform_assemblies->as… View resolved comment

[Copilot]

Copilot review overview

🔵 Needs a closer look

There are at least two correctness issues in changed code paths (mockcoreclr contract pointer deref and corerun get_assembly_names returning false for empty results) that should be fixed before merge.

Review tier: Lite
Findings: None

Issues resolved since last review (1)

Severity	Finding
	src/​mono/​mono/​mini/​monovm.c — property_value is used but never declared/initialized in `ves_icall_System_AppContext_TryGetHostPr… View resolved comment

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/coreclr/hosts/corerun/corerun.cpp:330

• The host-runtime contract allows get_assembly_names to succeed with count == 0 (both CoreCLR and Mono treat that as a valid result). Returning false when the list is empty makes the runtime fall back to the legacy TPA property string even though the callback mechanism is supported, and can become a hard failure if corerun is in CALLBACK mode (no property provided).
  src/native/corehost/test/mockcoreclr/mockcoreclr.cpp:64
• coreclr_initialize in the mock runtime unconditionally dereferences the parsed host_runtime_contract* (via contract->size) without validating that the pointer string parsed to a non-null pointer. If a test passes an invalid/empty HOST_RUNTIME_CONTRACT value, this will AV the mock instead of reporting 0 names (or otherwise continuing), making negative tests brittle.

[vitek-karas]

Possibly the same as Jan's comment below - I don't understand why we need to get the full list from the host - why not just build it on the fly. That way if the app doesn't need a certain assembly, it would never even allocate the TPA entry for it.

[elinor-fung]

I kept the names so that the case-insensitive hash would remain the runtime's responsibility. We could do something like replicate SString::HashCaseInsensitive in the host, but I didn't really want to push that responsibility onto the host (or custom host if they augment the TPA via the callbacks). It felt like since the runtime's binding behaviour is case-insensitive, it should stay the one to enforce that for the TPA too.

[vitek-karas]

I'm thinking about future improvements - the hostpolicy currently has to process all of the framework's JSON's in order to figure out the full list of assemblies - with this change this is really only needed here. If we remove this somehow, in theory the hostpolicy would not need to parse the JSON's or do version resolution until a specific assembly is asked for by the runtime.

You're right that it would mean that the host must resolve assemblies using case-insensitive lookup, but only once, the runtime could still maintain the hash table of all loaded assemblies, and any future loads would resolve from that table if already present.

It's not a big deal though - current hostpolicy would not be able to take advantage of this to optimize things, we would need to restructure it quite a bit.

[elinor-fung]

in theory the hostpolicy would not need to parse the JSON's or do version resolution until a specific assembly is asked for by the runtime

I had briefly wanted to do this (before actually starting on this PR), but didn't come up with a way that seemed particularly good to me. I basically had:

• parse JSONs on first assembly request (basically immediate?), keep some representation of the unresolved assets, and resolve to path on request
• re-parse and resolve every request
• augment deps.json in some way that we can index on asset name easily (only parse what is needed if possible?)

But honestly, I didn't actually try any of those. I think it'd warrant some experimentation, but the current change seemed like a clear 'do less and hold on to less' - so I just kind of went with 'should still work with anything we add now if we try more improvements later'.

You're right that it would mean that the host must resolve assemblies using case-insensitive lookup, but only once, the runtime could still maintain the hash table of all loaded assemblies, and any future loads would resolve from that table if already present.

Fair enough. I mostly looked at the case-insensitive hash implementation and thought it seemed like complex and possibly error-prone logic to push to hosts. (I also just found that docs call out that even though the runtime does case-insensitive binding, we recommend people treat assembly names as case-sensitive.)

[vitek-karas]

OK - I don't feel to strongly about this - as you say it's not easy to take advantage of it in the host as it currently works anyway (and if/when we rewrite hostpolicy to C# and move it into corelib, it can work differently anyway).

[AaronRobinsonMSFT]

@jkotas Are these changes to mono appropriate for .NET 12?

[jkotas]

We are keeping Mono working for a few more months until we complete the transition. We do not need to be improving it. So, choose the simplest approach that fits within these constraints. I’m not able to tell whether the current changes in this PR represent that simplest approach.

[elinor-fung]

The one simpler thing could be to not put this in a separate hostinformation file (stick it all in monovm.c). Other than that, I was just going for 'keep working' for Mono.

[Copilot]

Copilot review overview

🟡 Changes recommended

There are merge-blocking correctness issues in CoreCLR binder allocation handling and in corerun’s get_assembly_names callback behavior that can cause crashes or unintended fallback paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	src/​coreclr/​hosts/​corerun/​corerun.cpp — get_assembly_names returns false when the host-resolved assembly list is empty. Per the…

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/coreclr/binder/applicationcontext.cpp:111

• wszSimpleName is allocated with plain new [] and used unconditionally. In the binder we generally use new (nothrow) + an explicit E_OUTOFMEMORY path (see SAFE_NEW in bindertypes.hpp); otherwise a failed allocation can lead to a null deref in wcscpy_s (and in builds without C++ exceptions, plain new may return null).

This issue also appears on line 134 of the same file.

src/coreclr/binder/applicationcontext.cpp:141

• Same allocation issue here for both wszSimpleName and wszFileName: plain new [] is used with no failure handling. This can turn an OOM into a crash (or an unhandled exception, depending on the build configuration).

 LPWSTR wszSimpleName = new WCHAR[simpleName.GetCount() + 1];
wcscpy_s(wszSimpleName, simpleName.GetCount() + 1, simpleName.GetUnicode());
LPWSTR wszFileName = new WCHAR[fileName.GetCount() + 1];
wcscpy_s(wszFileName, fileName.GetCount() + 1, fileName.GetUnicode());
SimpleNameToFileNameMapEntry mapEntry{ wszSimpleName, wszFileName };
m_pTrustedPlatformAssemblyMap->AddOrReplace(mapEntry);

[Copilot]

Copilot review overview

🟡 Changes recommended

There are merge-blocking correctness issues in the updated binding/loader paths (notably missing allocation-failure checks in CoreCLR binder and a Mono preload-hook early-termination case) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 3 · 1

New issues introduced by this change (4)

Severity	Finding
	src/​coreclr/​binder/​applicationcontext.cpp — The TPA-name path uses new WCHAR[...] without any null/OOM check, but this function otherwise…
	src/​mono/​mono/​mini/​monovm.c — When using host-resolved TPA entries, failing to resolve a single assembly path currently breaks…
	src/​native/​corehost/​hostpolicy/​hostpolicy_context.cpp — name is derived by stripping only the last extension from the resolved path. This can produce a…
	src/​installer/​tests/​HostActivation.Tests/​DependencyResolution/​DependencyResolutionCommandResultExtensions.cs — Splitting stderr with Split(Environment.NewLine) is brittle if the captured output uses different…

Issues resolved since last review (1)

Severity	Finding
	src/​coreclr/​hosts/​corerun/​corerun.cpp — get_assembly_names returns false when the host-resolved assembly list is empty. Per the… View resolved comment

Suppressed comments (1)

src/coreclr/binder/applicationcontext.cpp:138

• Same issue in the legacy TPA-property parsing path: wszSimpleName/wszFileName allocations no longer check for failure before wcscpy_s, despite this method using HRESULT-style control flow for OOM elsewhere.

 LPWSTR wszSimpleName = new WCHAR[simpleName.GetCount() + 1];
wcscpy_s(wszSimpleName, simpleName.GetCount() + 1, simpleName.GetUnicode());
LPWSTR wszFileName = new WCHAR[fileName.GetCount() + 1];
wcscpy_s(wszFileName, fileName.GetCount() + 1, fileName.GetUnicode());