Skip to content

Commit

Permalink
[oil-language] Disallow POSIX shell arithmetic
Browse files Browse the repository at this point in the history
With a new option

    shopt -u parse_sh_arith

It was already disallowed in most places with

    shopt -u parse_dparen
    shopt -u parse_sh_assign

The remaining places are the static:

    echo $(( a[i] ))
    echo ${a[i]}

And the dynamic:

    unset a[i]
    printf -v a[i]
    ${!ref}
    declare -n not fully implemented

Follow-up to #1450.

With test/lint fix.
  • Loading branch information
Andy C committed Feb 22, 2023
1 parent 12430a5 commit a535011
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 0 deletions.
1 change: 1 addition & 0 deletions doc/oil-help-topics.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ X [External Lang] BEGIN END when (awk)
parse_dollar (-u) Is $ allowed for \$? Maybe $/d+/
parse_dparen (-u) Is (( legacy arithmetic allowed?
parse_ignored (-u) Parse, but ignore, certain redirects
parse_sh_arith (-u) Is legacy shell arithmetic allowed?
parse_sh_assign (-u) Are legacy a=b and PATH=. cmd allowed?
parse_sloppy_case (-u) Case patterns look like (*.py) not *.py)
X copy_env (-u) Use $[ENV->PYTHONPATH] when false
Expand Down
1 change: 1 addition & 0 deletions frontend/option_def.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ def DoneWithImplementedOptions(self):
('parse_dollar', True),
('parse_ignored', True),

('parse_sh_arith', True), # disallow all shell arithmetic, $(( )) etc.
('parse_sh_assign', True), # disallow x=y and PYTHONPATH=y
('parse_dparen', True), # disallow bash's ((
('parse_bare_word', True), # 'case bare' and 'for x in bare'
Expand Down
2 changes: 2 additions & 0 deletions osh/builtin_assign.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,8 @@ def Run(self, cmd_val):
# TODO:
# - It would make more sense to treat no args as an error (bash doesn't.)
# - Should we have strict builtins? Or just make it stricter?
# - Typed args: unset (mylist[0]) is like Python's del
# - It has the same word as 'setvar', which makes sense

class Unset(vm._Builtin):

Expand Down
9 changes: 9 additions & 0 deletions osh/sh_expr_eval.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,12 @@ def ParseLValue(self, s, span_id):
It uses the arith parser, so it behaves like the LHS of (( a[i] = x ))
"""
if not self.parse_ctx.parse_opts.parse_sh_arith():
# Do something simpler for Oil
if not match.IsValidVarName(s):
e_die('Invalid variable name %r (parse_sh_arith is off)' % s, loc.Span(span_id))
return lvalue.Named(s, span_id)

a_parser = self.parse_ctx.MakeArithParser(s)

with alloc.ctx_Location(self.arena, source.ArgvWord('dynamic place', span_id)):
Expand All @@ -211,6 +217,9 @@ def ParseLValue(self, s, span_id):
# Exception for builtins 'unset' and 'printf'
e_usage('got invalid place expression', span_id=span_id)

# Note: we parse '1+2', and then it becomes a runtime error because it's
# not a valid place. Could be a parse error.

if self.exec_opts.eval_unsafe_arith():
lval = self.arith_ev.EvalArithLhs(anode, span_id)
else:
Expand Down
24 changes: 24 additions & 0 deletions osh/tdop.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,5 +330,29 @@ def ParseUntil(self, rbp):

def Parse(self):
# type: () -> arith_expr_t

self.Next() # may raise ParseError

if not self.parse_opts.parse_sh_arith():
# Affects:
# echo $(( x ))
# ${a[i]} which should be $[a[i]] -- could have better error
#
# Note: sh_expr_eval.UnsafeArith has a dynamic e_die() check
#
# Doesn't affect:
# printf -v x # unsafe_arith.ParseLValue
# unset x # unsafe_arith.ParseLValue
# ${!ref} # unsafe_arith.ParseVarRef
# declare -n # not fully implemented yet
#
# a[i+1]= # parse_sh_assign
#
# (( a = 1 )) # parse_dparen
# for (( i = 0; ... # parse_dparen

p_die(
"POSIX shell arithmetic isn't allowed (parse_sh_arith)",
loc.Word(self.cur_word))

return self.ParseUntil(0)

0 comments on commit a535011

Please sign in to comment.