Merge master to features/readonly-members

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));
+        }
     }
 }

src/Compilers/Shared/BuildServerConnection.cs

@@ -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))
             {
@@ -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>

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;

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

@@ -16,65 +16,88 @@ namespace Microsoft.CodeAnalysis
     internal sealed partial class Checksum : IObjectWritable, IEquatable<Checksum>
     {
         /// <summary>
-        /// The intended size of the <see cref="Sha1Hash"/> 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 <see cref="HashData"/> structure. 
         /// </summary>
-        /// <seealso href="https://bugzilla.xamarin.com/show_bug.cgi?id=60298">LayoutKind.Explicit, Size = 12 ignored with 64bit alignment</seealso>
-        /// <seealso href="https://github.com/dotnet/roslyn/issues/23722">Checksum throws on Mono 64-bit</seealso>
-        private const int Sha1HashSize = 20;
+        private const int HashSize = 20;
 
-        public static readonly Checksum Null = new Checksum(Array.Empty<byte>());
+        public static readonly Checksum Null = new Checksum(default);
 
-        private Sha1Hash _checkSum;
+        private readonly HashData _checksum;
 
-        public unsafe Checksum(byte[] checksum)
+        /// <summary>
+        /// Create Checksum from given byte array. if byte array is bigger than
+        /// <see cref="HashSize"/>, it will be truncated to the size
+        /// </summary>
+        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<byte> checksum)
+        /// <summary>
+        /// Create Checksum from given byte array. if byte array is bigger than
+        /// <see cref="HashSize"/>, it will be truncated to the size
+        /// </summary>
+        public static Checksum From(ImmutableArray<byte> 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<Checksum> checksums)
         }
 
         /// <summary>
-        /// 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
         /// <c>byte[]</c>.
         /// </summary>
-        [StructLayout(LayoutKind.Explicit, Size = Sha1HashSize)]
-        private struct Sha1Hash : IEquatable<Sha1Hash>
+        [StructLayout(LayoutKind.Explicit, Size = HashSize)]
+        private struct HashData : IEquatable<HashData>
         {
             [FieldOffset(0)]
             private long Data1;
@@ -148,10 +171,10 @@ private struct Sha1Hash : IEquatable<Sha1Hash>
             [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

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);
+                }
             }
         }
 

src/Workspaces/Remote/ServiceHub/Shared/RoslynJsonConverter.cs

@@ -175,7 +175,7 @@ private class ChecksumJsonConverter : BaseJsonConverter<Checksum>
             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)