diff --git a/Directory.Packages.props b/Directory.Packages.props index 831b5a7..4aff80e 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -5,6 +5,7 @@ + diff --git a/GitBranchStateCache.Tests/Git/GitRunnerTests.cs b/GitBranchStateCache.Tests/Git/GitRunnerTests.cs index 53c2fbe..37387ef 100644 --- a/GitBranchStateCache.Tests/Git/GitRunnerTests.cs +++ b/GitBranchStateCache.Tests/Git/GitRunnerTests.cs @@ -2,6 +2,7 @@ namespace ktsu.GitBranchStateCache.Tests.Git; +using System.Collections; using System.Diagnostics; using System.Runtime.InteropServices; using ktsu.GitBranchStateCache.Configuration; @@ -144,35 +145,35 @@ private static string OutsideAnyRepository() } [TestMethod] - public void ApplyEnvironment_SetsTheFlagsThatKeepARunPredictable() + public void BuildEnvironment_SetsTheFlagsThatKeepARunPredictable() { - ProcessStartInfo startInfo = new(); - startInfo.Environment["GIT_DIR"] = "/somewhere/inherited"; - - GitRunner.ApplyEnvironment( - startInfo, + Dictionary environment = GitRunner.BuildEnvironment( new GitInvocation { Arguments = ["--version"], Timeout = TimeSpan.FromSeconds(1) }, - new GitBranchStateCacheOptions { MirrorRoot = TempRoot }); + new GitBranchStateCacheOptions { MirrorRoot = TempRoot }, + new Hashtable { ["GIT_DIR"] = "/somewhere/inherited", ["PATH"] = "/usr/bin" }); // GIT_NO_LAZY_FETCH turns a demand for filtered content into a visible error rather than an // enormous unplanned fetch, and no terminal prompt turns a missing credential into a refusal // rather than a process waiting on a terminal that is not there. - Assert.AreEqual("1", startInfo.Environment["GIT_NO_LAZY_FETCH"]); - Assert.AreEqual("0", startInfo.Environment["GIT_TERMINAL_PROMPT"]); - Assert.AreEqual("1", startInfo.Environment["GIT_CONFIG_NOSYSTEM"]); + Assert.AreEqual("1", environment["GIT_NO_LAZY_FETCH"]); + Assert.AreEqual("0", environment["GIT_TERMINAL_PROMPT"]); + Assert.AreEqual("1", environment["GIT_CONFIG_NOSYSTEM"]); + + // An inherited GIT_DIR would point every run at a repository nobody asked for. A null value in + // the overlay is what removes it from the child's environment. + Assert.IsTrue(environment.TryGetValue("GIT_DIR", out string? gitDir)); + Assert.IsNull(gitDir); - // An inherited GIT_DIR would point every run at a repository nobody asked for. - Assert.IsFalse(startInfo.Environment.ContainsKey("GIT_DIR")); + // Everything else is inherited untouched, so it is left out of the overlay. + Assert.IsFalse(environment.ContainsKey("PATH")); } [TestMethod] - public void ApplyEnvironment_WithACredential_ScopesItToTheUpstream() + public void BuildEnvironment_WithACredential_ScopesItToTheUpstream() { const string credential = "Basic dXNlcjp0b2tlbg=="; - ProcessStartInfo startInfo = new(); - GitRunner.ApplyEnvironment( - startInfo, + Dictionary environment = GitRunner.BuildEnvironment( new GitInvocation { Arguments = ["ls-remote", "https://github.com/studio/game.git"], @@ -180,30 +181,68 @@ public void ApplyEnvironment_WithACredential_ScopesItToTheUpstream() Authorization = credential, Timeout = TimeSpan.FromSeconds(1), }, - new GitBranchStateCacheOptions { MirrorRoot = TempRoot }); + new GitBranchStateCacheOptions { MirrorRoot = TempRoot }, + new Hashtable()); // Scoped to the upstream rather than set for all of http, because git matches this // configuration by URL prefix and a redirect leading off the forge would otherwise carry the // caller's credential with it. - Assert.AreEqual("2", startInfo.Environment["GIT_CONFIG_COUNT"]); - Assert.AreEqual("http.https://github.com/.extraHeader", startInfo.Environment["GIT_CONFIG_KEY_1"]); - Assert.AreEqual($"Authorization: {credential}", startInfo.Environment["GIT_CONFIG_VALUE_1"]); + Assert.AreEqual("2", environment["GIT_CONFIG_COUNT"]); + Assert.AreEqual("http.https://github.com/.extraHeader", environment["GIT_CONFIG_KEY_1"]); + Assert.AreEqual($"Authorization: {credential}", environment["GIT_CONFIG_VALUE_1"]); } [TestMethod] - public void ApplyEnvironment_WithoutACredential_SetsNoHeader() + public void BuildEnvironment_WithoutACredential_SetsNoHeader() { - ProcessStartInfo startInfo = new(); - - GitRunner.ApplyEnvironment( - startInfo, + Dictionary environment = GitRunner.BuildEnvironment( new GitInvocation { Arguments = ["--version"], Timeout = TimeSpan.FromSeconds(1) }, - new GitBranchStateCacheOptions { MirrorRoot = TempRoot }); + new GitBranchStateCacheOptions { MirrorRoot = TempRoot }, + new Hashtable()); + + Assert.AreEqual("1", environment["GIT_CONFIG_COUNT"]); + Assert.AreEqual("credential.helper", environment["GIT_CONFIG_KEY_0"]); + } - Assert.AreEqual("1", startInfo.Environment["GIT_CONFIG_COUNT"]); - Assert.AreEqual("credential.helper", startInfo.Environment["GIT_CONFIG_KEY_0"]); + [TestMethod] + public async Task RunAsync_ACommandThatReadsStandardInput_SeesEndOfStreamRatherThanWaiting() + { + // git is never fed anything, so a child that reads standard input has to find it closed. Left + // inherited, it would wait on whatever this service's own standard input is, which under a + // service manager can be a pipe nobody writes to, and the run would end only at its timeout. + GitResult result = await Build(ReaderExecutable()).RunAsync( + new GitInvocation { Arguments = ReaderArguments(), Timeout = TimeSpan.FromSeconds(20) }, + CancellationToken.None); + + Assert.IsFalse(result.TimedOut, "The command waited on standard input until it was killed."); + Assert.IsTrue(result.Succeeded, result.StandardError); + Assert.Contains("eof", result.StandardOutput); } + [TestMethod] + [OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)] + [DataRow("before\\n\\377\\376\\nafter\\n", DisplayName = "invalid bytes among valid text")] + [DataRow("\\377\\376", DisplayName = "invalid bytes alone")] + public async Task RunAsync_OutputThatIsNotUtf8_IsReportedRatherThanReadAsEmptyOrReplaced(string printfFormat) + { + // An undecodable branch or path name must fail loudly. Read leniently it becomes a name that + // matches nothing, and read as nothing it becomes "no branches", both of which look like + // answers. POSIX only, because it needs a shell that can write raw bytes. + GitResult result = await Build("/bin/sh").RunAsync( + new GitInvocation { Arguments = ["-c", $"printf '{printfFormat}'"], Timeout = TimeSpan.FromSeconds(20) }, + CancellationToken.None); + + Assert.IsFalse(result.Succeeded); + Assert.IsFalse(result.TimedOut); + Assert.Contains("not valid UTF-8", result.StandardError); + } + + private static string ReaderExecutable() => OnWindows ? "cmd.exe" : "/bin/sh"; + + private static string[] ReaderArguments() => OnWindows + ? ["/c", "set /p line= & echo eof"] + : ["-c", "if read line; then echo \"read:$line\"; else echo eof; fi"]; + [TestMethod] public async Task RunAsync_ExceedingItsTimeout_ReportsTimedOutAndKillsTheTree() { diff --git a/GitBranchStateCache/Git/GitRunner.cs b/GitBranchStateCache/Git/GitRunner.cs index 7ab4ee4..42bbad7 100644 --- a/GitBranchStateCache/Git/GitRunner.cs +++ b/GitBranchStateCache/Git/GitRunner.cs @@ -2,9 +2,11 @@ namespace ktsu.GitBranchStateCache.Git; -using System.Diagnostics; using System.Text; using ktsu.GitBranchStateCache.Configuration; +using ktsu.RunCommand; +using ktsu.Semantics.Paths; +using ktsu.Semantics.Strings; using Microsoft.Extensions.Options; /// @@ -36,16 +38,6 @@ namespace ktsu.GitBranchStateCache.Git; /// The configured options. public sealed class GitRunner(IOptions options) : IGitRunner { - /// - /// How long the output streams are drained for after a kill before they are given up on. - /// - /// - /// A killed process closes its pipes, so this normally completes immediately. It is bounded - /// because a grandchild that inherited the pipe and outlived the kill would otherwise hold the - /// read open, and this path already runs on a request that is being abandoned. - /// - private static readonly TimeSpan DrainTimeout = TimeSpan.FromSeconds(5); - /// /// Decodes git output strictly, so an undecodable path fails loudly instead of being replaced. /// @@ -59,19 +51,40 @@ public sealed class GitRunner(IOptions options) : IG throwOnInvalidBytes: true); /// + /// + /// Starting, reading and killing the process is 's + /// job: it kills the whole tree on cancellation, because git delegates transport to a helper child + /// that would otherwise be left holding a connection and a pipe, and it stops reading once the + /// process is gone rather than waiting on a pipe a surviving grandchild may hold open. What stays + /// here is what is particular to git: the environment, and telling this service's own timeout + /// apart from the caller giving up. + /// public async Task RunAsync(GitInvocation invocation, CancellationToken cancellationToken) { Ensure.NotNull(invocation); - using Process process = new() { StartInfo = BuildStartInfo(invocation) }; - process.Start(); + GitBranchStateCacheOptions settings = options.Value; - // git is never fed anything, and a child holding an open stdin it is waiting on is a hang - // rather than an error. - process.StandardInput.Close(); + StringBuilder standardOutput = new(); + StringBuilder standardError = new(); + OutputHandler output = new( + chunk => standardOutput.Append(chunk), + chunk => standardError.Append(chunk), + StrictUtf8); - Task standardOutput = process.StandardOutput.ReadToEndAsync(CancellationToken.None); - Task standardError = process.StandardError.ReadToEndAsync(CancellationToken.None); + CommandOptions commandOptions = new() + { + // Resolved here because the option only takes an absolute path, and a relative one has always + // meant relative to this process's current directory. + WorkingDirectory = invocation.WorkingDirectory is null + ? null + : Path.GetFullPath(invocation.WorkingDirectory).As(), + EnvironmentVariables = BuildEnvironment(invocation, settings, Environment.GetEnvironmentVariables()), + + // git is never fed anything, and a child holding an open stdin it is waiting on is a hang + // rather than an error. + StandardInput = StandardInputMode.Closed, + }; using CancellationTokenSource timeout = new(invocation.Timeout); using CancellationTokenSource linked = @@ -79,29 +92,24 @@ public async Task RunAsync(GitInvocation invocation, CancellationToke try { - await process.WaitForExitAsync(linked.Token).ConfigureAwait(false); + int exitCode = await RunCommand.ExecuteAsync( + settings.GitExecutable, + invocation.Arguments, + output, + commandOptions, + linked.Token).ConfigureAwait(false); + + return new GitResult(exitCode, standardOutput.ToString(), standardError.ToString(), TimedOut: false); } catch (OperationCanceledException) { - Kill(process); - await DrainAsync(standardOutput, standardError).ConfigureAwait(false); - // A timeout is this service's own decision and has an answer to report. A cancellation is // the caller giving up, and there is nobody left to report anything to. cancellationToken.ThrowIfCancellationRequested(); return new GitResult(-1, string.Empty, "The git command exceeded its timeout.", TimedOut: true); } - - try - { - return new GitResult( - process.ExitCode, - await standardOutput.ConfigureAwait(false), - await standardError.ConfigureAwait(false), - TimedOut: false); - } - catch (DecoderFallbackException) + catch (Exception failure) when (IsDecodeFailure(failure)) { return new GitResult( -1, @@ -112,118 +120,67 @@ await standardError.ConfigureAwait(false), } /// - /// Kills the process and everything it started. + /// Whether a failure is the strict encoding refusing git's output. /// /// - /// The tree, not just the process: git delegates transport to a helper child, and killing only the - /// parent leaves that helper holding a connection and a pipe. Leaking those is the most likely - /// operational failure of a service shaped like this. + /// The output is read on background tasks, so the decoder's exception can arrive wrapped. /// - private static void Kill(Process process) + private static bool IsDecodeFailure(Exception failure) => failure switch { - try - { - if (!process.HasExited) - { - process.Kill(entireProcessTree: true); - } - } - catch (InvalidOperationException) - { - // The process exited between the check and the kill. Nothing left to do. - } - catch (NotSupportedException) - { - // Killing a tree is unsupported on this platform, and the process is already gone or will - // be reaped when its handle is disposed. - } - } - - private static async Task DrainAsync(Task standardOutput, Task standardError) - { - try - { - await Task.WhenAll(standardOutput, standardError).WaitAsync(DrainTimeout).ConfigureAwait(false); - } - catch (Exception failure) when (failure is TimeoutException or DecoderFallbackException) - { - // The output of a killed command is not reported, so failing to read it changes nothing. - } - } - - private ProcessStartInfo BuildStartInfo(GitInvocation invocation) - { - GitBranchStateCacheOptions settings = options.Value; - - ProcessStartInfo startInfo = new() - { - FileName = settings.GitExecutable, - UseShellExecute = false, - CreateNoWindow = true, - RedirectStandardInput = true, - RedirectStandardOutput = true, - RedirectStandardError = true, - StandardOutputEncoding = StrictUtf8, - StandardErrorEncoding = StrictUtf8, - }; - - if (invocation.WorkingDirectory is not null) - { - startInfo.WorkingDirectory = invocation.WorkingDirectory; - } - - foreach (string argument in invocation.Arguments) - { - startInfo.ArgumentList.Add(argument); - } - - ApplyEnvironment(startInfo, invocation, settings); - return startInfo; - } + DecoderFallbackException => true, + AggregateException aggregate => aggregate.Flatten().InnerExceptions.Any(IsDecodeFailure), + _ => failure.InnerException is not null && IsDecodeFailure(failure.InnerException), + }; /// - /// Applies the environment every run gets, including the caller's credential. + /// Builds the environment every run gets, including the caller's credential, as an overlay on the + /// environment the child would otherwise inherit. /// /// /// Internal so the tests can assert on the environment directly. What it puts where is the whole /// of this class's security posture, and asserting it through the behaviour of a child process /// would only ever cover the parts a child happens to report. /// - /// The process being prepared. /// What is being run. /// The configured options. - internal static void ApplyEnvironment( - ProcessStartInfo startInfo, + /// The environment the child would otherwise inherit. + /// The variables to set, with a null value for each inherited variable to remove. + internal static Dictionary BuildEnvironment( GitInvocation invocation, - GitBranchStateCacheOptions settings) + GitBranchStateCacheOptions settings, + System.Collections.IDictionary inherited) { - foreach (string inherited in startInfo.Environment.Keys - .Where(key => key.StartsWith("GIT_", StringComparison.OrdinalIgnoreCase)) - .ToArray()) + Ensure.NotNull(inherited); + + Dictionary environment = new(StringComparer.OrdinalIgnoreCase); + + foreach (string name in inherited.Keys.OfType() + .Where(key => key.StartsWith("GIT_", StringComparison.OrdinalIgnoreCase))) { - startInfo.Environment.Remove(inherited); + environment[name] = null; } - startInfo.Environment["GIT_CONFIG_NOSYSTEM"] = "1"; - startInfo.Environment["GIT_CONFIG_GLOBAL"] = GlobalConfigPath(settings); + environment["GIT_CONFIG_NOSYSTEM"] = "1"; + environment["GIT_CONFIG_GLOBAL"] = GlobalConfigPath(settings); // No prompting, ever. Without this a missing or refused credential turns a request into a // process waiting on a terminal that is not there, which presents as a hang rather than a 401. - startInfo.Environment["GIT_TERMINAL_PROMPT"] = "0"; - startInfo.Environment["GCM_INTERACTIVE"] = "never"; + environment["GIT_TERMINAL_PROMPT"] = "0"; + environment["GCM_INTERACTIVE"] = "never"; // The mirrors are blobless, and nothing this service runs reads file content. If some future // operation does, this turns it into a visible error during testing rather than an enormous // unplanned fetch in production. - startInfo.Environment["GIT_NO_LAZY_FETCH"] = "1"; + environment["GIT_NO_LAZY_FETCH"] = "1"; - ApplyConfigEnvironment(startInfo, invocation); + ApplyConfigEnvironment(environment, invocation); + return environment; } /// /// Hands git its per-run configuration, including the caller's credential, through the environment. /// - private static void ApplyConfigEnvironment(ProcessStartInfo startInfo, GitInvocation invocation) + private static void ApplyConfigEnvironment(Dictionary environment, GitInvocation invocation) { List> entries = [ @@ -242,12 +199,12 @@ private static void ApplyConfigEnvironment(ProcessStartInfo startInfo, GitInvoca $"Authorization: {authorization}")); } - startInfo.Environment["GIT_CONFIG_COUNT"] = entries.Count.ToString(System.Globalization.CultureInfo.InvariantCulture); + environment["GIT_CONFIG_COUNT"] = entries.Count.ToString(System.Globalization.CultureInfo.InvariantCulture); for (int index = 0; index < entries.Count; index++) { - startInfo.Environment[$"GIT_CONFIG_KEY_{index}"] = entries[index].Key; - startInfo.Environment[$"GIT_CONFIG_VALUE_{index}"] = entries[index].Value; + environment[$"GIT_CONFIG_KEY_{index}"] = entries[index].Key; + environment[$"GIT_CONFIG_VALUE_{index}"] = entries[index].Value; } } diff --git a/GitBranchStateCache/GitBranchStateCache.csproj b/GitBranchStateCache/GitBranchStateCache.csproj index 9c3a722..409469b 100644 --- a/GitBranchStateCache/GitBranchStateCache.csproj +++ b/GitBranchStateCache/GitBranchStateCache.csproj @@ -19,6 +19,7 @@ +