From d87d8e23e03971dbb6e107bec04880626057bbcf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 11:30:12 +0000 Subject: [PATCH 1/3] Pass Unix commands to sh verbatim instead of escaping only quotes [patch] NativeCommandExecutor built `-c ""` by escaping only `"`, so backslashes already in the command combined with the inserted escapes under the MSVCRT splitting rules, and `echo "say \"hi\""` or `echo trailing\` failed with "Unterminated quoted string". On netcoreapp2.1+ the command now goes through ArgumentList, which needs no quoting. netstandard2.1 has no ArgumentList, so it quotes the command with a MSVCRT-correct escaper, tested by round-tripping arguments through ProcessStartInfo.Arguments. Fixes #54 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- CLAUDE.md | 2 +- .../NativeCommandExecutor.cs | 61 ++++++++++++++- Essentials.Tests/CommandExecutorTests.cs | 76 +++++++++++++++++++ 3 files changed, 137 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index dc50833..baecf0d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -146,7 +146,7 @@ Tests use **MSTest.Sdk** targeting net10.0 only. The test project (`Essentials.T - `RandomProviderTests.cs` - Contract tests over all 4 random providers (bounds, validation, shuffle and sampling invariants, uniformity of a range that does not divide 2^32), plus reference sequences for Xoshiro and Pcg produced by an independent transcription of each published algorithm. Those vectors make a seeded sequence part of the package contract: changing one is a breaking change and this is where it surfaces - `DistributionProviderTests.cs` - Tests all 10 distributions three ways: contract properties every distribution must have (CDF bounded and non-decreasing, quantile inverts it, density integrates to one, masses sum to one, survival function complements the CDF), reference values computed outside the codebase, and empirical checks that seeded samples match the analytic moments and deciles - `CacheProviderTests.cs` - Tests cache operations including expiration, and that a legitimately cached `null` is honoured rather than treated as a miss: `Get`/`GetAsync` return it instead of throwing, and `GetOrAdd`/`GetOrAddAsync` return it without re-invoking the factory. `TryGet`'s `bool` is the sole authority on presence — a null check on the value is not a proxy for it. It also pins that an expired entry's removal leaves a value set concurrently under the same key in place (a key type that runs the `Set` between `TryGet`'s read and its remove makes that interleaving deterministic), and that a `TimeSpan.MaxValue` or `TimeSpan.MinValue` time-to-live saturates rather than overflowing -- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them +- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, that a POSIX command's quotes and backslashes reach `sh -c` verbatim (issue #54), and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them - `EncodingProviderTests.cs` - Tests Base64 and Hex encoding - `ObfuscationProviderTests.cs` - Tests all obfuscation providers via round-trip (obfuscate → deobfuscate) - `FileSystemProviderTests.cs` - Tests filesystem operations diff --git a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs index fed0507..1fd8de8 100644 --- a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs +++ b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs @@ -8,6 +8,7 @@ namespace ktsu.Essentials.CommandExecutors.Native; using System.Diagnostics; using System.IO; using System.Runtime.InteropServices; +using System.Text; using System.Threading; using System.Threading.Tasks; @@ -183,7 +184,6 @@ private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDiction ProcessStartInfo startInfo = new() { FileName = isWindows ? "cmd.exe" : "/bin/sh", - Arguments = isWindows ? $"/c {command}" : $"-c \"{command.Replace("\"", "\\\"")}\"", WorkingDirectory = workingDirectory ?? string.Empty, RedirectStandardOutput = true, RedirectStandardError = true, @@ -191,6 +191,23 @@ private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDiction CreateNoWindow = true, }; + if (isWindows) + { + startInfo.Arguments = $"/c {command}"; + } + else + { + // The shell has to receive the command as one argument, byte for byte. ArgumentList hands it over + // without any quoting at all; where it does not exist the argument string is split by the MSVCRT + // rules, so the command is quoted for exactly those rules. See issue #54. +#if NETCOREAPP2_1_OR_GREATER + startInfo.ArgumentList.Add("-c"); + startInfo.ArgumentList.Add(command); +#else + startInfo.Arguments = $"-c {QuoteArgument(command)}"; +#endif + } + if (environmentVariables is not null) { foreach (KeyValuePair kvp in environmentVariables) @@ -202,6 +219,48 @@ private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDiction return startInfo; } + /// + /// Quotes so that splitting a command line by the MSVCRT rules, which + /// uses on every platform, yields it back unchanged. + /// + /// + /// Backslashes are literal except in a run that ends at a quote: there each one is doubled and the quote is + /// escaped, and a run at the very end is doubled so it cannot escape the closing quote. + /// + /// The argument to quote. + /// as a single quoted command-line argument. + internal static string QuoteArgument(string argument) + { + StringBuilder quoted = new(argument.Length + 2); + quoted.Append('"'); + + int backslashes = 0; + foreach (char c in argument) + { + if (c == '\\') + { + backslashes++; + continue; + } + + if (c == '"') + { + quoted.Append('\\', (backslashes * 2) + 1); + } + else + { + quoted.Append('\\', backslashes); + } + + backslashes = 0; + quoted.Append(c); + } + + quoted.Append('\\', backslashes * 2); + quoted.Append('"'); + return quoted.ToString(); + } + /// /// Kills a process that is being abandoned because the operation was cancelled, ignoring the races /// where it has already exited or was never started. diff --git a/Essentials.Tests/CommandExecutorTests.cs b/Essentials.Tests/CommandExecutorTests.cs index e14c724..dbd0a8f 100644 --- a/Essentials.Tests/CommandExecutorTests.cs +++ b/Essentials.Tests/CommandExecutorTests.cs @@ -11,6 +11,7 @@ namespace ktsu.Essentials.Tests; using System.Threading; using System.Threading.Tasks; using ktsu.Essentials; +using ktsu.Essentials.CommandExecutors.Native; using Microsoft.Extensions.DependencyInjection; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -320,6 +321,81 @@ public void CommandExecutor_Sync_Captures_The_Same_Bytes_As_Async(ICommandExecut $"{providerName} should return the same standard output from both paths"); } + /// + /// POSIX commands that carry backslashes next to quotes, with what sh -c prints for each. Up to + /// issue #54 the executor escaped only the quotes, and the backslashes the command already had combined + /// with the inserted escapes, so the shell received a mangled command. + /// + public static IEnumerable PosixCommandsWithBackslashes => + [ + ["echo \"say \\\"hi\\\"\"", "say \"hi\"\n"], + ["echo trailing\\", "trailing\\\n"], + ["printf '%s' 'a\\\\\"b'", "a\\\\\"b"], + ]; + + /// + /// Regression test for issue #54: the shell receives the command exactly as written, through both the + /// synchronous and the asynchronous path. + /// + /// The command to run. + /// What the shell prints for it. + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + [DynamicData(nameof(PosixCommandsWithBackslashes))] + public void NativeCommandExecutor_Passes_Backslashes_And_Quotes_To_The_Shell_Verbatim(string command, string expected) + { + ICommandExecutor executor = new NativeCommandExecutor(); + + CommandResult syncResult = executor.Execute(command, cancellationToken: TestContext.CancellationToken); + CommandResult asyncResult = executor.ExecuteAsync(command, cancellationToken: TestContext.CancellationToken).Result; + + Assert.AreEqual(expected, syncResult.StandardOutput, $"sync should run the command as written (stderr: {syncResult.StandardError})"); + Assert.AreEqual(expected, asyncResult.StandardOutput, $"async should run the command as written (stderr: {asyncResult.StandardError})"); + } + + /// + /// Arguments whose backslashes the MSVCRT rules treat specially: before a quote, at the end, and in runs. + /// + public static IEnumerable ArgumentsToQuote => + [ + [""], + ["plain"], + ["two words"], + ["say \\\"hi\\\""], + ["trailing\\"], + ["trailing\\\\"], + ["a\\\\\"b"], + ["\\\"\""], + ["c:\\path\\to\\file"], + ]; + + /// + /// is what the netstandard2.1 build passes to the shell, + /// which the tests cannot run. Splitting is the same on every target, so passing the quoted text through + /// here proves the round trip that build depends on. + /// + /// The argument to quote. + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + [DynamicData(nameof(ArgumentsToQuote))] + public void NativeCommandExecutor_QuoteArgument_Round_Trips_Through_Arguments(string argument) + { + using Process process = new(); + process.StartInfo = new ProcessStartInfo + { + FileName = "printf", + Arguments = $"%s {NativeCommandExecutor.QuoteArgument(argument)}", + RedirectStandardOutput = true, + UseShellExecute = false, + }; + + process.Start(); + string output = process.StandardOutput.ReadToEnd(); + process.WaitForExit(); + + Assert.AreEqual(argument, output, "the child should receive the argument unchanged"); + } + /// /// The synchronous members of as they were declared up to v2.3.1, in the /// shape a consumer compiled against that version emits a callvirt to. Optional parameters are not From 04b213bd3718b84e8ccc00a673ce8706331fec28 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 11:34:04 +0000 Subject: [PATCH 2/3] Move the quoting helper and its notes away from the cancellation code Keeps this change from touching the lines #53's fix edits, so the two PRs merge in either order. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- CLAUDE.md | 4 +- .../NativeCommandExecutor.cs | 48 +++++++++---------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index baecf0d..2c79cd7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,6 +124,8 @@ so no pool thread is held; declaring that one member converts all three synchron The reader threads read raw rather than going through the line-splitting `OutputDataReceived` callbacks, so the synchronous path returns byte-for-byte what the child wrote — the same text `ExecuteAsync` returns — instead of rewriting the terminators and adding one of its own. See issue #23. +On Unix the command reaches `/bin/sh -c` as one argument, byte for byte: through `ProcessStartInfo.ArgumentList` on +netcoreapp2.1+, and through the MSVCRT-correct `QuoteArgument` on netstandard2.1, which has no `ArgumentList`. See issue #54. Common patterns are centralized in `ProviderHelpers.cs`: @@ -146,7 +148,7 @@ Tests use **MSTest.Sdk** targeting net10.0 only. The test project (`Essentials.T - `RandomProviderTests.cs` - Contract tests over all 4 random providers (bounds, validation, shuffle and sampling invariants, uniformity of a range that does not divide 2^32), plus reference sequences for Xoshiro and Pcg produced by an independent transcription of each published algorithm. Those vectors make a seeded sequence part of the package contract: changing one is a breaking change and this is where it surfaces - `DistributionProviderTests.cs` - Tests all 10 distributions three ways: contract properties every distribution must have (CDF bounded and non-decreasing, quantile inverts it, density integrates to one, masses sum to one, survival function complements the CDF), reference values computed outside the codebase, and empirical checks that seeded samples match the analytic moments and deciles - `CacheProviderTests.cs` - Tests cache operations including expiration, and that a legitimately cached `null` is honoured rather than treated as a miss: `Get`/`GetAsync` return it instead of throwing, and `GetOrAdd`/`GetOrAddAsync` return it without re-invoking the factory. `TryGet`'s `bool` is the sole authority on presence — a null check on the value is not a proxy for it. It also pins that an expired entry's removal leaves a value set concurrently under the same key in place (a key type that runs the `Set` between `TryGet`'s read and its remove makes that interleaving deterministic), and that a `TimeSpan.MaxValue` or `TimeSpan.MinValue` time-to-live saturates rather than overflowing -- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, that a POSIX command's quotes and backslashes reach `sh -c` verbatim (issue #54), and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them +- `CommandExecutorTests.cs` - Tests command execution, including the synchronous path, cancellation before and during a run, a working directory that does not exist, that the synchronous and asynchronous paths capture the same bytes for a command whose output ends without a terminator, and that `ExecuteAndGetOutput` throws unwrapped. `ICommandExecutor`'s own synchronous defaults are reached through a test double that declares only the asynchronous members, since `NativeCommandExecutor` replaces them - `EncodingProviderTests.cs` - Tests Base64 and Hex encoding - `ObfuscationProviderTests.cs` - Tests all obfuscation providers via round-trip (obfuscate → deobfuscate) - `FileSystemProviderTests.cs` - Tests filesystem operations diff --git a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs index 1fd8de8..5061fde 100644 --- a/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs +++ b/Essentials.CommandExecutors.Native/NativeCommandExecutor.cs @@ -219,6 +219,30 @@ private static ProcessStartInfo CreateStartInfo(string command, IReadOnlyDiction return startInfo; } + /// + /// Kills a process that is being abandoned because the operation was cancelled, ignoring the races + /// where it has already exited or was never started. + /// + /// The process to kill. + private static void TryKill(Process process) + { + try + { + process.Kill(); + } + catch (Exception ex) when (ex is InvalidOperationException or System.ComponentModel.Win32Exception) + { + // The process exited between the wait timing out and this call, or could not be terminated. + } + } + + /// + /// Builds the result returned when an operation is cancelled. + /// + /// A failed describing the cancellation. + private static CommandResult Cancelled() => + new(-1, string.Empty, "Operation was cancelled."); + /// /// Quotes so that splitting a command line by the MSVCRT rules, which /// uses on every platform, yields it back unchanged. @@ -261,30 +285,6 @@ internal static string QuoteArgument(string argument) return quoted.ToString(); } - /// - /// Kills a process that is being abandoned because the operation was cancelled, ignoring the races - /// where it has already exited or was never started. - /// - /// The process to kill. - private static void TryKill(Process process) - { - try - { - process.Kill(); - } - catch (Exception ex) when (ex is InvalidOperationException or System.ComponentModel.Win32Exception) - { - // The process exited between the wait timing out and this call, or could not be terminated. - } - } - - /// - /// Builds the result returned when an operation is cancelled. - /// - /// A failed describing the cancellation. - private static CommandResult Cancelled() => - new(-1, string.Empty, "Operation was cancelled."); - /// /// Reads one redirected stream to the end on a thread of its own. /// From d13f14702c40ee3ad3c9a7c483c3c19e140499ba Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 11:49:41 +0000 Subject: [PATCH 3/3] Make the trailing-backslash test case shell-independent A lone backslash at the end of an `sh -c` script is up to the shell: dash keeps it, but the bash behind macOS's /bin/sh drops it as a line continuation, so the macOS leg failed although the command reached the shell verbatim. The case now ends in an even run of backslashes, which every shell reads the same way and which still fails against the old quotes-only escaping. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01D7wytU6jsZ1cH3f6grCTz5 --- Essentials.Tests/CommandExecutorTests.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Essentials.Tests/CommandExecutorTests.cs b/Essentials.Tests/CommandExecutorTests.cs index dbd0a8f..ca3da2b 100644 --- a/Essentials.Tests/CommandExecutorTests.cs +++ b/Essentials.Tests/CommandExecutorTests.cs @@ -326,10 +326,15 @@ public void CommandExecutor_Sync_Captures_The_Same_Bytes_As_Async(ICommandExecut /// issue #54 the executor escaped only the quotes, and the backslashes the command already had combined /// with the inserted escapes, so the shell received a mangled command. /// + /// + /// The command that ends in backslashes ends in an even run of them. A lone trailing backslash is left to + /// the shell to interpret, and the shells disagree: dash keeps it, while the bash behind macOS's + /// /bin/sh drops it as a line continuation. + /// public static IEnumerable PosixCommandsWithBackslashes => [ ["echo \"say \\\"hi\\\"\"", "say \"hi\"\n"], - ["echo trailing\\", "trailing\\\n"], + ["printf '%s\\n' trailing\\\\\\\\", "trailing\\\\\n"], ["printf '%s' 'a\\\\\"b'", "a\\\\\"b"], ];