Skip to content

Commit

Permalink
PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Ondřej Štorc committed Aug 10, 2020
1 parent 331f113 commit 20e4046
Show file tree
Hide file tree
Showing 19 changed files with 150 additions and 355 deletions.
1 change: 0 additions & 1 deletion ref/Microsoft.Build/net/Microsoft.Build.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,6 @@ public enum NodeAffinity
Any = 2,
InProc = 0,
OutOfProc = 1,
RarNode = 3,
}
public enum NodeEngineShutdownReason
{
Expand Down
1 change: 0 additions & 1 deletion ref/Microsoft.Build/netstandard/Microsoft.Build.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1090,7 +1090,6 @@ public enum NodeAffinity
Any = 2,
InProc = 0,
OutOfProc = 1,
RarNode = 3,
}
public enum NodeEngineShutdownReason
{
Expand Down
7 changes: 7 additions & 0 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2021,6 +2021,13 @@ private void PerformSchedulingActions(IEnumerable<ScheduleResponse> responses)
}
}

internal NodeInfo CreateRarNode()
{
NodeConfiguration configuration = GetNodeConfiguration();
configuration.RarNode = true;
return _nodeManager.CreateNode(configuration, NodeAffinity.OutOfProc);
}

/// <summary>
/// Completes a submission using the specified overall results.
/// </summary>
Expand Down
1 change: 0 additions & 1 deletion src/Build/BackEnd/Components/BuildComponentFactoryCollection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ public void RegisterDefaultFactories()

_componentEntriesByType[BuildComponentType.InProcNodeProvider] = new BuildComponentEntry(BuildComponentType.InProcNodeProvider, NodeProviderInProc.CreateComponent, CreationPattern.Singleton);
_componentEntriesByType[BuildComponentType.OutOfProcNodeProvider] = new BuildComponentEntry(BuildComponentType.OutOfProcNodeProvider, NodeProviderOutOfProc.CreateComponent, CreationPattern.Singleton);
_componentEntriesByType[BuildComponentType.OutOfProcNodeRarProvider] = new BuildComponentEntry(BuildComponentType.OutOfProcNodeRarProvider, NodeProviderOutOfProcRar.CreateComponent, CreationPattern.Singleton);
_componentEntriesByType[BuildComponentType.OutOfProcTaskHostNodeProvider] = new BuildComponentEntry(BuildComponentType.OutOfProcTaskHostNodeProvider, NodeProviderOutOfProcTaskHost.CreateComponent, CreationPattern.Singleton);

// PropertyCache,
Expand Down
7 changes: 1 addition & 6 deletions src/Build/BackEnd/Components/Communications/INodeProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,7 @@ internal enum NodeProviderType
/// <summary>
/// The provider provides remote nodes.
/// </summary>
Remote,

/// <summary>
/// The provider provides RAR nodes.
/// </summary>
ResolveAssemblyReference
Remote
}

/// <summary>
Expand Down
22 changes: 0 additions & 22 deletions src/Build/BackEnd/Components/Communications/NodeManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,6 @@ internal class NodeManager : INodeManager
/// </summary>
private INodeProvider _outOfProcNodeProvider;


/// <summary>
/// The node provider for out-of-proc RAR nodes.
/// </summary>
private INodeProvider _outOfProcRarNodeProvider;

/// <summary>
/// The build component host.
/// </summary>
Expand Down Expand Up @@ -119,11 +113,6 @@ public NodeInfo CreateNode(NodeConfiguration configuration, NodeAffinity nodeAff
nodeId = AttemptCreateNode(_outOfProcNodeProvider, configuration);
}

if (nodeId == InvalidNodeId && nodeAffinity == NodeAffinity.RarNode)
{
nodeId = AttemptCreateNode(_outOfProcRarNodeProvider, configuration);
}

if (nodeId == InvalidNodeId)
{
return null;
Expand Down Expand Up @@ -177,8 +166,6 @@ public void ShutdownConnectedNodes(bool enableReuse)
{
_outOfProcNodeProvider.ShutdownConnectedNodes(enableReuse);
}

_outOfProcRarNodeProvider?.ShutdownConnectedNodes(enableReuse);
}

/// <summary>
Expand All @@ -191,8 +178,6 @@ public void ShutdownAllNodes()
{
_outOfProcNodeProvider.ShutdownAllNodes();
}

_outOfProcRarNodeProvider?.ShutdownAllNodes();
}

#endregion
Expand All @@ -211,7 +196,6 @@ public void InitializeComponent(IBuildComponentHost host)

_inProcNodeProvider = _componentHost.GetComponent(BuildComponentType.InProcNodeProvider) as INodeProvider;
_outOfProcNodeProvider = _componentHost.GetComponent(BuildComponentType.OutOfProcNodeProvider) as INodeProvider;
_outOfProcRarNodeProvider = _componentHost.GetComponent(BuildComponentType.OutOfProcNodeRarProvider) as INodeProvider;

_componentShutdown = false;

