From 5a0aa7824f87eda71cadf6fa05839262a6c92864 Mon Sep 17 00:00:00 2001 From: Ethel Morgan Date: Mon, 8 Feb 2021 21:25:30 +0000 Subject: [PATCH] Saturate exit codes to 255 for all builtins After commit 6dd6a57c606afa6673f7950364f8a76610e4106d, 3 remaining builtins were affected by uint8_t overflow: `exit`, `return`, and `functions --query`. This commit: - Moves the overflow check from `builtin_set_query` to `builtin_run`. - Removes a conflicting int -> uint8_t conversion in `builtin_return`. - Adds tests for the 3 remaining affected builtins. - Simplifies the wording for the documentation for `set --query`. - Does not change documentation for `functions --query`, because it does not state the exit code in its API. - Updates the CHANGELOG to reflect the change to all builtins. --- CHANGELOG.rst | 2 +- doc_src/cmds/set.rst | 2 +- src/builtin.cpp | 10 +++++++++- src/builtin_return.cpp | 1 - src/builtin_set.cpp | 7 ------- src/proc.h | 2 ++ tests/checks/basic.fish | 12 ++++++++++++ tests/checks/functions.fish | 6 ++++++ 8 files changed, 31 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9e7845869..f0fe9029f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -183,7 +183,7 @@ Scripting improvements - ``fish -c`` now reads the remaining arguments into $argv (:issue:`2314`). - The ``pwd`` command supports the long options ``--logical`` and ``--physical``, matching other implementations (:issue:`6787`). - ``fish --profile`` now only starts the profile after fish's startup (including config.fish) is done. For profiling startup there is a new ``--profile-startup`` option that profiles only startup (:issue:`7648`). -- ``set --query``'s $status will now saturate at 255 instead of wrapping around when checking more than 255 variables at once (:issue:`7698`). +- Builtins return a maximum exit status of 255, rather than potentially overflowing. In particular, this affects ``exit``, ``return``, ``functions --query``, and ``set --query`` (:issue:`7698`, :issue:`7702`). - It is no longer an error to run builtin with closed stdin. For example ``count <&-`` now prints 0, instead of failing. diff --git a/doc_src/cmds/set.rst b/doc_src/cmds/set.rst index c7fa3e88e..2a6d47911 100644 --- a/doc_src/cmds/set.rst +++ b/doc_src/cmds/set.rst @@ -49,7 +49,7 @@ The following options are available: - ``-e`` or ``--erase`` causes the specified shell variables to be erased -- ``-q`` or ``--query`` test if the specified variable names are defined. Does not output anything, but the builtins exit status is the number of variables specified that were not defined, saturating at 255 if more than 255 variables are not defined. +- ``-q`` or ``--query`` test if the specified variable names are defined. Does not output anything, but the builtins exit status is the number of variables specified that were not defined, or 255 if more than 255 variables are not defined. - ``-n`` or ``--names`` List only the names of all defined variables, not their value. The names are guaranteed to be sorted. diff --git a/src/builtin.cpp b/src/builtin.cpp index bedd5c419..bd31ebe2d 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -473,7 +473,15 @@ proc_status_t builtin_run(parser_t &parser, wchar_t **argv, io_streams_t &stream if (!ret) { return proc_status_t::empty(); } - return proc_status_t::from_exit_code(ret.value()); + + // The exit code is cast to an 8-bit unsigned integer, so saturate to 255. Otherwise, + // multiples of 256 are reported as 0. + int code = ret.value(); + if (code > 255) { + code = 255; + } + + return proc_status_t::from_exit_code(code); } FLOGF(error, UNKNOWN_BUILTIN_ERR_MSG, argv[0]); diff --git a/src/builtin_return.cpp b/src/builtin_return.cpp index 2c384b23b..5f521960d 100644 --- a/src/builtin_return.cpp +++ b/src/builtin_return.cpp @@ -86,7 +86,6 @@ maybe_t builtin_return(parser_t &parser, io_streams_t &streams, wchar_t **a builtin_print_error_trailer(parser, streams.err, cmd); return STATUS_INVALID_ARGS; } - retval &= 0xFF; } // Find the function block. diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index 229b3e9b9..7c76dafe6 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -565,13 +565,6 @@ static int builtin_set_query(const wchar_t *cmd, set_cmd_opts_t &opts, int argc, free(dest); } - // The return value is cast to an 8-bit unsigned integer, - // so saturate the error count to 255. - // Otherwise 256 errors is reported as 0 errors. - if (retval > 255) { - retval = 255; - } - return retval; } diff --git a/src/proc.h b/src/proc.h index fa9ee7c1f..beb072997 100644 --- a/src/proc.h +++ b/src/proc.h @@ -81,6 +81,8 @@ class proc_status_t { // Some paranoia. constexpr int zerocode = w_exitcode(0, 0); static_assert(WIFEXITED(zerocode), "Synthetic exit status not reported as exited"); + + assert(ret < 256); return proc_status_t(w_exitcode(ret, 0 /* sig */)); } diff --git a/tests/checks/basic.fish b/tests/checks/basic.fish index 6f8170701..c9a2d58a8 100644 --- a/tests/checks/basic.fish +++ b/tests/checks/basic.fish @@ -110,6 +110,18 @@ end echo Test 5 $sta #CHECK: Test 5 pass + +function test_builtin_status_clamp_to_255 + return 300 +end +test_builtin_status_clamp_to_255 +echo $status +#CHECK: 255 + +$fish -c "exit 300" +echo $status +#CHECK: 255 + #################### # echo tests echo 'abc\ndef' diff --git a/tests/checks/functions.fish b/tests/checks/functions.fish index fdc705ba9..1668a991f 100644 --- a/tests/checks/functions.fish +++ b/tests/checks/functions.fish @@ -92,6 +92,12 @@ functions --erase ls type -t ls #CHECK: file +# ========== +# Verify that `functions --query` does not return 0 if there are 256 missing functions +functions --query a(seq 1 256) +echo $status +#CHECK: 255 + echo "function t; echo tttt; end" | source functions t # CHECK: # Defined via `source`