Permalink
Browse files

Bug fix: only run the EXIT trap at the top level of the root process.

It shouldn't fire when subshells/command sub/_HereDocWriterThunk thunk
exit.  Forking clones the stack, so the run_exit_trap boolean was not a
sufficient guard!

Related: the exit builtin has been reimplemented as static control flow
(using our _ControlFlow exception), rather than a dynamic builtin
calling sys.exit().

This makes it consistent with break/continue/return.

I had to adjust a couple tests, but this usage seems reasonable.  We'll
see if it makes and wild tests fail.

Examples:

- 'exit 7 8 9' is now a syntax error rather than a runtime error
- 'echo hi | exit 1' is also a syntax error.

All spec tests and benchmarks now complete.  The yash configure script
was the one that tickled the bug of running the EXIT trap from multiple
processes.
  • Loading branch information...
Andy Chu
Andy Chu committed Jan 22, 2018
1 parent 5235e52 commit 782a68467ecdddbdaaeb8b2b427f19efc4936897
Showing with 48 additions and 44 deletions.
  1. +3 −4 bin/oil.py
  2. +6 −18 core/builtin.py
  3. +29 −16 core/cmd_exec.py
  4. +1 −1 core/id_kind.py
  5. +1 −0 osh/lex.py
  6. +2 −2 spec/background.test.sh
  7. +6 −3 spec/builtins.test.sh