Expand All @@ -233,14 +217,8 @@ public void ShutdownComponent()
((IDisposable)_outOfProcNodeProvider).Dispose();
}

if (_outOfProcRarNodeProvider is IDisposable rarNodeProvider)
{
rarNodeProvider.Dispose();
}

_inProcNodeProvider = null;
_outOfProcNodeProvider = null;
_outOfProcRarNodeProvider = null;
_componentHost = null;
_componentShutdown = true;

Expand Down
42 changes: 34 additions & 8 deletions src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,15 @@ namespace Microsoft.Build.BackEnd
internal class NodeProviderOutOfProc : NodeProviderOutOfProcBase, INodeProvider
{
/// <summary>
/// A mapping of all the nodes managed by this provider.
/// A mapping of all normal nodes managed by this provider.
/// </summary>
private Dictionary<int, NodeContext> _nodeContexts;

/// <summary>
/// A mapping of all RAR nodes managed by this provider.
/// </summary>
private Dictionary<int, NodeContext> _rarNodeContexts;

/// <summary>
/// Constructor.
/// </summary>
Expand Down Expand Up @@ -87,7 +92,7 @@ public bool CreateNode(int nodeId, INodePacketFactory factory, NodeConfiguration
// want to start up just a standard MSBuild out-of-proc node.
// Note: We need to always pass /nodeReuse to ensure the value for /nodeReuse from msbuild.rsp
// (next to msbuild.exe) is ignored.
string commandLineArgs = $"/nologo /nodemode:1 /nodeReuse:{ComponentHost.BuildParameters.EnableNodeReuse.ToString().ToLower()} /low:{ComponentHost.BuildParameters.LowPriority.ToString().ToLower()}";
string commandLineArgs = $"/nologo /nodemode:{(configuration.RarNode ? 3 : 1)} /nodeReuse:{ComponentHost.BuildParameters.EnableNodeReuse.ToString().ToLower()} /low:{ComponentHost.BuildParameters.LowPriority.ToString().ToLower()}";

// Make it here.
CommunicationsUtilities.Trace("Starting to acquire a new or existing node to establish node ID {0}...", nodeId);
Expand All @@ -97,7 +102,14 @@ public bool CreateNode(int nodeId, INodePacketFactory factory, NodeConfiguration

if (null != context)
{
_nodeContexts[nodeId] = context;
if (configuration.RarNode)
{
_rarNodeContexts[nodeId] = context;
}
else
{
_nodeContexts[nodeId] = context;
}

// Start the asynchronous read.
context.BeginAsyncPacketRead();
Expand All @@ -118,9 +130,18 @@ public bool CreateNode(int nodeId, INodePacketFactory factory, NodeConfiguration
/// <param name="packet">The packet to send.</param>
public void SendData(int nodeId, INodePacket packet)
{
ErrorUtilities.VerifyThrow(_nodeContexts.ContainsKey(nodeId), "Invalid node id specified: {0}.", nodeId);

SendData(_nodeContexts[nodeId], packet);
if (_nodeContexts.ContainsKey(nodeId))
{
SendData(_nodeContexts[nodeId], packet);
}
else if (_rarNodeContexts.ContainsKey(nodeId))
{
SendData(_rarNodeContexts[nodeId], packet);
}
else
{
ErrorUtilities.ThrowInternalError("Invalid node id specified: {0}.", nodeId);
}
}

/// <summary>
Expand All @@ -135,8 +156,9 @@ public void ShutdownConnectedNodes(bool enableReuse)
lock (_nodeContexts)
{
contextsToShutDown = new List<NodeContext>(_nodeContexts.Values);
contextsToShutDown.AddRange(_rarNodeContexts.Values);
}

ShutdownConnectedNodes(contextsToShutDown, enableReuse);
}

Expand Down Expand Up @@ -170,6 +192,7 @@ public void InitializeComponent(IBuildComponentHost host)
{
this.ComponentHost = host;
_nodeContexts = new Dictionary<int, NodeContext>();
_rarNodeContexts = new Dictionary<int, NodeContext>();
}

/// <summary>
Expand Down Expand Up @@ -197,7 +220,10 @@ private void NodeContextTerminated(int nodeId)
{
lock (_nodeContexts)
{
_nodeContexts.Remove(nodeId);
if (!_nodeContexts.Remove(nodeId))
{
_rarNodeContexts.Remove(nodeId);
}
}
}
}
Expand Down
107 changes: 7 additions & 100 deletions src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,18 +125,18 @@ 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.GetHandshake(nodeReuse, false, true));
Stream nodeStream = NamedPipeUtil.TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, false, true));

// If we couldn't connect attempt to connect to the process with the handshake including low priority.
nodeStream ??= TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, true, true));
nodeStream ??= NamedPipeUtil.TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, true, true));

// Attempt to connect to the non-worker process
// Attempt to connect to the process with the handshake without low priority.
nodeStream ??= TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, false, false));
nodeStream ??= NamedPipeUtil.TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, false, false));
// If we couldn't connect attempt to connect to the process with the handshake including low priority.
nodeStream ??= TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, true, false));
nodeStream ??= NamedPipeUtil.TryConnectToProcess(nodeProcess.Id, timeout, NodeProviderOutOfProc.GetHandshake(nodeReuse, true, false));

