From f5f4a7c4f17d25d82f38a87d5e024f28e1350027 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 05:29:47 +0000 Subject: [PATCH] Strip a leading byte order mark once, not twice, for an encoding with a preamble [patch] StreamReader skips the encoding's own preamble whatever detectEncodingFromByteOrderMarks says, and AsyncProcessStreamReader then dropped a leading U+FEFF from the first chunk as well. For Encoding.UTF8 (OutputHandler's default), Encoding.Unicode and the other encodings with a preamble, output starting with a BOM followed by a real U+FEFF lost both, so the same bytes decoded differently through Encoding.UTF8 and new UTF8Encoding(false). The manual strip now runs only for an encoding with no preamble, where the reader leaves the BOM in place. Fixes ktsu-dev/RunCommand#145 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW45a4XVwC8wG7P95kY --- RunCommand.Test/RunCommandTests.cs | 52 ++++++++++++++++++++++++++ RunCommand/AsyncProcessStreamReader.cs | 20 +++++++--- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 3a202a5..1fa6117 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -3,6 +3,7 @@ namespace ktsu.RunCommand.Test; using System.Diagnostics; +using System.Globalization; using System.Runtime.CompilerServices; using System.Text; using System.Runtime.InteropServices; @@ -1185,6 +1186,57 @@ public void AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput() Assert.AreEqual("hello", RunOverPath(path, StrictUtf8)); } + /// + /// Returns the encoding a byte-order-mark test names, along with that encoding's own BOM. + /// + /// + /// Named rather than passed because only takes constants. The + /// cases split on whether the encoding has a preamble: skips a + /// matching preamble itself whatever detectEncodingFromByteOrderMarks says, so one with a + /// preamble must not be stripped a second time by the library. + /// + private static (Encoding Encoding, byte[] Bom) GetByteOrderMarkCase(string name) => name switch + { + "Encoding.UTF8" => (Encoding.UTF8, [0xEF, 0xBB, 0xBF]), + "UTF8Encoding(false)" => (new UTF8Encoding(false), [0xEF, 0xBB, 0xBF]), + "Encoding.Unicode" => (Encoding.Unicode, [0xFF, 0xFE]), + "UnicodeEncoding(false, false)" => (new UnicodeEncoding(bigEndian: false, byteOrderMark: false), [0xFF, 0xFE]), + _ => throw new ArgumentOutOfRangeException(nameof(name), name, "Unknown encoding case."), + }; + + [TestMethod] + [DataRow("Encoding.UTF8")] + [DataRow("UTF8Encoding(false)")] + [DataRow("Encoding.Unicode")] + [DataRow("UnicodeEncoding(false, false)")] + public void OnlyTheByteOrderMarkIsStrippedWhenARealU_FEFFFollowsIt(string encodingName) + { + // A BOM, then U+FEFF that is part of the output, then "A". The BOM goes and the content + // stays, whichever of two equivalent encodings the caller passed. With a preamble the reader + // dropped the BOM and the library then dropped the real U+FEFF after it. + (Encoding encoding, byte[] bom) = GetByteOrderMarkCase(encodingName); + byte[] bytes = [.. bom, .. encoding.GetBytes("\uFEFFA")]; + string path = WriteBytesForTest(bytes, $"{nameof(OnlyTheByteOrderMarkIsStrippedWhenARealU_FEFFFollowsIt)}.{new string([.. encodingName.Where(char.IsLetterOrDigit)])}"); + + string output = RunOverPath(path, encoding); + + Assert.AreEqual("FEFF 0041", string.Join(' ', output.Select(c => ((int)c).ToString("X4", CultureInfo.InvariantCulture)))); + } + + [TestMethod] + [DataRow("Encoding.UTF8")] + [DataRow("UTF8Encoding(false)")] + [DataRow("Encoding.Unicode")] + [DataRow("UnicodeEncoding(false, false)")] + public void ALoneByteOrderMarkIsStrippedWhicheverEncodingReadsIt(string encodingName) + { + (Encoding encoding, byte[] bom) = GetByteOrderMarkCase(encodingName); + byte[] bytes = [.. bom, .. encoding.GetBytes("hello")]; + string path = WriteBytesForTest(bytes, $"{nameof(ALoneByteOrderMarkIsStrippedWhicheverEncodingReadsIt)}.{new string([.. encodingName.Where(char.IsLetterOrDigit)])}"); + + Assert.AreEqual("hello", RunOverPath(path, encoding)); + } + [TestMethod] public void ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning() { diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index a529429..48ff508 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/RunCommand/AsyncProcessStreamReader.cs @@ -32,6 +32,12 @@ internal sealed class AsyncProcessStreamReader(Process process, OutputHandler ou private readonly StreamReader errorStream = new(process.StandardError.BaseStream, outputHandler.Encoding, detectEncodingFromByteOrderMarks: false); + // StreamReader skips the encoding's own preamble at the start of a stream whatever + // detectEncodingFromByteOrderMarks says, so for an encoding that has one (Encoding.UTF8, the + // default, and Encoding.Unicode among them) the BOM is already gone by the time a chunk arrives. + // Stripping again there removed a U+FEFF that was real output. + private readonly bool readerSkipsByteOrderMark = outputHandler.Encoding.GetPreamble().Length > 0; + public void Dispose() { outputStream.Dispose(); @@ -204,11 +210,13 @@ internal static async Task ReadChunks(TextReader reader, char[] buffer, Action /// - /// 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. + /// Turning off the detection above also turned off the stripping that came with it for an + /// encoding with no preamble, 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. An encoding with a preamble has had its BOM skipped by the reader + /// already, so a U+FEFF at the front of its first chunk is content and is kept. /// /// The chunk just read. /// Whether the chunk came from standard output. @@ -226,7 +234,7 @@ private string StripLeadingByteOrderMark(string data, bool isStandardOutput) errorHasEmitted = true; } - if (hasEmitted || data[0] != ByteOrderMark) + if (hasEmitted || readerSkipsByteOrderMark || data[0] != ByteOrderMark) { return data; }