Commit 68ff5a6b authored by jan.koester's avatar jan.koester
Browse files

fix runCaptured() terminal hang + trailing-pipe line continuation

runCaptured()'s forked child recursed into parse() while still sharing
whatever _heredocSource the parent had (the real script file, or a real
terminal) -- parse()'s own needsMoreInput() pull-loop could then read
from it looking for "more input" that a command-substitution's already-
complete, already-parenthesized text never legitimately needs. Found via
a real hang: the kernel's own scripts/syscallhdr.sh has
"guard=_UAPI_ASM_$(basename "$outfile" |\n\tsed ... \\\n\t-e ...)", and
evaluating it wedged the whole build reading from /dev/pts forever.
Nulling _heredocSource in the child fixes it.

That then exposed two more real bugs in the same area, both needed for
this exact substitution to produce the right guard string instead of a
truncated "_UAPI_ASM_":
- needsMoreInput() never recognized a bare trailing '|'/'&&' (a pipeline
  split across lines with no backslash) as incomplete, so "cmd1 |" and
  "cmd2" on the next line ran as two unrelated statements.
- the join for that case must use a space, not '\n' -- unlike every other
  needsMoreInput() reason (heredoc/if/while/case bodies), where the
  newline must survive as a real statement separator, here the two lines
  are the SAME command; joining with '\n' fed the per-statement splitter
  a false statement boundary right back.

