Scope .NET TaskHost handshake leniency to architecture - #14579
Scope .NET TaskHost handshake leniency to architecture#14579JanProvaznik wants to merge 2 commits into
Conversation
f8fb3a7 to
043c184
Compare
|
/review |
|
❌ Expert Code Review (command) failed. Please review the logs for details. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an over-broad .NET TaskHost handshake tolerance that previously allowed any mismatch in the handshake options word (when the parent suppressed its architecture bits) to be treated as acceptable, rather than limiting the tolerance strictly to architecture differences. This tightens correctness and prevents unintended cross-identity matching (notably around NodeReuse, LowPriority, and Administrator) while preserving the intended cross-arch connectivity scenario.
Changes:
- Scope
IsAllowedBitnessMismatchso onlyX64/Arm64bits may differ; all otherHandshakeOptionsflags must match. - Add unit tests covering previously-permitted (but incorrect) mismatches for
NodeReuse,LowPriority, andAdministrator, plus regression guards for version-byte masking and the motivating arch-tolerance scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Shared/NodeEndpointOutOfProcBase.cs | Restricts the handshake leniency to architecture-only differences by enforcing equality of all non-architecture handshake flags. |
| src/Build.UnitTests/BackEnd/NodeEndpointOutOfProcBase_Tests.cs | Adds targeted tests to prevent future regressions and validate the narrowed tolerance behavior. |
IsAllowedBitnessMismatch lets a parent that did not emit an architecture bit connect to an x64/arm64 .NET TaskHost, which is needed because a .NET Framework parent cannot describe the architecture of the SDK TaskHost it launches. The tolerance was not scoped: it returned true whenever the parent sent no architecture bit and the child expected x64/arm64, regardless of every other flag. A .NET TaskHost parent deliberately suppresses its architecture bit, so any such parent also matched a TaskHost that differed in identity flags. NodeReuse is what separates a long-lived sidecar TaskHost from a TaskHost that exits with the build, so a parent that never asked for a sidecar could be admitted to one, take it over for its own build, and afterwards hand it back to the global reconnectable pool. LowPriority and Administrator were equally ignored. Require every non-architecture flag to match and allow divergence only in X64/Arm64. The case the tolerance exists for is unaffected, since those parents differ from the TaskHost only in architecture. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
043c184 to
76059d2
Compare
x86 is encoded as the absence of X64/Arm64 rather than a flag of its own, so an x86 TaskHost node expects no architecture bit and the tolerance never applies to it. Nothing asserted that, leaving room for a future change to widen what an x86 TaskHost node accepts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ecd77e2-5945-45e3-b885-befa4a23d37a
|
/review |
|
❌ Expert Code Review (command) failed. Please review the logs for details. |
Part of the node-ownership work specced in #14508, but standalone: it depends on nothing else in that stack and is a correctness fix on its own.
The bug
IsAllowedBitnessMismatchexists so a parent that cannot describe the architecture of the SDK TaskHost it launches can still connect to it. A .NET Framework parent launching aRuntime="NET"TaskHost is the motivating case, and to make it work the parent deliberately suppresses its own architecture bit.The tolerance was never scoped to architecture. It returned true whenever the parent sent no architecture bit and the child expected x64/arm64 — regardless of every other flag in the options word:
So any parent that suppressed its architecture bit also matched a TaskHost that differed from it in identity flags.
That matters most for
NodeReuse, becauseNodeReuseis the only thing separating a long-lived sidecar TaskHost from one that exits with the build:effectiveNodeReuse = EnableNodeReuse && _useSidecarTaskHost(TaskHostTask.cs)GetHandshakeOptions(taskHost: true, nodeReuse: effectiveNodeReuse, ...), which setsHandshakeOptions.NodeReuseNothing else separates them for this connection: a sidecar is a .NET TaskHost, and .NET TaskHost handshakes deliberately neutralize the file-version components with the
NetTaskHostHandshakeVersion = 99sentinel, while the salt is the tools directory, which coincides whenever a parent resolves the same installed SDK.Consequence: a parent that never asked for a sidecar could be admitted to one, take it over for its own build, and afterwards return it to the global reconnectable pool.
LowPriorityandAdministratorwere ignored the same way.The fix
Require every non-architecture flag to match; allow divergence only in
X64/Arm64.The case the tolerance exists for is unaffected — those parents differ from the TaskHost only in architecture, which is exactly what stays tolerated.
Compatibility
The blast radius is flags that should already agree between a matched parent and child:
NodeReuseandLowPriorityare handed to the child as launch arguments by the same parent that computed themAdministratoris derived on both sides from elevation the child inheritsCLR2/NET/TaskHostare structuralThis is nonetheless the one change in the stack that can surface as MSB4216, so it is landing on its own, ahead of the lifetime work, and carries a cross-version check of a
Runtime="NET"UsingTaskin both directions across the 10.0.3xx boundary.Tests
Six added to
NodeEndpointOutOfProcBase_Tests. Four are load-bearing — verified failing against the unpatched method and passing after:NonSidecarParent_SidecarTaskHost_NotToleratedSidecarParent_NonSidecarTaskHost_NotToleratedDifferingLowPriority_NotToleratedDifferingAdministrator_NotToleratedTwo are regression guards that must keep passing either way:
SidecarParent_SidecarTaskHost_ArchitectureStillTolerated— the motivating case still connectsHandshakeVersionInUpperByte_IsIgnored— the version byte still does not affect comparisonAll 12 tests in the class pass (
net11.0).Why this is separate from the lifetime fix
This does not fix #14313 on its own. That leak is a sidecar disconnecting after a build and relisting on its pipe, which only the ownership-lifetime change stops.
The two are complementary and this one is a precondition: once sidecars are owned, an owned sidecar could still be adopted in the window before its owner connects, or by a differently-versioned MSBuild during rollout, if this hole stayed open.