[osh-language] Relax eval_unsafe_arith restrictions

This is to issue #1450, related to autoconf.

Instead of disallowing dynamic parsing in $(( x )) unconditionally, we
parse, and then disallow command subs, e.g.

    a[$(echo 42 | tee PWNED)]=1

in DATA as opposed to code.

I split shopt allow_csub_psub into _allow_command_sub _allow_process sub
to implement this.

Makes a bunch of spec tests pass.

core/executor.py

@@ -337,9 +337,14 @@ def RunSubshell(self, node):
   def RunCommandSub(self, cs_part):
     # type: (command_sub) -> str
 
-    if not self.exec_opts.allow_csub_psub():
-      e_die("Command subs not allowed here because status wouldn't be checked (strict_errexit).",
-            loc.WordPart(cs_part))
+    if not self.exec_opts._allow_command_sub():
+      # _allow_command_sub is used in two places.  Only one of them turns off _allow_process_sub
+      if not self.exec_opts._allow_process_sub():
+        why = "status wouldn't be checked (strict_errexit)"
+      else:
+        why = 'eval_unsafe_arith is off'
+
+      e_die("Command subs not allowed here because %s" % why, loc.WordPart(cs_part))
 
     node = cs_part.child
 
@@ -454,8 +459,8 @@ def RunProcessSub(self, cs_part):
       shopt -s process_sub_fail
       _process_sub_status
     """
-    if not self.exec_opts.allow_csub_psub():
-      e_die("Process subs not allowed here because status wouldn't be checked (strict_errexit).",
+    if not self.exec_opts._allow_process_sub():
+      e_die("Process subs not allowed here because status wouldn't be checked (strict_errexit)",
             loc.WordPart(cs_part))
 
     p = self._MakeProcess(cs_part.child)

core/shell.py

@@ -406,8 +406,8 @@ def Main(lang, arg_r, environ, login_shell, loader, readline):
   # Initialize Evaluators
   #
 
-  arith_ev = sh_expr_eval.ArithEvaluator(mem, exec_opts, parse_ctx, errfmt)
-  bool_ev = sh_expr_eval.BoolEvaluator(mem, exec_opts, parse_ctx, errfmt)
+  arith_ev = sh_expr_eval.ArithEvaluator(mem, exec_opts, mutable_opts, parse_ctx, errfmt)
+  bool_ev = sh_expr_eval.BoolEvaluator(mem, exec_opts, mutable_opts, parse_ctx, errfmt)
 
   if mylib.PYTHON:
     expr_ev = expr_eval.OilEvaluator(mem, mutable_opts, procs, splitter, errfmt)

core/state.py

@@ -186,7 +186,8 @@ def __init__(self, mutable_opts):
     # type: (MutableOpts) -> None
     self.strict = False
     if mutable_opts.Get(option_i.strict_errexit):
-      mutable_opts.Push(option_i.allow_csub_psub, False)
+      mutable_opts.Push(option_i._allow_command_sub, False)
+      mutable_opts.Push(option_i._allow_process_sub, False)
       self.strict = True
 
     self.mutable_opts = mutable_opts
@@ -198,7 +199,8 @@ def __enter__(self):
   def __exit__(self, type, value, traceback):
     # type: (Any, Any, Any) -> None
     if self.strict:
-      self.mutable_opts.Pop(option_i.allow_csub_psub)
+      self.mutable_opts.Pop(option_i._allow_command_sub)
+      self.mutable_opts.Pop(option_i._allow_process_sub)
 
 
 class ctx_OilExpr(object):
@@ -238,7 +240,8 @@ def __init__(self, mutable_opts, b, span_id):
 
     self.strict = False
     if mutable_opts.Get(option_i.strict_errexit):
-      mutable_opts.Push(option_i.allow_csub_psub, False)
+      mutable_opts.Push(option_i._allow_command_sub, False)
+      mutable_opts.Push(option_i._allow_process_sub, False)
       self.strict = True
 
     self.mutable_opts = mutable_opts
@@ -253,7 +256,8 @@ def __exit__(self, type, value, traceback):
     self.mutable_opts.Pop(option_i.errexit)
 
     if self.strict:
-      self.mutable_opts.Pop(option_i.allow_csub_psub)
+      self.mutable_opts.Pop(option_i._allow_command_sub)
+      self.mutable_opts.Pop(option_i._allow_process_sub)
 
 
 class ctx_HayNode(object):

core/test_lib.py

@@ -229,8 +229,8 @@ def InitCommandEvaluator(
 
   splitter = split.SplitContext(mem)
 
-  arith_ev = sh_expr_eval.ArithEvaluator(mem, exec_opts, parse_ctx, errfmt)
-  bool_ev = sh_expr_eval.BoolEvaluator(mem, exec_opts, parse_ctx, errfmt)
+  arith_ev = sh_expr_eval.ArithEvaluator(mem, exec_opts, mutable_opts, parse_ctx, errfmt)
+  bool_ev = sh_expr_eval.BoolEvaluator(mem, exec_opts, mutable_opts, parse_ctx, errfmt)
   expr_ev = expr_eval.OilEvaluator(mem, mutable_opts, procs, splitter, errfmt)
   word_ev = word_eval.NormalWordEvaluator(mem, exec_opts, mutable_opts,
                                           splitter, errfmt)

frontend/option_def.py

@@ -261,7 +261,9 @@ def _Init(opt_def):
   opt_def.Add('compat_array')  # ${array} is ${array[0]}
 
   # For implementing strict_errexit
-  opt_def.Add('allow_csub_psub', default=True)
+  opt_def.Add('_allow_command_sub', default=True)
+  opt_def.Add('_allow_process_sub', default=True)
+
   # For implementing 'proc'
   opt_def.Add('dynamic_scope', default=True)
 

osh/arith_parse_test.py

@@ -45,7 +45,9 @@ def ParseAndEval(code_str):
   word_ev = word_eval.CompletionWordEvaluator(mem, exec_opts, mutable_opts,
                                               splitter, errfmt)
 
-  arith_ev = sh_expr_eval.ArithEvaluator(mem, exec_opts, parse_ctx, arena)
+  arith_ev = sh_expr_eval.ArithEvaluator(
+      mem, exec_opts, mutable_opts, parse_ctx, arena)
+
   arith_ev.word_ev = word_ev
   return arith_ev.EvalToInt(anode)
 

osh/builtin_bracket.py

@@ -257,7 +257,7 @@ def Run(self, cmd_val):
     # We technically don't need mem because we don't support BASH_REMATCH here.
     # We want [ a -eq a ] to always be an error, unlike [[ a -eq a ]].  This is
     # a weird case of [[ being less strict.
-    bool_ev = sh_expr_eval.BoolEvaluator(self.mem, self.exec_opts, None,
+    bool_ev = sh_expr_eval.BoolEvaluator(self.mem, self.exec_opts, None, None,
                                          self.errfmt, always_strict=True)
     bool_ev.word_ev = word_ev
     bool_ev.CheckCircularDeps()

osh/cmd_eval.py

@@ -155,7 +155,8 @@ def _HasManyStatuses(node):
   """
   Code patterns that are bad for POSIX errexit.  For Oil's strict_errexit.
 
