From 504f4c04b7c91d492fc1b3f25d166fdc805c583f Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Wed, 24 Jun 2020 16:23:04 -0700 Subject: [PATCH 01/10] First step Second part Part 1 of not checking byte parent d86a1e168bdf295aa777d47ee1a4b988b8913889 author Nathan Mytelka 1591730709 -0700 committer Nathan Mytelka 1593461702 -0700 Remove outdated comment Part 2 of unmasking first byte Part 3 Part 4 Part 5 Part 6 Reenabled administrator privilege and cleanup Add test Improve RemoveDependenciesFromEntryIfMissing (#5392) Fixes #5180 The fix is a straightforward cache of files that have been detected to exist already, preventing duplicate file system checks on files we already know exist. There will be a future issue and PR addressing ConstructDependencyTable mentioned in a comment on the original issue. Testing on my machine shows an improvement in RemoveDependenciesFromEntryIfMissing of ~850ms down to 30~35ms. Spruce up ObjectModelHelpers assertions These fired while I was writing a new test but didn't have much useful information. Keep AssertItems from throwing an ArgumentOutOfRangeException on mismatched lengths, and give a clue or two about mismatched lengths in AssertItemHasMetadata. Regression tests for #5445 Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type. Respect item functions in lazy Update/Remove Fixes #5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures. Rename short-circuit-lazy-item-update check method The question this method answers is 'can I just remove/update every item in the group, or do I need to expand the value to match against existing items?' Renamed it for a bit more clarity there. Log CurrentUICulture in binlog (#5426) This will be useful to be able to open localized binlogs. If we know the culture of the log we can fetch the right resources from the MSBuild .dlls. Properly parse version Part 7 --- .../Evaluation/ItemEvaluation_Tests.cs | 37 +++ src/Build.UnitTests/Utilities_Tests.cs | 15 +- .../Communications/NodeEndpointOutOfProc.cs | 12 +- .../Communications/NodeProviderOutOfProc.cs | 17 +- .../NodeProviderOutOfProcBase.cs | 49 ++-- .../NodeProviderOutOfProcTaskHost.cs | 14 +- .../LazyItemEvaluator.LazyItemOperation.cs | 14 +- .../LazyItemEvaluator.RemoveOperation.cs | 2 +- .../LazyItemEvaluator.UpdateOperation.cs | 2 +- .../Logging/BinaryLogger/BinaryLogger.cs | 1 + src/MSBuild/NodeEndpointOutOfProcTaskHost.cs | 12 +- src/Shared/CommunicationsUtilities.cs | 220 ++++++------------ src/Shared/NodeEndpointOutOfProcBase.cs | 94 ++++---- src/Shared/UnitTests/ObjectModelHelpers.cs | 10 +- .../CanonicalTrackedInputFiles.cs | 25 +- .../CanonicalTrackedOutputFiles.cs | 37 +-- 16 files changed, 279 insertions(+), 282 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ItemEvaluation_Tests.cs b/src/Build.UnitTests/Evaluation/ItemEvaluation_Tests.cs index 8fb9ef51a22..cc00152c075 100644 --- a/src/Build.UnitTests/Evaluation/ItemEvaluation_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ItemEvaluation_Tests.cs @@ -120,6 +120,43 @@ public void RemoveShouldPreserveIntermediaryReferences(string content) ObjectModelHelpers.AssertItems(new string[0], itemsForI2); } + [Fact] + public void RemoveRespectsItemTransform() + { + var content = @" + + + + "; + + IList items = ObjectModelHelpers.GetItemsFromFragment(content, allItems: true); + + ObjectModelHelpers.AssertItems(new[] { "a", "c" }, items); + } + + [Fact] + public void UpdateRespectsItemTransform() + { + var content = @" + + + + m1_updated + + '%(Extension)')`> + m2_updated + "; + + IList items = ObjectModelHelpers.GetItemsFromFragment(content, allItems: true); + + ObjectModelHelpers.AssertItems(new[] { "a", "b", "c" }, items, + new[] { + new Dictionary(), + new Dictionary { ["m1"] = "m1_updated" }, + new Dictionary(), + }); + } + [Fact] public void UpdateShouldPreserveIntermediaryReferences() { diff --git a/src/Build.UnitTests/Utilities_Tests.cs b/src/Build.UnitTests/Utilities_Tests.cs index 0a4d92dc8e2..2219cc6bb72 100644 --- a/src/Build.UnitTests/Utilities_Tests.cs +++ b/src/Build.UnitTests/Utilities_Tests.cs @@ -3,19 +3,13 @@ using System; using System.Collections; -using System.Collections.Specialized; -using System.Text.RegularExpressions; -using Microsoft.Build.Framework; using Microsoft.Build.Shared; - - using CommunicationsUtilities = Microsoft.Build.Internal.CommunicationsUtilities; using InternalUtilities = Microsoft.Build.Internal.Utilities; using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException; using MSBuildApp = Microsoft.Build.CommandLine.MSBuildApp; -using Project = Microsoft.Build.Evaluation.Project; using ProjectCollection = Microsoft.Build.Evaluation.ProjectCollection; using Toolset = Microsoft.Build.Evaluation.Toolset; @@ -27,14 +21,21 @@ using Xunit; using System.Collections.Generic; using System.IO; +using Microsoft.Build.Internal; +using Shouldly; +using System.Linq; +using Xunit.Abstractions; namespace Microsoft.Build.UnitTests { public class UtilitiesTestStandard : UtilitiesTest { - public UtilitiesTestStandard() + private readonly ITestOutputHelper _output; + + public UtilitiesTestStandard(ITestOutputHelper output) { this.loadAsReadOnly = false; + _output = output; } [Fact] diff --git a/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs b/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs index a1b4c7ae411..f2d88b38ebf 100644 --- a/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs +++ b/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs @@ -67,17 +67,9 @@ internal NodeEndpointOutOfProc( /// /// Returns the host handshake for this node endpoint /// - protected override long GetHostHandshake() + protected override Handshake GetHandshake() { - return NodeProviderOutOfProc.GetHostHandshake(_enableReuse, _lowPriority); - } - - /// - /// Returns the client handshake for this node endpoint - /// - protected override long GetClientHandshake() - { - return NodeProviderOutOfProc.GetClientHandshake(_enableReuse, _lowPriority); + return new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, is64Bit: EnvironmentUtilities.Is64BitProcess, nodeReuse: _enableReuse, lowPriority: _lowPriority)); } #region Structs diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs index e94c194e33d..cf5e6551321 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs @@ -70,19 +70,10 @@ public int AvailableNodes /// /// Is reuse of build nodes allowed? /// Is the build running at low priority? - internal static long GetHostHandshake(bool enableNodeReuse, bool enableLowPriority) + internal static Handshake GetHandshake(bool enableNodeReuse, bool enableLowPriority) { CommunicationsUtilities.Trace("MSBUILDNODEHANDSHAKESALT=\"{0}\", msbuildDirectory=\"{1}\", enableNodeReuse={2}, enableLowPriority={3}", Traits.MSBuildNodeHandshakeSalt, BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, enableNodeReuse, enableLowPriority); - return CommunicationsUtilities.GetHostHandshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, nodeReuse: enableNodeReuse, lowPriority: enableLowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); - } - - /// - /// Magic number sent by the client to the host during the handshake. - /// Munged version of the host handshake. - /// - internal static long GetClientHandshake(bool enableNodeReuse, bool enableLowPriority) - { - return CommunicationsUtilities.GetClientHandshake(CommunicationsUtilities.GetHandshakeOptions(false, nodeReuse: enableNodeReuse, lowPriority: enableLowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); + return new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, nodeReuse: enableNodeReuse, lowPriority: enableLowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); } /// @@ -107,8 +98,8 @@ public bool CreateNode(int nodeId, INodePacketFactory factory, NodeConfiguration // Make it here. CommunicationsUtilities.Trace("Starting to acquire a new or existing node to establish node ID {0}...", nodeId); - long hostHandShake = NodeProviderOutOfProc.GetHostHandshake(ComponentHost.BuildParameters.EnableNodeReuse, ComponentHost.BuildParameters.LowPriority); - NodeContext context = GetNode(null, commandLineArgs, nodeId, factory, hostHandShake, NodeProviderOutOfProc.GetClientHandshake(ComponentHost.BuildParameters.EnableNodeReuse, ComponentHost.BuildParameters.LowPriority), NodeContextTerminated); + Handshake hostHandshake = new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: false, nodeReuse: ComponentHost.BuildParameters.EnableNodeReuse, lowPriority: ComponentHost.BuildParameters.LowPriority, is64Bit: EnvironmentUtilities.Is64BitProcess)); + NodeContext context = GetNode(null, commandLineArgs, nodeId, factory, hostHandshake, NodeContextTerminated); if (null != context) { diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index cc8507b4645..a749f776717 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -126,12 +126,12 @@ protected void ShutdownAllNodes(bool nodeReuse, NodeContextTerminateDelegate ter int timeout = 30; // Attempt to connect to the process with the handshake without low priority. - Stream nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHostHandshake(nodeReuse, false), NodeProviderOutOfProc.GetClientHandshake(nodeReuse, false)); + Stream nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, false)); if (null == nodeStream) { // If we couldn't connect attempt to connect to the process with the handshake including low priority. - nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHostHandshake(nodeReuse, true), NodeProviderOutOfProc.GetClientHandshake(nodeReuse, true)); + nodeStream = TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, true)); } if (null != nodeStream) @@ -149,7 +149,7 @@ protected void ShutdownAllNodes(bool nodeReuse, NodeContextTerminateDelegate ter /// Finds or creates a child process which can act as a node. /// /// The pipe stream representing the node. - protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, int nodeId, INodePacketFactory factory, long hostHandshake, long clientHandshake, NodeContextTerminateDelegate terminateNode) + protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, int nodeId, INodePacketFactory factory, Handshake hostHandshake, NodeContextTerminateDelegate terminateNode) { #if DEBUG if (Execution.BuildManager.WaitForDebugger) @@ -189,7 +189,7 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in } // Get the full context of this inspection so that we can always skip this process when we have the same taskhost context - string nodeLookupKey = GetProcessesToIgnoreKey(hostHandshake, clientHandshake, nodeProcess.Id); + string nodeLookupKey = GetProcessesToIgnoreKey(hostHandshake, nodeProcess.Id); if (_processesToIgnore.Contains(nodeLookupKey)) { continue; @@ -199,7 +199,7 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in _processesToIgnore.Add(nodeLookupKey); // Attempt to connect to each process in turn. - Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 0 /* poll, don't wait for connections */, hostHandshake, clientHandshake); + Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 0 /* poll, don't wait for connections */, hostHandshake); if (nodeStream != null) { // Connection successful, use this node. @@ -243,14 +243,14 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in // Create the node process int msbuildProcessId = LaunchNode(msbuildLocation, commandLineArgs); - _processesToIgnore.Add(GetProcessesToIgnoreKey(hostHandshake, clientHandshake, msbuildProcessId)); + _processesToIgnore.Add(GetProcessesToIgnoreKey(hostHandshake, msbuildProcessId)); // Note, when running under IMAGEFILEEXECUTIONOPTIONS registry key to debug, the process ID // gotten back from CreateProcess is that of the debugger, which causes this to try to connect // to the debugger process. Instead, use MSBUILDDEBUGONSTART=1 // Now try to connect to it. - Stream nodeStream = TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake, clientHandshake); + Stream nodeStream = TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake); if (nodeStream != null) { // Connection successful, use this node. @@ -293,9 +293,9 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in /// Generate a string from task host context and the remote process to be used as key to lookup processes we have already /// attempted to connect to or are already connected to /// - private string GetProcessesToIgnoreKey(long hostHandshake, long clientHandshake, int nodeProcessId) + private string GetProcessesToIgnoreKey(Handshake hostHandshake, int nodeProcessId) { - return hostHandshake.ToString(CultureInfo.InvariantCulture) + "|" + clientHandshake.ToString(CultureInfo.InvariantCulture) + "|" + nodeProcessId.ToString(CultureInfo.InvariantCulture); + return hostHandshake.ToString() + "|" + nodeProcessId.ToString(CultureInfo.InvariantCulture); } #if !FEATURE_PIPEOPTIONS_CURRENTUSERONLY @@ -322,7 +322,7 @@ private void ValidateRemotePipeSecurityOnWindows(NamedPipeClientStream nodeStrea /// /// Attempts to connect to the specified process. /// - private Stream TryConnectToProcess(int nodeProcessId, int timeout, long hostHandshake, long clientHandshake) + private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake handshake) { // Try and connect to the process. string pipeName = NamedPipeUtil.GetPipeNameOrPath("MSBuild" + nodeProcessId); @@ -352,19 +352,36 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, long hostHand } #endif - CommunicationsUtilities.Trace("Writing handshake to pipe {0}", pipeName); - nodeStream.WriteLongForHandshake(hostHandshake); + int index = 1; + foreach (int part in handshake.RetrieveHandshakeComponents()) + { + CommunicationsUtilities.Trace("Writing handshake part {0} to pipe {1}", index, pipeName); + nodeStream.WriteIntForHandshake(part); + index++; + } CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName); #if NETCOREAPP2_1 || MONO - long handshake = nodeStream.ReadLongForHandshake(timeout); + int response = nodeStream.ReadIntForHandshake(0x39, timeout); #else - long handshake = nodeStream.ReadLongForHandshake(); + int response = nodeStream.ReadIntForHandshake(0x39); #endif - if (handshake != clientHandshake) + if (response != 0x39393939) + { + CommunicationsUtilities.Trace("Handshake failed on part {0}. Probably the client is a different MSBuild build.", response, index); + throw new InvalidOperationException(); + } + + // This indicates that we have finished all the parts of our handshake; hopefully the endpoint has as well. + nodeStream.WriteIntForHandshake(-0x2a2a2a2a); + if (nodeStream.ReadIntForHandshake(0x12 +#if NETCOREAPP2_1 || MONO + , timeout +#endif + ) != 0x12812812) { - CommunicationsUtilities.Trace("Handshake failed. Received {0} from client not {1}. Probably the client is a different MSBuild build.", handshake, clientHandshake); + CommunicationsUtilities.Trace("Handshake failed. The client handshake has a different number of parts. Probably the client is a different MSBuild build."); throw new InvalidOperationException(); } diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs index e0aebf0ade8..167bfacd636 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs @@ -217,16 +217,7 @@ public void ShutdownConnectedNodes(bool enableReuse) /// public void ShutdownAllNodes() { - bool nodeReuse = ComponentHost.BuildParameters.EnableNodeReuse; - - // To avoid issues with mismatched priorities not shutting - // down all the nodes on exit, we will attempt to shutdown - // all matching notes with and without the priroity bit set. - // So precompute both versions of the handshake now. - long hostHandshake = NodeProviderOutOfProc.GetHostHandshake(nodeReuse, enableLowPriority: false); - long hostHandshakeWithLow = NodeProviderOutOfProc.GetHostHandshake(nodeReuse, enableLowPriority: true); - - ShutdownAllNodes(nodeReuse, NodeContextTerminated); + ShutdownAllNodes(ComponentHost.BuildParameters.EnableNodeReuse, NodeContextTerminated); } #endregion @@ -552,8 +543,7 @@ internal bool CreateNode(HandshakeOptions hostContext, INodePacketFactory factor commandLineArgs, (int)hostContext, this, - CommunicationsUtilities.GetHostHandshake(hostContext), - CommunicationsUtilities.GetClientHandshake(hostContext), + new Handshake(hostContext), NodeContextTerminated ); diff --git a/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs index 21bdd300107..03d07264cf2 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs @@ -324,7 +324,11 @@ protected bool NeedToExpandMetadataForEachItem(ImmutableList itemSpec, string referencedItemType) + /// + /// Is this spec a single reference to a specific item? + /// + /// True if the item is a simple reference to the referenced item type. + protected static bool ItemspecContainsASingleBareItemReference(ItemSpec itemSpec, string referencedItemType) { if (itemSpec.Fragments.Count != 1) { @@ -342,6 +346,14 @@ protected static bool ItemspecContainsASingleItemReference(ItemSpec itemSp return false; } + // If the itemSpec is a single call to an item function, like @(X->Something(...)), it may get this + // far, but shouldn't be treated as a single reference: the item function may return entirely + // different results from a bare reference like @(X). + if (itemExpressionFragment.Capture.Captures is object) + { + return false; + } + return true; } } diff --git a/src/Build/Evaluation/LazyItemEvaluator.RemoveOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.RemoveOperation.cs index fe2dcf43a85..e184cd7c538 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.RemoveOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.RemoveOperation.cs @@ -39,7 +39,7 @@ protected override void ApplyImpl(ImmutableList.Builder listBuilder, I new BuildEventFileInfo(string.Empty), "OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem"); - if (_matchOnMetadata.IsEmpty && ItemspecContainsASingleItemReference(_itemSpec, _itemElement.ItemType)) + if (_matchOnMetadata.IsEmpty && ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType)) { // Perf optimization: If the Remove operation references itself (e.g. ) // then all items are removed and matching is not necessary diff --git a/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs index 025f247cfec..b1e13d2ed83 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs @@ -46,7 +46,7 @@ protected override void ApplyImpl(ImmutableList.Builder listBuilder, I ItemSpecMatchesItem matchItemspec; bool? needToExpandMetadataForEachItem = null; - if (ItemspecContainsASingleItemReference(_itemSpec, _itemElement.ItemType)) + if (ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType)) { // Perf optimization: If the Update operation references itself (e.g. ) // then all items are updated and matching is not necessary diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 052ab6a9f3a..adbb3ba25c9 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -154,6 +154,7 @@ public void Initialize(IEventSource eventSource) private void LogInitialInfo() { LogMessage("BinLogFilePath=" + FilePath); + LogMessage("CurrentUICulture=" + System.Globalization.CultureInfo.CurrentUICulture.Name); } private void LogMessage(string text) diff --git a/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs b/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs index e3ea73f69f8..62858c5a4b5 100644 --- a/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs +++ b/src/MSBuild/NodeEndpointOutOfProcTaskHost.cs @@ -30,17 +30,9 @@ internal NodeEndpointOutOfProcTaskHost(string pipeName) /// /// Returns the host handshake for this node endpoint /// - protected override long GetHostHandshake() + protected override Handshake GetHandshake() { - return CommunicationsUtilities.GetHostHandshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: true)); - } - - /// - /// Returns the client handshake for this node endpoint - /// - protected override long GetClientHandshake() - { - return CommunicationsUtilities.GetClientHandshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: true)); + return new Handshake(CommunicationsUtilities.GetHandshakeOptions(taskHost: true)); } } } diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index 35131046269..51431783249 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -20,6 +20,50 @@ namespace Microsoft.Build.Internal { + internal readonly struct Handshake + { + readonly int options; + readonly int salt; + readonly int fileVersionMajor; + readonly int fileVersionMinor; + readonly int fileVersionBuild; + readonly int fileVersionPrivate; + readonly int sessionId; + + internal Handshake(HandshakeOptions nodeType) + { + // We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00. + // This indicates in the first byte that we are a modern build. + options = (int)nodeType | 0x01000000; + string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); + string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; + salt = CommunicationsUtilities.GetHandshakeHashCode(handshakeSalt + toolsDirectory); + Version fileVersion = new Version(FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location).FileVersion); + fileVersionMajor = fileVersion.Major; + fileVersionMinor = fileVersion.Minor; + fileVersionBuild = fileVersion.Build; + fileVersionPrivate = fileVersion.Revision; + sessionId = Process.GetCurrentProcess().SessionId; + } + + // This is used as a key, so it does not need to be human readable. + public override string ToString() + { + return string.Empty + options + salt + fileVersionMajor + fileVersionMinor + fileVersionBuild + fileVersionPrivate + sessionId; + } + + internal IEnumerable RetrieveHandshakeComponents() + { + yield return options; + yield return salt; + yield return fileVersionMajor; + yield return fileVersionMinor; + yield return fileVersionBuild; + yield return fileVersionPrivate; + yield return sessionId; + } + } + /// /// Enumeration of all possible (currently supported) options for handshakes. /// @@ -51,7 +95,12 @@ internal enum HandshakeOptions /// /// Building with BelowNormal priority /// - LowPriority = 16 + LowPriority = 16, + + /// + /// Building with administrator privileges + /// + Administrator = 32 } /// @@ -64,16 +113,6 @@ static internal class CommunicationsUtilities /// private const int DefaultNodeConnectionTimeout = 900 * 1000; // 15 minutes; enough time that a dev will typically do another build in this time - /// - /// Flag if we have already calculated the FileVersion hashcode - /// - private static bool s_fileVersionChecked; - - /// - /// A hashcode calculated from the fileversion - /// - private static int s_fileVersionHash; - /// /// Whether to trace communications /// @@ -102,48 +141,6 @@ static internal int NodeConnectionTimeout get { return GetIntegerVariableOrDefault("MSBUILDNODECONNECTIONTIMEOUT", DefaultNodeConnectionTimeout); } } - /// - /// Looks up the file version and caches the hashcode - /// This file version hashcode is used in calculating the handshake - /// - private static int FileVersionHash - { - get - { - if (!s_fileVersionChecked) - { - // We only hash in any complus_installroot value, not a file version. - // This is because in general msbuildtaskhost.exe does not load any assembly that - // the parent process loads, so they can't compare the version of a particular assembly. - // They can't compare their own versions, because if one of them is serviced, they - // won't match any more. The only known incompatibility is between a razzle and non-razzle - // parent and child. COMPLUS_Version can (and typically will) differ legitimately between - // them, so just check COMPLUS_InstallRoot. - string complusInstallRoot = Environment.GetEnvironmentVariable("COMPLUS_INSTALLROOT"); - - // This is easier in .NET 4+: - // var fileIdentity = typeof(CommunicationsUtilities).GetTypeInfo().Assembly.GetCustomAttribute().InformationalVersion; - // but we need to be 3.5 compatible here to work in MSBuildTaskHost - string fileIdentity = null; - foreach (var attribute in typeof(CommunicationsUtilities).GetTypeInfo().Assembly.GetCustomAttributes(false)) - { - if (attribute is AssemblyInformationalVersionAttribute informationalVersionAttribute) - { - fileIdentity = informationalVersionAttribute.InformationalVersion; - break; - } - } - - ErrorUtilities.VerifyThrow(fileIdentity != null, "Did not successfully retrieve InformationalVersion."); - - s_fileVersionHash = GetHandshakeHashCode(complusInstallRoot ?? fileIdentity); - s_fileVersionChecked = true; - } - - return s_fileVersionHash; - } - } - /// /// Get environment block /// @@ -291,68 +288,10 @@ internal static void SetEnvironment(IDictionary newEnvironment) } } - /// - /// Given a base handshake, generates the real handshake based on e.g. elevation level. - /// - private static long GenerateHostHandshakeFromBase(long baseHandshake) - { -#if FEATURE_SECURITY_PRINCIPAL_WINDOWS - // If we are running in elevated privs, we will only accept a handshake from an elevated process as well. - WindowsPrincipal principal = new WindowsPrincipal(WindowsIdentity.GetCurrent()); - - // Both the client and the host will calculate this separately, and the idea is that if they come out the same - // then we can be sufficiently confident that the other side has the same elevation level as us. This is complementary - // to the username check which is also done on connection. - if (principal.IsInRole(WindowsBuiltInRole.Administrator)) - { - unchecked - { - baseHandshake = baseHandshake ^ 0x5c5c5c5c5c5c5c5c + Process.GetCurrentProcess().SessionId; - } - } -#endif - - // Mask out the first byte. Modern builds expect the first byte to be zero to indicate that they are modern - // and should be treated as such. Older builds used a non-zero initial byte. See here: - // https://github.com/microsoft/msbuild/blob/584ca5f11b28971f5651b4b8de5f173ad1cb2786/src/Shared/NodeEndpointOutOfProcBase.cs#L403. - return baseHandshake & 0x00FFFFFFFFFFFFFF; - } - - /// - /// Magic number sent by the host to the client during the handshake. - /// Derived from the binary timestamp to avoid mixing binary versions. - /// - internal static long GetHostHandshake(HandshakeOptions nodeType) - { - string salt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); - string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; - int nodeHandshakeSalt = GetHandshakeHashCode(salt + toolsDirectory); - - Trace("MSBUILDNODEHANDSHAKESALT=\"{0}\", msbuildDirectory=\"{1}\", nodeType={2}, FileVersionHash={3}", salt, toolsDirectory, nodeType, FileVersionHash); - - //FileVersionHash (32 bits) is shifted 8 bits to avoid session ID collision - //nodeType (4 bits) is shifted just after the FileVersionHash - //nodeHandshakeSalt (32 bits) is shifted just after hostContext - //the most significant byte (leftmost 8 bits) will get zero'd out to avoid connecting to older builds. - //| masked out | nodeHandshakeSalt | hostContext | fileVersionHash | SessionID - // 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 - long baseHandshake = ((long)nodeHandshakeSalt << 44) | ((long)nodeType << 40) | ((long)FileVersionHash << 8); - return GenerateHostHandshakeFromBase(baseHandshake); - } - - /// - /// Magic number sent by the client to the host during the handshake. - /// Munged version of the host handshake. - /// - internal static long GetClientHandshake(HandshakeOptions hostContext) - { - return ~GetHostHandshake(hostContext); - } - /// /// Extension method to write a series of bytes to a stream /// - internal static void WriteLongForHandshake(this PipeStream stream, long value) + internal static void WriteIntForHandshake(this PipeStream stream, int value) { byte[] bytes = BitConverter.GetBytes(value); @@ -363,39 +302,22 @@ internal static void WriteLongForHandshake(this PipeStream stream, long value) Array.Reverse(bytes); } - ErrorUtilities.VerifyThrow(bytes.Length == 8, "Long should be 8 bytes"); + ErrorUtilities.VerifyThrow(bytes.Length == 4, "Int should be 4 bytes"); stream.Write(bytes, 0, bytes.Length); } - /// - /// Extension method to read a series of bytes from a stream - /// - internal static long ReadLongForHandshake(this PipeStream stream -#if NETCOREAPP2_1 || MONO - , int handshakeReadTimeout -#endif - ) - { - return stream.ReadLongForHandshake((byte[])null, 0 -#if NETCOREAPP2_1 || MONO - , handshakeReadTimeout -#endif - ); - } - /// /// Extension method to read a series of bytes from a stream. /// If specified, leading byte matches one in the supplied array if any, returns rejection byte and throws IOException. /// - internal static long ReadLongForHandshake(this PipeStream stream, byte[] leadingBytesToReject, - byte rejectionByteToReturn + internal static int ReadIntForHandshake(this PipeStream stream, int byteToAccept #if NETCOREAPP2_1 || MONO , int timeout #endif ) { - byte[] bytes = new byte[8]; + byte[] bytes = new byte[4]; #if NETCOREAPP2_1 || MONO if (!NativeMethodsShared.IsWindows) @@ -435,24 +357,18 @@ byte rejectionByteToReturn throw new IOException(String.Format(CultureInfo.InvariantCulture, "Unexpected end of stream while reading for handshake")); } - if (i == 0 && leadingBytesToReject != null) - { - foreach (byte reject in leadingBytesToReject) - { - if (read == reject) - { - stream.WriteByte(rejectionByteToReturn); // disconnect the host - - throw new IOException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} but this matched a byte to reject.", bytes[i])); // disconnect and quit - } - } - } - bytes[i] = Convert.ToByte(read); } + + if (byteToAccept != 0x00 && byteToAccept != bytes[0]) + { + stream.WriteIntForHandshake(0x0F0F0F0F); + stream.WriteIntForHandshake(0x0F0F0F0F); + throw new IOException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} instead of {1}.", bytes[0], byteToAccept)); + } } - long result; + int result; try { @@ -463,7 +379,7 @@ byte rejectionByteToReturn Array.Reverse(bytes); } - result = BitConverter.ToInt64(bytes, 0 /* start index */); + result = BitConverter.ToInt32(bytes, 0 /* start index */); } catch (ArgumentException ex) { @@ -531,6 +447,16 @@ internal static HandshakeOptions GetHandshakeOptions(bool taskHost, bool is64Bit { context |= HandshakeOptions.LowPriority; } +#if FEATURE_SECURITY_PRINCIPAL_WINDOWS + // If we are running in elevated privs, we will only accept a handshake from an elevated process as well. + // Both the client and the host will calculate this separately, and the idea is that if they come out the same + // then we can be sufficiently confident that the other side has the same elevation level as us. This is complementary + // to the username check which is also done on connection. + if (new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(WindowsBuiltInRole.Administrator)) + { + context |= HandshakeOptions.Administrator; + } +#endif return context; } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 8f24e1c56c8..63e48fc79ab 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -245,12 +245,7 @@ internal void InternalConstruct(string pipeName) /// /// Returns the host handshake for this node endpoint /// - protected abstract long GetHostHandshake(); - - /// - /// Returns the client handshake for this node endpoint - /// - protected abstract long GetClientHandshake(); + protected abstract Handshake GetHandshake(); /// /// Updates the current link status if it has changed and notifies any registered delegates. @@ -346,6 +341,7 @@ private void PacketPumpProc() bool gotValidConnection = false; while (!gotValidConnection) { + gotValidConnection = true; DateTime restartWaitTime = DateTime.UtcNow; // We only wait to wait the difference between now and the last original start time, in case we have multiple hosts attempting @@ -384,43 +380,14 @@ private void PacketPumpProc() // respond with another long. Once the handshake is complete, both sides can be assured the // other is ready to accept data. // To avoid mixing client and server builds, the long is the MSBuild binary timestamp. - - // Compatibility issue here. - // Previous builds of MSBuild 4.0 would exchange just a byte. - // Host would send either 0x5F or 0x60 depending on whether it was the toolset or not respectively. - // Client would return either 0xF5 or 0x06 respectively. - // Therefore an old host on a machine with new clients running will hang, - // sending a byte and waiting for a byte until it eventually times out; - // because the new client will want 7 more bytes before it returns anything. - // The other way around is not a problem, because the old client would immediately return the (wrong) - // byte on receiving the first byte of the long sent by the new host, and the new host would disconnect. - // To avoid the hang, special case here: - // Make sure our handshakes always start with 00. - // If we received ONLY one byte AND it's 0x5F or 0x60, return 0xFF (it doesn't matter what as long as - // it will cause the host to reject us; new hosts expect 00 and old hosts expect F5 or 06). + Handshake handshake = GetHandshake(); try { - long handshake = localReadPipe.ReadLongForHandshake(/* reject these leads */ new byte[] { 0x5F, 0x60 }, 0xFF /* this will disconnect the host; it expects leading 00 or F5 or 06 */ -#if NETCOREAPP2_1 || MONO - , ClientConnectTimeout /* wait a long time for the handshake from this side */ -#endif - ); - -#if FEATURE_SECURITY_PERMISSIONS - WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); -#endif - - if (handshake != GetHostHandshake()) - { - CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshake, GetHostHandshake()); - localPipeServer.Disconnect(); - continue; - } - #if FEATURE_SECURITY_PERMISSIONS // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the // user we were started by. + WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); WindowsIdentity clientIdentity = null; localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); @@ -431,6 +398,48 @@ private void PacketPumpProc() continue; } #endif + int index = 1; + foreach (int part in handshake.RetrieveHandshakeComponents()) + { + + int handshakePart = localReadPipe.ReadIntForHandshake(index == 1 ? 0x01 : 0x00 /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */ +#if NETCOREAPP2_1 || MONO + , ClientConnectTimeout /* wait a long time for the handshake from this side */ +#endif + ); + + if (handshakePart != part) + { + CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshakePart, part); + localWritePipe.WriteIntForHandshake(index); + gotValidConnection = false; + break; + } + index++; + } + + CommunicationsUtilities.Trace("Writing accept code to parent"); + localWritePipe.WriteIntForHandshake(0x39393939); + + // To ensure that our handshake and theirs have the same number of bytes, send and receive a magic number indicating EOS. This has to be different from the + // previous "accept" magic number so that the provider knows this is beyond the end of the handshake for the endpoint as well. + if (gotValidConnection) + { + if (localReadPipe.ReadIntForHandshake(0x01 /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */ +#if NETCOREAPP2_1 || MONO + , ClientConnectTimeout /* wait a long time for the handshake from this side */ +#endif + ) == -0x2a2a2a2a) + { + CommunicationsUtilities.Trace("Successfully connected to parent."); + localWritePipe.WriteIntForHandshake(0x12812812); + } + else + { + CommunicationsUtilities.Trace("Handshake was of a different length. Probably the host is a different MSBuild build."); + gotValidConnection = false; + } + } } catch (IOException e) { @@ -439,18 +448,19 @@ private void PacketPumpProc() // and if they don't match it disconnects immediately leaving us still trying to read the blank handshake // 2. The host is too old sending us bits we automatically reject in the handshake CommunicationsUtilities.Trace("Client connection failed but we will wait for another connection. Exception: {0}", e.Message); + + gotValidConnection = false; + } + + if (!gotValidConnection) + { if (localPipeServer.IsConnected) { localPipeServer.Disconnect(); } - continue; } - gotValidConnection = true; - - CommunicationsUtilities.Trace("Writing handshake to parent"); - localWritePipe.WriteLongForHandshake(GetClientHandshake()); ChangeLinkStatus(LinkStatus.Active); } catch (Exception e) diff --git a/src/Shared/UnitTests/ObjectModelHelpers.cs b/src/Shared/UnitTests/ObjectModelHelpers.cs index 62db95267d0..b7b47a4e20e 100644 --- a/src/Shared/UnitTests/ObjectModelHelpers.cs +++ b/src/Shared/UnitTests/ObjectModelHelpers.cs @@ -243,7 +243,10 @@ public static void AssertItems(string[] expectedItems, IList items, Di expectedItems.ShouldNotBeEmpty(); } - for (var i = 0; i < expectedItems.Length; i++) + // iterate to the minimum length; if the lengths don't match but there's a prefix match the count assertion below will trigger + int minimumLength = Math.Min(expectedItems.Length, items.Count); + + for (var i = 0; i < minimumLength; i++) { if (!normalizeSlashes) { @@ -258,7 +261,8 @@ public static void AssertItems(string[] expectedItems, IList items, Di AssertItemHasMetadata(expectedDirectMetadataPerItem[i], items[i]); } - items.Count.ShouldBe(expectedItems.Length); + items.Count.ShouldBe(expectedItems.Length, + () => $"got items \"{string.Join(", ", items)}\", expected \"{string.Join(", ", expectedItems)}\""); expectedItems.Length.ShouldBe(expectedDirectMetadataPerItem.Length); } @@ -422,7 +426,7 @@ internal static void AssertItemHasMetadata(Dictionary expected, { expected ??= new Dictionary(); - item.DirectMetadataCount.ShouldBe(expected.Keys.Count); + item.DirectMetadataCount.ShouldBe(expected.Keys.Count, () => $"Expected {expected.Keys.Count} metadata, ({string.Join(", ", expected.Keys)}), got {item.DirectMetadataCount}"); foreach (var key in expected.Keys) { diff --git a/src/Utilities/TrackedDependencies/CanonicalTrackedInputFiles.cs b/src/Utilities/TrackedDependencies/CanonicalTrackedInputFiles.cs index a6281533708..be7d6f571f5 100644 --- a/src/Utilities/TrackedDependencies/CanonicalTrackedInputFiles.cs +++ b/src/Utilities/TrackedDependencies/CanonicalTrackedInputFiles.cs @@ -1033,6 +1033,9 @@ private void RemoveDependencyFromEntry(string rootingMarker, ITaskItem dependenc /// Outputs that correspond ot the sources (used for same file processing) public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] correspondingOutputs) { + // Cache of files that have been checked and exist. + Dictionary fileCache = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (correspondingOutputs != null) { ErrorUtilities.VerifyThrowArgument(source.Length == correspondingOutputs.Length, "Tracking_SourcesAndCorrespondingOutputMismatch"); @@ -1041,7 +1044,7 @@ public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] // construct a combined root marker for the sources and outputs to remove from the graph string rootingMarker = FileTracker.FormatRootingMarker(source, correspondingOutputs); - RemoveDependenciesFromEntryIfMissing(rootingMarker); + RemoveDependenciesFromEntryIfMissing(rootingMarker, fileCache); // Remove entries for each individual source for (int sourceIndex = 0; sourceIndex < source.Length; sourceIndex++) @@ -1049,7 +1052,7 @@ public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] rootingMarker = correspondingOutputs != null ? FileTracker.FormatRootingMarker(source[sourceIndex], correspondingOutputs[sourceIndex]) : FileTracker.FormatRootingMarker(source[sourceIndex]); - RemoveDependenciesFromEntryIfMissing(rootingMarker); + RemoveDependenciesFromEntryIfMissing(rootingMarker, fileCache); } } @@ -1057,7 +1060,8 @@ public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] /// Remove the output graph entries for the given rooting marker /// /// - private void RemoveDependenciesFromEntryIfMissing(string rootingMarker) + /// The cache used to store whether each file exists or not. + private void RemoveDependenciesFromEntryIfMissing(string rootingMarker, Dictionary fileCache) { // In the event of incomplete tracking information (i.e. this root was not present), just continue quietly // as the user could have killed the tool being tracked, or another error occurred during its execution. @@ -1070,8 +1074,19 @@ private void RemoveDependenciesFromEntryIfMissing(string rootingMarker) { if (keyIndex++ > 0) { - // If we are ignoring missing files, then only record those that exist - if (FileUtilities.FileExistsNoThrow(file)) + // Record whether or not each file exists and cache it. + // We do this to save time (On^2), at the expense of data O(n). + bool inFileCache = fileCache.TryGetValue(file, out bool fileExists); + + // Have we cached the file yet? If not, cache whether or not it exists. + if (!inFileCache) + { + fileExists = FileUtilities.FileExistsNoThrow(file); + fileCache.Add(file, fileExists); + } + + // Does the cached file exist? + if (fileExists) { dependenciesWithoutMissingFiles.Add(file, dependencies[file]); } diff --git a/src/Utilities/TrackedDependencies/CanonicalTrackedOutputFiles.cs b/src/Utilities/TrackedDependencies/CanonicalTrackedOutputFiles.cs index 5c906de1683..7ca96f897c0 100644 --- a/src/Utilities/TrackedDependencies/CanonicalTrackedOutputFiles.cs +++ b/src/Utilities/TrackedDependencies/CanonicalTrackedOutputFiles.cs @@ -724,6 +724,9 @@ private void RemoveDependencyFromEntry(string rootingMarker, ITaskItem dependenc /// Outputs that correspond ot the sources (used for same file processing) public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] correspondingOutputs) { + // Cache of files and whether or not they exist. + Dictionary fileCache = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (correspondingOutputs != null) { ErrorUtilities.VerifyThrowArgument(source.Length == correspondingOutputs.Length, "Tracking_SourcesAndCorrespondingOutputMismatch"); @@ -732,21 +735,15 @@ public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] // construct a combined root marker for the sources and outputs to remove from the graph string rootingMarker = FileTracker.FormatRootingMarker(source, correspondingOutputs); - RemoveDependenciesFromEntryIfMissing(rootingMarker); + RemoveDependenciesFromEntryIfMissing(rootingMarker, fileCache); // Remove entries for each individual source for (int sourceIndex = 0; sourceIndex < source.Length; sourceIndex++) { - if (correspondingOutputs != null) - { - rootingMarker = FileTracker.FormatRootingMarker(source[sourceIndex], correspondingOutputs[sourceIndex]); - } - else - { - rootingMarker = FileTracker.FormatRootingMarker(source[sourceIndex]); - } - - RemoveDependenciesFromEntryIfMissing(rootingMarker); + rootingMarker = correspondingOutputs != null + ? FileTracker.FormatRootingMarker(source[sourceIndex], correspondingOutputs[sourceIndex]) + : FileTracker.FormatRootingMarker(source[sourceIndex]); + RemoveDependenciesFromEntryIfMissing(rootingMarker, fileCache); } } @@ -754,7 +751,8 @@ public void RemoveDependenciesFromEntryIfMissing(ITaskItem[] source, ITaskItem[] /// Remove the output graph entries for the given rooting marker /// /// - private void RemoveDependenciesFromEntryIfMissing(string rootingMarker) + /// The cache used to store whether each file exists or not. + private void RemoveDependenciesFromEntryIfMissing(string rootingMarker, Dictionary fileCache) { // In the event of incomplete tracking information (i.e. this root was not present), just continue quietly // as the user could have killed the tool being tracked, or another error occurred during its execution. @@ -767,8 +765,19 @@ private void RemoveDependenciesFromEntryIfMissing(string rootingMarker) { if (keyIndex++ > 0) { - // If we are ignoring missing files, then only record those that exist - if (FileUtilities.FileExistsNoThrow(file)) + // Record whether or not each file exists and cache it. + // We do this to save time (On^2), at the expense of data O(n). + bool inFileCache = fileCache.TryGetValue(file, out bool fileExists); + + // Have we cached the file yet? If not, cache its existence. + if (!inFileCache) + { + fileExists = FileUtilities.FileExistsNoThrow(file); + fileCache.Add(file, fileExists); + } + + // Does the cached file exist? + if (fileExists) { dependenciesWithoutMissingFiles.Add(file, dependencies[file]); } From 7a32206b376a6d36045a84360f27456a99112c20 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 29 Jun 2020 14:15:15 -0700 Subject: [PATCH 02/10] Moved user check --- src/Shared/NodeEndpointOutOfProcBase.cs | 40 ++++++++++++------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 63e48fc79ab..6bc14e4cf76 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -354,14 +354,11 @@ private void PacketPumpProc() // Wait for a connection #if FEATURE_APM IAsyncResult resultForConnection = localPipeServer.BeginWaitForConnection(null, null); -#else - Task connectionTask = localPipeServer.WaitForConnectionAsync(); -#endif CommunicationsUtilities.Trace("Waiting for connection {0} ms...", waitTimeRemaining); - -#if FEATURE_APM bool connected = resultForConnection.AsyncWaitHandle.WaitOne(waitTimeRemaining, false); #else + Task connectionTask = localPipeServer.WaitForConnectionAsync(); + CommunicationsUtilities.Trace("Waiting for connection {0} ms...", waitTimeRemaining); bool connected = connectionTask.Wait(waitTimeRemaining); #endif if (!connected) @@ -383,21 +380,6 @@ private void PacketPumpProc() Handshake handshake = GetHandshake(); try { -#if FEATURE_SECURITY_PERMISSIONS - // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they - // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the - // user we were started by. - WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); - WindowsIdentity clientIdentity = null; - localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); - - if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) - { - CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); - localPipeServer.Disconnect(); - continue; - } -#endif int index = 1; foreach (int part in handshake.RetrieveHandshakeComponents()) { @@ -425,7 +407,7 @@ private void PacketPumpProc() // previous "accept" magic number so that the provider knows this is beyond the end of the handshake for the endpoint as well. if (gotValidConnection) { - if (localReadPipe.ReadIntForHandshake(0x01 /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */ + if (localReadPipe.ReadIntForHandshake(0x00 /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */ #if NETCOREAPP2_1 || MONO , ClientConnectTimeout /* wait a long time for the handshake from this side */ #endif @@ -433,6 +415,22 @@ private void PacketPumpProc() { CommunicationsUtilities.Trace("Successfully connected to parent."); localWritePipe.WriteIntForHandshake(0x12812812); + +#if FEATURE_SECURITY_PERMISSIONS + // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they + // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the + // user we were started by. + WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); + WindowsIdentity clientIdentity = null; + localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); + + if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) + { + CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); + localPipeServer.Disconnect(); + continue; + } +#endif } else { From 166164de1c3a431d0f9114f6f105f49d825a0c6d Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 29 Jun 2020 17:42:17 -0700 Subject: [PATCH 03/10] Moved Handshake --- src/Shared/CommunicationsUtilities.cs | 88 +++++++++++++-------------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index 51431783249..bee81246c7a 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -20,50 +20,6 @@ namespace Microsoft.Build.Internal { - internal readonly struct Handshake - { - readonly int options; - readonly int salt; - readonly int fileVersionMajor; - readonly int fileVersionMinor; - readonly int fileVersionBuild; - readonly int fileVersionPrivate; - readonly int sessionId; - - internal Handshake(HandshakeOptions nodeType) - { - // We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00. - // This indicates in the first byte that we are a modern build. - options = (int)nodeType | 0x01000000; - string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); - string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; - salt = CommunicationsUtilities.GetHandshakeHashCode(handshakeSalt + toolsDirectory); - Version fileVersion = new Version(FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location).FileVersion); - fileVersionMajor = fileVersion.Major; - fileVersionMinor = fileVersion.Minor; - fileVersionBuild = fileVersion.Build; - fileVersionPrivate = fileVersion.Revision; - sessionId = Process.GetCurrentProcess().SessionId; - } - - // This is used as a key, so it does not need to be human readable. - public override string ToString() - { - return string.Empty + options + salt + fileVersionMajor + fileVersionMinor + fileVersionBuild + fileVersionPrivate + sessionId; - } - - internal IEnumerable RetrieveHandshakeComponents() - { - yield return options; - yield return salt; - yield return fileVersionMajor; - yield return fileVersionMinor; - yield return fileVersionBuild; - yield return fileVersionPrivate; - yield return sessionId; - } - } - /// /// Enumeration of all possible (currently supported) options for handshakes. /// @@ -571,4 +527,48 @@ internal static int GetHandshakeHashCode(string fileVersion) } } } + + internal readonly struct Handshake + { + readonly int options; + readonly int salt; + readonly int fileVersionMajor; + readonly int fileVersionMinor; + readonly int fileVersionBuild; + readonly int fileVersionPrivate; + readonly int sessionId; + + internal Handshake(HandshakeOptions nodeType) + { + // We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00. + // This indicates in the first byte that we are a modern build. + options = (int)nodeType | 0x01000000; + string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); + string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; + salt = CommunicationsUtilities.GetHandshakeHashCode(handshakeSalt + toolsDirectory); + Version fileVersion = new Version(FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location).FileVersion); + fileVersionMajor = fileVersion.Major; + fileVersionMinor = fileVersion.Minor; + fileVersionBuild = fileVersion.Build; + fileVersionPrivate = fileVersion.Revision; + sessionId = Process.GetCurrentProcess().SessionId; + } + + // This is used as a key, so it does not need to be human readable. + public override string ToString() + { + return string.Empty + options + salt + fileVersionMajor + fileVersionMinor + fileVersionBuild + fileVersionPrivate + sessionId; + } + + internal IEnumerable RetrieveHandshakeComponents() + { + yield return options; + yield return salt; + yield return fileVersionMajor; + yield return fileVersionMinor; + yield return fileVersionBuild; + yield return fileVersionPrivate; + yield return sessionId; + } + } } From 6ddfc570bc1921d1ac8ed902d691a7e188c9cf72 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 30 Jun 2020 09:17:48 -0700 Subject: [PATCH 04/10] Fixed build --- src/Build.UnitTests/Utilities_Tests.cs | 20 ------------------- .../Communications/NodeEndpointOutOfProc.cs | 1 + 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/src/Build.UnitTests/Utilities_Tests.cs b/src/Build.UnitTests/Utilities_Tests.cs index f6c62d77ccd..2219cc6bb72 100644 --- a/src/Build.UnitTests/Utilities_Tests.cs +++ b/src/Build.UnitTests/Utilities_Tests.cs @@ -64,26 +64,6 @@ public void GetTextFromTextNodeWithXmlComment7() // Should get XML; note space after x added Assert.Equal("", xmlContents); } - - [Fact] - public void HandshakesDiffer() - { - int numHandshakeOptions = (int)Math.Pow(2, Enum.GetNames(typeof(HandshakeOptions)).Length - 1); - Dictionary handshakes = new Dictionary(); - for (int i = 0; i < numHandshakeOptions; i++) - { - long nextKey = CommunicationsUtilities.GetHostHandshake((HandshakeOptions)i); - if (handshakes.TryGetValue(nextKey, out int collision)) - { - _output.WriteLine("There was a collision between {0} and {1}.", collision, i); - } - else - { - handshakes.Add(nextKey, i); - } - } - handshakes.Count.ShouldBe(numHandshakeOptions, "two or more combinations of handshake options hashed to the same value"); - } } public class UtilitiesTestReadOnlyLoad : UtilitiesTest diff --git a/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs b/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs index 5d4edf5a68e..caeb65b2bce 100644 --- a/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs +++ b/src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.Internal; using Microsoft.Build.Shared; namespace Microsoft.Build.BackEnd From 2348e3c2a1a2989a7ee2a24852dc676c7bc78669 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 30 Jun 2020 15:28:46 -0700 Subject: [PATCH 05/10] Refactoring --- .../NodeProviderOutOfProcBase.cs | 27 ++------ src/Shared/CommunicationsUtilities.cs | 61 ++++++++++++++++--- src/Shared/NodeEndpointOutOfProcBase.cs | 57 +++++++---------- 3 files changed, 82 insertions(+), 63 deletions(-) diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index c09cf2e2433..786a9a5a6d7 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -359,31 +359,16 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han index++; } - CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName); -#if NETCOREAPP2_1 || MONO - int response = nodeStream.ReadIntForHandshake(0x39, timeout); -#else - int response = nodeStream.ReadIntForHandshake(0x39); -#endif + // This indicates that we have finished all the parts of our handshake; hopefully the endpoint has as well. + nodeStream.WriteEndOfHandshakeSignal(); - if (response != 0x39393939) - { - CommunicationsUtilities.Trace("Handshake failed on part {0}. Probably the client is a different MSBuild build.", response, index); - throw new InvalidOperationException(); - } + CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName); - // This indicates that we have finished all the parts of our handshake; hopefully the endpoint has as well. - nodeStream.WriteIntForHandshake(-0x2a2a2a2a); - if (nodeStream.ReadIntForHandshake(0x12 #if NETCOREAPP2_1 || MONO - , timeout + nodeStream.ReadEndOfHandshakeSignal(true, timeout); +#else + nodeStream.ReadEndOfHandshakeSignal(true); #endif - ) != 0x12812812) - { - CommunicationsUtilities.Trace("Handshake failed. The client handshake has a different number of parts. Probably the client is a different MSBuild build."); - throw new InvalidOperationException(); - } - // We got a connection. CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName); return nodeStream; diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index bee81246c7a..4496a118657 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -64,6 +64,11 @@ internal enum HandshakeOptions /// static internal class CommunicationsUtilities { + /// + /// Indicates to the NodeEndpoint that all the various parts of the Handshake have been sent. + /// + private const int EndOfHandshakeSignal = -0x2a2a2a2a; + /// /// The timeout to connect to a node. /// @@ -244,6 +249,14 @@ internal static void SetEnvironment(IDictionary newEnvironment) } } + /// + /// Indicate to the client that all elements of the Handshake have been sent. + /// + internal static void WriteEndOfHandshakeSignal(this PipeStream stream) + { + stream.WriteIntForHandshake(EndOfHandshakeSignal); + } + /// /// Extension method to write a series of bytes to a stream /// @@ -263,6 +276,33 @@ internal static void WriteIntForHandshake(this PipeStream stream, int value) stream.Write(bytes, 0, bytes.Length); } + internal static void ReadEndOfHandshakeSignal(this PipeStream stream, bool isProvider +#if NETCOREAPP2_1 || MONO + , int timeout +#endif + ) + { + // Accept only the first byte of the EndOfHandshakeSignal + int valueRead = stream.ReadIntForHandshake(EndOfHandshakeSignal & 0xFF +#if NETCOREAPP2_1 || MONO + , int timeout +#endif + ); + + if (valueRead != EndOfHandshakeSignal) + { + if (isProvider) + { + CommunicationsUtilities.Trace("Handshake failed on part {0}. Probably the client is a different MSBuild build.", valueRead); + } + else + { + CommunicationsUtilities.Trace("Expected end of handshake signal but received {0}. Probably the host is a different MSBuild build.", valueRead); + } + throw new InvalidOperationException(); + } + } + /// /// Extension method to read a series of bytes from a stream. /// If specified, leading byte matches one in the supplied array if any, returns rejection byte and throws IOException. @@ -320,7 +360,7 @@ internal static int ReadIntForHandshake(this PipeStream stream, int byteToAccept { stream.WriteIntForHandshake(0x0F0F0F0F); stream.WriteIntForHandshake(0x0F0F0F0F); - throw new IOException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} instead of {1}.", bytes[0], byteToAccept)); + throw new InvalidOperationException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} instead of {1}.", bytes[0], byteToAccept)); } } @@ -526,6 +566,11 @@ internal static int GetHandshakeHashCode(string fileVersion) } } } + + internal static int AvoidEndOfHandshakeSignal(int x) + { + return x == EndOfHandshakeSignal ? ~x : x; + } } internal readonly struct Handshake @@ -562,13 +607,13 @@ public override string ToString() internal IEnumerable RetrieveHandshakeComponents() { - yield return options; - yield return salt; - yield return fileVersionMajor; - yield return fileVersionMinor; - yield return fileVersionBuild; - yield return fileVersionPrivate; - yield return sessionId; + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(options); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(salt); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMajor); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMinor); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionBuild); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionPrivate); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(sessionId); } } } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 84e1cfb86d4..c441b1bb93b 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -367,10 +367,9 @@ private void PacketPumpProc() localPipeServer.EndWaitForConnection(resultForConnection); #endif - // The handshake protocol is a simple long exchange. The host sends us a long, and we - // respond with another long. Once the handshake is complete, both sides can be assured the - // other is ready to accept data. - // To avoid mixing client and server builds, the long is the MSBuild binary timestamp. + // The handshake protocol is a series of int exchanges. The host sends us a each component, and we + // verify it. Afterwards, the host sends an "End of Handshake" signal, to which we respond in kind. + // Once the handshake is complete, both sides can be assured the other is ready to accept data. Handshake handshake = GetHandshake(); try { @@ -378,7 +377,7 @@ private void PacketPumpProc() foreach (int part in handshake.RetrieveHandshakeComponents()) { - int handshakePart = localReadPipe.ReadIntForHandshake(index == 1 ? 0x01 : 0x00 /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */ + int handshakePart = localReadPipe.ReadIntForHandshake(index == 1 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ #if NETCOREAPP2_1 || MONO , ClientConnectTimeout /* wait a long time for the handshake from this side */ #endif @@ -394,43 +393,32 @@ private void PacketPumpProc() index++; } - CommunicationsUtilities.Trace("Writing accept code to parent"); - localWritePipe.WriteIntForHandshake(0x39393939); - - // To ensure that our handshake and theirs have the same number of bytes, send and receive a magic number indicating EOS. This has to be different from the - // previous "accept" magic number so that the provider knows this is beyond the end of the handshake for the endpoint as well. if (gotValidConnection) { - if (localReadPipe.ReadIntForHandshake(0x00 /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */ + // To ensure that our handshake and theirs have the same number of bytes, receive and send a magic number indicating EOS. #if NETCOREAPP2_1 || MONO - , ClientConnectTimeout /* wait a long time for the handshake from this side */ + localReadPipe.ReadEndOfHandshakeSignal(ClientConnectTimeout); /* wait a long time for the handshake from this side */ +#else + localReadPipe.ReadEndOfHandshakeSignal(false); #endif - ) == -0x2a2a2a2a) - { - CommunicationsUtilities.Trace("Successfully connected to parent."); - localWritePipe.WriteIntForHandshake(0x12812812); + CommunicationsUtilities.Trace("Successfully connected to parent."); + localWritePipe.WriteEndOfHandshakeSignal(); #if FEATURE_SECURITY_PERMISSIONS - // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they - // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the - // user we were started by. - WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); - WindowsIdentity clientIdentity = null; - localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); - - if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) - { - CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); - localPipeServer.Disconnect(); - continue; - } -#endif - } - else + // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they + // haven't attempted to change those permissions out from under us. This ensures that the only way they can truly gain access is to be impersonating the + // user we were started by. + WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent(); + WindowsIdentity clientIdentity = null; + localPipeServer.RunAsClient(delegate () { clientIdentity = WindowsIdentity.GetCurrent(true); }); + + if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) { - CommunicationsUtilities.Trace("Handshake was of a different length. Probably the host is a different MSBuild build."); - gotValidConnection = false; + CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); + localPipeServer.Disconnect(); + continue; } +#endif } } catch (IOException e) @@ -439,6 +427,7 @@ private void PacketPumpProc() // 1. The host (OOP main node) connects to us, it immediately checks for user privileges // and if they don't match it disconnects immediately leaving us still trying to read the blank handshake // 2. The host is too old sending us bits we automatically reject in the handshake + // 3. We expected to read the EndOfHandshake signal, but we received something else CommunicationsUtilities.Trace("Client connection failed but we will wait for another connection. Exception: {0}", e.Message); gotValidConnection = false; From 16880056044072aa43c903e3825190d6255cd019 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 30 Jun 2020 16:08:41 -0700 Subject: [PATCH 06/10] Move fire byte calculation into loop --- src/Shared/CommunicationsUtilities.cs | 12 ++++++------ src/Shared/NodeEndpointOutOfProcBase.cs | 16 +++++++--------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index 4496a118657..86526a4d231 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -354,13 +354,13 @@ internal static int ReadIntForHandshake(this PipeStream stream, int byteToAccept } bytes[i] = Convert.ToByte(read); - } - if (byteToAccept != 0x00 && byteToAccept != bytes[0]) - { - stream.WriteIntForHandshake(0x0F0F0F0F); - stream.WriteIntForHandshake(0x0F0F0F0F); - throw new InvalidOperationException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} instead of {1}.", bytes[0], byteToAccept)); + if (i == 0 && byteToAccept != 0x00 && byteToAccept != bytes[0]) + { + stream.WriteIntForHandshake(0x0F0F0F0F); + stream.WriteIntForHandshake(0x0F0F0F0F); + throw new InvalidOperationException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} instead of {1}.", bytes[0], byteToAccept)); + } } } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index c441b1bb93b..b2eb25a5ea1 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -324,8 +324,6 @@ private void InitializeAsyncPacketThread() private void PacketPumpProc() { NamedPipeServerStream localPipeServer = _pipeServer; - PipeStream localWritePipe = _pipeServer; - PipeStream localReadPipe = _pipeServer; AutoResetEvent localPacketAvailable = _packetAvailable; AutoResetEvent localTerminatePacketPump = _terminatePacketPump; @@ -377,7 +375,7 @@ private void PacketPumpProc() foreach (int part in handshake.RetrieveHandshakeComponents()) { - int handshakePart = localReadPipe.ReadIntForHandshake(index == 1 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ + int handshakePart = _pipeServer.ReadIntForHandshake(index == 1 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ #if NETCOREAPP2_1 || MONO , ClientConnectTimeout /* wait a long time for the handshake from this side */ #endif @@ -386,7 +384,7 @@ private void PacketPumpProc() if (handshakePart != part) { CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshakePart, part); - localWritePipe.WriteIntForHandshake(index); + _pipeServer.WriteIntForHandshake(index); gotValidConnection = false; break; } @@ -397,12 +395,12 @@ private void PacketPumpProc() { // To ensure that our handshake and theirs have the same number of bytes, receive and send a magic number indicating EOS. #if NETCOREAPP2_1 || MONO - localReadPipe.ReadEndOfHandshakeSignal(ClientConnectTimeout); /* wait a long time for the handshake from this side */ + _pipeServer.ReadEndOfHandshakeSignal(ClientConnectTimeout); /* wait a long time for the handshake from this side */ #else - localReadPipe.ReadEndOfHandshakeSignal(false); + _pipeServer.ReadEndOfHandshakeSignal(false); #endif CommunicationsUtilities.Trace("Successfully connected to parent."); - localWritePipe.WriteEndOfHandshakeSignal(); + _pipeServer.WriteEndOfHandshakeSignal(); #if FEATURE_SECURITY_PERMISSIONS // We will only talk to a host that was started by the same user as us. Even though the pipe access is set to only allow this user, we want to ensure they @@ -464,8 +462,8 @@ private void PacketPumpProc() } RunReadLoop( - new BufferedReadStream(localReadPipe), - localWritePipe, + new BufferedReadStream(_pipeServer), + _pipeServer, localPacketQueue, localPacketAvailable, localTerminatePacketPump); CommunicationsUtilities.Trace("Ending read loop"); From 61051d8966b052430bc369975f3d734fe4069a07 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 30 Jun 2020 16:13:52 -0700 Subject: [PATCH 07/10] Save before committing :smiley: --- src/Shared/CommunicationsUtilities.cs | 90 ++++++++++++------------- src/Shared/NodeEndpointOutOfProcBase.cs | 2 +- 2 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index 86526a4d231..f9f3c126ea0 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -59,6 +59,50 @@ internal enum HandshakeOptions Administrator = 32 } + internal readonly struct Handshake + { + readonly int options; + readonly int salt; + readonly int fileVersionMajor; + readonly int fileVersionMinor; + readonly int fileVersionBuild; + readonly int fileVersionPrivate; + readonly int sessionId; + + internal Handshake(HandshakeOptions nodeType) + { + // We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00. + // This indicates in the first byte that we are a modern build. + options = (int)nodeType | 0x01000000; + string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); + string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; + salt = CommunicationsUtilities.GetHandshakeHashCode(handshakeSalt + toolsDirectory); + Version fileVersion = new Version(FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location).FileVersion); + fileVersionMajor = fileVersion.Major; + fileVersionMinor = fileVersion.Minor; + fileVersionBuild = fileVersion.Build; + fileVersionPrivate = fileVersion.Revision; + sessionId = Process.GetCurrentProcess().SessionId; + } + + // This is used as a key, so it does not need to be human readable. + public override string ToString() + { + return string.Empty + options + salt + fileVersionMajor + fileVersionMinor + fileVersionBuild + fileVersionPrivate + sessionId; + } + + internal IEnumerable RetrieveHandshakeComponents() + { + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(options); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(salt); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMajor); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMinor); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionBuild); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionPrivate); + yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(sessionId); + } + } + /// /// This class contains utility methods for the MSBuild engine. /// @@ -285,7 +329,7 @@ internal static void ReadEndOfHandshakeSignal(this PipeStream stream, bool isPro // Accept only the first byte of the EndOfHandshakeSignal int valueRead = stream.ReadIntForHandshake(EndOfHandshakeSignal & 0xFF #if NETCOREAPP2_1 || MONO - , int timeout + , timeout #endif ); @@ -572,48 +616,4 @@ internal static int AvoidEndOfHandshakeSignal(int x) return x == EndOfHandshakeSignal ? ~x : x; } } - - internal readonly struct Handshake - { - readonly int options; - readonly int salt; - readonly int fileVersionMajor; - readonly int fileVersionMinor; - readonly int fileVersionBuild; - readonly int fileVersionPrivate; - readonly int sessionId; - - internal Handshake(HandshakeOptions nodeType) - { - // We currently use 6 bits of this 32-bit integer. Very old builds will instantly reject any handshake that does not start with F5 or 06; slightly old builds always lead with 00. - // This indicates in the first byte that we are a modern build. - options = (int)nodeType | 0x01000000; - string handshakeSalt = Environment.GetEnvironmentVariable("MSBUILDNODEHANDSHAKESALT"); - string toolsDirectory = (nodeType & HandshakeOptions.X64) == HandshakeOptions.X64 ? BuildEnvironmentHelper.Instance.MSBuildToolsDirectory64 : BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32; - salt = CommunicationsUtilities.GetHandshakeHashCode(handshakeSalt + toolsDirectory); - Version fileVersion = new Version(FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location).FileVersion); - fileVersionMajor = fileVersion.Major; - fileVersionMinor = fileVersion.Minor; - fileVersionBuild = fileVersion.Build; - fileVersionPrivate = fileVersion.Revision; - sessionId = Process.GetCurrentProcess().SessionId; - } - - // This is used as a key, so it does not need to be human readable. - public override string ToString() - { - return string.Empty + options + salt + fileVersionMajor + fileVersionMinor + fileVersionBuild + fileVersionPrivate + sessionId; - } - - internal IEnumerable RetrieveHandshakeComponents() - { - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(options); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(salt); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMajor); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMinor); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionBuild); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionPrivate); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(sessionId); - } - } } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index b2eb25a5ea1..0dd8855a80e 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -395,7 +395,7 @@ private void PacketPumpProc() { // To ensure that our handshake and theirs have the same number of bytes, receive and send a magic number indicating EOS. #if NETCOREAPP2_1 || MONO - _pipeServer.ReadEndOfHandshakeSignal(ClientConnectTimeout); /* wait a long time for the handshake from this side */ + _pipeServer.ReadEndOfHandshakeSignal(false, ClientConnectTimeout); /* wait a long time for the handshake from this side */ #else _pipeServer.ReadEndOfHandshakeSignal(false); #endif From 6b4fed0075bbe55ecb139efeb0cbd69fbb01235a Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 30 Jun 2020 16:27:35 -0700 Subject: [PATCH 08/10] Catch uncaught exception --- src/Shared/CommunicationsUtilities.cs | 2 +- src/Shared/NodeEndpointOutOfProcBase.cs | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index f9f3c126ea0..3b1159d0353 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -327,7 +327,7 @@ internal static void ReadEndOfHandshakeSignal(this PipeStream stream, bool isPro ) { // Accept only the first byte of the EndOfHandshakeSignal - int valueRead = stream.ReadIntForHandshake(EndOfHandshakeSignal & 0xFF + int valueRead = stream.ReadIntForHandshake(0x00 #if NETCOREAPP2_1 || MONO , timeout #endif diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 0dd8855a80e..ac2398182bf 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -430,6 +430,10 @@ private void PacketPumpProc() gotValidConnection = false; } + catch (InvalidOperationException) + { + gotValidConnection = false; + } if (!gotValidConnection) { From fd0ca2a330353981444fac45aca09c17b68e7a73 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Thu, 2 Jul 2020 10:14:12 -0700 Subject: [PATCH 09/10] PR feedback --- .../NodeProviderOutOfProcBase.cs | 9 ++++---- src/Shared/CommunicationsUtilities.cs | 21 +++++++++++-------- src/Shared/NodeEndpointOutOfProcBase.cs | 15 +++++++------ 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs index 786a9a5a6d7..ace54e8b94c 100644 --- a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs +++ b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs @@ -351,12 +351,11 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han } #endif - int index = 1; - foreach (int part in handshake.RetrieveHandshakeComponents()) + int[] handshakeComponents = handshake.RetrieveHandshakeComponents(); + for (int i = 0; i < handshakeComponents.Length; i++) { - CommunicationsUtilities.Trace("Writing handshake part {0} to pipe {1}", index, pipeName); - nodeStream.WriteIntForHandshake(part); - index++; + CommunicationsUtilities.Trace("Writing handshake part {0} to pipe {1}", i, pipeName); + nodeStream.WriteIntForHandshake(handshakeComponents[i]); } // This indicates that we have finished all the parts of our handshake; hopefully the endpoint has as well. diff --git a/src/Shared/CommunicationsUtilities.cs b/src/Shared/CommunicationsUtilities.cs index 3b1159d0353..30eb1a79b79 100644 --- a/src/Shared/CommunicationsUtilities.cs +++ b/src/Shared/CommunicationsUtilities.cs @@ -88,18 +88,21 @@ internal Handshake(HandshakeOptions nodeType) // This is used as a key, so it does not need to be human readable. public override string ToString() { - return string.Empty + options + salt + fileVersionMajor + fileVersionMinor + fileVersionBuild + fileVersionPrivate + sessionId; + return String.Format("{0} {1} {2} {3} {4} {5} {6}", options, salt, fileVersionMajor, fileVersionMinor, fileVersionBuild, fileVersionPrivate, sessionId); } - internal IEnumerable RetrieveHandshakeComponents() + internal int[] RetrieveHandshakeComponents() { - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(options); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(salt); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMajor); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMinor); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionBuild); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionPrivate); - yield return CommunicationsUtilities.AvoidEndOfHandshakeSignal(sessionId); + return new int[] + { + CommunicationsUtilities.AvoidEndOfHandshakeSignal(options), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(salt), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMajor), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionMinor), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionBuild), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(fileVersionPrivate), + CommunicationsUtilities.AvoidEndOfHandshakeSignal(sessionId) + }; } } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index ac2398182bf..d735e88fb86 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -371,24 +371,23 @@ private void PacketPumpProc() Handshake handshake = GetHandshake(); try { - int index = 1; - foreach (int part in handshake.RetrieveHandshakeComponents()) + int[] handshakeComponents = handshake.RetrieveHandshakeComponents(); + for (int i = 0; i < handshakeComponents.Length; i++) { - int handshakePart = _pipeServer.ReadIntForHandshake(index == 1 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ + int handshakePart = _pipeServer.ReadIntForHandshake(i == 1 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ #if NETCOREAPP2_1 || MONO , ClientConnectTimeout /* wait a long time for the handshake from this side */ #endif ); - if (handshakePart != part) + if (handshakePart != handshakeComponents[i]) { - CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshakePart, part); - _pipeServer.WriteIntForHandshake(index); + CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshakePart, handshakeComponents[i]); + _pipeServer.WriteIntForHandshake(i); gotValidConnection = false; break; } - index++; } if (gotValidConnection) @@ -413,7 +412,7 @@ private void PacketPumpProc() if (clientIdentity == null || !String.Equals(clientIdentity.Name, currentIdentity.Name, StringComparison.OrdinalIgnoreCase)) { CommunicationsUtilities.Trace("Handshake failed. Host user is {0} but we were created by {1}.", (clientIdentity == null) ? "" : clientIdentity.Name, currentIdentity.Name); - localPipeServer.Disconnect(); + gotValidConnection = false; continue; } #endif From 467c2bb50e91676d8839929c633bf047f98c06f6 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 7 Jul 2020 21:37:14 -0700 Subject: [PATCH 10/10] Correct off-by-one error --- src/Shared/NodeEndpointOutOfProcBase.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index d735e88fb86..e416ea03574 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -374,8 +374,7 @@ private void PacketPumpProc() int[] handshakeComponents = handshake.RetrieveHandshakeComponents(); for (int i = 0; i < handshakeComponents.Length; i++) { - - int handshakePart = _pipeServer.ReadIntForHandshake(i == 1 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ + int handshakePart = _pipeServer.ReadIntForHandshake(i == 0 ? 0x01 : 0x00 /* this will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard */ #if NETCOREAPP2_1 || MONO , ClientConnectTimeout /* wait a long time for the handshake from this side */ #endif @@ -384,7 +383,7 @@ private void PacketPumpProc() if (handshakePart != handshakeComponents[i]) { CommunicationsUtilities.Trace("Handshake failed. Received {0} from host not {1}. Probably the host is a different MSBuild build.", handshakePart, handshakeComponents[i]); - _pipeServer.WriteIntForHandshake(i); + _pipeServer.WriteIntForHandshake(i + 1); gotValidConnection = false; break; }