parallel-checkout: fix stack buffer overflow in Windows poll() with many workers - #981
Open
tyrielv wants to merge 3 commits into
Open
parallel-checkout: fix stack buffer overflow in Windows poll() with many workers#981tyrielv wants to merge 3 commits into
tyrielv wants to merge 3 commits into
Conversation
tyrielv
force-pushed
the
vfs-fix-poll-worker-overflow
branch
2 times, most recently
from
September 1, 2026 19:14
04e80b5 to
7543596
Compare
The Windows implementation of poll() collects one wait handle per polled
descriptor in
HANDLE h, handle_array[FD_SETSIZE + 2];
and appends to it without a bounds check. It then writes a NULL sentinel
at handle_array[nhandles]. A caller with enough live descriptors
therefore writes past the end of the array and corrupts the stack. The
corruption is silent, and when it reaches the stack cookie the process
aborts with STATUS_STACK_BUFFER_OVERRUN.
The array is not the only limit. The collected handles are passed to
MsgWaitForMultipleObjects (nhandles, handle_array, FALSE,
wait_timeout, QS_ALLINPUT);
which waits on at most MAXIMUM_WAIT_OBJECTS objects, and QS_ALLINPUT adds
the thread message queue as one more object beyond the handles. The code
shows this, because it reports the message queue as
WAIT_OBJECT_0 + nhandles. One further handle is poll()'s own event
object. So at most MAXIMUM_WAIT_OBJECTS - 2 descriptors can be waited on,
which is the tighter of the two bounds and is well inside the array.
Define that limit as POLL_MAX_DESCRIPTORS next to the poll() declaration,
and refuse to collect beyond it, returning EINVAL. Two preprocessor
checks tie the constant to MAXIMUM_WAIT_OBJECTS and to the size of
handle_array, so the two cannot drift apart. poll() is now memory-safe
for every input, and a case that previously smashed the stack fails
cleanly.
Undo the WSAEventSelect() registrations before returning. The loop that
normally does this runs after the wait, and the new error path skips it,
which would otherwise leave those sockets associated with poll()'s static
event object and let later socket activity disturb an unrelated poll().
Note that the limit is on the number of 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. Sockets are
all multiplexed onto the one event object. Callers routinely pass sparse
arrays, for example run_processes_parallel(), which sizes its pollfd
array to the configured job count and leaves the unused slots at fd = -1.
Rejecting a large nfd would break such callers even though they never
come close to the wait limit.
For platforms with a native poll(), which has no such limit, define
POLL_MAX_DESCRIPTORS to INT_MAX so that callers can clamp against it
unconditionally.
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
On Windows, `git checkout` and `git reset --hard` can abort with
*** stack smashing detected ***: terminated
and exit code 0xC0000409 (STATUS_STACK_BUFFER_OVERRUN) when
checkout.workers is large, or when it is set to 0 on a machine with many
logical processors.
gather_results_from_workers() polls one pipe per checkout worker. Windows
has no native poll(), so compat/poll emulates it with
MsgWaitForMultipleObjects(), which cannot wait on more than
POLL_MAX_DESCRIPTORS descriptors at once. compat/poll collects one handle
per polled descriptor in a fixed stack array, so a higher worker count
writes past the end of that array and corrupts the stack.
run_parallel_checkout() clamped num_workers only by the number of files.
Clamp it to POLL_MAX_DESCRIPTORS as well. That function is the single
choke point before the workers start and the poll() loop runs. Clamp
silently: fewer workers is correct behaviour, and a warning would fire on
every checkout on a large machine. A single poll() loop cannot usefully
drive more readers than this anyway. On platforms with a native poll()
the limit is INT_MAX, so the clamp is a no-op.
The problem became reachable in 2.54. Before that, online_cpus() used
GetSystemInfo(), which reports only the processors in the current
processor group, and a group holds at most 64. That accidental ceiling
kept the array in bounds. The move to
GetLogicalProcessorInformationEx() is correct and reports the true
system-wide count, which exposed the latent bug.
Document the cap, because checkout.workers is otherwise described as
using one worker per logical core with no upper bound.
Add a test that asserts the clamp. test_checkout_workers() counts the
workers that were actually spawned, so the test can check the effective
count rather than only that the checkout succeeded. The test is limited
to Windows, because that is the only platform where the cap applies.
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
pp_buffer_io() polls one pipe for each child that is sending output, and a
second one for each child that is being fed on stdin. On Windows poll() is
emulated with MsgWaitForMultipleObjects(), which cannot wait on more than
POLL_MAX_DESCRIPTORS descriptors at once.
A job count above that limit is reachable in practice. fetch.parallel,
submodule.fetchJobs and hook.jobs all accept an explicit value, and a value
of 0 means "use online_cpus()", which on a machine with many cores is well
above the limit. Before the previous commit such a run corrupted the stack.
Now poll() returns EINVAL, and pp_buffer_io() turns that into
die_errno("poll"), so the operation fails outright.
Limit how many children run at the same time, so that a large job count
degrades into less concurrency instead of an error. Only concurrency is
limited. The configured maximum is still used for the size of the child and
pollfd arrays, and is still reported by the trace, so the number of tasks
that are run in total does not change. Unused pollfd slots hold -1 and are
skipped by poll(), so the larger array costs nothing.
Divide the limit by two, because a child can hold two descriptors: one for
its output and one for its input. Callers that group output are the only ones
affected; with opts.ungroup set the caller does its own I/O and poll() is not
involved.
On platforms with a native poll() there is no such limit,
POLL_MAX_DESCRIPTORS is INT_MAX, and this is a no-op.
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
tyrielv
force-pushed
the
vfs-fix-poll-worker-overflow
branch
from
September 1, 2026 20:35
7543596 to
6ca8024
Compare
tyrielv
marked this pull request as ready for review
September 1, 2026 23:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fork contains changes specific to monorepo scenarios. If you are an
external contributor, then please detail your reason for submitting to
this fork:
GVFS Protocol.
Upstream: git-for-windows#6395
(git-for-windows#6395) carries the same three
commits against
main. This PR takes them early so the fix reaches VFS forGit users before the next rebase onto git-for-windows.
Symptom
On Windows,
git checkoutandgit reset --hardcan abort withand exit code
0xC0000409(STATUS_STACK_BUFFER_OVERRUN). This is memorycorruption, not a normal error. The process dies before Trace2 writes its log,
so nothing shows up in a trace. A
.git/index.lockis left behind.It happens when
checkout.workersis large, or when it is0(meaning "useonline_cpus()") on a machine with many logical processors.Mechanism
gather_results_from_workers()inparallel-checkout.cpolls one pipe percheckout worker:
Windows has no native
poll(), socompat/poll/poll.cemulates it withMsgWaitForMultipleObjects(). It collects one handle per polled descriptor in afixed stack array:
FD_SETSIZEis the Winsock default 64, and nothing in the build overrides it.run_parallel_checkout()clampsnum_workersonly 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/polluses index 0 for its own event object.QS_ALLINPUTadds the thread message queue as an implicit wait object. Thecode confirms this, because it reports the message queue as
WAIT_OBJECT_0 + nhandles.So
nhandles + 1 <= MAXIMUM_WAIT_OBJECTS, which gives at mostMAXIMUM_WAIT_OBJECTS - 2= 62 descriptors.That value is defined once, as
POLL_MAX_DESCRIPTORSincompat/poll/poll.halongside the
poll()declaration it constrains, and both callers clamp againstit.
compat/posix.hdefines it toINT_MAXwhere a nativepoll()is used, sothe callers need no
#ifdef.Why the array cannot simply be enlarged
MAXIMUM_WAIT_OBJECTSis a kernel limit, not a header convenience. Passing morehandles fails with
ERROR_INVALID_PARAMETER. Growing the array would only turnmemory 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.candcompat/poll/poll.care unchanged between 2.53 and2.54. Only
online_cpus()changed:GetSystemInfo()GetLogicalProcessorInformationEx()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 andmust stay; it only exposed a latent bug.
The changes
compat/poll: do not collect more handles than the wait supports— definesPOLL_MAX_DESCRIPTORS(62) next to thepoll()declaration and refuses tocollect beyond it, returning
EINVALinstead of appending past the end of thearray. Two preprocessor assertions tie the constant to
MAXIMUM_WAIT_OBJECTSand to the size of
handle_array, so they cannot drift apart.poll()is nowmemory-safe for every input.
The error path also undoes the
WSAEventSelect()registrations made earlier inthe same call. The loop that normally does that runs after the wait, so
returning early would otherwise leave those sockets bound to
poll()'s staticevent object and let later socket activity disturb an unrelated
poll().The limit is on the handles actually collected, not on
nfd. Those aredifferent: 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 itspollfdarray to the configured job countand leaves the unused slots at
fd = -1. An earlier revision of this PRrejected a large
nfdinstead, which broket7406(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, soPOLL_MAX_DESCRIPTORSisINT_MAXand callers can clamp against itunconditionally.
parallel-checkout: limit worker count to what poll() can wait on— clampsnum_workerstoPOLL_MAX_DESCRIPTORSinrun_parallel_checkout(), the singlechoke 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.workerswas described as using one workerper logical core with no upper bound.
run-command: limit concurrent children to what poll() can wait on— thesame limit applied to the other
poll()fan-out.pp_buffer_io()polls onepipe per child sending output and a second per child being fed on stdin, and
fetch.parallel/submodule.fetchJobs/hook.jobsall accept a high value(or
0, meaningonline_cpus()). Without this, change 1 would convert the oldstack smash on that path into a hard
die_errno("poll"). Only concurrency islimited; 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.workersis used verbatim, andonline_cpus()isconsulted only when the value is
0or less.Before the fix, on a 12-core machine, this crashed 3 out of 3 runs with
exit=-1073740791(0xC0000409) and left.git/index.lockbehind. After thefix it exits 0 on 3 out of 3 runs, with the files correctly updated. A
checkout.workers 16checkout 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
nhandlesreflects the workers pending at that instant, not the workersspawned. On warm cache, pipes answer immediately and few workers stay pending.
Measured before the fix:
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_workerscounts the workers actually spawned from aTRACE2 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 theonly platform where the cap applies.
Testing
t/t2080-parallel-checkout-basics.sh.t2080,t2081,t2082,t0061,t5526andt7406all pass on Windows.regression check.
Workaround for affected users
Any value at or below 62 avoids the overflow. No downgrade is needed.