Honour OutputHandler.Encoding regardless of a byte order mark

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 @@
 					EnvironmentVariables = new Dictionary<string, string?> { ["ANY"] = "value" },
 				})).ConfigureAwait(false);
 	}
+
+	/// <summary>
+	/// Returns a command that writes a file's bytes to standard output unchanged, as an executable
+	/// plus separate arguments.
+	/// </summary>
+	/// <remarks>
+	/// Windows has no reliably byte-faithful built-in for this. <c>cmd /c type</c> 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. <see cref="EmitsBytesFaithfully"/> checks the result rather than
+	/// trusting it.
+	/// </remarks>
+	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();
+	}
+
+	/// <summary>
+	/// Checks that this platform's emitter really does put <paramref name="bytes"/> on the pipe
+	/// unchanged, so that a mangling emitter reports itself instead of being read as a result about
+	/// the library.
+	/// </summary>
+	/// <remarks>
+	/// This drives the emitter through <see cref="Process"/> directly and copies the raw
+	/// <see cref="StreamReader.BaseStream"/>, deliberately never touching the code under test. A
+	/// control that went through <see cref="RunCommand"/> 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.
+	/// </remarks>
+	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<AggregateException>(() => 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<AggregateException>(
+			() => 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}");
+	}
 }

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<string>? onData) =>
+	private async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action<string>? 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<int> readTask, char[] buffer, Action<string>? onData)
+	private void ReadCallback(Task<int> readTask, char[] buffer, Action<string>? 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);
 		}
 	}
+
+	/// <summary>
+	/// Drops a byte order mark from the front of a stream's first chunk.
+	/// </summary>
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	/// <param name="data">The chunk just read.</param>
+	/// <param name="isStandardOutput">Whether the chunk came from standard output.</param>
+	/// <returns>The chunk, less a leading byte order mark if this was the stream's first.</returns>
+	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..];
+	}
 }

RunCommand/RunCommand.cs

@@ -23,7 +23,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command) =>
 		ExecuteAsync(command).Result;
@@ -34,7 +34,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler) =>
 		ExecuteAsync(command, outputHandler).Result;
@@ -45,7 +45,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, Elevation elevation) =>
 		ExecuteAsync(command, elevation).Result;
@@ -108,7 +108,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command)
 		=> await ExecuteAsync(command, new OutputHandler()).ConfigureAwait(false);
@@ -119,7 +119,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default).ConfigureAwait(false);
@@ -462,7 +462,7 @@
 		}
 		else
 		{
-			AsyncProcessStreamReader outputReader = new(process, outputHandler);
+			using AsyncProcessStreamReader outputReader = new(process, outputHandler);
 			await Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false);
 		}