From 2d8880354257f31a5ea1a36f9cc9d8c03f2f5e55 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 5 Aug 2026 13:28:02 -0700 Subject: [PATCH 1/2] 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 083cd6df6e1a039bf92f5d668a922875aa1e92d8 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 13 Aug 2026 10:43:49 -0700 Subject: [PATCH 2/2] 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