Repository navigation
fix(expander): walk nested spans inside a double-quoted word - #192
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
readline 8.3 made rl_free_line_state free the context of those three
readers on SIGINT, on the bet that an application's handler longjmps and
the reader never runs again. bash does, and readline83-003 taught
readline's own rl_getc to abort to the top level for handlers that
return. rl_getc_hook replaces that rl_getc and hands the reader EOF, so
the reader unwound through a freed `cxt`. On readline 8.3 at Debian's
patch level:
freed by: rl_getc_hook -> rl_take_signals -> rl_check_signals
-> _rl_handle_signal -> rl_free_line_state
-> _rl_state_sigcleanup -> _rl_isearch_cleanup
-> _rl_scxt_dispose
double free: rl_search_history -> _rl_isearch_cleanup
-> _rl_scxt_dispose
ASan reports the double free for ^R and M-x. Under a readline built with
ASan all three readers report a heap-use-after-free WRITE first, in
_rl_search_getchar. A release build takes a SIGSEGV on ^R, 3 runs of 3,
which is the login shell gone. The `ps2` failures in CI were the same
crash: that scenario shares the session ^R killed.
Guarding only rl_take_signals' own look is not enough, and the tests say
so: readline also looks at its deferred signal at the end of every
rl_redisplay, inside the search, and a ^C sent with the keys of a ^R came
through there. rl_intr.c puts a filter in front of readline's SIGINT
handler instead. While such a reader is up the signal is parked, the
reader gets EOF and unwinds with its context intact, the path every
readline before 8.3 took, and back in the main loop the ^C is replayed
into readline. Outside those readers the filter only forwards, so ^C
anywhere else is unchanged. Blocking SIGINT from the key's return to the
next wait would also work, but needs lifting around every widget and
completion, and a child forked on a path that misses it inherits it.
prompt_interrupt_test.py gains nsearch, mx and redraw;
rl_read_race_test.py gains nsearch and M-x. 48 and 34 ok on readline
8.1.2, 8.2.13 and 8.3 at patch levels 0, 3 and 6, and against a readline
built with ASan. On the old code the same files fail on 8.3.3, the redraw
scenario included.
Closes #194
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
prompt_row_writes_test.py failed on ubuntu-26.04 with "7 of 13 rows were whole inside a single write()". No row was split. The trace has `GREEN<glyph> RESET \r GREEN<glyph> RESET` in one write(), every prompt: readline 8.3 before readline83-004 reprints a prompt holding multibyte characters from column 0, deciding on a screen position where it needs a buffer offset. Debian's 8.3-4 carries patches 001 to 003. Built here with 006 the second copy is gone and the old test passes; built at 003 it fails the way CI does. The invariant is that every row is whole in one write(). The test counted the writes holding a row and compared them with the rows, which assumes one row per write. It counts the rows now, so one really cut in two is still a failure, and it is not a change in what is pinned. The redraw itself is readline's, costs 28 bytes a prompt, lands on the same cells, and ends with 004. Refs #194 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sh_skip_quoted read a double-quoted span flat, to the next `"`. Inside a
$(...), that span can hold a nested $(...), ${...} or `...` with quotes of
its own, so find_cmd_sub_end stopped on the inner quote:
x=$(printf %s "$(echo b | tr -d '"')" c) # bash: bc
# hellish: $(printf %s "b" c)
The `'` after that wrong stop opened a span with no end, depth never reached
zero, and the outer substitution stayed in the word as literal text. No error,
exit 0, the command never ran, and `hellish -n` passes it. Only an odd count of
quote bytes inside shows it: an even one balances by accident, which is why
$(a "$(b "$c")") always worked.
Found by a born2root provisioner: a readiness check written this way failed on
every first boot, which runs it under hellish, and passed on every retry, which
runs it under bash.
sh_skip_quoted now delegates to span_skip, the scanner that already walks these
spans for the word expander, so no new walker is added. tests/cmdsub_nested_quote
has 18 cases compared against bash; 13 of them fail on 3.2.0.
Left alone, as a separate concern: a `case` with its `)` inside a nested
substitution, x=$(echo "$(case a in a) echo "q'r";; esac)"), is refused earlier
by the lexer's advance_cmdsub, which has no case handling.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LESdylan
force-pushed
the
fix/nested-cmdsub-quote
branch
from
November 4, 2026 14:45
f51018e to
28203eb
Compare
Member
Author
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.
Closes #190
Problem
sh_skip_quotedread a double-quoted span flat, so$(a "$(tr -d '"')" b)stopped on the inner quote and the whole outer substitution stayed in the word as literal text. No error, exit 0, the command never ran, andhellish -npassed it. Only an odd count of quote bytes shows it.Fix
sh_skip_quoteddelegates tospan_skip, the scanner that already walks these spans. No new walker, and the`...`and${...}neighbours are fixed with it.Tests
tests/cmdsub_nested_quote: 18 cases against bash, registered intests/tester. 13 of them fail on 3.2.0.make testagainst the pinned bash 5.3.9: 5381/5381.make alldebug build (-Werror): no warnings.🤖 Generated with Claude Code