diff --git a/GVFS/GVFS.Common/NamedPipes/NamedPipeClient.cs b/GVFS/GVFS.Common/NamedPipes/NamedPipeClient.cs index 69e27b111..18745788f 100644 --- a/GVFS/GVFS.Common/NamedPipes/NamedPipeClient.cs +++ b/GVFS/GVFS.Common/NamedPipes/NamedPipeClient.cs @@ -8,8 +8,8 @@ public class NamedPipeClient : IDisposable { private string pipeName; private NamedPipeClientStream clientStream; - private StreamReader reader; - private StreamWriter writer; + private NamedPipeStreamReader reader; + private NamedPipeStreamWriter writer; public NamedPipeClient(string pipeName) { @@ -37,8 +37,8 @@ public bool Connect(int timeoutMilliseconds = 3000) return false; } - this.reader = new StreamReader(this.clientStream); - this.writer = new StreamWriter(this.clientStream); + this.reader = new NamedPipeStreamReader(this.clientStream); + this.writer = new NamedPipeStreamWriter(this.clientStream); return true; } @@ -68,8 +68,7 @@ public void SendRequest(string message) try { - this.writer.WritePlatformIndependentLine(message); - this.writer.Flush(); + this.writer.WriteMessage(message); } catch (IOException e) { @@ -81,7 +80,7 @@ public string ReadRawResponse() { try { - string response = this.reader.ReadLine(); + string response = this.reader.ReadMessage(); if (response == null) { throw new BrokenPipeException("Unable to read from pipe", null); diff --git a/GVFS/GVFS.Common/NamedPipes/NamedPipeServer.cs b/GVFS/GVFS.Common/NamedPipes/NamedPipeServer.cs index d6eebe711..83ac690e3 100644 --- a/GVFS/GVFS.Common/NamedPipes/NamedPipeServer.cs +++ b/GVFS/GVFS.Common/NamedPipes/NamedPipeServer.cs @@ -88,7 +88,7 @@ private void OpenListeningPipe() private void OnNewConnection(IAsyncResult ar) { if (!this.isStopping) - { + { this.OnNewConnection(ar, createNewThreadIfSynchronous: true); } } @@ -137,14 +137,14 @@ private void OnNewConnection(IAsyncResult ar, bool createNewThreadIfSynchronous) if (!connectionBroken) { - try - { - this.handleConnection(new Connection(pipe, () => this.isStopping)); - } - catch (Exception e) - { - this.LogErrorAndExit("Unhandled exception in connection handler", e); - } + try + { + this.handleConnection(new Connection(pipe, this.tracer, () => this.isStopping)); + } + catch (Exception e) + { + this.LogErrorAndExit("Unhandled exception in connection handler", e); + } } } } @@ -174,16 +174,18 @@ private void LogErrorAndExit(string message, Exception e) public class Connection { private NamedPipeServerStream serverStream; - private StreamReader reader; - private StreamWriter writer; + private NamedPipeStreamReader reader; + private NamedPipeStreamWriter writer; + private ITracer tracer; private Func isStopping; - public Connection(NamedPipeServerStream serverStream, Func isStopping) + public Connection(NamedPipeServerStream serverStream, ITracer tracer, Func isStopping) { this.serverStream = serverStream; + this.tracer = tracer; this.isStopping = isStopping; - this.reader = new StreamReader(this.serverStream); - this.writer = new StreamWriter(this.serverStream); + this.reader = new NamedPipeStreamReader(this.serverStream); + this.writer = new NamedPipeStreamWriter(this.serverStream); } public bool IsConnected @@ -200,10 +202,17 @@ public string ReadRequest() { try { - return this.reader.ReadLine(); + return this.reader.ReadMessage(); } - catch (IOException) + catch (IOException e) { + EventMetadata metadata = new EventMetadata(); + metadata.Add("ExceptionMessage", e.Message); + metadata.Add("StackTrace", e.StackTrace); + this.tracer.RelatedError( + metadata: metadata, + message: $"Error reading message from NamedPipe: {e.Message}"); + return null; } } @@ -212,9 +221,7 @@ public bool TrySendResponse(string message) { try { - this.writer.WritePlatformIndependentLine(message); - this.writer.Flush(); - + this.writer.WriteMessage(message); return true; } catch (IOException) diff --git a/GVFS/GVFS.Common/NamedPipes/NamedPipeStreamReader.cs b/GVFS/GVFS.Common/NamedPipes/NamedPipeStreamReader.cs new file mode 100644 index 000000000..17ced2e20 --- /dev/null +++ b/GVFS/GVFS.Common/NamedPipes/NamedPipeStreamReader.cs @@ -0,0 +1,92 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text; + +namespace GVFS.Common.NamedPipes +{ + /// + /// Implements the NamedPipe protocol as described in NamedPipeServer. + /// + public class NamedPipeStreamReader + { + private const int DefaultBufferSize = 1024; + private const byte TerminatorByte = 0x3; + + private int bufferSize; + private byte[] buffer; + private Stream stream; + + public NamedPipeStreamReader(Stream stream, int bufferSize) + { + this.stream = stream; + this.bufferSize = bufferSize; + this.buffer = new byte[this.bufferSize]; + } + + public NamedPipeStreamReader(Stream stream) + : this(stream, DefaultBufferSize) + { + } + + /// + /// Read a message from the stream. + /// + /// The message read from the stream, or null if the end of the input stream has been reached. + public string ReadMessage() + { + int bytesRead = this.Read(); + if (bytesRead == 0) + { + // The end of the stream has been reached - return null to indicate this. + return null; + } + + // If we have read in the entire message in the first read (mainline scenario), + // then just process the data directly from the buffer. + if (this.buffer[bytesRead - 1] == TerminatorByte) + { + return Encoding.UTF8.GetString(this.buffer, 0, bytesRead - 1); + } + + // We need to process multiple chunks - collect data from multiple chunks + // into a single list + List bytes = new List(this.bufferSize * 2); + + while (true) + { + bool encounteredTerminatorByte = this.buffer[bytesRead - 1] == TerminatorByte; + int lengthToCopy = encounteredTerminatorByte ? bytesRead - 1 : bytesRead; + + bytes.AddRange(new ArraySegment(this.buffer, 0, lengthToCopy)); + if (encounteredTerminatorByte) + { + break; + } + + bytesRead = this.Read(); + + if (bytesRead == 0) + { + // We have read a partial message (the last byte received does not indicate that + // this was the end of the message), but the stream has been closed. Throw an exception + // and let upper layer deal with this condition. + + throw new IOException("Incomplete message read from stream. The end of the stream was reached without the expected terminating byte."); + } + } + + return Encoding.UTF8.GetString(bytes.ToArray()); + } + + /// + /// Read the next chunk of bytes from the stream. + /// + /// The number of bytes read. + private int Read() + { + return this.stream.Read(this.buffer, 0, this.buffer.Length); + } + } +} diff --git a/GVFS/GVFS.Common/NamedPipes/NamedPipeStreamWriter.cs b/GVFS/GVFS.Common/NamedPipes/NamedPipeStreamWriter.cs new file mode 100644 index 000000000..f3f11ecbe --- /dev/null +++ b/GVFS/GVFS.Common/NamedPipes/NamedPipeStreamWriter.cs @@ -0,0 +1,24 @@ +using System.IO; +using System.Text; + +namespace GVFS.Common.NamedPipes +{ + public class NamedPipeStreamWriter + { + private const byte TerminatorByte = 0x3; + private const string TerminatorByteString = "\x3"; + private Stream stream; + + public NamedPipeStreamWriter(Stream stream) + { + this.stream = stream; + } + + public void WriteMessage(string message) + { + byte[] byteBuffer = Encoding.UTF8.GetBytes(message + TerminatorByteString); + this.stream.Write(byteBuffer, 0, byteBuffer.Length); + this.stream.Flush(); + } + } +} diff --git a/GVFS/GVFS.Common/NamedPipes/StreamWriterExtensions.cs b/GVFS/GVFS.Common/NamedPipes/StreamWriterExtensions.cs deleted file mode 100644 index 58db185d6..000000000 --- a/GVFS/GVFS.Common/NamedPipes/StreamWriterExtensions.cs +++ /dev/null @@ -1,16 +0,0 @@ -using System.IO; - -namespace GVFS.Common.NamedPipes -{ - public static class StreamWriterExtensions - { - public const int Foo = 0; - - public static void WritePlatformIndependentLine(this StreamWriter writer, string value) - { - // WriteLine is not platform independent as on some platforms it terminates lines with \r\n - // and on others it uses \n - writer.Write(value + "\n"); - } - } -} diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs index f079e5464..0b3ccccce 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs @@ -381,6 +381,16 @@ public void AddFileInSubfolderAndCommitOnNewBranchSwitchDeleteFolderAndSwitchBac this.FileShouldHaveContents(newFileContents, newFilePath); } + [TestCase] + public void CommitWithNewlinesInMessage() + { + this.ValidateGitCommand("checkout -b tests/functional/commit_with_uncommon_arguments"); + this.CreateFile(); + this.ValidateGitCommand("status"); + this.ValidateGitCommand("add ."); + this.RunGitCommand("commit -m \"Message that contains \na\nnew\nline\""); + } + [TestCase] public void CaseOnlyRenameFileAndChangeBranches() { diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs index 5ca28eaa4..d729822d0 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs @@ -63,6 +63,16 @@ public void UpdateIndexRemoveAddFileOpenForWrite() GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "update-index --add Test_ConflictTests/AddedFiles/AddedByBothDifferentContent.txt"); } + [TestCase] + [Category(Categories.MacTODO.M4)] + public void UpdateIndexWithCacheInfo() + { + // Update Protocol.md with the contents from blob 583f1... + string command = $"update-index --cacheinfo 100644 \"583f1a56db7cc884d54534c5d9c56b93a1e00a2b\n\" Protocol.md"; + + this.ValidateGitCommand(command); + } + protected override void CreateEnlistment() { base.CreateEnlistment(); diff --git a/GVFS/GVFS.Hooks/GVFS.Hooks.Mac.csproj b/GVFS/GVFS.Hooks/GVFS.Hooks.Mac.csproj index ddde81618..dccf8bdd3 100644 --- a/GVFS/GVFS.Hooks/GVFS.Hooks.Mac.csproj +++ b/GVFS/GVFS.Hooks/GVFS.Hooks.Mac.csproj @@ -59,8 +59,11 @@ Common\NamedPipes\NamedPipeClient.cs - - Common\NamedPipes\StreamWriterExtensions.cs + + Common\NamedPipes\NamedPipeStreamReader.cs + + + Common\NamedPipes\NamedPipeStreamWriter.cs Common\NativeMethods.Shared.cs diff --git a/GVFS/GVFS.Hooks/GVFS.Hooks.Windows.csproj b/GVFS/GVFS.Hooks/GVFS.Hooks.Windows.csproj index 692c8832a..05629c39e 100644 --- a/GVFS/GVFS.Hooks/GVFS.Hooks.Windows.csproj +++ b/GVFS/GVFS.Hooks/GVFS.Hooks.Windows.csproj @@ -83,8 +83,11 @@ Common\NamedPipes\NamedPipeClient.cs - - Common\NamedPipes\StreamWriterExtensions.cs + + Common\NamedPipes\NamedPipeStreamReader.cs + + + Common\NamedPipes\NamedPipeStreamWriter.cs Common\NativeMethods.Shared.cs diff --git a/GVFS/GVFS.ReadObjectHook/main.cpp b/GVFS/GVFS.ReadObjectHook/main.cpp index 5fcb05af4..7a3f60887 100644 --- a/GVFS/GVFS.ReadObjectHook/main.cpp +++ b/GVFS/GVFS.ReadObjectHook/main.cpp @@ -18,8 +18,8 @@ #define DLO_REQUEST_LENGTH (4 + SHA1_LENGTH + 1) // Expected response: -// "S\n" -> Success -// "F\n" -> Failure +// "S\x3" -> Success +// "F\x3" -> Failure #define DLO_RESPONSE_LENGTH 2 enum ReadObjectHookErrorReturnCode @@ -33,7 +33,7 @@ int DownloadSHA(PIPE_HANDLE pipeHandle, const char *sha1) // Format: "DLO|<40 character SHA>" // Example: "DLO|920C34DCDDFC8F07AC4704C8C0D087D6F2095729" char request[DLO_REQUEST_LENGTH+1]; - if (snprintf(request, DLO_REQUEST_LENGTH+1, "DLO|%s\n", sha1) != DLO_REQUEST_LENGTH) + if (snprintf(request, DLO_REQUEST_LENGTH+1, "DLO|%s\x3", sha1) != DLO_REQUEST_LENGTH) { die(ReturnCode::InvalidSHA, "First argument must be a 40 character SHA, actual value: %s\n", sha1); } diff --git a/GVFS/GVFS.UnitTests/Common/NamedPipeStreamReaderWriterTests.cs b/GVFS/GVFS.UnitTests/Common/NamedPipeStreamReaderWriterTests.cs new file mode 100644 index 000000000..003ca048f --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/NamedPipeStreamReaderWriterTests.cs @@ -0,0 +1,108 @@ +using GVFS.Common.NamedPipes; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.IO; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class NamedPipeStreamReaderWriterTests + { + private const int BufferSize = 256; + + private MemoryStream stream; + private NamedPipeStreamWriter streamWriter; + private NamedPipeStreamReader streamReader; + + [SetUp] + public void Setup() + { + this.stream = new MemoryStream(); + this.streamWriter = new NamedPipeStreamWriter(this.stream); + this.streamReader = new NamedPipeStreamReader(this.stream, BufferSize); + } + + [Test] + [Description("Verify that we can transmit multiple messages")] + public void CanWriteAndReadMessages() + { + string firstMessage = @"This is a new message"; + this.TestTransmitMessage(firstMessage); + + string secondMessage = @"This is another message"; + this.TestTransmitMessage(secondMessage); + + string thirdMessage = @"This is the third message in a series of messages"; + this.TestTransmitMessage(thirdMessage); + + string longMessage = new string('T', 1024 * 5); + this.TestTransmitMessage(longMessage); + } + + [Test] + [Description("Verify that we can transmit a message that contains content that is the size of a NamedPipeStreamReader's buffer")] + public void CanSendBufferSizedContent() + { + string longMessage = new string('T', BufferSize); + this.TestTransmitMessage(longMessage); + } + + [Test] + [Description("Verify that we can transmit message that is the same size a NamedPipeStreamReader's buffer")] + public void CanSendBufferSizedMessage() + { + int numBytesInMessageTerminator = 1; + string longMessage = new string('T', BufferSize - numBytesInMessageTerminator); + this.TestTransmitMessage(longMessage); + } + + [Test] + [Description("Verify that the expected exception is thrown if message is not terminated with expected byte.")] + public void ReadingPartialMessgeThrows() + { + byte[] bytes = System.Text.Encoding.ASCII.GetBytes("This is a partial message"); + + this.stream.Write(bytes, 0, bytes.Length); + this.stream.Seek(0, SeekOrigin.Begin); + + Assert.Throws(() => this.streamReader.ReadMessage()); + } + + [Test] + [Description("Verify that we can transmit message that is larger than the buffer")] + public void CanSendMultiBufferSizedMessage() + { + string longMessage = new string('T', BufferSize * 3); + this.TestTransmitMessage(longMessage); + } + + [Test] + [Description("Verify that we can transmit message that newline characters")] + public void CanSendNewLines() + { + string messageWithNewLines = "This is a \nstringwith\nnewlines"; + this.TestTransmitMessage(messageWithNewLines); + } + + private void TestTransmitMessage(string message) + { + long pos = this.ReadStreamPosition(); + this.streamWriter.WriteMessage(message); + + this.SetStreamPosition(pos); + + string readMessage = this.streamReader.ReadMessage(); + readMessage.ShouldEqual(message, "The message read from the stream reader is not the same as the message that was sent."); + } + + private long ReadStreamPosition() + { + return this.stream.Position; + } + + private void SetStreamPosition(long position) + { + this.stream.Seek(position, SeekOrigin.Begin); + } + } +} diff --git a/GVFS/GVFS.VirtualFileSystemHook/main.cpp b/GVFS/GVFS.VirtualFileSystemHook/main.cpp index d0c1aea18..60498900e 100644 --- a/GVFS/GVFS.VirtualFileSystemHook/main.cpp +++ b/GVFS/GVFS.VirtualFileSystemHook/main.cpp @@ -31,7 +31,7 @@ int main(int argc, char *argv[]) int error = 0; bool success = WriteToPipe( pipeHandle, - "MPL|1\n", + "MPL|1\x3", messageLength, &bytesWritten, &error); @@ -49,6 +49,7 @@ int main(int argc, char *argv[]) int lastError; bool finishedReading = false; bool firstRead = true; + do { char *pMessage = &message[0]; @@ -80,7 +81,7 @@ int main(int argc, char *argv[]) messageLength -= 2; } - if (*(pMessage + messageLength - 1) == '\n') + if (*(pMessage + messageLength - 1) == '\x3') { finishedReading = true; messageLength -= 1;