Repository navigation
Commit 00a523f
parallel-checkout: fix stack buffer overflow in Windows poll() with many workers (#6395)
### 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:
```c
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:
```c
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.
```powershell
# 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.17 files changed
Lines changed: 410 additions & 3 deletions
File tree
- Documentation/config
- builtin
- compat
- poll
- t
- helper
- unit-tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
33 | 38 | | |
34 | 39 | | |
35 | 40 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1553 | 1553 | | |
1554 | 1554 | | |
1555 | 1555 | | |
| 1556 | + | |
1556 | 1557 | | |
1557 | 1558 | | |
1558 | 1559 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2351 | 2351 | | |
2352 | 2352 | | |
2353 | 2353 | | |
| 2354 | + | |
2354 | 2355 | | |
2355 | 2356 | | |
2356 | 2357 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2919 | 2919 | | |
2920 | 2920 | | |
2921 | 2921 | | |
| 2922 | + | |
2922 | 2923 | | |
2923 | 2924 | | |
2924 | 2925 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
303 | 303 | | |
304 | 304 | | |
305 | 305 | | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
306 | 343 | | |
307 | 344 | | |
308 | 345 | | |
| |||
504 | 541 | | |
505 | 542 | | |
506 | 543 | | |
507 | | - | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
508 | 554 | | |
509 | 555 | | |
510 | 556 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
62 | 78 | | |
63 | 79 | | |
64 | 80 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
133 | 133 | | |
134 | 134 | | |
135 | 135 | | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
136 | 146 | | |
137 | 147 | | |
138 | 148 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
798 | 798 | | |
799 | 799 | | |
800 | 800 | | |
| 801 | + | |
801 | 802 | | |
802 | 803 | | |
803 | 804 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
671 | 671 | | |
672 | 672 | | |
673 | 673 | | |
| 674 | + | |
| 675 | + | |
| 676 | + | |
| 677 | + | |
| 678 | + | |
| 679 | + | |
| 680 | + | |
674 | 681 | | |
675 | 682 | | |
676 | 683 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1658 | 1658 | | |
1659 | 1659 | | |
1660 | 1660 | | |
| 1661 | + | |
| 1662 | + | |
| 1663 | + | |
1661 | 1664 | | |
1662 | 1665 | | |
1663 | 1666 | | |
| |||
1893 | 1896 | | |
1894 | 1897 | | |
1895 | 1898 | | |
| 1899 | + | |
1896 | 1900 | | |
1897 | 1901 | | |
1898 | 1902 | | |
| |||
1902 | 1906 | | |
1903 | 1907 | | |
1904 | 1908 | | |
| 1909 | + | |
| 1910 | + | |
| 1911 | + | |
| 1912 | + | |
| 1913 | + | |
| 1914 | + | |
| 1915 | + | |
| 1916 | + | |
| 1917 | + | |
| 1918 | + | |
| 1919 | + | |
| 1920 | + | |
| 1921 | + | |
| 1922 | + | |
1905 | 1923 | | |
1906 | 1924 | | |
1907 | 1925 | | |
| |||
1923 | 1941 | | |
1924 | 1942 | | |
1925 | 1943 | | |
1926 | | - | |
| 1944 | + | |
1927 | 1945 | | |
1928 | 1946 | | |
1929 | 1947 | | |
| |||
0 commit comments