From 2d8880354257f31a5ea1a36f9cc9d8c03f2f5e55 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 5 Aug 2026 13:28:02 -0700 Subject: [PATCH 1/6] Fail blob hydration cleanly on a malformed (NUL-byte) placeholder SHA When a user process reads a virtualized placeholder whose stored content-id is corrupt - specifically 40 NUL bytes instead of a hex SHA - GVFS builds a loose-object path from it and Path.Combine throws System.ArgumentException ("Illegal characters in path"). ArgumentException is not in RetryWrapper.IsHandlableException, so it bypasses both the retry logic and the download fallback in GVFSGitObjects.TryCopyBlobContentStream and propagates to the virtualizer's outer catch, which returns FileNotAvailable to ProjFS. The placeholder can never hydrate, so the failing read repeats forever - a retry storm. This is the #1 blob-hydration failure cause on the LKG field build 1.0.26014.1 (38 machines; ~61 machines / ~6.2K events across 30d; one machine emitted ~2.49M error events). This is a corrupt content-id, NOT GVFSConstants.AllZeroSha: AllZeroSha is 40 ASCII '0' characters, which yields directory "00" and does not throw. Reject a malformed SHA before it is turned into a path: - GitRepo.GetLooseBlobState returns LooseBlobState.Invalid (a clean, non-retryable miss) for a SHA that is not 40 hex characters, so Path.Combine can never throw here again. - GitRepo.LooseObjectExists guards the same Path.Combine. - GVFSGitObjects.TryCopyBlobContentStream short-circuits a malformed SHA before the retry loop, so a bogus SHA never triggers a doomed 404 download or a retry storm. - SHA1Util.IsValidShaFormat is now null-safe; SHA1Util.ToLoggableShaString renders the bad value with non-hex characters escaped so telemetry stays greppable and free of control characters. - WindowsFileSystemVirtualizer routes the request's logged sha through ToLoggableShaString, so a malformed content-id can no longer enter telemetry with raw NUL/control bytes at the terminal hydration-failure error either (a no-op for a valid hex SHA). All three guard sites emit the same greppable Warning event (*_MalformedBlobSha) at Warning level with no unhandled exception. Per an existing decision this case stays telemetry category "Unexpected"; no new BlobHydrationFailureCategory is added. Stacked on #2071 (tyrielv/split-hydration-enum-telemetry): this branch is rebased onto it, so #2071's out BlobHydrationFailureCategory parameter is honored - the malformed-SHA short-circuit sets failureCategory = Unexpected, so the virtualizer's terminal telemetry tags the case exactly as before (it no longer reaches the outer catch because it no longer throws). This PR must NOT merge before #2071; after #2071 lands, rebase onto master. Unit tests assert that a 40-NUL-byte SHA, a 40-char SHA with an embedded path-illegal character, and other malformed SHAs return false from both GitRepo.TryCopyBlobContentStream and GVFSGitObjects.TryCopyBlobContentStream with no ArgumentException (Assert.DoesNotThrow), that no download/retry is attempted, and that the out category is Unexpected. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GVFSGitObjects.cs | 20 +++++ GVFS/GVFS.Common/Git/GitRepo.cs | 32 +++++++ GVFS/GVFS.Common/SHA1Util.cs | 32 ++++++- .../WindowsFileSystemVirtualizer.cs | 2 +- GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs | 25 ++++++ .../GVFS.UnitTests/Git/GVFSGitObjectsTests.cs | 86 +++++++++++++++++++ GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs | 7 ++ 7 files changed, 202 insertions(+), 2 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs index b232e7b74c..6630678261 100644 --- a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs +++ b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs @@ -67,6 +67,26 @@ public virtual bool TryCopyBlobContentStream( Action writeAction, out BlobHydrationFailureCategory failureCategory) { + // Short-circuit a malformed SHA (for example a corrupt placeholder's all-NUL + // content-id) before the retry loop. GitRepo already rejects it as a clean miss, + // but a bogus SHA can never be downloaded either (the server returns 404), so + // attempting it would only produce doomed download retries. Because the read is + // never satisfied, the caller re-requests it endlessly, which turns one corrupt + // placeholder into an unbounded error/retry storm. Fail fast and cheap instead. + // The cause stays categorized as Unexpected (no dedicated category); the caller + // tags its terminal telemetry from failureCategory below. + if (!SHA1Util.IsValidShaFormat(sha)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(sha)); + metadata.Add("RequestSource", requestSource.ToString()); + metadata.Add(TracingConstants.MessageKey.WarningMessage, "TryCopyBlobContentStream: Refusing to hydrate blob with malformed SHA"); + this.Tracer.RelatedEvent(EventLevel.Warning, nameof(this.TryCopyBlobContentStream) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + + failureCategory = BlobHydrationFailureCategory.Unexpected; + return false; + } + // Track the outcome of the most recent attempt so that the terminal failure // telemetry can attribute the failure to a cause (network vs. object-missing vs. // local copy) that is otherwise collapsed into the bool return value below. The diff --git a/GVFS/GVFS.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index 302b0e13e0..1ca4d97d01 100644 --- a/GVFS/GVFS.Common/Git/GitRepo.cs +++ b/GVFS/GVFS.Common/Git/GitRepo.cs @@ -113,6 +113,19 @@ public virtual bool CommitAndRootTreeExists(string commitSha, out string rootTre /// public virtual bool LooseObjectExists(string sha) { + // Guard against a malformed SHA (for example a corrupt placeholder's all-NUL + // content-id) so Path.Combine cannot throw ArgumentException below. Emit the same + // greppable Warning as the other malformed-SHA guards so a silent "does not exist" + // answer is still diagnosable in telemetry. + if (!SHA1Util.IsValidShaFormat(sha)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(sha)); + metadata.Add(TracingConstants.MessageKey.WarningMessage, nameof(this.LooseObjectExists) + ": Malformed SHA cannot exist as a loose object"); + this.tracer.RelatedEvent(EventLevel.Warning, nameof(this.LooseObjectExists) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + return false; + } + if (GVFSPlatform.Instance.Constants.CaseSensitiveFileSystem) { sha = sha.ToLower(); @@ -343,6 +356,25 @@ private LooseBlobState GetLooseBlobStateAtPath(string blobPath, Action writeAction, out long size) { + // A corrupt placeholder can carry a malformed content-id (for example 40 NUL + // bytes instead of a hex SHA). Such a value holds characters that are illegal + // in a file path, so Path.Combine below throws ArgumentException ("Illegal + // characters in path"). ArgumentException is not handled by RetryWrapper, so it + // bypasses both the retry logic and the download fallback and fails the + // hydration permanently. Reject the malformed SHA up front and report it as an + // invalid loose object, which the callers treat as a clean, non-retryable miss. + if (!SHA1Util.IsValidShaFormat(blobSha)) + { + size = -1; + + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(blobSha)); + metadata.Add(TracingConstants.MessageKey.WarningMessage, nameof(this.GetLooseBlobState) + ": Refusing to build loose object path from malformed blob SHA"); + this.tracer.RelatedEvent(EventLevel.Warning, nameof(this.GetLooseBlobState) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + + return LooseBlobState.Invalid; + } + // Ensure SHA path is lowercase for case-sensitive filesystems if (GVFSPlatform.Instance.Constants.CaseSensitiveFileSystem) { diff --git a/GVFS/GVFS.Common/SHA1Util.cs b/GVFS/GVFS.Common/SHA1Util.cs index 0fc20019de..01a2ba230c 100644 --- a/GVFS/GVFS.Common/SHA1Util.cs +++ b/GVFS/GVFS.Common/SHA1Util.cs @@ -9,7 +9,37 @@ public static class SHA1Util { public static bool IsValidShaFormat(string sha) { - return sha.Length == 40 && sha.All(c => Uri.IsHexDigit(c)); + return sha != null && sha.Length == 40 && sha.All(c => Uri.IsHexDigit(c)); + } + + /// + /// Returns a log-safe rendering of a value that was expected to be a + /// 40-character hex SHA but is not. Non-hex characters (for example the + /// NUL bytes of a corrupt placeholder content-id) are escaped as \uXXXX + /// so the value stays greppable in telemetry and carries no control + /// characters. + /// + public static string ToLoggableShaString(string sha) + { + if (sha == null) + { + return "(null)"; + } + + StringBuilder builder = new StringBuilder(sha.Length); + foreach (char c in sha) + { + if (Uri.IsHexDigit(c)) + { + builder.Append(c); + } + else + { + builder.AppendFormat("\\u{0:x4}", (int)c); + } + } + + return builder.ToString(); } public static string SHA1HashStringForUTF8String(string s) diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs index 7a6bad6f24..c99a602056 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs @@ -733,7 +733,7 @@ public HResult GetFileDataCallback( metadata.Add("streamGuid", streamGuid); metadata.Add("triggeringProcessId", triggeringProcessId); metadata.Add("triggeringProcessImageFileName", triggeringProcessImageFileName); - metadata.Add("sha", sha); + metadata.Add("sha", SHA1Util.ToLoggableShaString(sha)); metadata.Add("placeholderVersion", placeholderVersion); metadata.Add("commandId", commandId); diff --git a/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs b/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs index 90127fb996..3d868bcf70 100644 --- a/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs +++ b/GVFS/GVFS.UnitTests/Common/SHA1UtilTests.cs @@ -1,6 +1,7 @@ using GVFS.Common; using GVFS.Tests.Should; using NUnit.Framework; +using System.Linq; using System.Text; namespace GVFS.UnitTests.Common @@ -31,6 +32,30 @@ public void IsValidFullSHAIsFalseForEmptyString() SHA1Util.IsValidShaFormat(string.Empty).ShouldEqual(false); } + [TestCase] + public void IsValidShaFormatIsFalseForNull() + { + SHA1Util.IsValidShaFormat(null).ShouldEqual(false); + } + + [TestCase] + public void ToLoggableShaStringEscapesNonHexCharacters() + { + SHA1Util.ToLoggableShaString(null).ShouldEqual("(null)"); + SHA1Util.ToLoggableShaString(new string('\0', 3)).ShouldEqual("\\u0000\\u0000\\u0000"); + SHA1Util.ToLoggableShaString("abc\0").ShouldEqual("abc\\u0000"); + SHA1Util.ToLoggableShaString("abcDEF123").ShouldEqual("abcDEF123"); + + // Control characters and non-ASCII / high code points must be escaped and padded to 4 hex digits. + SHA1Util.ToLoggableShaString("a\tb\n").ShouldEqual("a\\u0009b\\u000a"); + SHA1Util.ToLoggableShaString("\u00e9\u1234").ShouldEqual("\\u00e9\\u1234"); + + // The realistic corrupt-content-id shape: a full 40-char value that is partly valid + // hex and partly NUL, rendered with the hex kept and the NULs escaped. + SHA1Util.ToLoggableShaString(new string('a', 20) + new string('\0', 20)) + .ShouldEqual(new string('a', 20) + string.Concat(Enumerable.Repeat("\\u0000", 20))); + } + [TestCase] public void IsValidFullSHAIsFalseForHexStringsNot40Chars() { diff --git a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs index 205d2b4de6..e50944e907 100644 --- a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs @@ -330,6 +330,36 @@ public void FailsNullBytePackDownloads() gitObjects => gitObjects.TryDownloadCommit("object0")); } + [TestCase] + public void TryCopyBlobContentStreamFailsCleanlyForCorruptAllNullByteSha() + { + // A corrupt placeholder can present a content-id of 40 NUL bytes instead of a + // hex SHA. Those characters are illegal in a file path, so building a loose + // object path from them used to throw an unhandled ArgumentException that + // bypassed retry and download fallback and permanently failed (and retry-stormed) + // the hydration. The read must now fail cleanly with no exception. + this.AssertMalformedShaHydrationFailsCleanly(new string('\0', 40)); + } + + [TestCase] + public void TryCopyBlobContentStreamFailsCleanlyForOtherMalformedShas() + { + this.AssertMalformedShaHydrationFailsCleanly(string.Empty); + this.AssertMalformedShaHydrationFailsCleanly("0123456789"); + this.AssertMalformedShaHydrationFailsCleanly(new string('0', 39)); + this.AssertMalformedShaHydrationFailsCleanly("000000000000000000000000000000000000000g"); + + // 40 chars long but with a NUL embedded among hex digits — the realistic + // corrupt-content-id shape that actually reproduces the original "Illegal + // characters in path" ArgumentException (length passes, hex check fails). + this.AssertMalformedShaHydrationFailsCleanly(new string('0', 20) + "\0" + new string('0', 19)); + + // 40 chars long with an embedded backslash. Unlike NUL this would NOT have thrown + // pre-fix (backslash is a legal path separator), but it is still non-hex, so the + // guard must reject it as a clean miss rather than probe a bogus path. + this.AssertMalformedShaHydrationFailsCleanly(new string('a', 20) + "\\" + new string('a', 19)); + } + [TestCase] public void CoalescesMultipleConcurrentRequestsForSameObject() { @@ -751,6 +781,62 @@ private void AssertRetryableExceptionOnDownload( } } + private void AssertMalformedShaHydrationFailsCleanly(string malformedSha) + { + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => + { + Assert.Fail("A malformed SHA must never be turned into a filesystem path"); + return false; + }; + + MockTracer tracer = new MockTracer(); + GVFSEnlistment enlistment = new GVFSEnlistment(TestEnlistmentRoot, "https://fakeRepoUrl", "fakeGitBinPath", authentication: null); + enlistment.InitializeCachePathsFromKey(TestLocalCacheRoot, TestObjectRoot); + GitRepo repo = new GitRepo(tracer, enlistment, fileSystem, () => new MockLibGit2Repo(tracer)); + GVFSContext context = new GVFSContext(tracer, fileSystem, repo, enlistment); + GVFSGitObjects gitObjects = new UnsafeGVFSGitObjects(context, new MockHttpGitObjects()); + + // GitRepo layer: must not throw ArgumentException ("Illegal characters in path"), + // and must report a clean miss. + bool repoResult = true; + Assert.DoesNotThrow( + () => repoResult = repo.TryCopyBlobContentStream( + malformedSha, + (stream, length) => Assert.Fail("Should not copy any content for a malformed SHA")), + "GitRepo.TryCopyBlobContentStream must not throw for a malformed SHA"); + repoResult.ShouldEqual(false); + + // GVFSGitObjects layer: must fail fast with no throw. The out category stays + // Unexpected (a malformed SHA gets no dedicated telemetry category). + bool copied = true; + GVFSGitObjects.BlobHydrationFailureCategory failureCategory = GVFSGitObjects.BlobHydrationFailureCategory.None; + Assert.DoesNotThrow( + () => copied = gitObjects.TryCopyBlobContentStream( + malformedSha, + new CancellationToken(), + GVFSGitObjects.RequestSource.FileStreamCallback, + (stream, length) => Assert.Fail("Should not copy any content for a malformed SHA"), + out failureCategory), + "GVFSGitObjects.TryCopyBlobContentStream must not throw for a malformed SHA"); + copied.ShouldEqual(false); + failureCategory.ShouldEqual(GVFSGitObjects.BlobHydrationFailureCategory.Unexpected); + + // The short-circuit must happen BEFORE the retry/download loop: a malformed SHA + // must never reach a server download. The retrier logs "Failed to provide blob + // contents" on every attempt, so its absence proves no download/retry ran. + bool anyDownloadAttemptLogged = tracer.RelatedErrorEvents + .Concat(tracer.RelatedWarningEvents) + .Any(e => e.Contains("Failed to provide blob contents")); + anyDownloadAttemptLogged.ShouldEqual(false); + + // The corrupt-placeholder read must stay diagnosable: both guard layers emit their + // distinct greppable *_MalformedBlobSha event. Assert the emission so a regression + // that silently dropped the warning would fail here. + tracer.RelatedEventNames.ShouldContain(e => e == "GetLooseBlobState_MalformedBlobSha"); + tracer.RelatedEventNames.ShouldContain(e => e == "TryCopyBlobContentStream_MalformedBlobSha"); + } + private GVFSGitObjects CreateTestableGVFSGitObjects(GitObjectsHttpRequestor httpObjects, MockFileSystemWithCallbacks fileSystem) { return this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out _); diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index c04be42048..d933584e94 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs @@ -16,6 +16,7 @@ public MockTracer() this.RelatedInfoEvents = new List(); this.RelatedWarningEvents = new List(); this.RelatedErrorEvents = new List(); + this.RelatedEventNames = new List(); } public MockTracer StartActivityTracer { get; private set; } @@ -25,6 +26,10 @@ public MockTracer() public List RelatedWarningEvents { get; } public List RelatedErrorEvents { get; } + // Names of events reported via RelatedEvent (which, unlike RelatedInfo/Warning/Error, + // do not otherwise get recorded). Lets tests assert a specific diagnostic event fired. + public List RelatedEventNames { get; } + public void WaitForRelatedEvent() { this.waitEvent.WaitOne(); @@ -32,6 +37,7 @@ public void WaitForRelatedEvent() public void RelatedEvent(EventLevel error, string eventName, EventMetadata metadata) { + this.RelatedEventNames.Add(eventName); if (eventName == this.WaitRelatedEventName) { this.waitEvent.Set(); @@ -40,6 +46,7 @@ public void RelatedEvent(EventLevel error, string eventName, EventMetadata metad public void RelatedEvent(EventLevel error, string eventName, EventMetadata metadata, Keywords keyword) { + this.RelatedEventNames.Add(eventName); if (eventName == this.WaitRelatedEventName) { this.waitEvent.Set(); From 8106a0c0074ac3865eea9c07a26fb00d6b069cfa Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 11 Aug 2026 20:17:39 +0200 Subject: [PATCH 2/6] Update default Microsoft Git version to v2.55.0.vfs.0.8 --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index e550568da4..d0b4a4509a 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -24,7 +24,7 @@ permissions: checks: read env: - GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.55.0.vfs.0.6' }} + GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.55.0.vfs.0.8' }} jobs: validate: From 64b0dbe82e41965b58aa5548056edf9cd511397c Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 13 Aug 2026 09:40:42 -0700 Subject: [PATCH 3/6] Record HTTP status code on blob-hydration failure telemetry When an on-demand loose-blob download fails, GVFS emits a terminal telemetry error with a BlobHydrationFailureCategory. The DownloadFailed category is the transient or unclassified bucket. It collapses genuine auth failures (401, 400, 302) and transient failures (timeout 408, 5xx, pool-exhaustion 503) into one value. The HTTP status of the failing download is known in the code, but it only reaches the on-box log, not shipped telemetry. So telemetry cannot tell a real auth failure apart from a transient one. Carry the HTTP status of the last download attempt to the terminal failure event through an internal DownloadAttemptResult type. Add HttpStatusCode and HttpStatusName to the event metadata only when the failure is attributable to the download itself (DownloadFailed or ObjectNotOnServer), so an earlier attempt's status cannot attach to a later local-IO or copy failure. The public TryDownloadAndSaveObject return type does not change. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GVFSGitObjects.cs | 87 ++++++++--- .../GVFS.UnitTests/Git/GVFSGitObjectsTests.cs | 146 +++++++++++++++++- 2 files changed, 208 insertions(+), 25 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs index b232e7b74c..f8a314b2e9 100644 --- a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs +++ b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs @@ -1,4 +1,4 @@ -using GVFS.Common.Http; +using GVFS.Common.Http; using GVFS.Common.Tracing; using System; using System.Collections.Concurrent; @@ -15,14 +15,14 @@ public class GVFSGitObjects : GitObjects private static readonly TimeSpan NegativeCacheTTL = TimeSpan.FromSeconds(30); private ConcurrentDictionary objectNegativeCache; - internal ConcurrentDictionary> inflightDownloads; + internal ConcurrentDictionary> inflightDownloads; public GVFSGitObjects(GVFSContext context, GitObjectsHttpRequestor objectRequestor) : base(context.Tracer, context.Enlistment, objectRequestor, context.FileSystem) { this.Context = context; this.objectNegativeCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); - this.inflightDownloads = new ConcurrentDictionary>(StringComparer.OrdinalIgnoreCase); + this.inflightDownloads = new ConcurrentDictionary>(StringComparer.OrdinalIgnoreCase); } public enum RequestSource @@ -58,6 +58,28 @@ public enum BlobHydrationFailureCategory Unexpected, // Unclassified exception. } + /// + /// Carries the outcome of an object download together with the HTTP status of the last + /// download attempt. The public enum only records + /// success/not-found/error, which collapses genuine auth failures (401/400/302) and + /// transient failures (408/5xx/pool-exhaustion 503) into a single "error" outcome. The + /// status is retained here so the terminal blob-hydration telemetry can tell them apart. + /// + internal class DownloadAttemptResult + { + public DownloadAttemptResult(DownloadAndSaveObjectResult result, HttpStatusCode? httpStatusCode) + { + this.Result = result; + this.HttpStatusCode = httpStatusCode; + } + + public DownloadAndSaveObjectResult Result { get; } + + // The HTTP status of the last download attempt, or null when no HTTP response was + // received (for example an exhausted retry that ended in an exception). + public HttpStatusCode? HttpStatusCode { get; } + } + protected GVFSContext Context { get; private set; } public virtual bool TryCopyBlobContentStream( @@ -72,7 +94,7 @@ public virtual bool TryCopyBlobContentStream( // local copy) that is otherwise collapsed into the bool return value below. The // final category is also surfaced via the out parameter so the caller can tag its // own terminal telemetry with the same cause. - DownloadAndSaveObjectResult lastDownloadResult = DownloadAndSaveObjectResult.Error; + DownloadAttemptResult lastDownloadResult = null; bool downloadSucceededButCopyFailed = false; BlobHydrationFailureCategory capturedCategory = BlobHydrationFailureCategory.None; @@ -109,7 +131,7 @@ public virtual bool TryCopyBlobContentStream( { category = BlobHydrationFailureCategory.LocalCopyFailed; } - else if (lastDownloadResult == DownloadAndSaveObjectResult.ObjectNotOnServer) + else if (lastDownloadResult?.Result == DownloadAndSaveObjectResult.ObjectNotOnServer) { category = BlobHydrationFailureCategory.ObjectNotOnServer; } @@ -124,6 +146,22 @@ public virtual bool TryCopyBlobContentStream( capturedCategory = category; metadata.Add(nameof(BlobHydrationFailureCategory), category.ToString()); + // Surface the HTTP status of the last download attempt so telemetry can tell a + // genuine auth failure (401/400/302) apart from a transient one (408/5xx/503), + // both of which otherwise land in the DownloadFailed bucket. Only attach it when + // the failure is attributable to the download itself (DownloadFailed or + // ObjectNotOnServer). On the exception (LocalIO/NetworkUnavailable) and + // LocalCopyFailed paths lastDownloadResult can hold a status captured on an + // earlier attempt, so the status would be stale and misattribute the failure. + bool statusIsAttributable = + category == BlobHydrationFailureCategory.DownloadFailed || + category == BlobHydrationFailureCategory.ObjectNotOnServer; + if (statusIsAttributable && lastDownloadResult?.HttpStatusCode != null) + { + metadata.Add("HttpStatusCode", (int)lastDownloadResult.HttpStatusCode.Value); + metadata.Add("HttpStatusName", lastDownloadResult.HttpStatusCode.Value.ToString()); + } + string message = "TryCopyBlobContentStream: Failed to provide blob contents"; if (errorArgs.WillRetry) { @@ -149,7 +187,7 @@ public virtual bool TryCopyBlobContentStream( // Pass in false for retryOnFailure because the retrier in this method manages multiple attempts lastDownloadResult = this.TryDownloadAndSaveObject(sha, cancellationToken, requestSource, retryOnFailure: false); - if (lastDownloadResult == DownloadAndSaveObjectResult.Success) + if (lastDownloadResult.Result == DownloadAndSaveObjectResult.Success) { if (this.Context.Repository.TryCopyBlobContentStream(sha, writeAction)) { @@ -169,7 +207,7 @@ public virtual bool TryCopyBlobContentStream( public DownloadAndSaveObjectResult TryDownloadAndSaveObject(string objectId, RequestSource requestSource) { - return this.TryDownloadAndSaveObject(objectId, CancellationToken.None, requestSource, retryOnFailure: true); + return this.TryDownloadAndSaveObject(objectId, CancellationToken.None, requestSource, retryOnFailure: true).Result; } public bool TryGetBlobSizeLocally(string sha, out long length) @@ -182,7 +220,7 @@ public bool TryGetBlobSizeLocally(string sha, out long length) return this.GitObjectRequestor.QueryForFileSizes(objectIds, cancellationToken); } - private DownloadAndSaveObjectResult TryDownloadAndSaveObject( + private DownloadAttemptResult TryDownloadAndSaveObject( string objectId, CancellationToken cancellationToken, RequestSource requestSource, @@ -190,7 +228,7 @@ private DownloadAndSaveObjectResult TryDownloadAndSaveObject( { if (objectId == GVFSConstants.AllZeroSha) { - return DownloadAndSaveObjectResult.Error; + return new DownloadAttemptResult(DownloadAndSaveObjectResult.Error, httpStatusCode: null); } DateTime negativeCacheRequestTime; @@ -198,7 +236,7 @@ private DownloadAndSaveObjectResult TryDownloadAndSaveObject( { if (negativeCacheRequestTime > DateTime.Now.Subtract(NegativeCacheTTL)) { - return DownloadAndSaveObjectResult.ObjectNotOnServer; + return new DownloadAttemptResult(DownloadAndSaveObjectResult.ObjectNotOnServer, httpStatusCode: null); } this.objectNegativeCache.TryRemove(objectId, out negativeCacheRequestTime); @@ -210,9 +248,9 @@ private DownloadAndSaveObjectResult TryDownloadAndSaveObject( // captured by the Lazy factory. Subsequent coalesced callers inherit those // settings. In practice this is fine because the primary concurrent path // (NamedPipeMessage from git.exe) always uses CancellationToken.None. - Lazy newLazy = new Lazy( + Lazy newLazy = new Lazy( () => this.DoDownloadAndSaveObject(objectId, cancellationToken, requestSource, retryOnFailure)); - Lazy lazy = this.inflightDownloads.GetOrAdd(objectId, newLazy); + Lazy lazy = this.inflightDownloads.GetOrAdd(objectId, newLazy); if (!ReferenceEquals(lazy, newLazy)) { @@ -240,13 +278,13 @@ private DownloadAndSaveObjectResult TryDownloadAndSaveObject( /// .NET Framework 4.7.1. When we upgrade to .NET 10 (backlog), this can be /// replaced with ConcurrentDictionary.TryRemove(KeyValuePair). /// - private bool TryRemoveInflightDownload(string objectId, Lazy lazy) + private bool TryRemoveInflightDownload(string objectId, Lazy lazy) { - return ((ICollection>>)this.inflightDownloads) - .Remove(new KeyValuePair>(objectId, lazy)); + return ((ICollection>>)this.inflightDownloads) + .Remove(new KeyValuePair>(objectId, lazy)); } - private DownloadAndSaveObjectResult DoDownloadAndSaveObject( + private DownloadAttemptResult DoDownloadAndSaveObject( string objectId, CancellationToken cancellationToken, RequestSource requestSource, @@ -273,21 +311,32 @@ private DownloadAndSaveObjectResult DoDownloadAndSaveObject( return new RetryWrapper.CallbackResult(new GitObjectsHttpRequestor.GitObjectTaskResult(true)); }); + // Capture the HTTP status of the last download attempt when a response was received. + // On failure the requestor propagates the real status (e.g. 401/404/503); on an + // exhausted retry that ended in an exception output.Result is null and no status is + // known. A default (zero) status means the result carried no HTTP response, so it is + // treated as "no status". + HttpStatusCode? httpStatusCode = null; + if (output.Result != null && output.Result.HttpStatusCodeResult != 0) + { + httpStatusCode = output.Result.HttpStatusCodeResult; + } + if (output.Result != null) { if (output.Succeeded && output.Result.Success) { - return DownloadAndSaveObjectResult.Success; + return new DownloadAttemptResult(DownloadAndSaveObjectResult.Success, httpStatusCode); } if (output.Result.HttpStatusCodeResult == HttpStatusCode.NotFound) { this.objectNegativeCache.AddOrUpdate(objectId, DateTime.Now, (unused1, unused2) => DateTime.Now); - return DownloadAndSaveObjectResult.ObjectNotOnServer; + return new DownloadAttemptResult(DownloadAndSaveObjectResult.ObjectNotOnServer, httpStatusCode); } } - return DownloadAndSaveObjectResult.Error; + return new DownloadAttemptResult(DownloadAndSaveObjectResult.Error, httpStatusCode); } } } \ No newline at end of file diff --git a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs index 205d2b4de6..5615361122 100644 --- a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs @@ -1,4 +1,4 @@ -using GVFS.Common; +using GVFS.Common; using GVFS.Common.Git; using GVFS.Common.Http; using GVFS.Common.Tracing; @@ -157,6 +157,124 @@ public void TerminalBlobHydrationFailureTagsObjectNotOnServer() terminalError.ShouldContain("\"BlobHydrationFailureCategory\":\"ObjectNotOnServer\""); } + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void TerminalBlobHydrationFailureRecordsHttpStatusCode() + { + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => true; + fileSystem.OnOpenFileStream = (path, mode, access) => + { + if (access == FileAccess.Write) + { + return new MemoryStream(); + } + + throw new FileNotFoundException(); + }; + + MockHttpGitObjects httpObjects = new MockHttpGitObjects(); + + // Force the download to fail with 401. The DownloadFailed bucket collapses auth and + // transient failures, so the terminal event must also carry the HTTP status to tell + // a real 401 apart from a transient failure. + httpObjects.StatusCodeToReturn = HttpStatusCode.Unauthorized; + GVFSGitObjects dut = this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out MockTracer tracer); + + bool copied = dut.TryCopyBlobContentStream( + ValidTestObjectFileSha1, + new CancellationToken(), + GVFSGitObjects.RequestSource.FileStreamCallback, + (stream, length) => Assert.Fail("Should not be able to call copy stream callback"), + out GVFSGitObjects.BlobHydrationFailureCategory failureCategory); + copied.ShouldEqual(false); + + // A 401 is not classified as ObjectNotOnServer, so it lands in the neutral + // DownloadFailed bucket; the HTTP status is what distinguishes it. + failureCategory.ShouldEqual(GVFSGitObjects.BlobHydrationFailureCategory.DownloadFailed); + string terminalError = tracer.RelatedErrorEvents.First(e => e.Contains("Failed to provide blob contents")); + terminalError.ShouldContain("\"BlobHydrationFailureCategory\":\"DownloadFailed\""); + terminalError.ShouldContain("\"HttpStatusCode\":401"); + terminalError.ShouldContain("\"HttpStatusName\":\"Unauthorized\""); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void TerminalBlobHydrationFailureRecordsTransientHttpStatusCode() + { + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => true; + fileSystem.OnOpenFileStream = (path, mode, access) => + { + if (access == FileAccess.Write) + { + return new MemoryStream(); + } + + throw new FileNotFoundException(); + }; + + MockHttpGitObjects httpObjects = new MockHttpGitObjects(); + + // A transient 503 must also carry the HTTP status so it can be told apart from a real + // auth failure - both share the DownloadFailed category. + httpObjects.StatusCodeToReturn = HttpStatusCode.ServiceUnavailable; + GVFSGitObjects dut = this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out MockTracer tracer); + + bool copied = dut.TryCopyBlobContentStream( + ValidTestObjectFileSha1, + new CancellationToken(), + GVFSGitObjects.RequestSource.FileStreamCallback, + (stream, length) => Assert.Fail("Should not be able to call copy stream callback"), + out GVFSGitObjects.BlobHydrationFailureCategory failureCategory); + copied.ShouldEqual(false); + + failureCategory.ShouldEqual(GVFSGitObjects.BlobHydrationFailureCategory.DownloadFailed); + string terminalError = tracer.RelatedErrorEvents.First(e => e.Contains("Failed to provide blob contents")); + terminalError.ShouldContain("\"BlobHydrationFailureCategory\":\"DownloadFailed\""); + terminalError.ShouldContain("\"HttpStatusCode\":503"); + terminalError.ShouldContain("\"HttpStatusName\":\"ServiceUnavailable\""); + } + + [TestCase] + [Category(CategoryConstants.ExceptionExpected)] + public void TerminalBlobHydrationFailureOmitsHttpStatusWhenDownloadHasNoStatus() + { + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => true; + fileSystem.OnOpenFileStream = (path, mode, access) => + { + if (access == FileAccess.Write) + { + return new MemoryStream(); + } + + throw new FileNotFoundException(); + }; + + MockHttpGitObjects httpObjects = new MockHttpGitObjects(); + + // The download fails without an HTTP response (no status). The terminal event must NOT + // carry a status - in particular it must never emit "HttpStatusCode":0 for a status that + // was never received. + httpObjects.FailWithoutStatus = true; + GVFSGitObjects dut = this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out MockTracer tracer); + + bool copied = dut.TryCopyBlobContentStream( + ValidTestObjectFileSha1, + new CancellationToken(), + GVFSGitObjects.RequestSource.FileStreamCallback, + (stream, length) => Assert.Fail("Should not be able to call copy stream callback"), + out GVFSGitObjects.BlobHydrationFailureCategory failureCategory); + copied.ShouldEqual(false); + + failureCategory.ShouldEqual(GVFSGitObjects.BlobHydrationFailureCategory.DownloadFailed); + string terminalError = tracer.RelatedErrorEvents.First(e => e.Contains("Failed to provide blob contents")); + terminalError.ShouldContain("\"BlobHydrationFailureCategory\":\"DownloadFailed\""); + terminalError.ShouldNotContain(false, "HttpStatusCode"); + terminalError.ShouldNotContain(false, "HttpStatusName"); + } + [TestCase] [Category(CategoryConstants.ExceptionExpected)] public void TerminalBlobHydrationFailureTagsLocalCopyFailed() @@ -707,15 +825,15 @@ public void StragglingFinallyDoesNotRemoveNewInflightDownload() wave2Started.Wait(TimeSpan.FromSeconds(5)).ShouldBeTrue("Wave 2 download should have started"); // Capture wave 2's Lazy from the dictionary - Lazy wave2Lazy; + Lazy wave2Lazy; dut.inflightDownloads.TryGetValue(ValidTestObjectFileSha1, out wave2Lazy).ShouldBeTrue("Wave 2 Lazy should be in dictionary"); // Simulate a straggling wave-1 thread: create a different Lazy and try to remove it. // With value-aware removal, this must NOT remove wave 2's Lazy. - Lazy staleLazy = - new Lazy(() => GitObjects.DownloadAndSaveObjectResult.Success); - bool staleRemoved = ((ICollection>>)dut.inflightDownloads) - .Remove(new KeyValuePair>(ValidTestObjectFileSha1, staleLazy)); + Lazy staleLazy = + new Lazy(() => new GVFSGitObjects.DownloadAttemptResult(GitObjects.DownloadAndSaveObjectResult.Success, httpStatusCode: null)); + bool staleRemoved = ((ICollection>>)dut.inflightDownloads) + .Remove(new KeyValuePair>(ValidTestObjectFileSha1, staleLazy)); staleRemoved.ShouldBeFalse("Straggling finally must not remove wave 2's Lazy"); dut.inflightDownloads.ContainsKey(ValidTestObjectFileSha1).ShouldBeTrue("Wave 2 Lazy must survive"); @@ -796,6 +914,12 @@ private MockHttpGitObjects(MockGVFSEnlistment enlistment) public Stream InputStream { get; set; } public string MediaType { get; set; } public HttpStatusCode? StatusCodeToReturn { get; set; } + + // When true, TryDownloadObjects returns a failing result built from GitObjectTaskResult(bool), + // i.e. Result is non-null but carries no HTTP status (HttpStatusCodeResult == 0). This + // exercises the "download failed without a status" branch of the telemetry status capture. + public bool FailWithoutStatus { get; set; } + public byte[] ContentBytesToServe { get; set; } public static MemoryStream GetRandomStream(int size) @@ -837,6 +961,16 @@ public override RetryWrapper.InvocationResult TryDownloadOb result: new GitObjectTaskResult(this.StatusCodeToReturn.Value)); } + if (this.FailWithoutStatus) + { + // A download that failed without an HTTP response: Result is non-null but its + // HttpStatusCodeResult stays 0, so no status should reach telemetry. + return new RetryWrapper.InvocationResult( + 0, + error: null, + result: new GitObjectTaskResult(false)); + } + // Serve a fresh stream per call when ContentBytesToServe is set so the download // succeeds even across retries (InputStream would be consumed after the first read). Stream contentStream = this.ContentBytesToServe != null From 105fae0bdeb9b33cf65d9171cc04778d0ba2ca5c Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 13 Aug 2026 10:13:10 -0700 Subject: [PATCH 4/6] Read git config strings via snapshot and owned marshalling in LibGit2Repo git_config_get_string returns a borrowed pointer whose lifetime is tied to the config object, so libgit2 only permits it on a snapshot (read-only) config. LibGit2Repo.GetConfigString called it on the live config returned by git_repository_config, which fails with "get_string called on a live config object". The read then threw LibGit2Exception, and callers silently fell back to their default value instead of honoring the configured setting. Take a git_config_snapshot of the live config and read the string from the snapshot, freeing the snapshot afterward. Also stop marshalling the result as an out string. git_config_get_string returns a borrowed const char* owned by the config; the interop marshaller would free that pointer with CoTaskMemFree, a mismatched-allocator free of memory libgit2 still owns, corrupting the heap. Retrieve the value as an IntPtr and copy it with Marshal.PtrToStringUTF8, which never frees the borrowed pointer. This matches the manual marshalling already used by GitConfigEntry. Not-found still returns null so the documented default applies without a spurious error. git_config_get_bool is unaffected (it parses the value rather than returning a borrowed pointer), so GetConfigBool is left unchanged. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 42 ++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index dafcc8d540..00bc55e73e 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -259,18 +259,39 @@ public virtual string GetConfigString(string name) } try { - string value; - Native.ResultCode resultCode = Native.Config.GetString(out value, configHandle, name); - if (resultCode == Native.ResultCode.NotFound) + // git_config_get_string returns a borrowed pointer whose lifetime is tied to the + // config, so libgit2 only allows it on a snapshot (read-only) config. Calling it on + // the live config returned by git_repository_config fails with "get_string called on + // a live config object". Snapshot the config first, then read the string from it. + IntPtr snapshotHandle; + if (Native.Config.Snapshot(out snapshotHandle, configHandle) != Native.ResultCode.Success) { - return null; + throw new LibGit2Exception($"Failed to snapshot config for '{name}': {Native.GetLastError()}"); } - else if (resultCode != Native.ResultCode.Success) + + try { - throw new LibGit2Exception($"Failed to get config value for '{name}': {Native.GetLastError()}"); - } + // git_config_get_string yields a borrowed pointer owned by the (snapshot) + // config, so it is retrieved as an IntPtr and copied manually. Marshalling it + // directly as an out string would make the interop marshaller free the pointer + // with CoTaskMemFree, corrupting libgit2's heap (mismatched allocator). + IntPtr valuePtr; + Native.ResultCode resultCode = Native.Config.GetString(out valuePtr, snapshotHandle, name); + if (resultCode == Native.ResultCode.NotFound) + { + return null; + } + else if (resultCode != Native.ResultCode.Success) + { + throw new LibGit2Exception($"Failed to get config value for '{name}': {Native.GetLastError()}"); + } - return value; + return valuePtr == IntPtr.Zero ? null : Marshal.PtrToStringUTF8(valuePtr); + } + finally + { + Native.Config.Free(snapshotHandle); + } } finally { @@ -585,8 +606,11 @@ public static class Config [DllImport(Git2NativeLibName, EntryPoint = "git_config_open_default")] public static extern ResultCode GetGlobalAndSystemConfig(out IntPtr configHandle); + [DllImport(Git2NativeLibName, EntryPoint = "git_config_snapshot")] + public static extern ResultCode Snapshot(out IntPtr snapshotConfigHandle, IntPtr configHandle); + [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_string")] - public static extern ResultCode GetString(out string value, IntPtr configHandle, string name); + public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_multivar_foreach")] public static extern ResultCode GetMultivarForeach( From 1a84811e67ec2e6bbf628200f343f0ef55f5f6c8 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 13 Aug 2026 10:39:04 -0700 Subject: [PATCH 5/6] Add functional test for libgit2 config reads Add LibGit2ConfigTests, a functional test that exercises the real libgit2 (git2.dll) config-read path in LibGit2Repo against a plain on-disk git repository. It creates a temp repo, sets a string and a bool config value, then reads them back through LibGit2Repo and asserts a missing key returns null. No unit test can cover this: the unit tests mock the native layer, so the "get_string called on a live config object" failure only manifests through the real P/Invoke. This test fails before the snapshot/marshalling fix and passes after, guarding the regression. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../Tests/LibGit2ConfigTests.cs | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 GVFS/GVFS.FunctionalTests/Tests/LibGit2ConfigTests.cs diff --git a/GVFS/GVFS.FunctionalTests/Tests/LibGit2ConfigTests.cs b/GVFS/GVFS.FunctionalTests/Tests/LibGit2ConfigTests.cs new file mode 100644 index 0000000000..f1ed89acd6 --- /dev/null +++ b/GVFS/GVFS.FunctionalTests/Tests/LibGit2ConfigTests.cs @@ -0,0 +1,79 @@ +using GVFS.Common.Git; +using GVFS.Common.Tracing; +using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.IO; +using GitProcess = GVFS.FunctionalTests.Tools.GitProcess; + +namespace GVFS.FunctionalTests.Tests +{ + /// + /// Exercises the real libgit2 (git2.dll) config-read path in + /// against a plain on-disk git repository. This is a regression guard for the + /// "get_string called on a live config object" failure, which no mock-based unit + /// test can catch because it only manifests through the native P/Invoke. + /// + [TestFixture] + public class LibGit2ConfigTests + { + private const string StringConfigKey = "gvfs.functionaltests-teststring"; + private const string StringConfigValue = "libgit2-value-42"; + private const string BoolConfigKey = "gvfs.functionaltests-testbool"; + private const string MissingConfigKey = "gvfs.functionaltests-missing"; + + private string repoRoot; + + [OneTimeSetUp] + public void CreateRepo() + { + this.repoRoot = Path.Combine(Path.GetTempPath(), "GVFS.LibGit2ConfigTests_" + Path.GetRandomFileName()); + Directory.CreateDirectory(this.repoRoot); + + GitProcess.Invoke(this.repoRoot, "init"); + GitProcess.Invoke(this.repoRoot, "config user.name \"Functional Test User\""); + GitProcess.Invoke(this.repoRoot, "config user.email \"functional@test.com\""); + GitProcess.Invoke(this.repoRoot, $"config {StringConfigKey} {StringConfigValue}"); + GitProcess.Invoke(this.repoRoot, $"config {BoolConfigKey} true"); + } + + [OneTimeTearDown] + public void DeleteRepo() + { + if (this.repoRoot != null) + { + RepositoryHelpers.DeleteTestDirectory(this.repoRoot); + } + } + + [TestCase] + public void GetConfigStringReturnsValueFromLiveConfig() + { + // Before the snapshot fix this threw LibGit2Exception + // ("get_string called on a live config object") and callers silently + // fell back to their default value. + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + repo.GetConfigString(StringConfigKey).ShouldEqual(StringConfigValue); + } + } + + [TestCase] + public void GetConfigStringReturnsNullWhenKeyMissing() + { + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + repo.GetConfigString(MissingConfigKey).ShouldBeNull(); + } + } + + [TestCase] + public void GetConfigBoolReturnsValueFromLiveConfig() + { + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + repo.GetConfigBool(BoolConfigKey).ShouldEqual(true); + } + } + } +} From 083cd6df6e1a039bf92f5d668a922875aa1e92d8 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 13 Aug 2026 10:43:49 -0700 Subject: [PATCH 6/6] Reject malformed object SHAs before the cache-server download The malformed-SHA guard in TryCopyBlobContentStream only covers the blob hydration path. Two other callers reach the object download directly, so a corrupt (NUL-byte) placeholder SHA from them still reached the network: - the git.exe read-object hook (RequestSource.NamedPipeMessage, via InProcessMount), and - the gitattributes GVFSVerb (RequestSource.GVFSVerb). On .NET Framework the local Path.Combine threw ArgumentException on such a value, so the download was never reached. On modern .NET (which 2.0 runs) Path.Combine no longer validates path characters, so the malformed SHA silently misses the local object store and is sent to the cache server. The Application Gateway rejects the malformed URL with HTTP 400, and GVFS then treats the 400 as an auth failure and erases a valid credential, producing a credential-prompt storm (ICM 850075166). Reject a malformed object SHA at the download chokepoint (GVFSGitObjects.TryDownloadAndSaveObject, next to the existing AllZeroSha guard) for every request source, before any request is built, and emit the same greppable *_MalformedBlobSha Warning as the other guards. Also correct the GetLooseBlobState comment: the ArgumentException it described is .NET-Framework-only, so validating (not relying on the throw) is what makes the guard correct on modern .NET. Unit test asserts a malformed SHA returns Error from TryDownloadAndSaveObject across FileStreamCallback / NamedPipeMessage / GVFSVerb, never reaches the network (download call count stays 0), and is logged. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GVFSGitObjects.cs | 21 +++++++ GVFS/GVFS.Common/Git/GitRepo.cs | 20 +++++-- .../GVFS.UnitTests/Git/GVFSGitObjectsTests.cs | 56 +++++++++++++++++++ 3 files changed, 91 insertions(+), 6 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs index 6630678261..302e42a92d 100644 --- a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs +++ b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs @@ -208,6 +208,27 @@ private DownloadAndSaveObjectResult TryDownloadAndSaveObject( RequestSource requestSource, bool retryOnFailure) { + // Defense in depth for a malformed object id (for example a corrupt placeholder's + // all-NUL content-id). On .NET Framework Path.Combine threw ArgumentException on + // such a value; on modern .NET it does not, so a malformed SHA silently misses the + // local object store and would otherwise be sent to the cache server, which rejects + // the URL with HTTP 400 - and GVFS then erases a valid credential (HttpRequestor + // treats 400 as an auth failure), producing a credential-prompt storm. Callers other + // than blob hydration reach this method WITHOUT going through the + // TryCopyBlobContentStream guard - the git.exe read-object hook (NamedPipeMessage, + // via InProcessMount) and the gitattributes GVFSVerb - so reject a malformed SHA here + // for every caller before any request is built. + if (!SHA1Util.IsValidShaFormat(objectId)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("sha", SHA1Util.ToLoggableShaString(objectId)); + metadata.Add("RequestSource", requestSource.ToString()); + metadata.Add(TracingConstants.MessageKey.WarningMessage, nameof(this.TryDownloadAndSaveObject) + ": Refusing to download object with malformed SHA"); + this.Tracer.RelatedEvent(EventLevel.Warning, nameof(this.TryDownloadAndSaveObject) + "_MalformedBlobSha", metadata, Keywords.Telemetry); + + return DownloadAndSaveObjectResult.Error; + } + if (objectId == GVFSConstants.AllZeroSha) { return DownloadAndSaveObjectResult.Error; diff --git a/GVFS/GVFS.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index 1ca4d97d01..055f85f617 100644 --- a/GVFS/GVFS.Common/Git/GitRepo.cs +++ b/GVFS/GVFS.Common/Git/GitRepo.cs @@ -357,12 +357,20 @@ private LooseBlobState GetLooseBlobStateAtPath(string blobPath, Action writeAction, out long size) { // A corrupt placeholder can carry a malformed content-id (for example 40 NUL - // bytes instead of a hex SHA). Such a value holds characters that are illegal - // in a file path, so Path.Combine below throws ArgumentException ("Illegal - // characters in path"). ArgumentException is not handled by RetryWrapper, so it - // bypasses both the retry logic and the download fallback and fails the - // hydration permanently. Reject the malformed SHA up front and report it as an - // invalid loose object, which the callers treat as a clean, non-retryable miss. + // bytes instead of a hex SHA). Reject it up front and report an invalid loose + // object, which the callers treat as a clean, non-retryable miss. + // + // The behavior of Path.Combine below is runtime-dependent, so validating here + // (rather than relying on an exception) is required on modern .NET: + // - On .NET Framework, Path.Combine throws ArgumentException ("Illegal + // characters in path") on the NUL bytes. ArgumentException is not handled by + // RetryWrapper, so it bypasses both the retry logic and the download fallback + // and fails the hydration permanently (a retry storm - the original symptom). + // - On modern .NET (.NET Core/5+), Path.Combine no longer validates path + // characters, so it does NOT throw; the bogus path simply misses on disk and + // the request would fall through to a server download that the gateway rejects + // with HTTP 400 (ICM 850075166). The download path is guarded separately in + // GVFSGitObjects.TryDownloadAndSaveObject. if (!SHA1Util.IsValidShaFormat(blobSha)) { size = -1; diff --git a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs index e50944e907..9abe55752e 100644 --- a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs @@ -360,6 +360,56 @@ public void TryCopyBlobContentStreamFailsCleanlyForOtherMalformedShas() this.AssertMalformedShaHydrationFailsCleanly(new string('a', 20) + "\\" + new string('a', 19)); } + [TestCase] + public void TryDownloadAndSaveObjectDoesNotSendMalformedShaToServer() + { + // Regression for the customer HTTP-400 mode (ICM 850075166). On modern .NET a + // corrupt placeholder's all-NUL SHA does not throw in Path.Combine, so it misses + // locally and, without this guard, is sent to the cache server, which rejects the + // URL with HTTP 400 - and GVFS then erases a valid credential, producing a GCM + // prompt storm. This download path is reached by callers OTHER than blob hydration + // (the git.exe read-object hook via NamedPipeMessage, and the gitattributes + // GVFSVerb), which do not go through the TryCopyBlobContentStream guard, so it must + // be rejected at the download method itself for every request source. + MockFileSystemWithCallbacks fileSystem = new MockFileSystemWithCallbacks(); + fileSystem.OnFileExists = (path) => false; + fileSystem.OnOpenFileStream = (path, mode, access) => new MemoryStream(); + + MockHttpGitObjects httpObjects = new MockHttpGitObjects(); + GVFSGitObjects dut = this.CreateTestableGVFSGitObjects(httpObjects, fileSystem, out MockTracer tracer); + + string[] malformedShas = + { + new string('\0', 40), + string.Empty, + new string('0', 39), + new string('0', 20) + "\0" + new string('0', 19), + }; + + GVFSGitObjects.RequestSource[] sources = + { + GVFSGitObjects.RequestSource.FileStreamCallback, + GVFSGitObjects.RequestSource.NamedPipeMessage, + GVFSGitObjects.RequestSource.GVFSVerb, + }; + + foreach (string malformedSha in malformedShas) + { + foreach (GVFSGitObjects.RequestSource source in sources) + { + GitObjects.DownloadAndSaveObjectResult result = GitObjects.DownloadAndSaveObjectResult.Success; + Assert.DoesNotThrow( + () => result = dut.TryDownloadAndSaveObject(malformedSha, source), + "TryDownloadAndSaveObject must not throw for a malformed SHA"); + result.ShouldEqual(GitObjects.DownloadAndSaveObjectResult.Error); + } + } + + // No malformed SHA reached the network, and the rejection is diagnosable. + httpObjects.TryDownloadObjectsCallCount.ShouldEqual(0); + tracer.RelatedEventNames.ShouldContain(e => e == "TryDownloadAndSaveObject_MalformedBlobSha"); + } + [TestCase] public void CoalescesMultipleConcurrentRequestsForSameObject() { @@ -884,6 +934,10 @@ private MockHttpGitObjects(MockGVFSEnlistment enlistment) public HttpStatusCode? StatusCodeToReturn { get; set; } public byte[] ContentBytesToServe { get; set; } + // Number of times a network download was actually attempted. Lets a test prove a + // malformed SHA is rejected before any request reaches the server. + public int TryDownloadObjectsCallCount { get; private set; } + public static MemoryStream GetRandomStream(int size) { Random randy = new Random(0); @@ -913,6 +967,8 @@ public override RetryWrapper.InvocationResult TryDownloadOb Action.ErrorEventArgs> onFailure, bool preferBatchedLooseObjects) { + this.TryDownloadObjectsCallCount++; + if (this.StatusCodeToReturn.HasValue) { // Simulate the server returning a non-OK status (e.g. 404) so callers can exercise