View
@@ -406,10 +406,9 @@ def OshMain(argv, login_shell):
if do_exec:
_tlog('Execute(node)')
status = ex.Execute(node, run_exit_trap=True)
# We only do this in the "happy" case for now. ex.Execute() can raise
# exceptions.
status = ex.ExecuteAndRunExitTrap(node)
# NOTE: 'exit 1' is ControlFlow and gets here, but subshell/commandsub
# don't because they call sys.exit().
if opts.runtime_mem_dump:
# This might be superstition, but we want to let the value stabilize
# after parsing. bash -c 'cat /proc/$$/status' gives different results
View
@@ -57,7 +57,7 @@
CD PUSHD POPD DIRS
EXPORT UNSET SET SHOPT
TRAP UMASK
EXIT SOURCE DOT EVAL EXEC WAIT JOBS
SOURCE DOT EVAL EXEC WAIT JOBS
COMPLETE COMPGEN DEBUG_LINE
TRUE FALSE
COLON
@@ -77,7 +77,6 @@
".": EBuiltin.DOT,
"eval": EBuiltin.EVAL,
"exec": EBuiltin.EXEC,
"exit": EBuiltin.EXIT,
"export": EBuiltin.EXPORT,
"set": EBuiltin.SET,
@@ -245,21 +244,6 @@ def Echo(argv):
return 0
def Exit(argv):
if len(argv) > 1:
util.error('exit: too many arguments')
return 1
try:
code = int(argv[0])
except IndexError:
code = 0
except ValueError as e:
print("Invalid argument %r" % argv[0], file=sys.stderr)
code = 1 # Runtime Error
# TODO: Should this be turned into our own SystemExit exception?
sys.exit(code)
# TODO: remove getopt
import getopt
@@ -296,8 +280,9 @@ def Wait(argv, waiter, job_state, mem):
try:
opts, args = getopt.getopt(argv, 'n')
except getopt.GetoptError as e:
# TODO: Should be args.UsageError()
util.usage(str(e))
sys.exit(2)
return 2
for name, val in opts:
if name == '-n':
opt_n = True
@@ -994,10 +979,13 @@ def __init__(self, ex, node):
def __call__(self, unused_signalnum, unused_frame):
"""For Python's signal module."""
# TODO: set -o xtrace/verbose should enable this.
#log('*** RUNNING TRAP for %d ***', unused_signalnum)
self.Run()
def Run(self):
"""For hooks."""
#log('*** RUNNING TRAP for hook')
unused_status = self.ex.Execute(self.node)
def __str__(self):
View
@@ -82,14 +82,17 @@ def __init__(self, token, arg):
def IsReturn(self):
return self.token.id == Id.ControlFlow_Return
def IsExit(self):
return self.token.id == Id.ControlFlow_Exit
def IsBreak(self):
return self.token.id == Id.ControlFlow_Break
def IsContinue(self):
return self.token.id == Id.ControlFlow_Continue
def ReturnValue(self):
assert self.IsReturn()
def StatusCode(self):
assert self.IsReturn() or self.IsExit()
return self.arg
def __repr__(self):
@@ -248,7 +251,7 @@ def _Source(self, argv):
except _ControlFlow as e:
if e.IsReturn():
return e.ReturnValue()
return e.StatusCode()
else:
raise
finally:
@@ -301,9 +304,6 @@ def _RunBuiltin(self, builtin_id, argv):
elif builtin_id == EBuiltin.EXPORT:
status = builtin.Export(argv, self.mem)
elif builtin_id == EBuiltin.EXIT:
status = builtin.Exit(argv)
elif builtin_id == EBuiltin.WAIT:
status = builtin.Wait(argv, self.waiter, self.job_state, self.mem)
@@ -856,7 +856,7 @@ def _Dispatch(self, node, fork_external):
assert val.tag == value_e.Str
arg = int(val.s) # They all take integers
else:
arg = 0 # return 0, break 0 levels, etc.
arg = 0 # return 0, exit 0, break 0 levels, etc.
# NOTE: We don't do anything about a top-level 'return' here. Unlike in
# bash, that is OK. If you can return from a sourced script, it makes
@@ -1187,25 +1187,38 @@ def Execute(self, node, fork_external=True, run_exit_trap=False):
status = self._Execute(node, fork_external=fork_external)
except _ControlFlow as e:
# Return at top level is OK, unlike in bash.
if e.IsReturn():
status = e.ReturnValue()
if e.IsReturn() or e.IsExit():
status = e.StatusCode()
else:
raise
except util.FatalRuntimeError as e:
ui.PrettyPrintError(e, self.arena)
print('osh failed: %s' % e.UserErrorString(), file=sys.stderr)
status = e.exit_status if e.exit_status is not None else 1
# TODO: dump self.mem if requested. Maybe speify with OIL_DUMP_PREFIX.
finally:
if run_exit_trap:
# NOTE: The trap itself can call exit!
thunk = self.traps.get('EXIT')
if thunk:
thunk.Run()
# Other exceptions: SystemExit for sys.exit()
return status
def ExecuteAndRunExitTrap(self, node):
"""For the top level program, called by bin/oil.py."""
status = self.Execute(node)
# NOTE: 'exit 1' is ControlFlow and gets here, but subshell/commandsub
# don't because they call sys.exit().
# NOTE: --runtime-mem-dump runs in a similar place.
#log('-- EXIT pid %d', os.getpid())
#import traceback
#traceback.print_stack()
# NOTE: The trap itself can call exit!
thunk = self.traps.get('EXIT')
if thunk:
thunk.Run()
return status
def RunCommandSub(self, node):
p = self._MakeProcess(node,
disable_errexit=not self.exec_opts.strict_errexit)
@@ -1367,7 +1380,7 @@ def RunFunc(self, func_node, argv):
status = self._Execute(func_node.body)
except _ControlFlow as e:
if e.IsReturn():
status = e.ReturnValue()
status = e.StatusCode()
else:
# break/continue used in the wrong place
e_die('Unexpected %r (in function call)', e.token.val, token=e.token)
View
@@ -395,7 +395,7 @@ def _AddKinds(spec):
# Unlike bash, we parse control flow statically. They're not
# dynamically-resolved builtins.
spec.AddKind('ControlFlow', ['Break', 'Continue', 'Return'])
spec.AddKind('ControlFlow', ['Break', 'Continue', 'Return', 'Exit'])
# For C-escaped strings.
spec.AddKind('Char', [
View
@@ -285,6 +285,7 @@
C('break', Id.ControlFlow_Break),
C('continue', Id.ControlFlow_Continue),
C('return', Id.ControlFlow_Return),
C('exit', Id.ControlFlow_Exit),
]
View
@@ -42,13 +42,13 @@ wait
# stdout-json: ""
### Pipeline in Background
echo hi | exit 99 &
echo hi | { exit 99; } &
wait $!
echo status=$?
# stdout: status=99
### Wait sets PIPESTATUS
{ echo hi; exit 55; } | exit 99 &
{ echo hi; exit 55; } | { exit 99; } &
echo "pipestatus=${PIPESTATUS[@]}"
wait $!
echo status=$?
View
@@ -99,10 +99,13 @@ exit invalid
# OK dash/bash status: 2
### Exit builtin with too many args
# This is a parse error in OSH.
exit 7 8 9
echo "no exit: $?"
# status: 0
# stdout-json: "no exit: 1\n"
echo status=$?
# status: 2
# stdout-json: ""
# BUG bash status: 0
# BUG bash stdout: status=1
# BUG dash status: 7
# BUG dash stdout-json: ""
# OK mksh status: 1

0 comments on commit 782a684

Please sign in to comment.