From aaed7b57baa1ba0506a65c21b829ac0915b5b9ed Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Mon, 18 Feb 2019 08:24:24 -0800 Subject: [PATCH 1/2] 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 876ae0c08e0ad..45659e9427acb 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 67efa1d6de0c7..496bb756f6d05 100644 --- a/src/Compilers/Shared/BuildServerConnection.cs +++ b/src/Compilers/Shared/BuildServerConnection.cs @@ -378,10 +378,16 @@ internal static async Task 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)) { @@ -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 5c7cfb72b0c8f..640694999fa65 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; From ec5be09861786f937042f4cf7358ff319ad4241e Mon Sep 17 00:00:00 2001 From: Heejae Chang Date: Mon, 18 Feb 2019 20:34:16 -0800 Subject: [PATCH 2/2] made Checksum type to truncate byte array given to it if it is bigger than HashSize (#33363) * replace SHA1 to SHA256 * tweak * Update src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs Co-Authored-By: heejaechang * Update src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs Co-Authored-By: heejaechang * Update src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs Co-Authored-By: heejaechang * tweaks based on PR feedback * removed unnecessary unsafe * updated comment * moved comments around and removed reference to SHA in checksum --- .../Portable/Workspace/Solution/Checksum.cs | 121 +++++++++++------- .../Workspace/Solution/Checksum_Factory.cs | 48 ++++--- .../ServiceHub/Shared/RoslynJsonConverter.cs | 2 +- 3 files changed, 100 insertions(+), 71 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs index 7b3d2fc0394c2..45876eaa65ac7 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs @@ -16,65 +16,88 @@ namespace Microsoft.CodeAnalysis internal sealed partial class Checksum : IObjectWritable, IEquatable { /// - /// The intended size of the structure. Some runtime environments are known to deviate - /// from this size, so the implementation code should remain functionally correct even when the structure size - /// is greater than this value. + /// The intended size of the structure. /// - /// LayoutKind.Explicit, Size = 12 ignored with 64bit alignment - /// Checksum throws on Mono 64-bit - private const int Sha1HashSize = 20; + private const int HashSize = 20; - public static readonly Checksum Null = new Checksum(Array.Empty()); + public static readonly Checksum Null = new Checksum(default); - private Sha1Hash _checkSum; + private readonly HashData _checksum; - public unsafe Checksum(byte[] checksum) + /// + /// Create Checksum from given byte array. if byte array is bigger than + /// , it will be truncated to the size + /// + public static Checksum From(byte[] checksum) { if (checksum.Length == 0) { - _checkSum = default; - return; - } - else if (checksum.Length != Sha1HashSize) - { - throw new ArgumentException($"{nameof(checksum)} must be a SHA-1 hash", nameof(checksum)); + return Null; } - fixed (byte* data = checksum) + if (checksum.Length < HashSize) { - // Avoid a direct dereferencing assignment since sizeof(Sha1Hash) may be greater than Sha1HashSize. - _checkSum = Sha1Hash.FromPointer((Sha1Hash*)data); + throw new ArgumentException($"checksum must be equal or bigger than the hash size: {HashSize}", nameof(checksum)); } + + return FromWorker(checksum); } - public unsafe Checksum(ImmutableArray checksum) + /// + /// Create Checksum from given byte array. if byte array is bigger than + /// , it will be truncated to the size + /// + public static Checksum From(ImmutableArray checksum) { if (checksum.Length == 0) { - _checkSum = default; - return; + return Null; } - else if (checksum.Length != Sha1HashSize) + + if (checksum.Length < HashSize) { - throw new ArgumentException($"{nameof(checksum)} must be a SHA-1 hash", nameof(checksum)); + throw new ArgumentException($"{nameof(checksum)} must be equal or bigger than the hash size: {HashSize}", nameof(checksum)); } using (var pooled = SharedPools.ByteArray.GetPooledObject()) { var bytes = pooled.Object; - checksum.CopyTo(bytes); + checksum.CopyTo(sourceIndex: 0, bytes, destinationIndex: 0, length: HashSize); + + return FromWorker(bytes); + } + } - fixed (byte* data = bytes) - { - // Avoid a direct dereferencing assignment since sizeof(Sha1Hash) may be greater than Sha1HashSize. - _checkSum = Sha1Hash.FromPointer((Sha1Hash*)data); - } + public static Checksum FromSerialized(byte[] checksum) + { + if (checksum.Length == 0) + { + return Null; + } + + if (checksum.Length != HashSize) + { + throw new ArgumentException($"{nameof(checksum)} must be equal to the hash size: {HashSize}", nameof(checksum)); + } + + return FromWorker(checksum); + } + + private static unsafe Checksum FromWorker(byte[] checksum) + { + fixed (byte* data = checksum) + { + // Avoid a direct dereferencing assignment since sizeof(HashData) may be greater than HashSize. + // + // ex) "https://bugzilla.xamarin.com/show_bug.cgi?id=60298" - LayoutKind.Explicit, Size = 12 ignored with 64bit alignment + // or "https://github.com/dotnet/roslyn/issues/23722" - Checksum throws on Mono 64-bit + return new Checksum(HashData.FromPointer((HashData*)data)); } } - private Checksum(Sha1Hash hash) + private Checksum(HashData hash) { - _checkSum = hash; + _checksum = hash; } public bool Equals(Checksum other) @@ -84,24 +107,24 @@ public bool Equals(Checksum other) return false; } - return _checkSum == other._checkSum; + return _checksum == other._checksum; } public override bool Equals(object obj) => Equals(obj as Checksum); public override int GetHashCode() - => _checkSum.GetHashCode(); + => _checksum.GetHashCode(); public override unsafe string ToString() { - var data = new byte[sizeof(Sha1Hash)]; + var data = new byte[sizeof(HashData)]; fixed (byte* dataPtr = data) { - *(Sha1Hash*)dataPtr = _checkSum; + *(HashData*)dataPtr = _checksum; } - return Convert.ToBase64String(data, 0, Sha1HashSize); + return Convert.ToBase64String(data, 0, HashSize); } public static bool operator ==(Checksum left, Checksum right) @@ -117,10 +140,10 @@ public override unsafe string ToString() bool IObjectWritable.ShouldReuseInSerialization => true; public void WriteTo(ObjectWriter writer) - => _checkSum.WriteTo(writer); + => _checksum.WriteTo(writer); public static Checksum ReadFrom(ObjectReader reader) - => new Checksum(Sha1Hash.ReadFrom(reader)); + => new Checksum(HashData.ReadFrom(reader)); public static string GetChecksumLogInfo(Checksum checksum) { @@ -133,11 +156,11 @@ public static string GetChecksumsLogInfo(IEnumerable checksums) } /// - /// This structure stores the 20-byte SHA 1 hash as an inline value rather than requiring the use of + /// This structure stores the 20-byte hash as an inline value rather than requiring the use of /// byte[]. /// - [StructLayout(LayoutKind.Explicit, Size = Sha1HashSize)] - private struct Sha1Hash : IEquatable + [StructLayout(LayoutKind.Explicit, Size = HashSize)] + private struct HashData : IEquatable { [FieldOffset(0)] private long Data1; @@ -148,10 +171,10 @@ private struct Sha1Hash : IEquatable [FieldOffset(16)] private int Data3; - public static bool operator ==(Sha1Hash x, Sha1Hash y) + public static bool operator ==(HashData x, HashData y) => x.Equals(y); - public static bool operator !=(Sha1Hash x, Sha1Hash y) + public static bool operator !=(HashData x, HashData y) => !x.Equals(y); public void WriteTo(ObjectWriter writer) @@ -161,18 +184,18 @@ public void WriteTo(ObjectWriter writer) writer.WriteInt32(Data3); } - public static unsafe Sha1Hash FromPointer(Sha1Hash* hash) + public static unsafe HashData FromPointer(HashData* hash) { - Sha1Hash result = default; + HashData result = default; result.Data1 = hash->Data1; result.Data2 = hash->Data2; result.Data3 = hash->Data3; return result; } - public static Sha1Hash ReadFrom(ObjectReader reader) + public static HashData ReadFrom(ObjectReader reader) { - Sha1Hash result = default; + HashData result = default; result.Data1 = reader.ReadInt64(); result.Data2 = reader.ReadInt64(); result.Data3 = reader.ReadInt32(); @@ -186,9 +209,9 @@ public override int GetHashCode() } public override bool Equals(object obj) - => obj is Sha1Hash other && Equals(other); + => obj is HashData other && Equals(other); - public bool Equals(Sha1Hash other) + public bool Equals(HashData other) { return Data1 == other.Data1 && Data2 == other.Data2 diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs index c4be6ea88a087..5a694c9d95612 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs @@ -18,31 +18,37 @@ public static Checksum Create(Stream stream) { using (var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA1)) { - return ComputeChecksum(stream, hash); - } - } - - private static Checksum ComputeChecksum(Stream stream, IncrementalHash hash) - { - using (var pooledBuffer = SharedPools.ByteArray.GetPooledObject()) - { - stream.Seek(0, SeekOrigin.Begin); - - var buffer = pooledBuffer.Object; - var bufferLength = buffer.Length; - int bytesRead; - do + using (var pooledBuffer = SharedPools.ByteArray.GetPooledObject()) { - bytesRead = stream.Read(buffer, 0, bufferLength); - if (bytesRead > 0) + stream.Seek(0, SeekOrigin.Begin); + + var buffer = pooledBuffer.Object; + var bufferLength = buffer.Length; + int bytesRead; + do { - hash.AppendData(buffer, 0, bytesRead); + bytesRead = stream.Read(buffer, 0, bufferLength); + if (bytesRead > 0) + { + hash.AppendData(buffer, 0, bytesRead); + } } - } - while (bytesRead > 0); + while (bytesRead > 0); - var bytes = hash.GetHashAndReset(); - return new Checksum(bytes); + var bytes = hash.GetHashAndReset(); + + // if bytes array is bigger than certain size, checksum + // will truncate it to predetermined size. for more detail, + // see the Checksum type + // + // the truncation can happen since different hash algorithm or + // same algorithm on different platform can have different hash size + // which might be bigger than the Checksum HashSize. + // + // hash algorithm used here should remain functionally correct even + // after the truncation + return Checksum.From(bytes); + } } } diff --git a/src/Workspaces/Remote/ServiceHub/Shared/RoslynJsonConverter.cs b/src/Workspaces/Remote/ServiceHub/Shared/RoslynJsonConverter.cs index 9a5eb2729afeb..53cd382395d97 100644 --- a/src/Workspaces/Remote/ServiceHub/Shared/RoslynJsonConverter.cs +++ b/src/Workspaces/Remote/ServiceHub/Shared/RoslynJsonConverter.cs @@ -175,7 +175,7 @@ private class ChecksumJsonConverter : BaseJsonConverter protected override Checksum ReadValue(JsonReader reader, JsonSerializer serializer) { var value = (string)reader.Value; - return value == null ? null : new Checksum(Convert.FromBase64String(value)); + return value == null ? null : Checksum.FromSerialized(Convert.FromBase64String(value)); } protected override void WriteValue(JsonWriter writer, Checksum value, JsonSerializer serializer)