Repository navigation
Commit b409946
Cover submodule working trees in safe.directory on Cygwin
Add the gitdb and smmap submodule working tree paths to the
`safe.directory` config in the Cygwin CI workflow. Without this, when
GitPython opens a submodule as a `Repo` and runs
`git cat-file --batch-check` against it, git rejects the repository for
dubious ownership. The user-visible failure modes (`ValueError`,
`IndexError`, `AssertionError`) all trace back to this rejection.
Why `[gitdb]` failed and `[smmap]` passed
-----------------------------------------
The trust check in `test_fixture_health.py` failed for `[gitdb]` but
passed for `[smmap]`, even though neither submodule's working tree was
in the workflow's `safe.directory` list before this commit. The
asymmetry comes down to which Owner SID NTFS records on each path, and
which paths git's ownership check requires to be owned.
There are six paths git's check might consider for the two submodules:
gitdb's `.git` gitfile, worktree `git/ext/gitdb`, and gitdir
`<repo>/.git/modules/gitdb`; smmap's `.git` gitfile, worktree
`git/ext/gitdb/gitdb/ext/smmap`, and gitdir
`<repo>/.git/modules/gitdb/modules/smmap`. Of these, only one had NTFS
Owner = `BUILTIN\Administrators` (Cygwin uid 544): gitdb's worktree. The
other five had Owner = `runneradmin`, whose Cygwin uid (197108) was the
value `geteuid()` returned in the test process.
The same Owner pattern held both with and without
`submodules: recursive` in `actions/checkout`. The single
`Administrators`-owned path was gitdb's worktree. All other paths were
`runneradmin`-owned, including the ones that Git for Windows's recursive
submodule clone had just produced when `submodules: recursive` was set.
The differentiator is not which git binary clones the submodules, but
that `git/ext/gitdb` is created by the outer `git clone`'s checkout
phase. When `git checkout` materializes a tree entry of mode 160000, it
calls `mkdir(path, 0777)` to create an empty submodule directory (see
`entry.c::write_entry`, case `S_IFGITLINK` [1] [2]).
On Windows GHA runners, jobs run as `runneradmin`. This is the built-in
local Administrator account (its Cygwin uid 197108 = 196608 + 500
matches Cygwin's mapping [3] for machine-local accounts: 0x30000 plus
the SID suffix 500, the well-known suffix of that account's SID). That
account is exempt from UAC token filtering by default (Admin Approval
Mode for the built-in Administrator account is disabled [4]), so its
processes hold the full administrative access token. `CreateProcessW`
propagates the parent's primary token unchanged through
`actions/checkout`'s process tree. Inside that tree, the outer
`git clone`'s `mkdir(path, 0777)` produces directories whose NTFS Owner
is `BUILTIN\Administrators` -- as observed on every workspace directory
the outer clone materialized, including the `git/ext/gitdb` placeholder.
Subsequent submodule-update operations -- both Git for Windows if
`actions/checkout` does a recursive clone, and Cygwin git if it happens
later due to `init-tests-after-clone.sh` -- produce paths that NTFS
records as `runneradmin`-owned. Both flows go through a process whose
primary token's `TokenOwner` field has been rewritten from
`BUILTIN\Administrators` to the user SID by a Cygwin or Cygwin-derived
runtime at DLL initialization. The rewrite propagates to every
descendant via `CreateProcessW`'s primary-token inheritance [5], so
every `mkdir` issued after that point produces a directory owned by
the user.
- Cygwin git triggers the rewrite directly. `cygheap_user::init`
in `cygwin1.dll` calls
`NtSetInformationToken(hProcToken, TokenOwner, &effec_cygsid, ...)`
at process startup [6].
- Git for Windows triggers it indirectly. `git submodule` is not a
builtin (only `submodule--helper` is) [7]. So it falls through to
`execv_dashed_external` and runs `git-submodule.sh`, a shell script
whose shebang is resolved at runtime to `sh.exe` in the Git for
Windows "Git Bash" environment. That `sh.exe` is an MSYS2 binary
linked against `msys-2.0.dll`, a Cygwin fork that performs the
same `TokenOwner` rewrite. From there, every `git.exe` the script
spawns inherits the user-SID `TokenOwner` and produces user-owned
directories.
Cygwin's `lstat().st_uid` reports the actual NTFS Owner SID mapped
through Cygwin's SID-to-uid table. `is_path_owned_by_current_user`
reduces to `lstat(p).st_uid == geteuid()` on Cygwin (no Administrators
group exemption). `ensure_valid_ownership` returns 1 (accepting the
repository) without consulting `safe.directory` when the gitfile,
worktree, and gitdir ALL pass that owned-by-current-user check.
Otherwise it falls through to comparing the worktree's `real_pathdup`
against each configured `safe.directory` entry.
For gitdb the three Owners were `runneradmin` (gitfile),
**`Administrators` (worktree)**, and `runneradmin` (gitdir), so the
all-paths-owned check failed on the worktree. The workflow's
`safe.directory` before this commit contained only `$(pwd)` and
`$(pwd)/.git`, neither of which exact-matches `git/ext/gitdb`, so the
`safe.directory` comparison also failed, and `ensure_valid_ownership`
returned 0 -- git rejected the repository. For smmap the three Owners
were all `runneradmin`, so the all-paths-owned check passed.
Cygwin's `chown` cannot rewrite the gitdb worktree's Owner SID from
`Administrators` to `runneradmin`: it returns "Permission denied".
Adding both submodule worktree paths to `safe.directory` is the correct
fix and is robust against shifts in what paths inherit which Owner.
Why `actions/checkout`'s own `safe.directory` does not help
-----------------------------------------------------------
`actions/checkout`'s `set-safe-directory: true` default writes the main
repository path to `safe.directory` in a temporary config it points its
own spawned git child at by overriding `HOME` for that child process.
That `HOME` override applies only to git invocations the action itself
spawns; subsequent steps' processes inherit the runner user's real
`HOME` (e.g., `C:\Users\runneradmin` on the Windows runner) and read its
actual `~/.gitconfig`, which never received the entry. So no git in a
later step, whether Git for Windows or Cygwin git, sees it. That's why
the `cygwin-test` workflow sets Cygwin git's `safe.directory` itself.
This commit extends that to cover the gitdb and smmap working trees.
The distinction between Cygwin git and Git for Windows is also why the
bug affected the Cygwin jobs and no other Windows jobs. `compat/mingw.c`
defines `is_path_owned_by_current_sid` [8], which accepts
`BUILTIN\Administrators`-owned paths when the current user is a member
of `Administrators`. Cygwin git compiles against the POSIX path
(`is_path_owned_by_current_uid` in `git-compat-util.h` [9]) without that
leniency. So the same `Administrators`-owned `git/ext/gitdb` that Cygwin
git rejects is silently accepted by Git for Windows, and the main CI
workflow's `windows-latest` jobs never trip the trust check.
Verification
------------
The `reproduce-safe-dir` matrix on the previous commits produces
failures for all three affected tests; this commit's CI run shows those
tests passing instead.
The Owner-SID claim above is verified by the `diag-token` job introduced
for this purpose. That job creates a directory through four code paths
(PowerShell-only, Cygwin-only, Cygwin-bash spawning Win32 `git init`,
and a PowerShell -> Cygwin-bash -> PowerShell sandwich) and reports the
NTFS Owner of each. The observed Owners match the predicted values in
every case, including the load-bearing Cygwin -> Win32 propagation case
(test C) and the sandwich case (test D) showing that the determinant is
whether some process in the ancestry has loaded a Cygwin-family runtime,
not the identity of the file-creating binary.
The commit immediately preceding this one temporarily sets
`submodules: recursive` on `actions/checkout` in every workflow that
runs the test suite. Its CI run shows the bug still triggering on Cygwin
(the gitdb worktree directory itself is created by the outer
`git clone`'s checkout phase, before any submodule init runs, regardless
of which mechanism subsequently populates the submodule contents).
A subsequent commit will revert that change; its CI run shows this fix
continues to hold without `submodules: recursive`, confirming the fix is
independent of submodule source.
[1]: https://github.com/git/git/blob/v2.51.0/entry.c#L397
[2]: https://github.com/git-for-windows/git/blob/v2.54.0.windows.1/entry.c#L397
[3]: https://cygwin.com/cygwin-ug-net/ntsec.html
[4]: https://learn.microsoft.com/en-us/windows/security/application-security/application-control/user-account-control/settings-and-configuration
[5]: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
[6]: https://sourceware.org/cgit/newlib-cygwin/tree/winsup/cygwin/uinfo.cc?h=cygwin-3.6.9#n82
[7]: https://github.com/git-for-windows/git/blob/v2.54.0.windows.1/git.c#L661
[8]: https://github.com/git-for-windows/git/blob/v2.54.0.windows.1/compat/mingw.c#L3931
[9]: https://github.com/git/git/blob/v2.51.0/git-compat-util.h#L346
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>1 parent 8ef4c21 commit b409946
1 file changed
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -61,6 +61,8 @@ jobs: | |||
| 61 | 61 | run: | | |
| 62 | 62 | git config --global --add safe.directory "$(pwd)" | |
| 63 | 63 | git config --global --add safe.directory "$(pwd)/.git" | |
| 64 | + git config --global --add safe.directory "$(pwd)/git/ext/gitdb" | ||
| 65 | + git config --global --add safe.directory "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" | ||
| 64 | 66 | git config --global core.autocrlf false | |
| 65 | 67 | ||
| 66 | 68 | - &prepare-repo | |
0 commit comments