From 3bef75fb79f0c10b8377dae7d65073a2a8b8dd0e Mon Sep 17 00:00:00 2001
From: Fabian Boehm <FHomborg@gmail.com>
Date: Tue, 14 Mar 2023 10:50:22 +0100
Subject: [PATCH] builtins: Don't crash for negative return values

Another from the "why are we asserting instead of doing something
sensible" department.

The alternative is to make exit() and return() compute their own exit
code, but tbh I don't want any *other* builtin to hit this either?

Fixes #9659

(cherry picked from commit a16abf22d9292f232ec7d57b3cf42e2e93ffbb0a)
---
 src/builtin.cpp                |  6 ++++++
 tests/checks/status-value.fish | 23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/builtin.cpp b/src/builtin.cpp
index 085b278da..6ec0119c5 100644
--- a/src/builtin.cpp
+++ b/src/builtin.cpp
@@ -475,6 +475,12 @@ proc_status_t builtin_run(parser_t &parser, const wcstring_list_t &argv, io_stre
             return proc_status_t::empty();
         }
         if (code < 0) {
+            // If the code is below 0, constructing a proc_status_t
+            // would assert() out, which is a terrible failure mode
+            // So instead, what we do is we get a positive code,
+            // and we avoid 0.
+            code = abs((256 + code) % 256);
+            if (code == 0) code = 255;
             FLOGF(warning, "builtin %ls returned invalid exit code %d", cmdname.c_str(), code);
         }
         return proc_status_t::from_exit_code(code);
diff --git a/tests/checks/status-value.fish b/tests/checks/status-value.fish
index 1a296acd9..9c9452130 100644
--- a/tests/checks/status-value.fish
+++ b/tests/checks/status-value.fish
@@ -1,4 +1,4 @@
-# RUN: %fish %s
+# RUN: %fish -C 'set -g fish %fish' %s
 
 # Empty commands should be 123
 set empty_var
@@ -24,3 +24,24 @@ echo $status
 # CHECKERR: {{.*}} No matches for wildcard '*gibberishgibberishgibberish*'. {{.*}}
 # CHECKERR: echo *gibberishgibberishgibberish*
 # CHECKERR:      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~^
+
+$fish -c 'exit -5'
+# CHECKERR: warning: builtin exit returned invalid exit code 251
+echo $status
+# CHECK: 251
+
+$fish -c 'exit -1'
+# CHECKERR: warning: builtin exit returned invalid exit code 255
+echo $status
+# CHECK: 255
+
+# (we avoid 0, so this is turned into 255 again)
+$fish -c 'exit -256'
+# CHECKERR: warning: builtin exit returned invalid exit code 255
+echo $status
+# CHECK: 255
+
+$fish -c 'exit -512'
+# CHECKERR: warning: builtin exit returned invalid exit code 255
+echo $status
+# CHECK: 255