From 178a67cc7365f135fc9b79776ea43416dfae6615 Mon Sep 17 00:00:00 2001 From: Cameron Taggart Date: Wed, 19 Aug 2026 06:08:04 +0100 Subject: [PATCH] fix(builtins): avoid deadlock when a builtin reads a process substitution `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> --- brush-builtins/src/mapfile.rs | 6 ++- brush-builtins/src/read.rs | 8 +++- brush-core/src/openfiles.rs | 38 ++++++++++++++++ .../tests/cases/compat/builtins/mapfile.yaml | 45 +++++++++++++++++++ .../tests/cases/compat/builtins/read.yaml | 30 +++++++++++++ .../word_expansion/command_substitution.yaml | 18 ++++++++ 6 files changed, 141 insertions(+), 4 deletions(-) diff --git a/brush-builtins/src/mapfile.rs b/brush-builtins/src/mapfile.rs index ba5505350..2d6f7f044 100644 --- a/brush-builtins/src/mapfile.rs +++ b/brush-builtins/src/mapfile.rs @@ -82,8 +82,10 @@ impl builtins::Command for MapFileCommand { .try_fd(self.fd) .ok_or_else(|| ErrorKind::BadFileDescriptor(self.fd))?; - // Read! - let results = self.read_entries(input_file)?; + // Read! The read is synchronous and can block until the write end is closed, + // so it must not hold onto the runtime worker while it waits. + let results = + brush_core::openfiles::without_parking_worker(|| self.read_entries(input_file))?; if let Some(origin) = self.origin { // -O: preserve existing array, assign at offset. diff --git a/brush-builtins/src/read.rs b/brush-builtins/src/read.rs index 56809c60d..910354473 100644 --- a/brush-builtins/src/read.rs +++ b/brush-builtins/src/read.rs @@ -114,8 +114,12 @@ impl builtins::Command for ReadCommand { // Convert timeout to Duration. let timeout = self.timeout_in_seconds.map(Duration::from_secs_f64); - // Perform the read operation (potentially with timeout). - let read_result = self.read_line(input_stream, context.stderr(), timeout)?; + // Perform the read operation (potentially with timeout). Without -t this blocks + // until the write end is closed, so it must not hold onto the runtime worker + // while it waits. + let read_result = brush_core::openfiles::without_parking_worker(|| { + self.read_line(input_stream, context.stderr(), timeout) + })?; // Determine whether to skip IFS splitting (for -N option). let skip_ifs_splitting = self.return_after_n_chars_no_delimiter.is_some(); diff --git a/brush-core/src/openfiles.rs b/brush-core/src/openfiles.rs index f51ece0de..292be668d 100644 --- a/brush-core/src/openfiles.rs +++ b/brush-core/src/openfiles.rs @@ -102,6 +102,44 @@ pub fn null() -> Result { Ok(file.into()) } +/// Runs a closure that may block indefinitely on a descriptor, without parking the +/// runtime worker it is running on. +/// +/// Builtins that consume a descriptor (`mapfile`, `read`) read it synchronously, and +/// they run inline on the task that invoked them. On the multi-threaded runtime that +/// task is a worker thread, and a worker sitting in `read(2)` cannot poll the tasks +/// held in its own queues. That deadlocks whenever one of those tasks is the producer +/// responsible for closing the write end being read from. +/// +/// `mapfile -t lines < <(cmd)` inside a command substitution is exactly that shape: the +/// producer for `<(cmd)` is spawned from the very worker that then blocks, so it lands +/// in that worker's LIFO slot, which other workers cannot steal. Nothing ever writes the +/// data or drops the write end, so EOF never arrives and the read never returns. +/// +/// [`tokio::task::block_in_place`] hands the current worker's queues to another worker +/// before blocking, which keeps the producer runnable. +#[cfg(any(unix, windows))] +pub fn without_parking_worker(f: impl FnOnce() -> T) -> T { + // `block_in_place` is only meaningful on the multi-threaded runtime, and panics on + // the current-thread one. Outside a runtime there is no worker to park. + if tokio::runtime::Handle::try_current() + .is_ok_and(|h| h.runtime_flavor() == tokio::runtime::RuntimeFlavor::MultiThread) + { + tokio::task::block_in_place(f) + } else { + f() + } +} + +/// Runs a closure that may block indefinitely on a descriptor. +/// +/// This target only ever runs a current-thread runtime, so there is no worker to hand +/// off to and the closure is run inline. +#[cfg(not(any(unix, windows)))] +pub fn without_parking_worker(f: impl FnOnce() -> T) -> T { + f() +} + impl Clone for OpenFile { fn clone(&self) -> Self { match self { diff --git a/brush-shell/tests/cases/compat/builtins/mapfile.yaml b/brush-shell/tests/cases/compat/builtins/mapfile.yaml index 28a641d7e..d0584cfc1 100644 --- a/brush-shell/tests/cases/compat/builtins/mapfile.yaml +++ b/brush-shell/tests/cases/compat/builtins/mapfile.yaml @@ -200,3 +200,48 @@ cases: declare -n ref=target mapfile -t -O 2 ref < <(echo -e "a\nb") declare -p target + + # + # Process substitution inside command substitution. + # + # These deadlocked before the read was moved off the runtime worker: the + # producer for `<(...)` is spawned from the same worker that mapfile then + # blocked on, so it could never run to close the write end. + # + - name: "mapfile from process substitution inside command substitution" + timeout_in_seconds: 10 + stdin: | + x=$(mapfile -t arr < <(echo -e "a\nb"); echo "${arr[*]}") + echo "$x" + + - name: "mapfile from process substitution in function in command substitution" + timeout_in_seconds: 10 + stdin: | + f() { + mapfile -t arr < <(echo -e "a\nb") + echo "${arr[*]}" + } + x=$(f) + echo "$x" + + - name: "repeated mapfile from process substitution in command substitution" + timeout_in_seconds: 10 + stdin: | + f() { + mapfile -t a < <(echo one) + mapfile -t b < <(echo two) + echo "${a[*]} ${b[*]}" + } + x=$(f) + echo "$x" + + - name: "mapfile from process substitution nested two command substitutions deep" + timeout_in_seconds: 10 + stdin: | + g() { + mapfile -t arr < <(echo deep) + echo "${arr[*]}" + } + f() { echo "$(g)"; } + x=$(f) + echo "$x" diff --git a/brush-shell/tests/cases/compat/builtins/read.yaml b/brush-shell/tests/cases/compat/builtins/read.yaml index 8dc0e358a..5ed3d65a2 100644 --- a/brush-shell/tests/cases/compat/builtins/read.yaml +++ b/brush-shell/tests/cases/compat/builtins/read.yaml @@ -390,3 +390,33 @@ cases: echo "arr[0]: ${arr[0]}" echo "arr[1]: ${arr[1]}" echo "arr[2]: ${arr[2]}" + + # + # Process substitution inside command substitution. + # + # These deadlocked before the read was moved off the runtime worker: the + # producer for `<(...)` is spawned from the same worker that read then + # blocked on, so it could never run to close the write end. + # + - name: "while read loop fed by process substitution inside command substitution" + timeout_in_seconds: 10 + stdin: | + x=$(while read -r line; do echo "got $line"; done < <(echo -e "a\nb")) + echo "$x" + + - name: "while read loop fed by process substitution in function in command substitution" + timeout_in_seconds: 10 + stdin: | + f() { + while read -r line; do + echo "got $line" + done < <(echo -e "a\nb") + } + x=$(f) + echo "$x" + + - name: "read from process substitution inside command substitution" + timeout_in_seconds: 10 + stdin: | + x=$(read -r line < <(echo hello); echo "$line") + echo "$x" diff --git a/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml b/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml index 45951b9b4..ce3617689 100644 --- a/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml +++ b/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml @@ -220,3 +220,21 @@ cases: $($(:)) echo "exit code: $?" + + # + # A command substitution whose body redirects a builtin from a process + # substitution. The producer for `<(...)` is spawned from the worker running + # the substitution, so a builtin that reads it synchronously must not hold + # that worker while it waits. + # + - name: "Command substitution containing a builtin fed by process substitution" + timeout_in_seconds: 10 + stdin: | + x=$(mapfile -t arr < <(echo -e "one\ntwo"); echo "${arr[*]}") + echo "$x" + + - name: "Command substitution containing an external command fed by process substitution" + timeout_in_seconds: 10 + stdin: | + x=$(cat < <(echo hello)) + echo "$x"