From 1811a2d7256f10078ba7cb15eafa23894072e875 Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sun, 25 Sep 2022 12:19:55 -0500
Subject: [PATCH] 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         |  3 +++
 src/builtins/return.cpp | 12 ++++++++++--
 src/proc.h              |  3 +++
 3 files changed, 16 insertions(+), 2 deletions(-)

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