fix(builtins): avoid deadlock when a builtin reads a process substitution - #5
Conversation
…tion
`mapfile -t lines < <(cmd)` never returns when it appears inside a command
substitution, while the same redirection at top level completes immediately:
x=$(mapfile -t a < <(printf "one\ntwo\n"); echo "${a[*]}") # hangs
mapfile -t a < <(printf "one\ntwo\n"); echo "${a[*]}" # fine
The cause is scheduling, not descriptor lifetime. `setup_process_substitution`
spawns the producer with `tokio::spawn`. A spawn issued from a runtime worker
lands in that worker's LIFO slot, which other workers are not able to steal.
`mapfile` and `read` then read their descriptor synchronously, inline on the
task that invoked them -- so inside a command substitution they block the very
worker holding the producer. Nothing writes the data, nothing drops the write
end, and the read waits for an EOF that cannot arrive.
At top level the producer is spawned from the thread running `block_on`, which
is outside the worker pool, so the task goes to the global queue and any worker
can pick it up. An external consumer such as `cat` is likewise unaffected: it
awaits a child process and yields, leaving the worker free to poll its own LIFO
slot. Only an in-process builtin that blocks can wedge this way.
Add `openfiles::without_parking_worker`, which runs a potentially blocking read
inside `tokio::task::block_in_place` so the worker hands its queues -- LIFO slot
included -- to another worker before blocking, and apply it to the two builtins
that read a descriptor synchronously: `mapfile`/`readarray`, and `read` (which
polls only when `-t` is supplied). It is a no-op outside a multi-threaded
runtime, where `block_in_place` is both unavailable and unnecessary.
The `OpenFile` reference-counting contract is deliberately left alone; sharing
handles by `Arc` is what keeps deeply nested execution from exhausting the
process-wide descriptor table, and it was never the problem here.
Adds compat cases covering the builtin, the `while read ... done < <(...)` loop
form, and nesting two command substitutions deep. All of them fail by timing out
without the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
256c6e8 to
178a67c
Compare
|
Correcting a claim in this PR's description, now that it has been tested rather than reasoned about. The description says:
That overstates the risk. Built with $ x=$(cat < <(echo hello)); echo "[$x]" # [hello]
$ x=$(head -1 < <(echo hello)); echo "[$x]" # [hello]
$ x=$(sort < <(printf "b\na\n")); echo "[$x]" # [a b]
$ x=$(wc -l < <(printf "a\nb\n")); echo "[$x]" # [2]All complete immediately. The reason is the split in So the fix in this PR is not leaving a known hole behind the |
|
Correcting my earlier comment on this PR — the conclusion held up, the explanation did not. I claimed the bundled coreutils are safe from the deadlock because they dispatch through First, a single-command pipeline does not take the owned-shell path at all. Second, and the actual reason: the bundled coreutils do not run in-process. As documented in $ brush -c 'echo "shell pid: $$"; cat /proc/self/status | grep -E "^(Name|Pid):"'
shell pid: 402965
Name: brush
Pid: 402983 # different process, re-executed brush binarySo The practical upshot is unchanged and slightly stronger: the fix here is correctly scoped, and the bundled coreutils are not a latent instance of this bug. But the invariant is narrower than my comment implied — any builtin that blocks on a descriptor in-process is at risk, and the inline path is the common case rather than the exceptional one. I've opened a follow-up to write that down where builtin authors will actually see it. |
…tion (#5) `mapfile -t lines < <(cmd)` never returns when it appears inside a command substitution, while the same redirection at top level completes immediately: x=$(mapfile -t a < <(printf "one\ntwo\n"); echo "${a[*]}") # hangs mapfile -t a < <(printf "one\ntwo\n"); echo "${a[*]}" # fine The cause is scheduling, not descriptor lifetime. `setup_process_substitution` spawns the producer with `tokio::spawn`. A spawn issued from a runtime worker lands in that worker's LIFO slot, which other workers are not able to steal. `mapfile` and `read` then read their descriptor synchronously, inline on the task that invoked them -- so inside a command substitution they block the very worker holding the producer. Nothing writes the data, nothing drops the write end, and the read waits for an EOF that cannot arrive. At top level the producer is spawned from the thread running `block_on`, which is outside the worker pool, so the task goes to the global queue and any worker can pick it up. An external consumer such as `cat` is likewise unaffected: it awaits a child process and yields, leaving the worker free to poll its own LIFO slot. Only an in-process builtin that blocks can wedge this way. Add `openfiles::without_parking_worker`, which runs a potentially blocking read inside `tokio::task::block_in_place` so the worker hands its queues -- LIFO slot included -- to another worker before blocking, and apply it to the two builtins that read a descriptor synchronously: `mapfile`/`readarray`, and `read` (which polls only when `-t` is supplied). It is a no-op outside a multi-threaded runtime, where `block_in_place` is both unavailable and unnecessary. The `OpenFile` reference-counting contract is deliberately left alone; sharing handles by `Arc` is what keeps deeply nested execution from exhausting the process-wide descriptor table, and it was never the problem here. Adds compat cases covering the builtin, the `while read ... done < <(...)` loop form, and nesting two command substitutions deep. All of them fail by timing out without the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
mapfile -t lines < <(cmd)never returns when it appears inside a command substitution, while the identical redirection at top level completes immediately:while read ... done < <(cmd)inside a command substitution hangs the same way. There is no error, no output, and no timeout — the shell simply never comes back.Cause
Scheduling, not descriptor lifetime.
setup_process_substitutionstarts the producer withtokio::spawn. A spawn issued from a runtime worker lands in that worker's LIFO slot, which other workers are not able to steal.mapfileandreadread their descriptor synchronously and run inline on the task that invoked them — so inside a command substitution they block the very worker that is holding the producer task. Nothing writes the data, nothing drops the write end, and the read waits on an EOF that cannot arrive.The two cases that do work confirm the mechanism rather than contradicting it:
mapfile < <(...)at top levelmain, i.e. theblock_onthreadmapfile < <(...)inside$( )tokio-rt-workercat < <(...)inside$( )tokio-rt-workerOnly an in-process builtin that blocks can wedge this way. Confirmed by instrumenting the write end with a
Weakhandle: itsArcstrong count goes1 → 0correctly in every passing case, and in the hanging case the producer task body never executes at all.Fix
Add
openfiles::without_parking_worker, which runs a potentially blocking read insidetokio::task::block_in_placeso the worker hands its queues — LIFO slot included — to another worker before blocking, keeping the producer runnable.Applied to the two builtins that read a descriptor synchronously, which are the only two in the default builtin set:
mapfile/readarrayread(which polls only when-tis given; without it the read blocks)It is a no-op outside a multi-threaded runtime, where
block_in_placeis both unavailable and unnecessary.The
OpenFilereference-counting contract is deliberately left alone. Sharing handles byArcis what keeps deeply nested execution from exhausting the process-wide descriptor table, and it was never the problem here.Tests
Nine compat cases covering the builtin form, the
while read ... done < <(...)loop form,readdirectly, and nesting two command substitutions deep — plus acatcontrol that was never affected and must keep passing.Validated against a build identical in every byte except that the body of
without_parking_workerwas replaced with a plain call, so the difference is attributable to the fix alone:The 8-case delta is exactly the new tests: all eight fail by timing out (SIGKILL) without the fix. The remaining 4 failures are pre-existing parser cases (
Error: newline before ...) present in both runs and untouched by this change.cargo fmt --checkandcargo clippyare clean.Overhead
block_in_placeis not free in principle, so it was measured. It is free here in practice, because the existing byte-at-a-time read loop dominates it:while readover 20k linesmapfile20k linesmapfilefrom process substitutionNotes
experimental-bundled-coreutilsbuiltins (cat,head, …) also read real descriptors and are not covered here. They are off by default; worth a look if that feature is ever promoted.setup_process_substitutionstill discards the producer's result and keeps itsTODO(execute): Don't execute synchronously!. Not part of this deadlock, but it is why a failing producer fails silently. Left for a separate change.This is the third defect of this class found by running a large real-world bash script under brush, after #2 (here-document pipe buffer) and #3 (here-document line continuations). The affected shape — a function calling helpers that read a process substitution — occurs 128 times in the ~95k-line script that surfaced it, so it is ordinary bash rather than an exotic corner.