Release M279.2

.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:

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 @@
         private static readonly TimeSpan NegativeCacheTTL = TimeSpan.FromSeconds(30);
 
         private ConcurrentDictionary<string, DateTime> objectNegativeCache;
-        internal ConcurrentDictionary<string, Lazy<DownloadAndSaveObjectResult>> inflightDownloads;
+        internal ConcurrentDictionary<string, Lazy<DownloadAttemptResult>> inflightDownloads;
 
         public GVFSGitObjects(GVFSContext context, GitObjectsHttpRequestor objectRequestor)
             : base(context.Tracer, context.Enlistment, objectRequestor, context.FileSystem)
         {
             this.Context = context;
             this.objectNegativeCache = new ConcurrentDictionary<string, DateTime>(StringComparer.OrdinalIgnoreCase);
-            this.inflightDownloads = new ConcurrentDictionary<string, Lazy<DownloadAndSaveObjectResult>>(StringComparer.OrdinalIgnoreCase);
+            this.inflightDownloads = new ConcurrentDictionary<string, Lazy<DownloadAttemptResult>>(StringComparer.OrdinalIgnoreCase);
         }
 
         public enum RequestSource
@@ -58,6 +58,28 @@
             Unexpected,           // Unclassified exception.
         }
 
+        /// <summary>
+        /// Carries the outcome of an object download together with the HTTP status of the last
+        /// download attempt. The public <see cref="DownloadAndSaveObjectResult"/> 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.
+        /// </summary>
+        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(
@@ -67,12 +89,32 @@
             Action<Stream, long> 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
             // 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 +151,7 @@
                     {
                         category = BlobHydrationFailureCategory.LocalCopyFailed;
                     }
-                    else if (lastDownloadResult == DownloadAndSaveObjectResult.ObjectNotOnServer)
+                    else if (lastDownloadResult?.Result == DownloadAndSaveObjectResult.ObjectNotOnServer)
                     {
                         category = BlobHydrationFailureCategory.ObjectNotOnServer;
                     }
@@ -124,6 +166,22 @@
                     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 +207,7 @@
 
                         // 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 +227,7 @@
 
         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,23 +240,44 @@
             return this.GitObjectRequestor.QueryForFileSizes(objectIds, cancellationToken);
         }
 
-        private DownloadAndSaveObjectResult TryDownloadAndSaveObject(
+        private DownloadAttemptResult TryDownloadAndSaveObject(
             string objectId,
             CancellationToken cancellationToken,
             RequestSource requestSource,
             bool retryOnFailure)
         {
-            if (objectId == GVFSConstants.AllZeroSha)
+            // 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 new DownloadAttemptResult(DownloadAndSaveObjectResult.Error, httpStatusCode: null);
+            }
+
             DateTime negativeCacheRequestTime;
             if (this.objectNegativeCache.TryGetValue(objectId, out negativeCacheRequestTime))
             {
                 if (negativeCacheRequestTime > DateTime.Now.Subtract(NegativeCacheTTL))
                 {
-                    return DownloadAndSaveObjectResult.ObjectNotOnServer;
+                    return new DownloadAttemptResult(DownloadAndSaveObjectResult.ObjectNotOnServer, httpStatusCode: null);
                 }
 
                 this.objectNegativeCache.TryRemove(objectId, out negativeCacheRequestTime);
@@ -210,9 +289,9 @@
             // 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<DownloadAndSaveObjectResult> newLazy = new Lazy<DownloadAndSaveObjectResult>(
+            Lazy<DownloadAttemptResult> newLazy = new Lazy<DownloadAttemptResult>(
                 () => this.DoDownloadAndSaveObject(objectId, cancellationToken, requestSource, retryOnFailure));
-            Lazy<DownloadAndSaveObjectResult> lazy = this.inflightDownloads.GetOrAdd(objectId, newLazy);
+            Lazy<DownloadAttemptResult> lazy = this.inflightDownloads.GetOrAdd(objectId, newLazy);
 
             if (!ReferenceEquals(lazy, newLazy))
             {
@@ -240,13 +319,13 @@
         /// .NET Framework 4.7.1. When we upgrade to .NET 10 (backlog), this can be
         /// replaced with ConcurrentDictionary.TryRemove(KeyValuePair).
         /// </summary>
-        private bool TryRemoveInflightDownload(string objectId, Lazy<DownloadAndSaveObjectResult> lazy)
+        private bool TryRemoveInflightDownload(string objectId, Lazy<DownloadAttemptResult> lazy)
         {
-            return ((ICollection<KeyValuePair<string, Lazy<DownloadAndSaveObjectResult>>>)this.inflightDownloads)
-                .Remove(new KeyValuePair<string, Lazy<DownloadAndSaveObjectResult>>(objectId, lazy));
+            return ((ICollection<KeyValuePair<string, Lazy<DownloadAttemptResult>>>)this.inflightDownloads)
+                .Remove(new KeyValuePair<string, Lazy<DownloadAttemptResult>>(objectId, lazy));
         }
 
-        private DownloadAndSaveObjectResult DoDownloadAndSaveObject(
+        private DownloadAttemptResult DoDownloadAndSaveObject(
             string objectId,
             CancellationToken cancellationToken,
             RequestSource requestSource,
@@ -273,21 +352,32 @@
                     return new RetryWrapper<GitObjectsHttpRequestor.GitObjectTaskResult>.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);
         }
     }
 }

GVFS/GVFS.Common/Git/GitRepo.cs

@@ -113,6 +113,19 @@ public virtual bool CommitAndRootTreeExists(string commitSha, out string rootTre
         /// </summary>
         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,33 @@ private LooseBlobState GetLooseBlobStateAtPath(string blobPath, Action<Stream, l
 
         private LooseBlobState GetLooseBlobState(string blobSha, Action<Stream, long> writeAction, out long size)
         {
+            // A corrupt placeholder can carry a malformed content-id (for example 40 NUL
+            // 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;
+
+                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)
             {