parallel-checkout: fix stack buffer overflow in Windows poll() with many workers

[tyrielv]

Symptom

On Windows, git checkout and git reset --hard can abort with

*** stack smashing detected ***: terminated

and exit code 0xC0000409 (STATUS_STACK_BUFFER_OVERRUN). This is memory
corruption, not a normal error. The process dies before Trace2 writes its log,
so nothing shows up in a trace. A .git/index.lock is left behind.

It happens when checkout.workers is large, or when it is 0 (meaning "use
online_cpus()") on a machine with many logical processors.

Mechanism

gather_results_from_workers() in parallel-checkout.c polls one pipe per
checkout worker:

CALLOC_ARRAY(pfds, num_workers);
...
poll(pfds, num_workers, -1);

Windows has no native poll(), so compat/poll/poll.c emulates it with
MsgWaitForMultipleObjects(). It collects one handle per polled descriptor in a
fixed stack array:

HANDLE h, handle_array[FD_SETSIZE + 2];   /* 64 + 2 = 66 entries */
...
handle_array[nhandles++] = h;             /* no bounds check */
...
handle_array[nhandles] = NULL;            /* sentinel, no bounds check */

FD_SETSIZE is the Winsock default 64, and nothing in the build overrides it.
run_parallel_checkout() clamps num_workers only against the number of files,
never against the array size or the Windows wait limit. A high worker count
therefore writes past the end of the array and smashes the stack.

Sockets are not involved: they are multiplexed onto a single event through
WSAEventSelect, so only non-socket descriptors consume a slot.

Why 62, and where the limit lives

Two of the wait slots are never available for descriptors:

• compat/poll uses index 0 for its own event object.
• QS_ALLINPUT adds the thread message queue as an implicit wait object. The
  code confirms this, because it reports the message queue as
  WAIT_OBJECT_0 + nhandles.

So nhandles + 1 <= MAXIMUM_WAIT_OBJECTS, which gives at most
MAXIMUM_WAIT_OBJECTS - 2 = 62 descriptors.

That value is defined once, as POLL_MAX_DESCRIPTORS in compat/poll/poll.h
alongside the poll() declaration it constrains, and both callers clamp against
it. compat/posix.h defines it to INT_MAX where a native poll() is used, so
the callers need no #ifdef.

Why the array cannot simply be enlarged

MAXIMUM_WAIT_OBJECTS is a kernel limit, not a header convenience. Passing more
handles fails with ERROR_INVALID_PARAMETER. Growing the array would only turn
memory corruption into a functional failure. Support for more descriptors needs
a wait tree (helper threads each waiting on at most 62 handles) or completion
ports, which is out of scope here.

Why it surfaced in 2.54

parallel-checkout.c and compat/poll/poll.c are unchanged between 2.53 and
2.54. Only online_cpus() changed:

Version	API	Result
2.53	GetSystemInfo()	processors in the current processor group only; a group holds at most 64
2.54+	GetLogicalProcessorInformationEx()	true system-wide logical processor count

The old API could never report more than 64, so the array always fit. That
ceiling was accidental, not deliberate. The online_cpus() change is correct and
must stay; it only exposed a latent bug.

The changes

1. compat/poll: do not collect more handles than the wait supports — defines
   POLL_MAX_DESCRIPTORS (62) next to the poll() declaration and refuses to
   collect beyond it, returning EINVAL instead of appending past the end of the
   array. Two preprocessor assertions tie the constant to MAXIMUM_WAIT_OBJECTS
   and to the size of handle_array, so they cannot drift apart. poll() is now
   memory-safe for every input.

   The error path also undoes the WSAEventSelect() registrations made earlier in
   the same call. The loop that normally does that runs after the wait, so
   returning early would otherwise leave those sockets bound to poll()'s static
   event object and let later socket activity disturb an unrelated poll().

   The limit is on the handles actually collected, not on nfd. Those are
   different: a descriptor only takes a handle when it is non-negative, is not a
   socket, and has no events pending yet. Callers routinely pass sparse arrays —
   run_processes_parallel() sizes its pollfd array to the configured job count
   and leaves the unused slots at fd = -1. An earlier revision of this PR
   rejected a large nfd instead, which broke t7406 (submodule.fetchJobs 67,
   with only a handful of live pipes) with fatal: poll: Invalid argument.

   On platforms with a native poll() there is no such limit, so
   POLL_MAX_DESCRIPTORS is INT_MAX and callers can clamp against it
   unconditionally.

2. parallel-checkout: limit worker count to what poll() can wait on — clamps
   num_workers to POLL_MAX_DESCRIPTORS in run_parallel_checkout(), the single
   choke point before the workers start. The clamp is silent: fewer workers is
   correct, and a warning would fire on every checkout on a large machine. Also
   documents the cap, since checkout.workers was described as using one worker
   per logical core with no upper bound.

3. run-command: limit concurrent children to what poll() can wait on — the
   same limit applied to the other poll() fan-out. pp_buffer_io() polls one
   pipe per child sending output and a second per child being fed on stdin, and
   fetch.parallel / submodule.fetchJobs / hook.jobs all accept a high value
   (or 0, meaning online_cpus()). Without this, change 1 would convert the old
   stack smash on that path into a hard die_errno("poll"). Only concurrency is
   limited; the configured maximum still sizes the arrays and is still reported by
   the trace, so the total number of tasks run is unchanged.

Reproduction

No clone, no special hardware, about 10 seconds. A many-core machine is not
required: a positive checkout.workers is used verbatim, and online_cpus() is
consulted only when the value is 0 or less.

# Use a NEW directory every attempt (see the note on timing below).
$repo = "C:\tmp\poll-repro-$(Get-Random)"
New-Item -ItemType Directory -Force -Path $repo | Out-Null
Set-Location $repo

git init -q -b main .
git config user.email repro@example.com
git config user.name  repro
git config checkout.workers 200
git config checkout.thresholdForParallelism 1

New-Item -ItemType Directory -Force -Path dir | Out-Null
1..400 | ForEach-Object { Set-Content -Path "dir\f$_.txt" -Value "base $_" -NoNewline }
git add -A; git commit -qm base

git checkout -qb other
1..400 | ForEach-Object { Set-Content -Path "dir\f$_.txt" -Value "changed $_ padding padding padding" -NoNewline }
git commit -qam changed

git checkout -q main
Write-Host "exit=$LASTEXITCODE"

Before the fix, on a 12-core machine, this crashed 3 out of 3 runs with
exit=-1073740791 (0xC0000409) and left .git/index.lock behind. After the
fix it exits 0 on 3 out of 3 runs, with the files correctly updated. A
checkout.workers 16 checkout still works, as before.

The crash is not deterministic

poll() only appends a descriptor when the worker's pipe has no data ready yet,
so nhandles reflects the workers pending at that instant, not the workers
spawned. On warm cache, pipes answer immediately and few workers stay pending.
Measured before the fix:

workers	result
16, 64, 65, 70, 72, 74, 76, 78	pass
80	crashed once, then passed 3 times
200, repeated checkouts in the same repo	passed 4 times
200, fresh repository each run	crashed 3 of 3

The first out-of-bounds write happens at 65 descriptors by arithmetic, but the
corruption does not reliably reach the stack cookie until well past that. The
corruption is real from 65 onward whether or not it crashes. That is why the fix
targets the contract (62), not the observed crash point.

For the same reason, the added test asserts the effective worker count rather
than a crash. test_checkout_workers counts the workers actually spawned from a
TRACE2 log, so the test verifies the clamp took effect (62) instead of merely
checking that the checkout did not crash. It is MINGW-gated, because that is the
only platform where the cap applies.

Testing

• New test in t/t2080-parallel-checkout-basics.sh. t2080, t2081, t2082,
  t0061, t5526 and t7406 all pass on Windows.
• Manual verification with the reproduction above, plus a low-worker-count
  regression check.

Workaround for affected users

git config checkout.workers 16

Any value at or below 62 avoids the overflow. No downgrade is needed.

[Copilot]

🟡 Changes recommended

The run-command cap lacks direct regression coverage and documentation for affected user-facing settings.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents Windows poll() handle overflow and caps affected parallel operations.

Changes:

• Enforces the 62-descriptor Windows wait limit.
• Caps checkout and child-process concurrency.
• Adds checkout documentation and regression coverage.

File summaries

File	Description
compat/poll/poll.c	Adds bounds checking and socket cleanup.
compat/poll/poll.h	Defines the descriptor limit.
compat/posix.h	Provides an unlimited native-poll fallback.
parallel-checkout.c	Caps checkout workers.
run-command.c	Caps concurrent buffered children.
Documentation/config/checkout.adoc	Documents the checkout cap.
t/t2080-parallel-checkout-basics.sh	Tests checkout worker limiting.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[dscho]

I really wish that there had been a way to steer your review, @Copilot. As it were, you completely missed the point that compat/poll/ is vendored in, and you really needed (and failed) to determine the original source of that code and to then figure out whether the upstream source code already contains a fix for the same bug. Please correct that omission.

[dscho]

Unfortunately, this Copilot review proved pretty much a waste of my time, and only a local session where I could heavily steer Copilot to do what I need it to do was able to win back some of that lost time.

This local analysis shows that this code hails from gnulib, and even current gnulib does not contain a fix for this overflow. At commit coreutils/gnulib@6753261, lib/poll.c still allocates handle_array[FD_SETSIZE + 2], appends without checking that bound, passes nhandles to MsgWaitForMultipleObjects(), and writes the sentinel afterward.

Git's 49521af imported this file verbatim from gnulib, f0bd664 updated it from gnulib again, and the Makefile marks compat/poll/% as THIRD_PARTY_SOURCES. This is therefore an unfixed upstream issue, not a Git-local implementation.

Since this is a pretty urgent fix that cannot wait for the lengthy process where the fix is first submitted upstream, then we would need to wait for an upstream gnulib release, and then we would need to ingest all of the upstream changes, I am very much in favor of getting this merged downstream (here) first and then upstream the applicable changes.

[derrickstolee]

I'm happy with these changes, now that the context of this contrib/ directory are explained to me. Is this something we should be polishing for an upstream contribution?