diff --git a/src/builtin.cpp b/src/builtin.cpp index ed21d9848..1b935942b 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -472,6 +472,9 @@ proc_status_t builtin_run(parser_t &parser, const wcstring_list_t &argv, io_stre if (code == 0 && !builtin_ret.has_value()) { return proc_status_t::empty(); } + if (code < 0) { + FLOGF(warning, "builtin %ls returned invalid exit code %d", cmdname.c_str(), code); + } return proc_status_t::from_exit_code(code); } diff --git a/src/builtins/return.cpp b/src/builtins/return.cpp index 89f9f952b..a0c01e699 100644 --- a/src/builtins/return.cpp +++ b/src/builtins/return.cpp @@ -96,8 +96,16 @@ maybe_t builtin_return(parser_t &parser, io_streams_t &streams, const wchar } } - // If we're not in a function, exit the current script, - // but not an interactive shell. + // *nix does not support negative return values, but our `return` builtin happily accepts being + // called with negative literals (e.g. `return -1`). + // Map negative values to (256 - their absolute value). This prevents `return -1` from + // evaluating to a `$status` of 0 and keeps us from running into undefined behavior by trying to + // left shift a negative value in W_EXITCODE(). + if (retval < 0) { + retval = 256 - (std::abs(retval) % 256); + } + + // If we're not in a function, exit the current script (but not an interactive shell). if (!has_function_block) { if (!parser.libdata().is_interactive) { parser.libdata().exit_current_script = true; diff --git a/src/proc.h b/src/proc.h index 331517bea..3e09d6d14 100644 --- a/src/proc.h +++ b/src/proc.h @@ -94,6 +94,9 @@ class proc_status_t { /// Construct directly from an exit code. static proc_status_t from_exit_code(int ret) { + assert(ret >= 0 && "trying to create proc_status_t from failed wait{,id,pid}() call" + " or invalid builtin exit code!"); + // Some paranoia. constexpr int zerocode = w_exitcode(0, 0); static_assert(WIFEXITED(zerocode), "Synthetic exit status not reported as exited");