Co-Authored-By: default avatarClaude Sonnet 5 <noreply@anthropic.com>
parent d942f18e
Loading
Loading
Loading
Loading
+89 −1
Changes for src/shell.cpp: 89 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -383,7 +383,14 @@ std::vector<std::string> fresh::tokenizeShellArgs(const std::string &segment){
            }
        }

        if(c==' ' || c=='\t'){
        // A literal embedded '\n' reaches here only as a line-continuation-
        // join artifact (needsMoreInput()'s pull-loop glues a non-backslash
        // continuation, e.g. a trailing "cmd |", onto the next physical
        // line with a real '\n', not a space) -- never as meaningful
        // content of its own (a heredoc body is pulled out separately by
        // collectHeredocBody(), never reaching this tokenizer at all), so
        // it separates words exactly like a space/tab would.
        if(c==' ' || c=='\t' || c=='\n'){
            flushPending();
            flushToken(tokens,current,have,anyquoted,anyUnquotedGlobChar,inLeadingAssignments,assignmentShaped);
            ++i;
@@ -1742,6 +1749,32 @@ std::string fresh::Shell::runCaptured(const std::string &cmd){
        close(pipefd[0]);
        dup2(pipefd[1],1);
        close(pipefd[1]);
        // `cmd` is always a complete, self-contained command text by the
        // time it gets here -- whoever called runCaptured() (the "$(...)"/
        // backtick scanner in Environment::expand()/the tokenizer) only
        // does so after its own depth-counting already found the matching
        // closing paren/backtick, so there is never a legitimate reason for
        // parse()'s own needsMoreInput() check to go pull further lines
        // from _heredocSource here. Left as whatever this forked child
        // inherited from the parent (the real script file, or a real
        // terminal for an interactive shell), that inherited source is
        // exactly wrong either way -- more of the outer script file would
        // desync the parent's own still-in-progress read of it (see the
        // KNOWN LIMITATION comment on needsMoreInput() above), and a real
        // terminal (inherited all the way down from an interactive/tty
        // invocation, or - as with a plain "sh script.sh" build-tool
        // invocation - simply whatever fd 0 the whole process tree started
        // with) blocks forever reading input nobody will ever type: found
        // via a hung "$(basename ... | sed ...)" substitution in the
        // kernel's own scripts/syscallhdr.sh, whose multi-line body (a
        // trailing "|" then a "\"-continued sed line) tripped
        // needsMoreInput()'s incompleteness heuristic once replayed here
        // outside the top-level line-reading loop that normally joins
        // exactly that shape before parse() ever sees it. Nulling it out
        // makes an incomplete-looking `cmd` just run as-is instead -- the
        // same thing any real shell does with an already-fully-parenthesized
        // "$(...)" body, complete or not.
        _heredocSource=nullptr;
        int rc=parse(cmd);
        std::cout.flush();
        std::cerr.flush();
@@ -2198,6 +2231,28 @@ bool fresh::quotesUnbalanced(const std::string &line){
// entirely, untouched by this function - see the return statement's own
// note), nor a construct whose own closing keyword is already resolvable
// before reaching the heredoc.
// True if `text`'s last non-whitespace character is a bare, unquoted '|'
// or "&&" -- a pipeline/list continuing on the next physical line (e.g.
// "basename "$outfile" |\n\tsed ..."). Used both by needsMoreInput() (to
// know another line is still needed) and by parse()'s own join right
// below it (to know THIS particular join must use a plain space instead
// of the literal '\n' every other needsMoreInput() reason uses -- unlike
// a heredoc/if-while-case body, where the newline must stay a real
// newline so the per-statement splitter keeps treating it as one, here
// the two joined lines are the SAME command, and a literal embedded '\n'
// between "|" and the next line desyncs that exact splitter instead,
// handing "cmd1 |" and "cmd2" back out as two unrelated statements).
bool fresh::endsWithBareOperator(const std::string &text){
    size_t e=text.find_last_not_of(" \t\r\n");
    if(e==std::string::npos)
        return false;
    if(text[e]=='|')
        return true;
    if(text[e]=='&' && e>0 && text[e-1]=='&')
        return true;
    return false;
}

bool fresh::needsMoreInput(const std::string &text){
    size_t n=text.length();
    size_t i=0;
@@ -2394,6 +2449,29 @@ bool fresh::needsMoreInput(const std::string &text){
    if(depth>0 || quoteState!=0 || parenDepth>0)
        return true;

    // A bare, unquoted trailing '|' or "&&" at the very end (only
    // whitespace after it) means this pipeline/list continues on the next
    // physical line -- e.g. the kernel's own scripts/syscallhdr.sh:
    // "basename "$outfile" |\n\tsed -e ... \\\n\t-e ...". readLogicalLine()
    // (fresh.cpp) only joins a REAL "\<newline>" continuation or an
    // unbalanced quote; neither fires for "cmd |\ncmd2" (no backslash,
    // quotes fully balanced already), so without this check "basename ..."
    // and "sed ..." were handed to parse() as two entirely separate,
    // unrelated statements -- the "sed" one running detached (its own
    // fresh stdin, no input from "basename" ever reaching it), and every
    // generated syscall header's own #ifndef guard silently coming out as
    // just "_UAPI_ASM_" (the wrong-headed "|"-only "basename" stage on its
    // own contributes nothing) instead of "_UAPI_ASM_UNISTD_64_H" etc.
    // Safe to check via a plain trailing scan (not a fresh quote-aware
    // pass) because the text as a whole is already known-balanced at this
    // point (quoteState==0 just above): the only way the very last
    // non-whitespace character could be an unquoted '|' or '&' is if it
    // genuinely is one -- had it instead been the last character of a
    // quoted segment, that segment would have to close with an actual
    // quote character, not '|'/'&'.
    if(fresh::endsWithBareOperator(text))
        return true;

    // One more case the depth/quote scan above can't see at all: a
    // statement that is JUST a bare function-definition header ("name()"
    // or "function name"/"function name()"), full stop, with no compound-
@@ -3452,6 +3530,16 @@ int fresh::Shell::parse(const std::string &input,bool applyErrexit){
            break;
        if(endsWithContinuation(trimmed))
            trimmed.pop_back();
        else if(fresh::endsWithBareOperator(trimmed))
            // A plain space, not '\n': these two lines are the SAME
            // pipeline/list, and the per-statement splitter just below
            // (which must keep treating every OTHER needsMoreInput() join's
            // embedded '\n' as a real statement boundary, e.g. inside an
            // if/while/case body) would otherwise cut "cmd1 |" and "cmd2"
            // right back apart into the two unrelated statements this loop
            // just went to the trouble of pulling `more` to avoid -- see
            // endsWithBareOperator()'s own comment.
            trimmed+=" ";
        else
            trimmed+="\n";
        trimmed+=more;
+6 −0
Changes for src/shell.h: 6 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -235,6 +235,12 @@ namespace fresh {
    // recognized as completing the construct.
    bool needsMoreInput(const std::string &text);

    // True if `text`'s last non-whitespace character is a bare, unquoted
    // '|' or "&&" -- see the definition's own comment (shell.cpp, right
    // above needsMoreInput()) for why parse()'s join loop needs this as a
    // separate query, not just a private detail of needsMoreInput() itself.
    bool endsWithBareOperator(const std::string &text);

    // If words[i] starts a "<<[-]DELIM" heredoc-open marker, returns the
    // unquoted DELIM and sets `consumed` to how many words the marker
    // occupies: 1 when DELIM is glued to the operator ("<<EOF"), 2 when