Skip to content

Commit

Permalink
interp: process substitutions are not waited for
Browse files Browse the repository at this point in the history
That is, when running

    test -e <(echo foo)

the fact that `echo foo` there is being left hanging should not mean
that `test -e` should be blocked and prevented from finishing.
This is Bash's logic anyway, and doing otherwise breaks scripts.

While here, I also noticed that `wait` is documented to always succeed
when given no arguments, so implement that too.

Fixes #1032.
  • Loading branch information
mvdan committed Jan 19, 2025
1 parent 5437ba3 commit 2884acd
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 22 deletions.
8 changes: 1 addition & 7 deletions interp/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import (
"sync"
"time"

"golang.org/x/sync/errgroup"

"mvdan.cc/sh/v3/expand"
"mvdan.cc/sh/v3/syntax"
)
Expand Down Expand Up @@ -109,10 +107,6 @@ type Runner struct {
// rand is used mainly to generate temporary files.
rand *rand.Rand

// wgProcSubsts allows waiting for any process substitution sub-shells
// to finish running.
wgProcSubsts sync.WaitGroup

filename string // only if Node was a File

// >0 to break or continue out of N enclosing loops
Expand All @@ -137,7 +131,7 @@ type Runner struct {
exit int
lastExit int

bgShells errgroup.Group
bgShells sync.WaitGroup

opts runnerOpts

Expand Down
6 changes: 2 additions & 4 deletions interp/builtin.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,8 @@ func (r *Runner) builtinCode(ctx context.Context, pos syntax.Pos, name string, a
if len(args) > 0 {
panic("wait with args not handled yet")
}
err := r.bgShells.Wait()
if _, ok := IsExitStatus(err); err != nil && !ok {
r.setErr(err)
}
// Note that "wait" without arguments always returns exit status zero.
r.bgShells.Wait()
case "builtin":
if len(args) < 1 {
break
Expand Down
5 changes: 3 additions & 2 deletions interp/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,9 @@ var modCases = []struct {
opts: []interp.RunnerOption{
interp.ExecHandlers(blocklistAllExec),
},
src: "{ malicious; true; } & { malicious; true; } & wait",
want: "blocklisted: malicious",
src: "{ malicious; true; } & { malicious; true; } & wait",
// Note that "wait" with no arguments always succeeds.
want: "",
},
{
name: "ExecPrintWouldExec",
Expand Down
13 changes: 10 additions & 3 deletions interp/interp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3237,17 +3237,24 @@ var runTestsUnix = []runTest{
"nested\n",
},
{
"echo foo_interp_missing bar_interp_missing > >(sed 's/o/e/g')",
// The tests here use "wait" because otherwise the parent may finish before
// the subprocess has had time to process the input and print the result.
"echo foo_interp_missing bar_interp_missing > >(sed 's/o/e/g'); wait",
"fee_interp_missing bar_interp_missing\n",
},
{
"echo foo_interp_missing bar_interp_missing | tee >(sed 's/o/e/g') >/dev/null",
"echo foo_interp_missing bar_interp_missing | tee >(sed 's/o/e/g') >/dev/null; wait",
"fee_interp_missing bar_interp_missing\n",
},
{
"echo nested > >(cat > >(cat))",
"echo nested > >(cat > >(cat); wait); wait",
"nested\n",
},
{
// The reader here does not consume the named pipe.
"test -e <(echo foo)",
"",
},
// echo trace
{
`set -x; animals=("dog", "cat", "otter"); echo "hello ${animals[*]}"`,
Expand Down
15 changes: 9 additions & 6 deletions interp/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ func (r *Runner) fillExpandConfig(ctx context.Context) {

r2 := r.Subshell()
stdout := r.origStdout
r.wgProcSubsts.Add(1)
// TODO: note that `man bash` mentions that `wait` only waits for the last
// process substitution; the logic here would mean we wait for all of them.
r.bgShells.Add(1)
go func() {
defer r.wgProcSubsts.Done()
defer r.bgShells.Done()
switch ps.Op {
case syntax.CmdIn:
f, err := os.OpenFile(path, os.O_WRONLY, 0)
Expand Down Expand Up @@ -291,17 +293,18 @@ func (r *Runner) stmt(ctx context.Context, st *syntax.Stmt) {
r2 := r.Subshell()
st2 := *st
st2.Background = false
r.bgShells.Go(func() error {
return r2.Run(ctx, &st2)
})
r.bgShells.Add(1)
go func() {
r2.Run(ctx, &st2)
r.bgShells.Done()
}()
} else {
r.stmtSync(ctx, st)
}
r.lastExit = r.exit
}

func (r *Runner) stmtSync(ctx context.Context, st *syntax.Stmt) {
defer r.wgProcSubsts.Wait()
oldIn, oldOut, oldErr := r.stdin, r.stdout, r.stderr
for _, rd := range st.Redirs {
cls, err := r.redir(ctx, rd)
Expand Down

0 comments on commit 2884acd

Please sign in to comment.