From 9d4960481799348266a7cf4bc32ea94c9a2d7826 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 10:34:22 +0000 Subject: [PATCH] Stop the admission probe decoding output it never reads, and guard the large non-UTF-8 diff [patch] The ls-remote admission probe only asks whether git succeeded, yet its output was decoded as strict UTF-8, so one branch name git allows but UTF-8 cannot read refused every caller of the repository. GitInvocation gains DiscardStandardOutput: the output is still drained, but thrown away undecoded, and standard error, which classifies a refusal, is kept. The probe sets it. The diff-tree stall itself no longer reproduces: git invocation now goes through ktsu.RunCommand 1.9.5, which kills the command and rethrows when a read fails (ktsu-dev/RunCommand#89). A regression test with a non-UTF-8 path ahead of 200 KB of output now guards that it stays reported as "not valid UTF-8" rather than a timeout. Fixes ktsu-dev/GitBranchStateCache#50 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016rdMXULeUT13t6FCwNfocx --- .../Admission/AdmissionGateTests.cs | 13 ++++ .../Git/GitRunnerTests.cs | 60 +++++++++++++++++++ .../Admission/AdmissionGate.cs | 4 ++ GitBranchStateCache/Git/GitInvocation.cs | 11 ++++ GitBranchStateCache/Git/GitRunner.cs | 21 +++++-- 5 files changed, 105 insertions(+), 4 deletions(-) diff --git a/GitBranchStateCache.Tests/Admission/AdmissionGateTests.cs b/GitBranchStateCache.Tests/Admission/AdmissionGateTests.cs index 3b4fa40..abdaa6b 100644 --- a/GitBranchStateCache.Tests/Admission/AdmissionGateTests.cs +++ b/GitBranchStateCache.Tests/Admission/AdmissionGateTests.cs @@ -48,6 +48,19 @@ public async Task AdmitAsync_WhenLsRemoteSucceeds_Admits() Assert.AreEqual("ls-remote", runner.Invocations.Single().Arguments[0]); } + [TestMethod] + public async Task AdmitAsync_DiscardsTheProbesOutput() + { + // The probe proves a credential by succeeding, and its output is never read. Decoding it would + // let one branch name that is not valid UTF-8 refuse every caller of the repository + // (ktsu-dev/GitBranchStateCache#50). + (AdmissionGate gate, FakeGitRunner runner, _) = Build(); + + await AdmitAsync(gate); + + Assert.IsTrue(runner.Invocations.Single().DiscardStandardOutput); + } + [TestMethod] public async Task AdmitAsync_PassesTheCredentialThroughTheEnvironmentAndNeverAnArgument() { diff --git a/GitBranchStateCache.Tests/Git/GitRunnerTests.cs b/GitBranchStateCache.Tests/Git/GitRunnerTests.cs index 37387ef..4571b42 100644 --- a/GitBranchStateCache.Tests/Git/GitRunnerTests.cs +++ b/GitBranchStateCache.Tests/Git/GitRunnerTests.cs @@ -237,6 +237,66 @@ public async Task RunAsync_OutputThatIsNotUtf8_IsReportedRatherThanReadAsEmptyOr Assert.Contains("not valid UTF-8", result.StandardError); } + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + public async Task RunAsync_OutputThatIsNotUtf8AndLargerThanThePipe_IsReportedPromptlyRatherThanAsATimeout() + { + // A diff-tree with one non-UTF-8 path among enough others to fill the pipe. Once the decoder + // refuses the bad byte, the rest still has to be drained or the process ended, or git blocks on + // the full pipe and the run is reported as a timeout instead (ktsu-dev/GitBranchStateCache#50). + Stopwatch elapsed = Stopwatch.StartNew(); + + GitResult result = await Build("/bin/sh").RunAsync( + new GitInvocation + { + Arguments = ["-c", "printf 'b\\377.uasset\\0'; head -c 204800 /dev/zero | tr '\\0' a"], + Timeout = TimeSpan.FromSeconds(20), + }, + CancellationToken.None); + + Assert.IsFalse(result.TimedOut, "The run stalled on a full pipe until it was killed."); + Assert.Contains("not valid UTF-8", result.StandardError); + Assert.IsLessThan(TimeSpan.FromSeconds(10), elapsed.Elapsed); + } + + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + public async Task RunAsync_DiscardingStandardOutput_SucceedsWhateverTheOutputHolds() + { + // The admission probe asks only whether ls-remote succeeded. A branch name git allows but + // UTF-8 cannot read must not turn a reachable repository into a refused one. + GitResult result = await Build("/bin/sh").RunAsync( + new GitInvocation + { + Arguments = ["-c", "printf 'deadbeef\\trefs/heads/caf\\351\\n'; head -c 204800 /dev/zero | tr '\\0' a"], + Timeout = TimeSpan.FromSeconds(20), + DiscardStandardOutput = true, + }, + CancellationToken.None); + + Assert.IsTrue(result.Succeeded, result.StandardError); + Assert.AreEqual(string.Empty, result.StandardOutput); + } + + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + public async Task RunAsync_DiscardingStandardOutput_StillReportsStandardError() + { + // What a refused probe says on standard error is what classifies it, so only standard output + // is thrown away. + GitResult result = await Build("/bin/sh").RunAsync( + new GitInvocation + { + Arguments = ["-c", "printf 'fatal: Authentication failed\\n' >&2; exit 128"], + Timeout = TimeSpan.FromSeconds(20), + DiscardStandardOutput = true, + }, + CancellationToken.None); + + Assert.IsFalse(result.Succeeded); + Assert.Contains("Authentication failed", result.StandardError); + } + private static string ReaderExecutable() => OnWindows ? "cmd.exe" : "/bin/sh"; private static string[] ReaderArguments() => OnWindows diff --git a/GitBranchStateCache/Admission/AdmissionGate.cs b/GitBranchStateCache/Admission/AdmissionGate.cs index f6cefdb..abe5a42 100644 --- a/GitBranchStateCache/Admission/AdmissionGate.cs +++ b/GitBranchStateCache/Admission/AdmissionGate.cs @@ -109,6 +109,10 @@ public async Task AdmitAsync( CredentialScope = upstreamBase, Authorization = authorization, Timeout = options.Value.ProbeTimeout, + + // Only whether the probe succeeded matters, so a branch name that is not valid UTF-8 + // cannot refuse a whole repository (ktsu-dev/GitBranchStateCache#50). + DiscardStandardOutput = true, }, cancellationToken).ConfigureAwait(false); diff --git a/GitBranchStateCache/Git/GitInvocation.cs b/GitBranchStateCache/Git/GitInvocation.cs index d3521ad..33451d2 100644 --- a/GitBranchStateCache/Git/GitInvocation.cs +++ b/GitBranchStateCache/Git/GitInvocation.cs @@ -39,6 +39,17 @@ public sealed class GitInvocation /// public string? Authorization { get; init; } + /// + /// Gets whether standard output is thrown away unread rather than decoded and returned. + /// + /// + /// For a command whose answer is its exit code alone. Its output is still drained, so the command + /// never blocks on a full pipe, but it is not held to strict UTF-8: a branch name git allows and + /// UTF-8 cannot read would otherwise fail a command whose output nobody looks at + /// (ktsu-dev/GitBranchStateCache#50). + /// + public bool DiscardStandardOutput { get; init; } + /// Gets how long the command may run before it is killed. public required TimeSpan Timeout { get; init; } } diff --git a/GitBranchStateCache/Git/GitRunner.cs b/GitBranchStateCache/Git/GitRunner.cs index 42bbad7..8f4cfce 100644 --- a/GitBranchStateCache/Git/GitRunner.cs +++ b/GitBranchStateCache/Git/GitRunner.cs @@ -50,6 +50,14 @@ public sealed class GitRunner(IOptions options) : IG encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); + /// + /// Decodes the output of a run whose standard output is discarded, where only standard error is + /// kept and it is only ever shown or searched for git's own ASCII messages. + /// + private static readonly Encoding LenientUtf8 = new UTF8Encoding( + encoderShouldEmitUTF8Identifier: false, + throwOnInvalidBytes: false); + /// /// /// Starting, reading and killing the process is 's @@ -67,10 +75,15 @@ public async Task RunAsync(GitInvocation invocation, CancellationToke StringBuilder standardOutput = new(); StringBuilder standardError = new(); - OutputHandler output = new( - chunk => standardOutput.Append(chunk), - chunk => standardError.Append(chunk), - StrictUtf8); + OutputHandler output = invocation.DiscardStandardOutput + ? new( + _ => { }, + chunk => standardError.Append(chunk), + LenientUtf8) + : new( + chunk => standardOutput.Append(chunk), + chunk => standardError.Append(chunk), + StrictUtf8); CommandOptions commandOptions = new() {