Prevent undefined behavior by intercepting return -1

While we hardcode the return values for the rest of our builtins, the `return`
builtin bubbles up whatever the user returned in their fish script, allowing
invalid return values such as negative numbers to make it into our C++ side of
things.

In creating a `proc_status_t` from the return code of a builtin, we invoke
W_EXITCODE() which is a macro that shifts left the return code by some amount,
and left-shifting a negative integer is undefined behavior.

Aside from causing us to land in UB territory, it also can cause some negative
return values to map to a "successful" exit code of 0, which was probably not
the fish script author's intention.

This patch also adds error logging to help catch any inadvertent additions of
cases where a builtin returns a negative value (should one forget that unix
return codes are always positive) and an assertion protecting against UB.

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);
     }
 

src/builtins/return.cpp

@@ -96,8 +96,16 @@ maybe_t<int> 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;

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");