Skip to content

Fail fast on SHA256 repositories in FastFetch and gvfs clone - #2116

Draft
Tyrie Vella (tyrielv) wants to merge 6 commits into
microsoft:vnextfrom
tyrielv:tyrielv/sha256-failfast
Draft

Tyrie Vella (tyrielv) wants to merge 6 commits into
microsoft:vnextfrom
tyrielv:tyrielv/sha256-failfast

Conversation

@tyrielv

@tyrielv Tyrie Vella (tyrielv) commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem and Context

Git 3.0 changes git init's default hash algorithm from SHA1 to SHA256. The GVFS-protocol server (the only known implementer) has no plans to support SHA256, and VFS for Git's own code is hardcoded to SHA1's 20-byte / 40-hex-char object ids throughout (Sha1Id, SHA1Util.IsValidShaFormat, GitIndexGenerator, FastFetch's own index reader/writer). If a SHA256 repository ever reached this code, the result would be data corruption or an unpredictable crash rather than a clear, actionable error.

There is no supported way to convert an existing SHA256 repository to SHA1 in place: an object's id is a hash of its content plus the algorithm used to compute it, so "downgrading" would mean rewriting every object with a new identity - effectively a fresh clone. The only sound mitigation is detecting a SHA256 repository early and failing fast, rather than attempting an automatic conversion.

