diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ac3a0fce4..7d4eec2be 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -18,6 +18,7 @@ Scripting improvements - Exit codes are better aligned with bash. A failed exec now reports ``$status`` of 127 if the file is not found, and 126 if it is not executable. - ``echo`` no longer writes its output one byte at a time, improving performance and allowing use with linux' special API files (``/proc``, ``/sys`` and such) (:issue:`7836`). - fish should now better handle ``cd`` on filesystems with broken ``stat(3)`` responses (:issue:`7577`). +- Builtins now properly report a ``$status`` of 1 upon unsuccessful writes (:issue:`7857`). Interactive improvements ------------------------- diff --git a/src/builtin.cpp b/src/builtin.cpp index 3bfce8ed1..f9c218f1d 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -469,20 +469,29 @@ proc_status_t builtin_run(parser_t &parser, const wcstring_list_t &argv, io_stre } if (const builtin_data_t *data = builtin_lookup(cmdname)) { - // Construct the permutable argv array which the builtin expects. + // Construct the permutable argv array which the builtin expects, and execute the builtin. null_terminated_array_t argv_arr(argv); - maybe_t ret = data->func(parser, streams, argv_arr.get()); - if (!ret) { - return proc_status_t::empty(); - } + maybe_t builtin_ret = data->func(parser, streams, argv_arr.get()); + + // Flush our out and error streams, and check for their errors. + int out_ret = streams.out.flush_and_check_error(); + int err_ret = streams.err.flush_and_check_error(); + + // Resolve our status code. + // If the builtin itself produced an error, use that error. + // Otherwise use any errors from writing to out and writing to err, in that order. + int code = builtin_ret ? *builtin_ret : 0; + if (code == 0) code = out_ret; + if (code == 0) code = err_ret; // 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; - } + if (code > 255) code = 255; + // Handle the case of an empty status. + if (code == 0 && !builtin_ret.has_value()) { + return proc_status_t::empty(); + } return proc_status_t::from_exit_code(code); } diff --git a/src/exec.cpp b/src/exec.cpp index 6e6fcc02b..60357a71d 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -502,11 +502,6 @@ static void handle_builtin_output(parser_t &parser, const std::shared_ptr const output_stream_t &err) { assert(p->type == process_type_t::builtin && "Process is not a builtin"); - // Mark if we discarded output. - if (out.discarded() || err.discarded()) { - p->status = proc_status_t::from_exit_code(STATUS_READ_TOO_MUCH); - } - // Figure out any data remaining to write. We may have none, in which case we can short-circuit. std::string outbuff = wcs2string(out.contents()); std::string errbuff = wcs2string(err.contents()); diff --git a/src/io.cpp b/src/io.cpp index f8f91d648..8529a8594 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -299,6 +299,8 @@ void output_stream_t::append_with_separation(const wchar_t *s, size_t len, separ const wcstring &output_stream_t::contents() const { return g_empty_string; } +int output_stream_t::flush_and_check_error() { return STATUS_CMD_OK; } + void fd_output_stream_t::append(const wchar_t *s, size_t amt) { if (errored_) return; int res = wwrite_to_fd(s, amt, this->fd_); @@ -309,6 +311,11 @@ void fd_output_stream_t::append(const wchar_t *s, size_t amt) { } } +int fd_output_stream_t::flush_and_check_error() { + // Return a generic 1 on any write failure. + return errored_ ? STATUS_CMD_ERROR : STATUS_CMD_OK; +} + void null_output_stream_t::append(const wchar_t *, size_t) {} void string_output_stream_t::append(const wchar_t *s, size_t amt) { contents_.append(s, amt); } @@ -324,4 +331,9 @@ void buffered_output_stream_t::append_with_separation(const wchar_t *s, size_t l buffer_->append(wcs2string(s, len), type); } -bool buffered_output_stream_t::discarded() const { return buffer_->discarded(); } +int buffered_output_stream_t::flush_and_check_error() { + if (buffer_->discarded()) { + return STATUS_READ_TOO_MUCH; + } + return 0; +} diff --git a/src/io.h b/src/io.h index 6aa5fc613..4b8bf8fd0 100644 --- a/src/io.h +++ b/src/io.h @@ -363,14 +363,15 @@ class output_stream_t { /// Required override point. The output stream receives a string \p s with \p amt chars. virtual void append(const wchar_t *s, size_t amt) = 0; - /// \return true if output was discarded. This only applies to buffered output streams. - virtual bool discarded() const { return false; } - /// \return any internally buffered contents. /// This is only implemented for a string_output_stream; others flush data to their underlying /// receiver (fd, or separated buffer) immediately and so will return an empty string here. virtual const wcstring &contents() const; + /// Flush any unwritten data to the underlying device, and return an error code. + /// A 0 code indicates success. The base implementation returns 0. + virtual int flush_and_check_error(); + /// An optional override point. This is for explicit separation. virtual void append_with_separation(const wchar_t *s, size_t len, separation_type_t type); @@ -420,6 +421,8 @@ class fd_output_stream_t final : public output_stream_t { /// Construct from a file descriptor, which must be nonegative. explicit fd_output_stream_t(int fd) : fd_(fd) { assert(fd_ >= 0 && "Invalid fd"); } + int flush_and_check_error() override; + void append(const wchar_t *s, size_t amt) override; private: @@ -453,7 +456,7 @@ class buffered_output_stream_t final : public output_stream_t { void append(const wchar_t *s, size_t amt) override; void append_with_separation(const wchar_t *s, size_t len, separation_type_t type) override; - bool discarded() const override; + int flush_and_check_error() override; private: /// The buffer we are filling. diff --git a/tests/checks/status.fish b/tests/checks/status.fish index 5257a64f3..cc2e8dbfc 100644 --- a/tests/checks/status.fish +++ b/tests/checks/status.fish @@ -80,3 +80,28 @@ if false end echo $status #CHECK: 0 + +# Verify errors from writes - see #7857. +if test -e /dev/full + # Failed writes to stdout produce 1. + echo foo > /dev/full + if test $status -ne 1 + echo "Wrong status when writing to /dev/full" + end + + # Here the builtin should fail with status 2, + # and also the write should fail with status 1. + # The builtin has precedence. + builtin string --not-a-valid-option 2> /dev/full + if test $status -ne 2 + echo "Wrong status for failing builtin" + end + echo "Failed write tests finished" +else + echo "Failed write tests skipped" + echo "write: skipped" 1>&2 + echo "write: skipped" 1>&2 +end +# CHECK: Failed write tests {{finished|skipped}} +# CHECKERR: write: {{.*}} +# CHECKERR: write: {{.*}}