expand: avoid panics on division by zero

Arithm now returns one more type of error for this case.
Update the interpreter accordingly, and add tests.

The behavior we adopt follows what Bash does when not run interactively,
so start documenting that in the package godoc.

Also add a TODO as we should have some standard to tell whether errors
coming from the expand package or the user's handlers are fatal.
That is, whether they should exit the shell, and with what status code.

Fixes #892.

Author: mvdan

expand/arith.go

@@ -92,7 +92,7 @@ func Arithm(cfg *Config, expr syntax.ArithmExpr) (int, error) {
 		if err != nil {
 			return 0, err
 		}
-		return binArit(x.Op, left, right), nil
+		return binArit(x.Op, left, right)
 	default:
 		panic(fmt.Sprintf("unexpected arithm expr: %T", x))
 	}
@@ -129,6 +129,9 @@ func (cfg *Config) assgnArit(b *syntax.BinaryArithm) (int, error) {
 	case syntax.MulAssgn:
 		val *= arg
 	case syntax.QuoAssgn:
+		if arg == 0 {
+			return 0, fmt.Errorf("division by zero")
+		}
 		val /= arg
 	case syntax.RemAssgn:
 		val %= arg
@@ -161,48 +164,51 @@ func intPow(a, b int) int {
 	return p
 }
 
-func binArit(op syntax.BinAritOperator, x, y int) int {
+func binArit(op syntax.BinAritOperator, x, y int) (int, error) {
 	switch op {
 	case syntax.Add:
-		return x + y
+		return x + y, nil
 	case syntax.Sub:
-		return x - y
+		return x - y, nil
 	case syntax.Mul:
-		return x * y
+		return x * y, nil
 	case syntax.Quo:
-		return x / y
+		if y == 0 {
+			return 0, fmt.Errorf("division by zero")
+		}
+		return x / y, nil
 	case syntax.Rem:
-		return x % y
+		return x % y, nil
 	case syntax.Pow:
-		return intPow(x, y)
+		return intPow(x, y), nil
 	case syntax.Eql:
-		return oneIf(x == y)
+		return oneIf(x == y), nil
 	case syntax.Gtr:
-		return oneIf(x > y)
+		return oneIf(x > y), nil
 	case syntax.Lss:
-		return oneIf(x < y)
+		return oneIf(x < y), nil
 	case syntax.Neq:
-		return oneIf(x != y)
+		return oneIf(x != y), nil
 	case syntax.Leq:
-		return oneIf(x <= y)
+		return oneIf(x <= y), nil
 	case syntax.Geq:
-		return oneIf(x >= y)
+		return oneIf(x >= y), nil
 	case syntax.And:
-		return x & y
+		return x & y, nil
 	case syntax.Or:
-		return x | y
+		return x | y, nil
 	case syntax.Xor:
-		return x ^ y
+		return x ^ y, nil
 	case syntax.Shr:
-		return x >> uint(y)
+		return x >> uint(y), nil
 	case syntax.Shl:
-		return x << uint(y)
+		return x << uint(y), nil
 	case syntax.AndArit:
-		return oneIf(x != 0 && y != 0)
+		return oneIf(x != 0 && y != 0), nil
 	case syntax.OrArit:
-		return oneIf(x != 0 || y != 0)
+		return oneIf(x != 0 || y != 0), nil
 	default: // syntax.Comma
 		// x is executed but its result discarded
-		return y
+		return y, nil
 	}
 }

interp/api.go

@@ -4,6 +4,13 @@
 // Package interp implements an interpreter that executes shell
 // programs. It aims to support POSIX, but its support is not complete
 // yet. It also supports some Bash features.
+//
+// The interpreter generally aims to behave like Bash,
+// but it does not support all of its features.
+//
+// The interpreter currently aims to behave like a non-interactive shell,
+// which how most shells run scripts, and is more useful to machines.
+// In the future, it may gain an option to behave like an interactive shell.
 package interp
 
 import (

interp/handler.go

@@ -67,6 +67,10 @@ type HandlerContext struct {
 // Returning a non-nil error will halt the Runner.
 type CallHandlerFunc func(ctx context.Context, args []string) ([]string, error)
 
+// TODO: consistently treat handler errors as non-fatal by default,
+// but have an interface or API to specify fatal errors which should make
+// the shell exit with a particular status code.
+
 // ExecHandlerFunc is a handler which executes simple commands.
 // It is called for all CallExpr nodes where the first argument is neither a
 // declared function nor a builtin.

interp/handler_test.go

@@ -21,29 +21,29 @@ import (
 	"mvdan.cc/sh/v3/syntax"
 )
 
-func blacklistBuiltinExec(name string) ExecHandlerFunc {
+func blocklistBuiltinExec(name string) ExecHandlerFunc {
 	return func(ctx context.Context, args []string) error {
 		if args[0] == name {
-			return fmt.Errorf("%s: blacklisted builtin", name)
+			return fmt.Errorf("%s: blocklisted builtin", name)
 		}
 		return testExecHandler(ctx, args)
 	}
 }
 
-func blacklistAllExec(ctx context.Context, args []string) error {
-	return fmt.Errorf("blacklisted: %s", args[0])
+func blocklistAllExec(ctx context.Context, args []string) error {
+	return fmt.Errorf("blocklisted: %s", args[0])
 }
 
-func blacklistNondevOpen(ctx context.Context, path string, flags int, mode os.FileMode) (io.ReadWriteCloser, error) {
+func blocklistNondevOpen(ctx context.Context, path string, flags int, mode os.FileMode) (io.ReadWriteCloser, error) {
 	if path != "/dev/null" {
 		return nil, fmt.Errorf("non-dev: %s", path)
 	}
 
 	return testOpenHandler(ctx, path, flags, mode)
 }
 
-func blacklistGlob(ctx context.Context, path string) ([]os.FileInfo, error) {
-	return nil, fmt.Errorf("blacklisted: glob")
+func blocklistGlob(ctx context.Context, path string) ([]os.FileInfo, error) {
+	return nil, fmt.Errorf("blocklisted: glob")
 }
 
 // runnerCtx allows us to give handler functions access to the Runner, if needed.
@@ -67,40 +67,40 @@ var modCases = []struct {
 	want    string
 }{
 	{
-		name: "ExecBlacklist",
-		exec: blacklistBuiltinExec("sleep"),
+		name: "Execblocklist",
+		exec: blocklistBuiltinExec("sleep"),
 		src:  "echo foo; sleep 1",
-		want: "foo\nsleep: blacklisted builtin",
+		want: "foo\nsleep: blocklisted builtin",
 	},
 	{
 		name: "ExecWhitelist",
-		exec: blacklistBuiltinExec("faa"),
+		exec: blocklistBuiltinExec("faa"),
 		src:  "a=$(echo foo | sed 's/o/a/g'); echo $a; $a args",
-		want: "faa\nfaa: blacklisted builtin",
+		want: "faa\nfaa: blocklisted builtin",
 	},
 	{
 		name: "ExecSubshell",
-		exec: blacklistAllExec,
+		exec: blocklistAllExec,
 		src:  "(malicious)",
-		want: "blacklisted: malicious",
+		want: "blocklisted: malicious",
 	},
 	{
 		name: "ExecPipe",
-		exec: blacklistAllExec,
+		exec: blocklistAllExec,
 		src:  "malicious | echo foo",
-		want: "foo\nblacklisted: malicious",
+		want: "foo\nblocklisted: malicious",
 	},
 	{
 		name: "ExecCmdSubst",
-		exec: blacklistAllExec,
+		exec: blocklistAllExec,
 		src:  "a=$(malicious)",
-		want: "blacklisted: malicious\nexit status 1",
+		want: "blocklisted: malicious\n", // TODO: why the newline?
 	},
 	{
 		name: "ExecBackground",
-		exec: blacklistAllExec,
+		exec: blocklistAllExec,
 		src:  "{ malicious; true; } & { malicious; true; } & wait",
-		want: "blacklisted: malicious",
+		want: "blocklisted: malicious",
 	},
 	{
 		name: "ExecBuiltin",
@@ -110,13 +110,13 @@ var modCases = []struct {
 	},
 	{
 		name: "OpenForbidNonDev",
-		open: blacklistNondevOpen,
+		open: blocklistNondevOpen,
 		src:  "echo foo >/dev/null; echo bar >/tmp/x",
 		want: "non-dev: /tmp/x",
 	},
 	{
 		name: "CallReplaceWithBlank",
-		open: blacklistNondevOpen,
+		open: blocklistNondevOpen,
 		call: func(ctx context.Context, args []string) ([]string, error) {
 			return []string{"echo", "blank"}, nil
 		},
@@ -144,9 +144,9 @@ var modCases = []struct {
 	},
 	{
 		name:    "GlobForbid",
-		readdir: blacklistGlob,
+		readdir: blocklistGlob,
 		src:     "echo *",
-		want:    "blacklisted: glob\nexit status 1",
+		want:    "blocklisted: glob\n",
 	},
 }
 

interp/interp_test.go

@@ -1818,6 +1818,10 @@ var runTests = []runTest{
 		"a=b b=a; echo $(($a))",
 		"0\n #IGNORE bash prints a warning",
 	},
+	{
+		"let x=3; let 3/0; ((3/0)); echo $((x/y)); let x/=0",
+		"division by zero\ndivision by zero\ndivision by zero\ndivision by zero\nexit status 1 #JUSTERR",
+	},
 
 	// set/shift
 	{
@@ -2307,7 +2311,7 @@ set +o pipefail
 	},
 	{
 		`a=(b); echo ${a[-2]}`,
-		"negative array index\nexit status 1 #JUSTERR",
+		"negative array index\n #JUSTERR",
 	},
 	// TODO: also test with gaps in arrays.
 	{

interp/runner.go

@@ -6,6 +6,7 @@ package interp
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"math"
@@ -155,7 +156,19 @@ func (r *Runner) updateExpandOpts() {
 
 func (r *Runner) expandErr(err error) {
 	if err != nil {
-		r.errf("%v\n", err)
+		errMsg := err.Error()
+		fmt.Fprintln(r.stderr, errMsg)
+		switch {
+		case errors.As(err, &expand.UnsetParameterError{}):
+		case errMsg == "invalid indirect expansion":
+			// TODO: These errors are treated as fatal by bash.
+			// Make the error type reflect that.
+		case strings.HasSuffix(errMsg, "not supported"):
+			// TODO: This "contains" is a temporary measure until the expand
+			// package supports all syntax nodes like extended globbing.
+		default:
+			return // other cases do not exit
+		}
 		r.exitShell(context.TODO(), 1)
 	}
 }