parallel-checkout: fix stack buffer overflow in Windows poll() with many workers - #981
Draft
tyrielv wants to merge 2 commits into
Draft
parallel-checkout: fix stack buffer overflow in Windows poll() with many workers#981tyrielv wants to merge 2 commits into
tyrielv wants to merge 2 commits into
Conversation
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(). That function waits on at most
MAXIMUM_WAIT_OBJECTS objects, and compat/poll collects one handle per
polled descriptor in a fixed stack array, without a bounds check. A high
worker count therefore writes past the end of that array and corrupts the
stack.
Two of the wait slots are not available for descriptors: compat/poll uses
the first for its own event object, and QS_ALLINPUT adds the thread
message queue as an implicit object. The code confirms this, because it
reports the message queue as WAIT_OBJECT_0 + nhandles. So the usable
limit is MAXIMUM_WAIT_OBJECTS - 2 descriptors.
Clamp the worker count to that limit in run_parallel_checkout(), which 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-threaded poll() loop
cannot usefully drive more readers than this anyway.
Enlarging the array does not help. MAXIMUM_WAIT_OBJECTS is a kernel
limit, so passing more handles fails with ERROR_INVALID_PARAMETER. That
would replace memory corruption with a functional failure. Support for
more descriptors needs a wait tree or completion ports, which is out of
scope here.
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.
Add a test that runs a checkout with a very high worker count and asserts
that it succeeds. The test does not verify that the clamp happens, and it
cannot easily do so: the clamp is not reported anywhere. It also cannot
assert that any particular worker count fails without the clamp, because
poll() only takes a handle for a worker whose pipe has no data yet, so
the number of handles depends on timing and I/O state.
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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. So at most MAXIMUM_WAIT_OBJECTS - 1 handles can
be collected, which is the tighter of the two bounds and is well inside
the array.
Refuse to collect beyond that, and return EINVAL. This makes poll()
memory-safe for every input, and turns a case that previously smashed the
stack into a clean error.
Note that the bound 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. 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.
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tyrielv
force-pushed
the
vfs-fix-poll-worker-overflow
branch
from
September 1, 2026 03:00
8fbf79e to
04e80b5
Compare
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.
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 not 64
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.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
parallel-checkout: limit worker count on Windows— clampnum_workersto
MAXIMUM_WAIT_OBJECTS - 2inrun_parallel_checkout(), the single chokepoint before the workers start. The clamp is silent: fewer workers is correct,
and a warning would fire on every checkout on a large machine. There is no
measurable cost, because a single-threaded
poll()loop cannot usefully drivemore concurrent pipe readers than that.
compat/poll: do not collect more handles than the wait supports— boundthe number of collected handles by
MAXIMUM_WAIT_OBJECTS - 1and returnEINVALinstead of appending past the end of the array.poll()is thenmemory-safe for every input, and a case that previously smashed the stack now
fails cleanly.
The bound 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.The two changes address different bounds. The
poll()guard makes the emulationsafe for any caller. The clamp keeps parallel checkout inside the wait limit so
it keeps using workers instead of failing.
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 that a high worker count succeeds.
It does not verify that the clamp happens (the clamp is not reported anywhere),
and it does not assert that any particular worker count fails without the fix,
because such a test would be flaky.
Testing
t/t2080-parallel-checkout-basics.sh;t2080,t2081,t2082and
t7406all pass on Windows.regression check.
Workaround for affected users
Any value at or below 62 avoids the overflow. No downgrade is needed.