diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 4d0c40b..8fb5335 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -2,7 +2,9 @@ namespace ktsu.RunCommand.Test; +using System.Diagnostics; using System.Runtime.CompilerServices; +using System.Text; using System.Runtime.InteropServices; using ktsu.Semantics.Paths; @@ -610,4 +612,168 @@ await Assert.ThrowsAsync( EnvironmentVariables = new Dictionary { ["ANY"] = "value" }, })).ConfigureAwait(false); } + + /// + /// Returns a command that writes a file's bytes to standard output unchanged, as an executable + /// plus separate arguments. + /// + /// + /// Windows has no reliably byte-faithful built-in for this. cmd /c type looked like one + /// but transcodes a file carrying a UTF-16 byte order mark instead of copying it, which is + /// exactly the input these tests need, so PowerShell writes the raw bytes to the standard + /// output stream instead. checks the result rather than + /// trusting it. + /// + private static (string FileName, string[] Arguments) GetEmitFileBytesCommand(string path) => + RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + ? ("powershell", [ + "-NoProfile", + "-NonInteractive", + "-Command", + $"$b=[IO.File]::ReadAllBytes('{path}'); $s=[Console]::OpenStandardOutput(); $s.Write($b,0,$b.Length); $s.Flush()"]) + : ("cat", [path]); + + private static readonly Encoding StrictUtf8 = new UTF8Encoding(false, throwOnInvalidBytes: true); + + // Runs the emitter through the library, so the test controls the exact bytes that reach the + // pipe and can assert on what the library makes of them. + private static string RunOverPath(string path, Encoding encoding) + { + StringBuilder output = new(); + (string fileName, string[] arguments) = GetEmitFileBytesCommand(path); + _ = RunCommand.Execute(fileName, arguments, new OutputHandler(o => output.Append(o), null, encoding)); + + return output.ToString(); + } + + /// + /// Checks that this platform's emitter really does put on the pipe + /// unchanged, so that a mangling emitter reports itself instead of being read as a result about + /// the library. + /// + /// + /// This drives the emitter through directly and copies the raw + /// , deliberately never touching the code under test. A + /// control that went through would measure the very defect these tests + /// exist to catch and blame the emitter for it, which would turn a regression into an + /// inconclusive result instead of a failure. + /// + private static bool EmitsBytesFaithfully(string path, byte[] bytes, out string diagnostic) + { + byte[] actual; + + try + { + (string fileName, string[] arguments) = GetEmitFileBytesCommand(path); + ProcessStartInfo startInfo = new() + { + FileName = fileName, + RedirectStandardOutput = true, + UseShellExecute = false, + CreateNoWindow = true, + }; + + foreach (string argument in arguments) + { + startInfo.ArgumentList.Add(argument); + } + + using Process process = Process.Start(startInfo)!; + using MemoryStream captured = new(); + process.StandardOutput.BaseStream.CopyTo(captured); + process.WaitForExit(); + actual = captured.ToArray(); + } + catch (System.ComponentModel.Win32Exception ex) + { + diagnostic = $"This platform's byte emitter could not be started: {ex.Message}"; + return false; + } + + bool faithful = actual.SequenceEqual(bytes); + diagnostic = faithful + ? string.Empty + : "This platform's byte emitter altered the bytes, so the library cannot be judged from them. " + + $"Expected [{Convert.ToHexString(bytes)}], the pipe carried [{Convert.ToHexString(actual)}]."; + + return faithful; + } + + // Writes the bytes to a file of this test's own and confirms the platform can actually put them + // on a pipe unchanged, returning the path to feed to the library. + private static string WriteBytesForTest(byte[] bytes, string caller) + { + string path = Path.Join(CreateDirectoryForTest(caller), "bytes.bin"); + File.WriteAllBytes(path, bytes); + + if (!EmitsBytesFaithfully(path, bytes, out string diagnostic)) + { + Assert.Inconclusive(diagnostic); + } + + return path; + } + + private static void AssertDecodeFailure(byte[] bytes, [CallerMemberName] string caller = "") + { + // The emitter check has to happen out here: an Assert.Inconclusive raised inside the lambda + // below would be caught by Assert.ThrowsExactly and reported as a failing test. + string path = WriteBytesForTest(bytes, caller); + + AggregateException thrown = Assert.ThrowsExactly(() => RunOverPath(path, StrictUtf8)); + Assert.IsTrue( + thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), + $"Expected a decode failure, got: {thrown}"); + } + + [TestMethod] + public void OutputEncodingIsNotReplacedByAUtf16ByteOrderMark() => + // "hi" in UTF-16LE behind its byte order mark. Process builds its StandardOutput reader with + // byte-order-mark detection on, which used to switch the reader to UTF-16LE and return "hi" + // even though the caller asked for strict UTF-8 and these are not valid UTF-8 bytes. + AssertDecodeFailure([0xFF, 0xFE, (byte)'h', 0x00, (byte)'i', 0x00]); + + [TestMethod] + public void OutputThatIsOnlyAUtf16ByteOrderMarkIsNotReportedAsSuccess() => + // The same detection consumed a lone FF FE as a byte order mark, leaving nothing to decode, + // so a strict encoding reported no output and no error for bytes it should have rejected. + AssertDecodeFailure([0xFF, 0xFE]); + + [TestMethod] + public void AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput() + { + // A byte order mark matching the requested encoding is still dropped, so turning the + // detection off did not start leaking U+FEFF into captured output. + byte[] bytes = [0xEF, 0xBB, 0xBF, (byte)'h', (byte)'e', (byte)'l', (byte)'l', (byte)'o']; + string path = WriteBytesForTest(bytes, nameof(AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput)); + + Assert.AreEqual("hello", RunOverPath(path, StrictUtf8)); + } + + [TestMethod] + public void ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + Assert.Inconclusive("Needs a shell that can emit bytes and then stay alive. The race this covers is in platform independent code, so the other legs cover it."); + } + + // The read loop stops when the process exits, so a read that faults while the process is + // still running used to be replaced by a fresh read on the next pass and its decode failure + // thrown away. Sleeping after the bad byte keeps the process alive long enough for that pass + // to happen, which makes the race deterministic rather than roughly one run in six. + string path = Path.Join(CreateDirectoryForTest(), "bytes.bin"); + File.WriteAllBytes(path, [(byte)'o', (byte)'k', 0xFF]); + + StringBuilder output = new(); + AggregateException thrown = Assert.ThrowsExactly( + () => RunCommand.Execute( + "sh", + ["-c", $"cat '{path}'; sleep 1"], + new OutputHandler(o => output.Append(o), null, StrictUtf8))); + + Assert.IsTrue( + thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), + $"Expected a decode failure, got: {thrown}"); + } } diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index 174e383..1a8c809 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/RunCommand/AsyncProcessStreamReader.cs @@ -4,11 +4,34 @@ namespace ktsu.RunCommand; using System.Diagnostics; -internal sealed class AsyncProcessStreamReader(Process process, OutputHandler outputHandler) +internal sealed class AsyncProcessStreamReader(Process process, OutputHandler outputHandler) : IDisposable { + private const char ByteOrderMark = '\uFEFF'; + private readonly char[] outputBuffer = new char[4096]; private readonly char[] errorBuffer = new char[4096]; + private bool outputHasEmitted; + private bool errorHasEmitted; + + // Read the raw pipes with the caller's encoding rather than through + // process.StandardOutput/StandardError. Process builds those StreamReaders with byte-order-mark + // detection switched on, which replaces the encoding the caller asked for whenever the output + // happens to begin with a BOM. Output starting with FF FE was decoded as UTF-16LE no matter what + // OutputHandler.Encoding said, and a strict encoding reported no error on bytes it should have + // rejected. + private readonly StreamReader outputStream = + new(process.StandardOutput.BaseStream, outputHandler.Encoding, detectEncodingFromByteOrderMarks: false); + + private readonly StreamReader errorStream = + new(process.StandardError.BaseStream, outputHandler.Encoding, detectEncodingFromByteOrderMarks: false); + + public void Dispose() + { + outputStream.Dispose(); + errorStream.Dispose(); + } + internal async Task Start() { Task outputTask = Task.CompletedTask; @@ -17,14 +40,23 @@ internal async Task Start() // Continuously read until the process has exited. do { + // A faulted task is a completed one, so without this the checks below would replace a + // failed read with a fresh one and the failure it carries would never be observed. That + // made a decode error on a long-running command a coin toss: it surfaced only when the + // process happened to exit before the loop came back around. + if (outputTask.IsFaulted || errorTask.IsFaulted) + { + break; + } + if (outputTask.IsCompleted) { - outputTask = ReadAndCallback(process.StandardOutput, outputBuffer, outputHandler.HandleStandardOutputData); + outputTask = ReadAndCallback(outputStream, outputBuffer, outputHandler.HandleStandardOutputData, isStandardOutput: true); } if (errorTask.IsCompleted) { - errorTask = ReadAndCallback(process.StandardError, errorBuffer, outputHandler.HandleStandardErrorData); + errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); } await Task.WhenAny(outputTask, errorTask).ConfigureAwait(false); @@ -34,24 +66,65 @@ internal async Task Start() await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); // Read any remaining data after process exit. - outputTask = ReadAndCallback(process.StandardOutput, outputBuffer, outputHandler.HandleStandardOutputData); - errorTask = ReadAndCallback(process.StandardError, errorBuffer, outputHandler.HandleStandardErrorData); + outputTask = ReadAndCallback(outputStream, outputBuffer, outputHandler.HandleStandardOutputData, isStandardOutput: true); + errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); } - private static async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action? onData) => + private async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action? onData, bool isStandardOutput) => await streamReader.ReadAsync(buffer, 0, buffer.Length) - .ContinueWith(t => ReadCallback(t, buffer, onData), TaskScheduler.Current) + .ContinueWith(t => ReadCallback(t, buffer, onData, isStandardOutput), TaskScheduler.Current) .ConfigureAwait(false); - private static void ReadCallback(Task readTask, char[] buffer, Action? onData) + private void ReadCallback(Task readTask, char[] buffer, Action? onData, bool isStandardOutput) { - int bytesRead = readTask.Result; + int charsRead = readTask.Result; + + if (charsRead <= 0) + { + return; + } - if (bytesRead > 0) + string data = new(buffer, 0, charsRead); + data = StripLeadingByteOrderMark(data, isStandardOutput); + + if (data.Length > 0) { - string data = new(buffer, 0, bytesRead); onData?.Invoke(data); } } + + /// + /// Drops a byte order mark from the front of a stream's first chunk. + /// + /// + /// Turning off the detection above also turned off the stripping that came with it, and a + /// leading U+FEFF in captured output is a change no caller asked for. Every encoding decodes its + /// own BOM to U+FEFF, so dropping that one character covers each of them without guessing at the + /// encoding. A BOM belonging to a different encoding no longer decodes to U+FEFF, which is + /// exactly the case that should surface as mis-decoded bytes rather than be silently honoured. + /// + /// The chunk just read. + /// Whether the chunk came from standard output. + /// The chunk, less a leading byte order mark if this was the stream's first. + private string StripLeadingByteOrderMark(string data, bool isStandardOutput) + { + bool hasEmitted = isStandardOutput ? outputHasEmitted : errorHasEmitted; + + if (isStandardOutput) + { + outputHasEmitted = true; + } + else + { + errorHasEmitted = true; + } + + if (hasEmitted || data[0] != ByteOrderMark) + { + return data; + } + + return data[1..]; + } } diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index bf0b386..bcf37bb 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -462,7 +462,7 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle } else { - AsyncProcessStreamReader outputReader = new(process, outputHandler); + using AsyncProcessStreamReader outputReader = new(process, outputHandler); await Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); }