gvfs mount was initially left out of scope, on the assumption that once gvfs clone's git init call is pinned to --object-format=sha1 (a separate change, since merged into vnext), the only enlistments gvfs mount would ever see are ones gvfs clone itself created. Manually testing that assumption against a hand-crafted SHA256 repository (core.repositoryformatversion=1, extensions.objectformat=sha256) showed the actual failure mode was worse than "an unsupported, self-inflicted state":

  • TrySetRequiredGitConfigSettings unconditionally force-writes core.repositoryformatversion back to 0 (to match plain git init, with no awareness of extensions.objectformat). Applied to a SHA256 repo, this leaves core.repositoryformatversion=0 with a v1-only extension still present - a config state real git itself refuses to parse at all ("repo version is 0, but v1-only extension found"). This corrupted the enlistment's git config in place; even gvfs status/gvfs unmount stopped working until the config was hand-repaired.
  • The parent gvfs mount CLI then surfaced a raw BrokenPipeException with a full .NET stack trace on the console (the mount process's pipe breaks when it exits), not an actionable message.

Given that, gvfs mount is now covered by this PR too, as a second layer of defense alongside the git init pin.

Note on the shared root cause: the corruption mechanism above (RequiredGitConfig unconditionally force-writing core.repositoryformatversion back to 0, clobbering whatever a repo-format-v1 extension requires) is not specific to SHA256 - the same force-write bricks a reftable-backed repo identically, as documented in #2119. This PR deliberately does not touch RequiredGitConfig itself; it adds a pre-check ahead of each call site that would otherwise reach the corrupting write, scoped to the extensions.objectformat=sha256 axis only. The ref-storage/reftable axis is addressed separately in #2121, which follows the same pattern at the same four call sites. This PR will need a rebase once #2121 merges to resolve the resulting conflicts (both PRs insert at the same lines in FastFetchVerb.cs, InProcessMount.cs, CloneVerb.cs, and GitConfigRepairJob.cs).

Changes

  • Add GVFS.Common.Git.ObjectFormat, a small helper that reads extensions.objectformat from a repository's local git config and reports whether it identifies a SHA256 repository (following the existing GitProcess/CacheServerResolver config-reading pattern). Exposes both a simple IsSha256Repo and a TryIsSha256Repo(git, out isSha256, out error) that distinguishes "key absent" (safe default, SHA1) from a genuine config-read failure.
  • Check it in FastFetch's startup (FastFetchVerb.cs), right after the enlistment is resolved and before any code that assumes SHA1-shaped object ids runs. FastFetch operates against arbitrary pre-existing repositories, not just ones gvfs clone created, so it is the realistic path where a SHA256 repository could actually be encountered.
  • Check it in gvfs clone's TryInitRepo (CloneVerb.cs), right after git init succeeds - defense-in-depth alongside the --object-format=sha1 pin, covering a pre-existing repo that reached v1 by another route.
  • Check it in gvfs mount's InProcessMount.Mount (GVFS.Mount), right after the repo is confirmed valid and before TrySetRequiredGitConfigSettings runs, preventing the config corruption described above.
  • Check it in gvfs repair's GitConfigRepairJob: TryFixIssues fails clearly before the repair job wipes and rebuilds .git/config (a blind rebuild would otherwise erase extensions.objectformat and leave a SHA1-shaped config over live SHA256-hashed objects), and HasIssue reports a SHA256 enlistment as an unfixable issue so gvfs diagnose/gvfs repair no longer call it healthy.
  • A genuine config-read failure (as opposed to the key simply being absent, which reports the repo as SHA1) is fatal at FastFetch, clone, and mount - continuing in that state risks the very corruption these checks exist to prevent. Repair is the deliberate exception: a read failure there is surfaced as a warning but does not block the repair, since repair's whole purpose is to rebuild a corrupt config. Unlike ref storage format (Fail fast on reftable repositories, and pin clone to files ref storage #2121), SHA256 has no physical on-disk marker independent of git config, so this is an inherent limitation of the object-format axis rather than something fixable here.
  • Clean up GVFSEnlistment.WaitUntilMounted's BrokenPipeException handler to use e.Message instead of e.ToString() for the console-facing error, so a mount process that exits early (for this or any other reason) no longer dumps a raw stack trace to the console. The full exception detail still goes to the trace log. This is a narrowly-scoped fix to one handler; Report the real reason a mount fails instead of a broken pipe #2105 is a much larger rework of mount failure reporting (including a GetStatus.MountError field) that touches the same area and should be reconciled with this change when it lands.
  • Add unit tests for the detection helper (SHA1/SHA256/case-insensitive/missing-key/read-failure cases) and for the WaitUntilMounted message-cleanup fix (using a real named pipe to deterministically trigger the changed code path).

All four call sites report the same clear error message explaining that VFS for Git only supports SHA1 repositories and that the repository must be re-cloned or re-initialized as SHA1 - none of them suggest or attempt any automatic conversion.

Both the mount-path fix and the GitConfigRepairJob fix were manually verified end-to-end against a real GVFS enlistment (a hand-crafted SHA256 repo, mounted with a locally-built GVFS.Mount.exe/gvfs.exe): the config is left untouched instead of corrupted, and the mount process's log shows the clear SHA1-only error text. Neither InProcessMount nor RepairJobs has an existing unit-test seam in this codebase (confirmed no test files reference either), so no automated test covers these two call sites directly - the manual, real-repo verification stands in for that coverage.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analysis in the PR description is excellent — in particular documenting that InProcessMount and RepairJobs have no unit-test seam and that manual real-repo verification stands in for that coverage. That is the right disclosure to make rather than quietly leaving the gap.

The most useful framing for this review, though, is that #2121 is the same pattern, by you, on the same four files, at the same insertion points, against the same base — and #2121 is better on three axes. Most of what follows is "bring this in line with your own newer PR."

Eight findings, ranked.


1. High — GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs:85 fails open on the most destructive path

if (ObjectFormat.TryIsSha256Repo(new GitProcess(this.Enlistment), out bool isSha256Repo, out string objectFormatReadError) &&
    isSha256Repo)

On a config-read failure TryIsSha256Repo returns false, the && collapses, and repair proceeds to wipe and rebuild .git/config — which the comment directly above describes as the hazard being prevented: "would silently erase extensions.objectformat and leave a SHA1-shaped config over live SHA256-hashed objects." objectFormatReadError is captured and never read.

So the one site where guessing wrong is most destructive is the only one of the four that ignores the read error, while FastFetchVerb, InProcessMount and CloneVerb all at least warn.

#2121's GitConfigRepairJob.TryFixIssues gets this right: it separates configReadable from isReftableByConfig, adds the physical .git/reftable/ directory fallback for exactly the "config too corrupt to read" case that repair runs in, and traces the read failure when it proceeds anyway. Please apply the same shape here.

2. Medium — no HasIssue check, so gvfs diagnose reports SHA256 enlistments as healthy

#2121 adds a check to GitConfigRepairJob.HasIssue returning IssueType.CantFix. This PR only guards TryFixIssues. Without the HasIssue half, gvfs diagnose and gvfs repair will declare a SHA256 enlistment healthy and say nothing.

3. Medium — GVFS/GVFS/CommandLine/CloneVerb.cs:860-865 comment is stale before it merges

"'git init' above is not currently pinned to SHA1 anywhere in this codebase - that pin is expected to land in a separate change. Until it does, this check is the only thing preventing..."

#2115 ("Pin git init to sha1 during clone") merged earlier today, so this is already wrong. Plan narration in product code goes stale by construction. State the invariant instead — git init pins --object-format=sha1, and this check is defense-in-depth for repos that reached v1 by another route — and leave the sequencing in the PR description where it reads correctly forever.

4. Medium — two more comments that describe the work rather than the code

  • GitConfigRepairJob.cs:81: "a worse, silent form of the same corruption hazard this PR fixes elsewhere". "This PR" is meaningless to the next person who opens this file.
  • The new WaitUntilMounted test: "which is what e.ToString() (instead of e.Message) used to produce" — history narration; the test should describe the behavior it asserts now.

5. Medium — GVFS/GVFS.Common/Git/ObjectFormat.cs, IsSha256Repo(GitProcess) is dead

Zero product callers — only ObjectFormatTests uses it. Its own doc comment even points readers away from it: "Use TryIsSha256Repo if the caller wants to distinguish and surface that failure", which every real caller does.

Either drop it, or make it earn its keep in HasIssue per finding 2 — which is exactly what #2121 does with RefStorage.IsReftableRepo.

6. Medium — GVFS/GVFS.Common/GVFSEnlistment.cs, WaitUntilMounted is a separate fix

The BrokenPipeException change from e.ToString() to e.Message is a different bug from "fail fast on SHA256". It changes user-facing text for every broken-pipe case, not just this one, and it overlaps #2105 ("Report the real reason..."). It is disclosed in the PR body so it is clearly deliberate, but it would be cleaner as its own PR, or at minimum cross-referenced on #2105 so the two changes do not fight over the same handler.

7. Medium — opposite read-failure policy from #2121, in the same methods

This PR warns and continues on a config-read failure at FastFetchVerb, InProcessMount and CloneVerb. #2121 makes the equivalent failure fatal at all three. Once both land, two adjacent checks inside MountWithLockAcquired and TryInitRepo will have opposite fail-open/fail-closed semantics, which is going to confuse whoever touches this next.

Worth picking one. #2121's fail-closed looks right to me, and the green functional suite there is good evidence that normal key-absent repos do not trip it.

8. Medium — this will conflict with #2121

Both PRs insert at the same lines of GVFS/FastFetch/FastFetchVerb.cs (~204), GVFS/GVFS.Mount/InProcessMount.cs (~280), GVFS/GVFS/CommandLine/CloneVerb.cs (~857) and GVFS/GVFS/RepairJobs/GitConfigRepairJob.cs, both targeting vnext. Whichever merges second needs a rebase that also reconciles finding 7. Worth noting in both descriptions so it is not a surprise.


One thing I checked and want to explicitly clear: I suspected a FormatException hazard in this.tracer.RelatedWarning("Could not determine the repository's object format: " + objectFormatReadError) if git stderr contained a brace, since JsonTracer.RelatedWarning(string, params object[]) calls string.Format unconditionally. It is fine — ITracer also declares RelatedWarning(string message), and C# overload resolution prefers the non-params overload for a single string argument. No action needed. (#2121's comment on the FailMountAndExit variant is correct, though, because that one only has a params object[] overload.)

Also confirmed: no stray .github / AGENTS.md AI artifacts in the diff.

Git 3.0 changes 'git init' to default to the SHA256 object format instead
of SHA1. VFS for Git's own code assumes SHA1 (20-byte / 40-hex-char)
object ids throughout (Sha1Id, SHA1Util, GitIndexGenerator, FastFetch's
index reader/writer), and the GVFS-protocol server does not support
SHA256. Without a check, a SHA256 repository reaching this code would
produce data corruption or an unpredictable crash instead of a clear
error.

There is no supported way to convert an existing SHA256 repository to
SHA1 in place - an object's id is a hash of its content plus the
algorithm used to compute it, so "downgrading" means rewriting every
object with a new identity, effectively a fresh clone. The only sound
mitigation is detecting a SHA256 repository early and failing with an
actionable error.

Changes:
- Add GVFS.Common.Git.ObjectFormat, a small helper that reads
  extensions.objectformat from a repository's local git config and
  reports whether it identifies a SHA256 repository.
- Check it in FastFetch's startup, right after the enlistment is
  resolved and before any code that assumes SHA1-shaped object ids
  runs. FastFetch operates against arbitrary pre-existing repositories,
  not just ones 'gvfs clone' created, so it is the realistic path where
  a SHA256 repository could be encountered.
- Check it in 'gvfs clone' right after 'git init', as defense-in-depth
  in case a future code path invokes 'git init' without pinning
  --object-format=sha1.

'gvfs mount' is intentionally out of scope: once 'gvfs clone' pins
'git init' to SHA1, the only enlistments 'gvfs mount' ever operates on
are ones 'gvfs clone' itself created. A SHA256 .gvfs enlistment reaching
'gvfs mount' would require hand-editing .git config after the fact, an
unsupported, self-inflicted state not worth guarding against.

Assisted-by: Claude Sonnet 5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Self-review (review-swarm) surfaced two cross-model-convergent findings on
the SHA256 detection change:

- GVFS.Common.Git.ObjectFormat.IsSha256Repo collapsed "config key missing"
  (a legitimate, safe default meaning SHA1) and "git config genuinely
  failed to read" into the same false result, so a real config-read
  failure was silently treated as "not SHA256" with no visibility.
  Split the helper into a TryIsSha256Repo(git, out isSha256, out error)
  that surfaces a read failure distinctly, plus a convenience
  IsSha256Repo wrapper. Both call sites now log/print a config-read
  error instead of swallowing it, while intentionally keeping the same
  fail-open behavior as GitProcess.TryGetFromConfig elsewhere in this
  codebase for optional config reads (documented in the XML doc
  comment).

- CloneVerb.TryInitRepo's comment described its check as
  "defense-in-depth" behind a SHA1 pin on 'git init', but that pin does
  not exist anywhere in this codebase yet (it is a separate, planned
  change). Reworded the comment to say plainly that, until that pin
  lands, this check is the only thing preventing 'gvfs clone' from
  producing an unusable SHA256 enlistment once a Git 3.0+ client
  defaults 'init' to SHA256.

Added three more unit tests for the new tri-state TryIsSha256Repo API
(success/sha256, success/missing-key, and genuine read failure).

Assisted-by: Claude Sonnet 5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Empirically tested the originally out-of-scope 'gvfs mount' path against a
hand-crafted SHA256 repo (core.repositoryformatversion=1,
extensions.objectformat=sha256) and found the current failure mode
significantly worse than expected:

- The background mount process (GVFS.Mount.exe) proceeds until
  TrySetRequiredGitConfigSettings, which unconditionally writes
  core.repositoryformatversion back to 0 (to match plain 'git init', with
  no knowledge of extensions.objectformat). Applied to a SHA256 repo, this
  leaves core.repositoryformatversion=0 with a v1-only extension still
  present - a config state real git itself refuses to parse at all
  ("repo version is 0, but v1-only extension found"). This corrupts the
  enlistment's git config in place; even 'gvfs status'/'gvfs unmount'
  stopped working until the config was hand-repaired.
- The parent 'gvfs mount' CLI then surfaces a raw BrokenPipeException with
  a full .NET stack trace on the console (the mount process's pipe breaks
  when it exits), not any actionable message.

Changes:
- GVFS.Mount.InProcessMount.Mount now checks ObjectFormat.TryIsSha256Repo
  right after confirming the repo is valid (git.IsValidRepo()) and before
  TrySetRequiredGitConfigSettings runs, so a SHA256 repo is rejected with
  the same clear error used by FastFetch and 'gvfs clone', before any
  config mutation can corrupt it.
- GVFSEnlistment.WaitUntilMounted's BrokenPipeException handler now uses
  e.Message instead of e.ToString() for the console-facing error, so a
  mount process that exits early (for this or any other reason) no longer
  dumps a raw stack trace to the console. The full exception detail still
  goes to the trace log, and the existing "Run 'gvfs log ...' for more
  info" hint continues to point at the mount process's own log, which
  carries the specific failure reason.

Manually verified end-to-end against a real GVFS enlistment (a
dotnet-built GVFS.Mount.exe run both standalone and via a matching
dotnet-built gvfs.exe): the SHA256 repo's config is left untouched
(repositoryformatversion stays 1, no corruption), the mount process exits
cleanly with the SHA1-only error text in its log, and the console now
shows a short "Could not connect to GVFS.Mount: Unable to send: GetStatus"
line instead of a stack trace. No automated test is added for this
end-to-end mount path: InProcessMount has no existing unit-test seam (it
is not referenced by GVFS.UnitTests), and a functional test would need a
real ProjFS mount, which this worktree's build cache does not have seeded
(no native payload build). The manual, real-repo verification above
stands in for that coverage.

Assisted-by: Claude Sonnet 5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Self-review (review-swarm iteration 2) on the mount commit surfaced one
cross-model-convergent finding worth fixing, plus one test-coverage gap:

- GitConfigRepairJob.TryFixIssues calls the same
  GVFSVerb.TrySetRequiredGitConfigSettings that InProcessMount now guards,
  but was not itself guarded. Worse, TryFixIssues first wipes .git/config
  to empty and rebuilds it from only the required/optional settings this
  codebase knows about - so on a SHA256 repo, 'gvfs repair' would have
  silently erased extensions.objectformat and rewritten a SHA1-shaped
  config over live SHA256-hashed objects, a more silent form of the same
  corruption hazard this PR fixes elsewhere. Added the same
  ObjectFormat.TryIsSha256Repo check to the top of TryFixIssues, before
  the config file is touched. There is no existing unit-test
  infrastructure for RepairJobs at all in this codebase, so no new test
  was added for this specific fix, consistent with the rest of this PR's
  manually-verified pieces.

- GVFSEnlistment.WaitUntilMounted's BrokenPipeException message-cleanup
  fix (from the previous commit) had no test coverage, unlike the
  existing WaitUntilMountedProcessTrackingTests.cs, which only covers the
  connect-retry path. Added a test that opens a real named pipe server,
  accepts the client's connection, reads its first request, then
  disappears without responding - deterministically driving the exact
  BrokenPipeException branch this PR changed - and asserts the
  console-facing message stays short while the full exception detail is
  still captured via the tracer.

Assisted-by: Claude Sonnet 5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
PR microsoft#2121 adds an equivalent fail-fast check for reftable repositories
(extensions.refstorage) at the same four call sites this PR already
checks for SHA256 repositories (extensions.objectformat): FastFetch,
'gvfs clone', 'gvfs mount', and 'gvfs repair'. Reviewing both together
surfaced a mismatch: this PR's ObjectFormat check warned and continued on
a genuine config-read failure, while microsoft#2121's RefStorage check fails
closed. Once both checks sit side by side at each call site, having one
warn-and-continue and the other fail-closed for the same class of error
(an unreadable config, as opposed to a missing key) is inconsistent and
confusing. Align this PR's policy with microsoft#2121's, since fail-closed is the
more defensible default: a missing extensions.objectformat key already
reports the repository as SHA1 through the success path, so the error
path only fires when the config is genuinely anomalous, and continuing
in that state risks the very corruption this check exists to prevent.

- FastFetch, clone (TryInitRepo), and mount: a config-read failure is now
  fatal, matching a missing/successfully-read config being the only
  non-fatal outcomes.
- Mount passes the read error to FailMountAndExit as a format argument
  ("{0}") rather than concatenating it into the message string:
  FailMountAndExit routes through ITracer.RelatedError(string, params
  object[]), which runs string.Format, so a literal '{' in git's stderr
  (possible with a crafted or corrupt .git/config) would otherwise throw
  a FormatException instead of failing the mount cleanly.
- Repair (GitConfigRepairJob) keeps its existing exception to this
  policy: a config-read failure there is surfaced as a warning but does
  not block the repair, because repair's whole purpose is to rebuild a
  corrupt config. Unlike ref storage format, SHA256 has no physical
  on-disk marker independent of git config, so - unlike microsoft#2121's
  reftable check, which falls back to checking for a .git/reftable/
  directory when the config is unreadable - a SHA256 repo whose config is
  also corrupt cannot be distinguished from an ordinary corrupt SHA1 repo
  here. This is an inherent limitation of the object-format axis, not
  something to fix in this change.
- Added a HasIssue check to GitConfigRepairJob so a SHA256 repository is
  reported as an unfixable issue (matching microsoft#2121's structure), instead of
  'gvfs diagnose'/'gvfs repair' treating it as healthy.

Added a unit test locking in that the non-Try IsSha256Repo overload still
swallows a genuine read failure and reports "not SHA256" (only the Try
overload distinguishes it), mirroring the equivalent RefStorage test.

Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Rebase onto current vnext to pick up the already-merged --object-format=sha1
pin in GitProcess.Init (from the separately-landed "Pin git init to sha1
during clone" change). This was a clean rebase with no conflicts, since
that change only touches GitProcess.Init and its own test.

Address the remaining actionable findings from review on this PR:

- The comment in CloneVerb.TryInitRepo described 'git init' as "not
  currently pinned to SHA1 anywhere in this codebase," which was already
  stale relative to vnext (the pin landed there before this PR was even
  opened) and would only get staler. State the invariant instead of
  narrating the sequencing between the two changes: 'git init' pins
  --object-format=sha1, and this check is defense-in-depth for a
  pre-existing repo that reached v1 by another route.
- Removed a "used to produce" history narration from the WaitUntilMounted
  unit test's comment in favor of describing the behavior the assertion
  checks now.

Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants