diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index e550568da..d0b4a4509 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: diff --git a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs index b232e7b74..06a1af990 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( @@ -67,12 +89,32 @@ 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 // 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 @@ public virtual bool TryCopyBlobContentStream( { category = BlobHydrationFailureCategory.LocalCopyFailed; } - else if (lastDownloadResult == DownloadAndSaveObjectResult.ObjectNotOnServer) + else if (lastDownloadResult?.Result == DownloadAndSaveObjectResult.ObjectNotOnServer) { category = BlobHydrationFailureCategory.ObjectNotOnServer; } @@ -124,6 +166,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 +207,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 +227,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,15 +240,36 @@ 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, 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 new DownloadAttemptResult(DownloadAndSaveObjectResult.Error, httpStatusCode: null); + } + if (objectId == GVFSConstants.AllZeroSha) { - return DownloadAndSaveObjectResult.Error; + return new DownloadAttemptResult(DownloadAndSaveObjectResult.Error, httpStatusCode: null); } DateTime negativeCacheRequestTime; @@ -198,7 +277,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 +289,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 +319,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 +352,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.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index 302b0e13e..055f85f61 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,33 @@ 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). 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) { diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index dafcc8d54..00bc55e73 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( diff --git a/GVFS/GVFS.Common/SHA1Util.cs b/GVFS/GVFS.Common/SHA1Util.cs index 0fc20019d..01a2ba230 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.FunctionalTests/Tests/LibGit2ConfigTests.cs b/GVFS/GVFS.FunctionalTests/Tests/LibGit2ConfigTests.cs new file mode 100644 index 000000000..f1ed89acd --- /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); + } + } + } +} diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs index 7a6bad6f2..c99a60205 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 90127fb99..3d868bcf7 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 205d2b4de..60826ca69 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() @@ -330,6 +448,86 @@ 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 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() { @@ -707,15 +905,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"); @@ -751,6 +949,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 _); @@ -796,8 +1050,18 @@ 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; } + // 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); @@ -827,6 +1091,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 @@ -837,6 +1103,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 diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index c04be4204..d933584e9 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();