-  Note: The other part of strict_errexit is shopt --unset allow_csub_psub
+  Note: strict_errexit also uses
+    shopt --unset _allow_command_sub _allow_process_sub
   """
   # Sentence check is for   if false;   versus   if false
   if node.tag_() == command_e.Sentence:

osh/sh_expr_eval.py

@@ -29,6 +29,7 @@
     braced_var_sub, simple_var_sub,
     loc
 )
+from _devbuild.gen.option_asdl import option_i
 from _devbuild.gen.types_asdl import bool_arg_type_e
 from asdl import runtime
 from core import alloc
@@ -266,11 +267,12 @@ class ArithEvaluator(object):
   2. Look up variables and evaluate words.
   """
 
-  def __init__(self, mem, exec_opts, parse_ctx, errfmt):
-    # type: (Mem, optview.Exec, Optional[parse_lib.ParseContext], ErrorFormatter) -> None
+  def __init__(self, mem, exec_opts, mutable_opts, parse_ctx, errfmt):
+    # type: (Mem, optview.Exec, state.MutableOpts, Optional[parse_lib.ParseContext], ErrorFormatter) -> None
     self.word_ev = None  # type: word_eval.StringWordEvaluator
     self.mem = mem
     self.exec_opts = exec_opts
+    self.mutable_opts = mutable_opts
     self.parse_ctx = parse_ctx
     self.errfmt = errfmt
 
@@ -341,7 +343,7 @@ def _StringToInteger(self, s, span_id=runtime.NO_SPID):
       # doesn't look like an integer
 
       # note: 'test' and '[' never evaluate recursively
-      if self.exec_opts.eval_unsafe_arith() and self.parse_ctx:
+      if self.parse_ctx:
         arena = self.parse_ctx.arena
 
         # Special case so we don't get EOF error
@@ -350,7 +352,8 @@ def _StringToInteger(self, s, span_id=runtime.NO_SPID):
 
         # For compatibility: Try to parse it as an expression and evaluate it.
         a_parser = self.parse_ctx.MakeArithParser(s)
-        # don't know var name here
+
+        # TODO: Fill in the variable name
         with alloc.ctx_Location(arena, source.Variable(None, span_id)):
           try:
             node2 = a_parser.Parse()  # may raise error.Parse
@@ -362,8 +365,18 @@ def _StringToInteger(self, s, span_id=runtime.NO_SPID):
         # to itself, and you don't want to reparse it as a word.
         if node2.tag_() == arith_expr_e.Word:
           e_die("Invalid integer constant %r" % s, loc.Span(span_id))
