From aaed7b57baa1ba0506a65c21b829ac0915b5b9ed Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Mon, 18 Feb 2019 08:24:24 -0800 Subject: [PATCH] Ensure pipename argument quoted 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: #32257 --- .../CompilerServerApiTest.cs | 34 +++++++++++++++++++ src/Compilers/Shared/BuildServerConnection.cs | 29 ++++++++++++++-- .../Assert/ConditionalFactAttribute.cs | 6 ++++ 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs b/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs index 876ae0c08e0..45659e9427a 100644 --- a/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs +++ b/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs @@ -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)); + } } } diff --git a/src/Compilers/Shared/BuildServerConnection.cs b/src/Compilers/Shared/BuildServerConnection.cs index 67efa1d6de0..496bb756f6d 100644 --- a/src/Compilers/Shared/BuildServerConnection.cs +++ b/src/Compilers/Shared/BuildServerConnection.cs @@ -378,10 +378,16 @@ internal static bool IsCompilerServerSupported(string tempPath) } } - 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)) { @@ -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('\\', '_'); } /// diff --git a/src/Test/Utilities/Portable/Assert/ConditionalFactAttribute.cs b/src/Test/Utilities/Portable/Assert/ConditionalFactAttribute.cs index 5c7cfb72b0c..640694999fa 100644 --- a/src/Test/Utilities/Portable/Assert/ConditionalFactAttribute.cs +++ b/src/Test/Utilities/Portable/Assert/ConditionalFactAttribute.cs @@ -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; -- GitLab