if (null != nodeStream)
if (nodeStream != null)
{
// If we're able to connect to such a process, send a packet requesting its termination
CommunicationsUtilities.Trace("Shutting down node with pid = {0}", nodeProcess.Id);
Expand Down Expand Up @@ -201,7 +201,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);
Stream nodeStream = NamedPipeUtil.TryConnectToProcess(nodeProcess.Id, 0 /* poll, don't wait for connections */, hostHandshake);
if (nodeStream != null)
{
// Connection successful, use this node.
Expand Down Expand Up @@ -252,7 +252,7 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in
// to the debugger process. Instead, use MSBUILDDEBUGONSTART=1

// Now try to connect to it.
Stream nodeStream = TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake);
Stream nodeStream = NamedPipeUtil.TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake);
if (nodeStream != null)
{
// Connection successful, use this node.
Expand Down Expand Up @@ -300,99 +300,6 @@ private string GetProcessesToIgnoreKey(Handshake hostHandshake, int nodeProcessI
return hostHandshake.ToString() + "|" + nodeProcessId.ToString(CultureInfo.InvariantCulture);
}

#if !FEATURE_PIPEOPTIONS_CURRENTUSERONLY
// This code needs to be in a separate method so that we don't try (and fail) to load the Windows-only APIs when JIT-ing the code
// on non-Windows operating systems
private void ValidateRemotePipeSecurityOnWindows(NamedPipeClientStream nodeStream)
{
SecurityIdentifier identifier = WindowsIdentity.GetCurrent().Owner;
#if FEATURE_PIPE_SECURITY
PipeSecurity remoteSecurity = nodeStream.GetAccessControl();
#else
var remoteSecurity = new PipeSecurity(nodeStream.SafePipeHandle, System.Security.AccessControl.AccessControlSections.Access |
System.Security.AccessControl.AccessControlSections.Owner | System.Security.AccessControl.AccessControlSections.Group);
#endif
IdentityReference remoteOwner = remoteSecurity.GetOwner(typeof(SecurityIdentifier));
if (remoteOwner != identifier)
{
CommunicationsUtilities.Trace("The remote pipe owner {0} does not match {1}", remoteOwner.Value, identifier.Value);
throw new UnauthorizedAccessException();
}
}
#endif

/// <summary>
/// Attempts to connect to the specified process.
/// </summary>
private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake handshake)
{
// Try and connect to the process.
string pipeName = NamedPipeUtil.GetPipeNameOrPath("MSBuild" + nodeProcessId);

NamedPipeClientStream nodeStream = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut, PipeOptions.Asynchronous
#if FEATURE_PIPEOPTIONS_CURRENTUSERONLY
| PipeOptions.CurrentUserOnly
#endif
);
CommunicationsUtilities.Trace("Attempting connect to PID {0} with pipe {1} with timeout {2} ms", nodeProcessId, pipeName, timeout);

try
{
nodeStream.Connect(timeout);

#if !FEATURE_PIPEOPTIONS_CURRENTUSERONLY
if (NativeMethodsShared.IsWindows && !NativeMethodsShared.IsMono)
{
// Verify that the owner of the pipe is us. This prevents a security hole where a remote node has
// been faked up with ACLs that would let us attach to it. It could then issue fake build requests back to
// us, potentially causing us to execute builds that do harmful or unexpected things. The pipe owner can
// only be set to the user's own SID by a normal, unprivileged process. The conditions where a faked up
// remote node could set the owner to something else would also let it change owners on other objects, so
// this would be a security flaw upstream of us.
ValidateRemotePipeSecurityOnWindows(nodeStream);
}
#endif

int[] handshakeComponents = handshake.RetrieveHandshakeComponents();
for (int i = 0; i < handshakeComponents.Length; i++)
{
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.
nodeStream.WriteEndOfHandshakeSignal();

CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName);

#if NETCOREAPP2_1 || MONO
nodeStream.ReadEndOfHandshakeSignal(true, timeout);
#else
nodeStream.ReadEndOfHandshakeSignal(true);
#endif
// We got a connection.
CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName);
return nodeStream;
}
catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
{
// Can be:
// UnauthorizedAccessException -- Couldn't connect, might not be a node.
// IOException -- Couldn't connect, already in use.
// TimeoutException -- Couldn't connect, might not be a node.
// InvalidOperationException – Couldn’t connect, probably a different build
CommunicationsUtilities.Trace("Failed to connect to pipe {0}. {1}", pipeName, e.Message.TrimEnd());

// If we don't close any stream, we might hang up the child
if (nodeStream != null)
{
nodeStream.Dispose();
}
}

return null;
}

/// <summary>
/// Creates a new MSBuild process
/// </summary>
Expand Down
Loading

0 comments on commit 20e4046

Please sign in to comment.