-        else:
+
+        if self.exec_opts.eval_unsafe_arith():
           integer = self.EvalToInt(node2)
+        else:
+          # BoolEvaluator doesn't have parse_ctx or mutable_opts
+          assert self.mutable_opts is not None
+
+          # We don't need to flip _allow_process_sub, because they can't be
+          # parsed.  See spec/bugs.test.sh.
+          with state.ctx_Option(self.mutable_opts, [option_i._allow_command_sub], False):
+            integer = self.EvalToInt(node2)
+
       else:
         if len(s.strip()) == 0 or match.IsValidVarName(s):
           # x42 could evaluate to 0
@@ -847,9 +860,9 @@ class BoolEvaluator(ArithEvaluator):
   where x='1+2'
   """
 
-  def __init__(self, mem, exec_opts, parse_ctx, errfmt, always_strict=False):
-    # type: (Mem, optview.Exec, Optional[parse_lib.ParseContext], ErrorFormatter, bool) -> None
-    ArithEvaluator.__init__(self, mem, exec_opts, parse_ctx, errfmt)
+  def __init__(self, mem, exec_opts, mutable_opts, parse_ctx, errfmt, always_strict=False):
+    # type: (Mem, optview.Exec, Optional[state.MutableOpts], Optional[parse_lib.ParseContext], ErrorFormatter, bool) -> None
+    ArithEvaluator.__init__(self, mem, exec_opts, mutable_opts, parse_ctx, errfmt)
     self.always_strict = always_strict
 
   def _StringToIntegerOrError(self, s, blame_word=None):

osh/word_eval.py

@@ -477,7 +477,7 @@ def __init__(self, mem, exec_opts, mutable_opts, splitter, errfmt):
 
     self.mem = mem  # for $HOME, $1, etc.
     self.exec_opts = exec_opts  # for nounset
-    self.mutable_opts = mutable_opts  # for allow_csub_psub
+    self.mutable_opts = mutable_opts  # for _allow_command_sub
     self.splitter = splitter
     self.errfmt = errfmt
 

spec/arith.test.sh

@@ -270,16 +270,14 @@ foo=5
 x=oo
 echo $(( foo + f$x + 1 ))
 ## stdout: 11
-## OK osh stdout: 6
 
-#### Bizarre recursive name evaluation - result of runtime parse/eval
+#### Recursive name evaluation is a result of runtime parse/eval
 foo=5
 bar=foo
 spam=bar
 eggs=spam
 echo $((foo+1)) $((bar+1)) $((spam+1)) $((eggs+1))
 ## stdout: 6 6 6 6
-## OK osh stdout: 6 1 1 1
 ## N-I dash stdout-json: ""
 ## N-I dash status: 2
 

spec/bugs.test.sh

@@ -256,3 +256,50 @@ echo $as_val
 ## STDOUT:
 2
 ## END
+
+#### command execution $(echo 42 | tee PWNED) not allowed
+
+rm -f PWNED
+
+x='a[$(echo 42 | tee PWNED)]=1'
+echo $(( x ))
+
+if test -f PWNED; then
+  cat PWNED
+else
+  echo NOPE
+fi
+
+## status: 1
+## OK dash/ash status: 2
+## stdout-json: ""
+## BUG bash/mksh/zsh status: 0
+## BUG bash/mksh/zsh STDOUT:
+1
+42
+## END
+
+#### process sub <(echo 42 | tee PWNED) not allowed
+
+rm -f PWNED
+
+x='a[<(echo 42 | tee PWNED)]=1'
+echo $(( x ))
+
+if test -f PWNED; then
+  cat PWNED
+else
+  echo NOPE
+fi
+
+## status: 1
+## stdout-json: ""
+
+## OK dash/ash status: 2
+
+# bash keeps going
+## BUG bash status: 0
+## BUG bash STDOUT:
+NOPE
+## END
+

spec/dbracket.test.sh

@@ -248,8 +248,6 @@ expr='1+2'
 true
 true
 ## END
-## N-I osh stdout-json: ""
-## N-I osh status: 1
 
 #### -eq coercion produces weird results
 shopt -u strict_arith || true

spec/type-compat.test.sh

@@ -71,9 +71,6 @@ echo "$s|$i|$j"
 ## STDOUT:
 3|3|2
 ## END
-## N-I osh status: 1
-## N-I osh STDOUT:
-## END
 
 #### declare array vs. string: mixing -a +a and () ''
 # dynamic parsing of first argument.

test/spec.sh

@@ -294,7 +294,7 @@ osh-only() {
 
 # Regress bugs
 bugs() {
-  sh-spec spec/bugs.test.sh --osh-failures-allowed 2 \
+  sh-spec spec/bugs.test.sh --osh-failures-allowed 1 \
     ${REF_SHELLS[@]} $ZSH $BUSYBOX_ASH $OSH_LIST "$@"
 }