From 3aac4138b4c6e08b86ae736fa501f42f30d5b19c Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Tue, 11 Sep 2018 11:33:30 -0400 Subject: [PATCH 1/3] Tests demonstrating issue with newline in Git commands --- .../Tests/GitCommands/GitCommandsTests.cs | 11 +++++++++++ .../Tests/GitCommands/UpdateIndexTests.cs | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs index f079e5464..f7f5a6d47 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs @@ -381,6 +381,17 @@ public void AddFileInSubfolderAndCommitOnNewBranchSwitchDeleteFolderAndSwitchBac this.FileShouldHaveContents(newFileContents, newFilePath); } + [TestCase] + [Ignore("Currently failing regression test for how GVFS handles newlines in git command line")] + 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..df6871f24 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs @@ -63,6 +63,17 @@ public void UpdateIndexRemoveAddFileOpenForWrite() GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "update-index --add Test_ConflictTests/AddedFiles/AddedByBothDifferentContent.txt"); } + [TestCase] + [Category(Categories.MacTODO.M4)] + [Ignore("Currently failing regression test for how GVFS handles newlines in git command line")] + 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(); From fb44c71f86bfb1304bed0321930f76be11651f2b Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Fri, 7 Sep 2018 10:37:22 -0400 Subject: [PATCH 2/3] Tweak the protocol GVFS used to communicate over named pipes GVFS currently sends a text based data across a named pipe with lines / messages separated by newline characters. This prohibits sending messages that include a newline as part of the message content. The main current manifestation of this problem is that GVFS does not handle git command lines that contain a newline character. There is another problem that GVFS needs to send paths across the named pipe (for ModifiedPathsList queries), and these paths can include newline characters on certain filesystems (macOS, Linux). Additionally, the protocol that GVFS uses to communicate across the named pipe is currently not consistent. It usually uses a text based stream, but for the ModifiedPathsList response, it uses null bytes to seperate entries in a list. To address these issues, this change tweaks the protocol used to communicate via the named pipe to use the 0x3 byte to indicate the end of a line / message (this is the End of text ASCII code). --- .../GVFS.Common/NamedPipes/NamedPipeClient.cs | 13 ++- .../GVFS.Common/NamedPipes/NamedPipeServer.cs | 45 +++++---- .../NamedPipes/NamedPipeStreamReader.cs | 92 +++++++++++++++++++ .../NamedPipes/NamedPipeStreamWriter.cs | 24 +++++ .../NamedPipes/StreamWriterExtensions.cs | 16 ---- .../Tests/GitCommands/GitCommandsTests.cs | 1 - .../Tests/GitCommands/UpdateIndexTests.cs | 1 - GVFS/GVFS.Hooks/GVFS.Hooks.Mac.csproj | 7 +- GVFS/GVFS.Hooks/GVFS.Hooks.Windows.csproj | 7 +- GVFS/GVFS.ReadObjectHook/main.cpp | 6 +- GVFS/GVFS.VirtualFileSystemHook/main.cpp | 5 +- 11 files changed, 164 insertions(+), 53 deletions(-) create mode 100644 GVFS/GVFS.Common/NamedPipes/NamedPipeStreamReader.cs create mode 100644 GVFS/GVFS.Common/NamedPipes/NamedPipeStreamWriter.cs delete mode 100644 GVFS/GVFS.Common/NamedPipes/StreamWriterExtensions.cs 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 f7f5a6d47..0b3ccccce 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/GitCommandsTests.cs @@ -382,7 +382,6 @@ public void AddFileInSubfolderAndCommitOnNewBranchSwitchDeleteFolderAndSwitchBac } [TestCase] - [Ignore("Currently failing regression test for how GVFS handles newlines in git command line")] public void CommitWithNewlinesInMessage() { this.ValidateGitCommand("checkout -b tests/functional/commit_with_uncommon_arguments"); diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs index df6871f24..d729822d0 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/UpdateIndexTests.cs @@ -65,7 +65,6 @@ public void UpdateIndexRemoveAddFileOpenForWrite() [TestCase] [Category(Categories.MacTODO.M4)] - [Ignore("Currently failing regression test for how GVFS handles newlines in git command line")] public void UpdateIndexWithCacheInfo() { // Update Protocol.md with the contents from blob 583f1... 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.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; From 659c2d4ba5e2059f0372226f8824778c0ba80e8a Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Fri, 21 Sep 2018 15:33:23 -0400 Subject: [PATCH 3/3] Add automated tests around NamedPipeStream protocol --- .../NamedPipeStreamReaderWriterTests.cs | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 GVFS/GVFS.UnitTests/Common/NamedPipeStreamReaderWriterTests.cs 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); + } + } +}