Skip to content

Commit

Permalink
Ensure pipename argument quoted
Browse files Browse the repository at this point in the history
The name for the named pipe used between MSBuild and VBCSCompiler is
generated from a combination of values including the current user name,
specifically `%USERNAME%`. It is possible for this value to have spaces
in it and hence the argument must be quoted when passing it to the
command line of VBCSCompiler instances.

Regression initially introduced: dotnet#32257
  • Loading branch information
jaredpar committed Feb 18, 2019
1 parent 6d07d39 commit cb84901
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 3 deletions.
34 changes: 34 additions & 0 deletions src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -542,5 +542,39 @@ public async Task IncorrectServerHashReturnsIncorrectHashResponse()
Assert.Equal(BuildResponse.ResponseType.IncorrectHash, buildResponse.Type);
}
}

[ConditionalFact(typeof(WindowsDesktopOnly))]
[WorkItem(33452, "https://github.com/dotnet/roslyn/issues/33452")]
public void QuotePipeName_Desktop()
{
var serverInfo = BuildServerConnection.GetServerProcessInfo(@"q:\tools", "name with space");
Assert.Equal(@"q:\tools\VBCSCompiler.exe", serverInfo.processFilePath);
Assert.Equal(@"q:\tools\VBCSCompiler.exe", serverInfo.toolFilePath);
Assert.Equal(@"""-pipename:name with space""", serverInfo.commandLineArguments);
}

[ConditionalFact(typeof(CoreClrOnly))]
[WorkItem(33452, "https://github.com/dotnet/roslyn/issues/33452")]
public void QuotePipeName_CoreClr()
{
var toolDir = ExecutionConditionUtil.IsWindows
? @"q:\tools"
: "/tools";
var serverInfo = BuildServerConnection.GetServerProcessInfo(toolDir, "name with space");
var vbcsFilePath = Path.Combine(toolDir, "VBCSCompiler.dll");
Assert.Equal(vbcsFilePath, serverInfo.toolFilePath);
Assert.Equal($@"exec ""{vbcsFilePath}"" ""-pipename:name with space""", serverInfo.commandLineArguments);
}

[Theory]
[InlineData(@"name with space.T.basename", "name with space", true, "basename")]
[InlineData(@"ha_ha.T.basename", @"ha""ha", true, "basename")]
[InlineData(@"jared.T.ha_ha", @"jared", true, @"ha""ha")]
[InlineData(@"jared.F.ha_ha", @"jared", false, @"ha""ha")]
[InlineData(@"jared.F.ha_ha", @"jared", false, @"ha\ha")]
public void GetPipeNameCore(string expectedName, string userName, bool isAdmin, string basePipeName)
{
Assert.Equal(expectedName, BuildServerConnection.GetPipeNameCore(userName, isAdmin, basePipeName));
}
}
}
29 changes: 26 additions & 3 deletions src/Compilers/Shared/BuildServerConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -378,10 +378,16 @@ internal static async Task<NamedPipeClientStream> TryConnectToServerAsync(
}
}

internal static bool TryCreateServerCore(string clientDir, string pipeName)
internal static (string processFilePath, string commandLineArguments, string toolFilePath) GetServerProcessInfo(string clientDir, string pipeName)
{
var serverPathWithoutExtension = Path.Combine(clientDir, "VBCSCompiler");
var serverInfo = RuntimeHostInfo.GetProcessInfo(serverPathWithoutExtension, $"-pipename:{pipeName}");
var commandLineArgs = $@"""-pipename:{pipeName}""";
return RuntimeHostInfo.GetProcessInfo(serverPathWithoutExtension, commandLineArgs);
}

internal static bool TryCreateServerCore(string clientDir, string pipeName)
{
var serverInfo = GetServerProcessInfo(clientDir, pipeName);

if (!File.Exists(serverInfo.toolFilePath))
{
Expand Down Expand Up @@ -480,7 +486,24 @@ internal static string GetPipeNameForPathOpt(string compilerExeDirectory)
return null;
}

return $"{userName}.{(isAdmin ? 'T' : 'F')}.{basePipeName}";
return GetPipeNameCore(userName, isAdmin, basePipeName);
}

internal static string GetPipeNameCore(string userName, bool isAdmin, string basePipeName)
{
var pipeName = $"{userName}.{(isAdmin ? 'T' : 'F')}.{basePipeName}";

// The pipe name is passed between processes as a command line argument as a
// quoted value. Unfortunately we can't use ProcessStartInfo.ArgumentList as
// we still target net472 (API only available on CoreClr + netstandard). To
// make the problem approachable we remove the troublesome characters.
//
// This does mean if two users on the same machine are building simultaneously
// and the user names differ only be a " or / and a _ then there will be a
// conflict. That seems rather obscure though.
return pipeName
.Replace('"', '_')
.Replace('\\', '_');
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,12 @@ public class ClrOnly : ExecutionCondition
public override string SkipReason => "Test not supported on Mono";
}

public class CoreClrOnly : ExecutionCondition
{
public override bool ShouldSkip => !ExecutionConditionUtil.IsCoreClr;
public override string SkipReason => "Test only supported on CoreClr";
}

public class DesktopOnly : ExecutionCondition
{
public override bool ShouldSkip => !ExecutionConditionUtil.IsDesktop;
Expand Down

0 comments on commit cb84901

Please sign in to comment.