implement limits on command substitution output

This makes command substitutions impose the same limit on the amount
of data they accept as the `read` builtin. It does not limit output of
external commands or builtins in other contexts.

Fixes #3822

CHANGELOG.md

@@ -2,7 +2,7 @@
 This section is for changes merged to the `major` branch that are not also merged to 2.7.
 
 ## Deprecations
-- The `IFS` variable (#4156).
+- The `IFS` variable is deprecated and will be removed in fish 4.0 (#4156).
 
 ## Notable fixes and improvements
 - Local exported (`set -lx`) vars are now visible to functions (#1091).
@@ -10,6 +10,7 @@ This section is for changes merged to the `major` branch that are not also merge
 - `complete` now has a `-k`/`--keep-order` flag to keep the order of the OPTION_ARGUMENTS (#361).
 - A "--delimiter" option to `read` as a better alternative to the `IFS` variable (#4256).
 - `set` has a new `--show` option to show lots of information about variables (#4265).
+- Command substitution output is now limited to 10 MB by default (#3822).
 
 ## Other significant changes
 - `read` now requires at least one var name (#4220).

src/exec.cpp

@@ -382,13 +382,15 @@ void exec_job(parser_t &parser, job_t *j) {
     // Verify that all IO_BUFFERs are output. We used to support a (single, hacked-in) magical input
     // IO_BUFFER used by fish_pager, but now the claim is that there are no more clients and it is
     // removed. This assertion double-checks that.
+    size_t stdout_read_limit = 0;
     const io_chain_t all_ios = j->all_io_redirections();
     for (size_t idx = 0; idx < all_ios.size(); idx++) {
         const shared_ptr<io_data_t> &io = all_ios.at(idx);
 
         if ((io->io_mode == IO_BUFFER)) {
             io_buffer_t *io_buffer = static_cast<io_buffer_t *>(io.get());
-            assert(!io_buffer->is_input);  //!OCLINT(multiple unary operator)
+            assert(!io_buffer->is_input);
+            stdout_read_limit = io_buffer->get_buffer_limit();
         }
     }
 
@@ -605,7 +607,7 @@ void exec_job(parser_t &parser, job_t *j) {
         shared_ptr<io_buffer_t> block_output_io_buffer;
 
         // This is the io_streams we pass to internal builtins.
-        std::unique_ptr<io_streams_t> builtin_io_streams;
+        std::unique_ptr<io_streams_t> builtin_io_streams(new io_streams_t(stdout_read_limit));
 
         switch (p->type) {
             case INTERNAL_FUNCTION: {
@@ -754,7 +756,6 @@ void exec_job(parser_t &parser, job_t *j) {
                         stdin_is_directly_redirected = stdin_io && stdin_io->io_mode != IO_CLOSE;
                     }
 
-                    builtin_io_streams.reset(new io_streams_t());
                     builtin_io_streams->stdin_fd = local_builtin_stdin;
                     builtin_io_streams->out_is_redirected =
                         has_fd(process_net_io_chain, STDOUT_FILENO);
@@ -882,8 +883,9 @@ void exec_job(parser_t &parser, job_t *j) {
                     const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER;
                     const bool no_stdout_output = stdout_buffer.empty();
                     const bool no_stderr_output = stderr_buffer.empty();
+                    const bool stdout_discarded = builtin_io_streams->out.output_discarded();
 
-                    if (no_stdout_output && no_stderr_output) {
+                    if (!stdout_discarded && no_stdout_output && no_stderr_output) {
                         // The builtin produced no output and is not inside of a pipeline. No
                         // need to fork or even output anything.
                         debug(3, L"Skipping fork: no output for internal builtin '%ls'",
@@ -897,13 +899,15 @@ void exec_job(parser_t &parser, job_t *j) {
                               p->argv0());
 
                         io_buffer_t *io_buffer = static_cast<io_buffer_t *>(stdout_io.get());
+                        if (stdout_discarded) {
+                            io_buffer->set_discard();
+                        } else {
                             const std::string res = wcs2string(builtin_io_streams->out.buffer());
-
                             io_buffer->out_buffer_append(res.data(), res.size());
+                        }
                         fork_was_skipped = true;
                     } else if (stdout_io.get() == NULL && stderr_io.get() == NULL) {
-                        // We are writing to normal stdout and stderr. Just do it - no need to
+                        // We are writing to normal stdout and stderr. Just do it - no need to fork.
-                        // fork.
                         debug(3, L"Skipping fork: ordinary output for internal builtin '%ls'",
                               p->argv0());
                         const std::string outbuff = wcs2string(stdout_buffer);
@@ -915,6 +919,7 @@ void exec_job(parser_t &parser, job_t *j) {
                             debug(0, "!builtin_io_done and errno != EPIPE");
                             show_stackframe(L'E');
                         }
+                        if (stdout_discarded) p->status = STATUS_READ_TOO_MUCH;
                         fork_was_skipped = true;
                     }
                 }
@@ -1099,8 +1104,8 @@ void exec_job(parser_t &parser, job_t *j) {
     }
 }
 
-static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst,
+static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst, bool apply_exit_status,
-                                  bool apply_exit_status) {
+                                  bool is_subcmd) {
     ASSERT_IS_MAIN_THREAD();
     bool prev_subshell = is_subshell;
     const int prev_status = proc_get_last_status();
@@ -1116,7 +1121,8 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst,
 
     // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may
     // be null.
-    const shared_ptr<io_buffer_t> io_buffer(io_buffer_t::create(STDOUT_FILENO, io_chain_t()));
+    const shared_ptr<io_buffer_t> io_buffer(
+        io_buffer_t::create(STDOUT_FILENO, io_chain_t(), is_subcmd ? read_byte_limit : 0));
     if (io_buffer.get() != NULL) {
         parser_t &parser = parser_t::principal_parser();
         if (parser.eval(cmd, io_chain_t(io_buffer), SUBST) == 0) {
@@ -1126,6 +1132,8 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst,
         io_buffer->read();
     }
 
+    if (io_buffer->output_discarded()) subcommand_status = STATUS_READ_TOO_MUCH;
+
     // If the caller asked us to preserve the exit status, restore the old status. Otherwise set the
     // status of the subcommand.
     proc_set_last_status(apply_exit_status ? subcommand_status : prev_status);
@@ -1166,12 +1174,13 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst,
     return subcommand_status;
 }
 
-int exec_subshell(const wcstring &cmd, std::vector<wcstring> &outputs, bool apply_exit_status) {
+int exec_subshell(const wcstring &cmd, std::vector<wcstring> &outputs, bool apply_exit_status,
+                  bool is_subcmd) {
     ASSERT_IS_MAIN_THREAD();
-    return exec_subshell_internal(cmd, &outputs, apply_exit_status);
+    return exec_subshell_internal(cmd, &outputs, apply_exit_status, is_subcmd);
 }
 
-int exec_subshell(const wcstring &cmd, bool apply_exit_status) {
+int exec_subshell(const wcstring &cmd, bool apply_exit_status, bool is_subcmd) {
     ASSERT_IS_MAIN_THREAD();
-    return exec_subshell_internal(cmd, NULL, apply_exit_status);
+    return exec_subshell_internal(cmd, NULL, apply_exit_status, is_subcmd);
 }

src/exec.h

@@ -35,8 +35,9 @@ void exec_job(parser_t &parser, job_t *j);
 /// \param outputs The list to insert output into.
 ///
 /// \return the status of the last job to exit, or -1 if en error was encountered.
-int exec_subshell(const wcstring &cmd, std::vector<wcstring> &outputs, bool preserve_exit_status);
+int exec_subshell(const wcstring &cmd, std::vector<wcstring> &outputs, bool preserve_exit_status,
-int exec_subshell(const wcstring &cmd, bool preserve_exit_status);
+                  bool is_subcmd = false);
+int exec_subshell(const wcstring &cmd, bool preserve_exit_status, bool is_subcmd = false);
 
 /// Loops over close until the syscall was run without being interrupted.
 void exec_close(int fd);

src/expand.cpp

@@ -102,7 +102,8 @@ static bool expand_is_clean(const wcstring &in) {
 /// Append a syntax error to the given error list.
 static void append_syntax_error(parse_error_list_t *errors, size_t source_start, const wchar_t *fmt,
                                 ...) {
-    if (errors != NULL) {
+    if (!errors) return;
+
     parse_error_t error;
     error.source_start = source_start;
     error.source_length = 0;
@@ -114,13 +115,15 @@ static void append_syntax_error(parse_error_list_t *errors, size_t source_start,
     va_end(va);
 
     errors->push_back(error);
-    }
 }
 
-/// Append a cmdsub error to the given error list.
+/// Append a cmdsub error to the given error list. But only do so if the error hasn't already been
+/// recorded. This is needed because command substitution is a recursive process and some errors
+/// could consequently be recorded more than once.
 static void append_cmdsub_error(parse_error_list_t *errors, size_t source_start, const wchar_t *fmt,
                                 ...) {
-    if (errors != NULL) {
+    if (!errors) return;
+
     parse_error_t error;
     error.source_start = source_start;
     error.source_length = 0;
@@ -131,8 +134,11 @@ static void append_cmdsub_error(parse_error_list_t *errors, size_t source_start,
     error.text = vformat_string(fmt, va);
     va_end(va);
 
-        errors->push_back(error);
+    for (auto it : *errors) {
+        if (error.text == it.text) return;
     }
+
+    errors->push_back(error);
 }
 
 /// Return the environment variable value for the string starting at \c in.
@@ -1024,24 +1030,23 @@ static expand_error_t expand_brackets(const wcstring &instr, expand_flags_t flag
 }
 
 /// Perform cmdsubst expansion.
-static int expand_cmdsubst(const wcstring &input, std::vector<completion_t> *out_list,
+static bool expand_cmdsubst(const wcstring &input, std::vector<completion_t> *out_list,
                             parse_error_list_t *errors) {
-    wchar_t *paran_begin = 0, *paran_end = 0;
+    wchar_t *paren_begin = nullptr, *paren_end = nullptr;
-    std::vector<wcstring> sub_res;
+    wchar_t *tail_begin = nullptr;
     size_t i, j;
-    wchar_t *tail_begin = 0;
 
     const wchar_t *const in = input.c_str();
 
     int parse_ret;
-    switch (parse_ret = parse_util_locate_cmdsubst(in, &paran_begin, &paran_end, false)) {
+    switch (parse_ret = parse_util_locate_cmdsubst(in, &paren_begin, &paren_end, false)) {
         case -1: {
             append_syntax_error(errors, SOURCE_LOCATION_UNKNOWN, L"Mismatched parenthesis");
-            return 0;
+            return false;
         }
         case 0: {
             append_completion(out_list, input);
-            return 1;
+            return true;
         }
         case 1: {
             break;
@@ -1052,15 +1057,22 @@ static int expand_cmdsubst(const wcstring &input, std::vector<completion_t> *out
         }
     }
 
-    const wcstring subcmd(paran_begin + 1, paran_end - paran_begin - 1);
+    wcstring_list_t sub_res;
-
+    const wcstring subcmd(paren_begin + 1, paren_end - paren_begin - 1);
-    if (exec_subshell(subcmd, sub_res, true /* do apply exit status */) == -1) {
+    if (exec_subshell(subcmd, sub_res, true /* apply_exit_status */, true /* is_subcmd */) == -1) {
         append_cmdsub_error(errors, SOURCE_LOCATION_UNKNOWN,
                             L"Unknown error while evaulating command substitution");
-        return 0;
+        return false;
     }
 
-    tail_begin = paran_end + 1;
+    if (proc_get_last_status() == STATUS_READ_TOO_MUCH) {
+        append_cmdsub_error(
+            errors, in - paren_begin,
+            _(L"Too much data emitted by command substitution so it was discarded\n"));
+        return false;
+    }
+
+    tail_begin = paren_end + 1;
     if (*tail_begin == L'[') {
         std::vector<long> slice_idx;
         std::vector<size_t> slice_source_positions;
@@ -1072,7 +1084,7 @@ static int expand_cmdsubst(const wcstring &input, std::vector<completion_t> *out
             parse_slice(slice_begin, &slice_end, slice_idx, slice_source_positions, sub_res.size());
         if (bad_pos != 0) {
             append_syntax_error(errors, slice_begin - in + bad_pos, L"Invalid index value");
-            return 0;
+            return false;
         }
 
         wcstring_list_t sub_res2;
@@ -1110,7 +1122,7 @@ static int expand_cmdsubst(const wcstring &input, std::vector<completion_t> *out
             const wcstring &tail_item = tail_expand.at(j).completion;
 
             // sb_append_substring( &whole_item, in, len1 );
-            whole_item.append(in, paran_begin - in);
+            whole_item.append(in, paren_begin - in);
 
             // sb_append_char( &whole_item, INTERNAL_SEPARATOR );
             whole_item.push_back(INTERNAL_SEPARATOR);
@@ -1129,7 +1141,8 @@ static int expand_cmdsubst(const wcstring &input, std::vector<completion_t> *out
         }
     }
 
-    return 1;
+    if (proc_get_last_status() == STATUS_READ_TOO_MUCH) return false;
+    return true;
 }
 
 // Given that input[0] is HOME_DIRECTORY or tilde (ugh), return the user's name. Return the empty
@@ -1304,7 +1317,6 @@ typedef expand_error_t (*expand_stage_t)(const wcstring &input,           //!OCL
 
 static expand_error_t expand_stage_cmdsubst(const wcstring &input, std::vector<completion_t> *out,
                                             expand_flags_t flags, parse_error_list_t *errors) {
-    expand_error_t result = EXPAND_OK;
     if (EXPAND_SKIP_CMDSUBST & flags) {
         wchar_t *begin, *end;
         if (parse_util_locate_cmdsubst(input.c_str(), &begin, &end, true) == 0) {
@@ -1312,15 +1324,14 @@ static expand_error_t expand_stage_cmdsubst(const wcstring &input, std::vector<c
         } else {
             append_cmdsub_error(errors, SOURCE_LOCATION_UNKNOWN,
                                 L"Command substitutions not allowed");
-            result = EXPAND_ERROR;
+            return EXPAND_ERROR;
         }
     } else {
-        int cmdsubst_ok = expand_cmdsubst(input, out, errors);
+        bool cmdsubst_ok = expand_cmdsubst(input, out, errors);
-        if (!cmdsubst_ok) {
+        if (!cmdsubst_ok) return EXPAND_ERROR;
-            result = EXPAND_ERROR;
     }
-    }
+
-    return result;
+    return EXPAND_OK;
 }
 
 static expand_error_t expand_stage_variables(const wcstring &input, std::vector<completion_t> *out,

src/fish_tests.cpp

@@ -2013,7 +2013,7 @@ static bool run_one_test_test(int expected, wcstring_list_t &lst, bool bracket)
         i++;
     }
     argv[i + 1] = NULL;
-    io_streams_t streams;
+    io_streams_t streams(0);
     int result = builtin_test(parser, streams, argv);
     delete[] argv;
     return expected == result;
@@ -2040,7 +2040,7 @@ static bool run_test_test(int expected, const wcstring &str) {
 static void test_test_brackets() {
     // Ensure [ knows it needs a ].
     parser_t parser;
-    io_streams_t streams;
+    io_streams_t streams(0);
 
     const wchar_t *argv1[] = {L"[", L"foo", NULL};
     do_test(builtin_test(parser, streams, (wchar_t **)argv1) != 0);
@@ -3930,7 +3930,7 @@ int builtin_string(parser_t &parser, io_streams_t &streams, wchar_t **argv);
 static void run_one_string_test(const wchar_t **argv, int expected_rc,
                                 const wchar_t *expected_out) {
     parser_t parser;
-    io_streams_t streams;
+    io_streams_t streams(0);
     streams.stdin_is_directly_redirected = false;  // read from argv instead of stdin
     int rc = builtin_string(parser, streams, const_cast<wchar_t **>(argv));
     wcstring args;

src/io.cpp

@@ -75,10 +75,11 @@ bool io_buffer_t::avoid_conflicts_with_io_chain(const io_chain_t &ios) {
     return result;
 }
 
-shared_ptr<io_buffer_t> io_buffer_t::create(int fd, const io_chain_t &conflicts) {
+shared_ptr<io_buffer_t> io_buffer_t::create(int fd, const io_chain_t &conflicts,
+                                            size_t buffer_limit) {
     bool success = true;
     assert(fd >= 0);
-    shared_ptr<io_buffer_t> buffer_redirect(new io_buffer_t(fd));
+    shared_ptr<io_buffer_t> buffer_redirect(new io_buffer_t(fd, buffer_limit));
 
     if (exec_pipe(buffer_redirect->pipe_fd) == -1) {
         debug(1, PIPE_ERROR);

src/io.h

@@ -20,6 +20,7 @@ using std::tr1::shared_ptr;
 #endif
 
 #include "common.h"
+#include "env.h"
 
 /// Describes what type of IO operation an io_data_t represents.
 enum io_mode_t { IO_FILE, IO_PIPE, IO_FD, IO_BUFFER, IO_CLOSE };
@@ -99,10 +100,18 @@ class io_pipe_t : public io_data_t {
 class io_chain_t;
 class io_buffer_t : public io_pipe_t {
    private:
+    /// True if we're discarding input.
+    bool discard;
+    /// Limit on how much data we'll buffer. Zero means no limit.
+    size_t buffer_limit;
     /// Buffer to save output in.
     std::vector<char> out_buffer;
 
-    explicit io_buffer_t(int f) : io_pipe_t(IO_BUFFER, f, false /* not input */), out_buffer() {}
+    explicit io_buffer_t(int f, size_t limit)
+        : io_pipe_t(IO_BUFFER, f, false /* not input */),
+          discard(false),
+          buffer_limit(limit),
+          out_buffer() {}
 
    public:
     virtual void print() const;
@@ -111,6 +120,12 @@ class io_buffer_t : public io_pipe_t {
 
     /// Function to append to the buffer.
     void out_buffer_append(const char *ptr, size_t count) {
+        if (discard) return;
+        if (buffer_limit && out_buffer.size() + count > buffer_limit) {
+            discard = true;
+            out_buffer.clear();
+            return;
+        }
         out_buffer.insert(out_buffer.end(), ptr, ptr + count);
     }
 
@@ -122,6 +137,19 @@ class io_buffer_t : public io_pipe_t {
     /// Function to get the size of the buffer.
     size_t out_buffer_size(void) const { return out_buffer.size(); }
 
+    /// Function that returns true if we discarded the input because there was too much data.
+    bool output_discarded(void) { return discard; }
+
+    /// Function to explicitly put the object in discard mode. Meant to be used when moving
+    /// the results from an output_stream_t to an io_buffer_t.
+    void set_discard(void) {
+        discard = true;
+        out_buffer.clear();
+    }
+
+    /// This is used to transfer the buffer limit for this object to a output_stream_t object.
+    size_t get_buffer_limit(void) { return buffer_limit; }
+
     /// Ensures that the pipes do not conflict with any fd redirections in the chain.
     bool avoid_conflicts_with_io_chain(const io_chain_t &ios);
 
@@ -134,7 +162,8 @@ class io_buffer_t : public io_pipe_t {
     /// \param fd the fd that will be mapped in the child process, typically STDOUT_FILENO
     /// \param conflicts A set of IO redirections. The function ensures that any pipe it makes does
     /// not conflict with an fd redirection in this list.
-    static shared_ptr<io_buffer_t> create(int fd, const io_chain_t &conflicts);
+    static shared_ptr<io_buffer_t> create(int fd, const io_chain_t &conflicts,
+                                          size_t buffer_limit = 0);
 };
 
 class io_chain_t : public std::vector<shared_ptr<io_data_t> > {
@@ -164,37 +193,79 @@ bool pipe_avoid_conflicts_with_io_chain(int fds[2], const io_chain_t &ios);
 /// Class representing the output that a builtin can generate.
 class output_stream_t {
    private:
+    /// Limit on how much data we'll buffer. Zero means no limit.
+    size_t buffer_limit;
+    /// True if we're discarding input.
+    bool discard;
     // No copying.
     output_stream_t(const output_stream_t &s);
     void operator=(const output_stream_t &s);
 
     wcstring buffer_;
 
+    void check_for_overflow() {
+        if (buffer_limit && buffer_.size() > buffer_limit) {
+            discard = true;
+            buffer_.clear();
+        }
+    }
+
    public:
-    output_stream_t() {}
+    output_stream_t(size_t buffer_limit_) : buffer_limit(buffer_limit_), discard(false) {}
 
-    void append(const wcstring &s) { this->buffer_.append(s); }
+#if 0
+    void set_buffer_limit(size_t buffer_limit_) { buffer_limit = buffer_limit_; }
+#endif
 
-    void append(const wchar_t *s) { this->buffer_.append(s); }
+    void append(const wcstring &s) {
+        if (discard) return;
+        buffer_.append(s);
+        check_for_overflow();
+    }
 
-    void append(wchar_t s) { this->buffer_.push_back(s); }
+    void append(const wchar_t *s) {
+        if (discard) return;
+        buffer_.append(s);
+        check_for_overflow();
+    }
 
-    void append(const wchar_t *s, size_t amt) { this->buffer_.append(s, amt); }
+    void append(wchar_t s) {
+        if (discard) return;
+        buffer_.push_back(s);
+        check_for_overflow();
+    }
 
-    void push_back(wchar_t c) { this->buffer_.push_back(c); }
+    void append(const wchar_t *s, size_t amt) {
+        if (discard) return;
+        buffer_.append(s, amt);
+        check_for_overflow();
+    }
+
+    void push_back(wchar_t c) {
+        if (discard) return;
+        buffer_.push_back(c);
+        check_for_overflow();
+    }
 
     void append_format(const wchar_t *format, ...) {
+        if (discard) return;
         va_list va;
         va_start(va, format);
-        ::append_formatv(this->buffer_, format, va);
+        ::append_formatv(buffer_, format, va);
         va_end(va);
+        check_for_overflow();
     }
 
     void append_formatv(const wchar_t *format, va_list va_orig) {
-        ::append_formatv(this->buffer_, format, va_orig);
+        if (discard) return;
+        ::append_formatv(buffer_, format, va_orig);
+        check_for_overflow();
     }
 
-    const wcstring &buffer() const { return this->buffer_; }
+    const wcstring &buffer() const { return buffer_; }
+
+    /// Function that returns true if we discarded the input because there was too much data.
+    bool output_discarded(void) { return discard; }
 
     bool empty() const { return buffer_.empty(); }
 };
@@ -218,8 +289,10 @@ struct io_streams_t {
     // Actual IO redirections. This is only used by the source builtin. Unowned.
     const io_chain_t *io_chain;
 
-    io_streams_t()
+    io_streams_t(size_t read_limit)
-        : stdin_fd(-1),
+        : out(read_limit),
+          err(read_limit),
+          stdin_fd(-1),
           stdin_is_directly_redirected(false),
           out_is_redirected(false),
           err_is_redirected(false),

src/parse_execution.cpp

@@ -388,7 +388,7 @@ parse_execution_result_t parse_execution_context_t::run_function_statement(
     const wcstring contents_str =
         wcstring(this->src, contents_start, contents_end - contents_start);
     int definition_line_offset = this->line_offset_of_character_at_offset(contents_start);
-    io_streams_t streams;
+    io_streams_t streams(0);  // no limit on the amount of output from builtin_function()
     int err =
         builtin_function(*parser, streams, argument_list, contents_str, definition_line_offset);
     proc_set_last_status(err);

src/parser.cpp

@@ -724,7 +724,6 @@ int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io,
     this->pop_block(scope_block);
 
     job_reap(0);  // reap again
-
     return result;
 }
 

tests/test8.in

@@ -16,7 +16,7 @@ set test1[1..$n] $test; echo $test1
 set test1[$n..1] $test; echo $test1
 set test1[2..4 -2..-4] $test1[4..2 -4..-2]; echo $test1
 
-echo Test command substitution
+echo Test using slices of command substitution
 echo (seq 5)[-1..1]
 echo (seq $n)[3..5 -2..2]
 

tests/test8.out

@@ -9,7 +9,7 @@ Test variable set
 1 2 3 4 5 6 7 8 9 10
 10 9 8 7 6 5 4 3 2 1
 10 7 8 9 6 5 2 3 4 1
-Test command substitution
+Test using slices of command substitution
 5 4 3 2 1
 3 4 5 9 8 7 6 5 4 3 2
 Test more

tests/test_cmdsub.err

@@ -0,0 +1,22 @@
+# Command sub at the limit should fail.
+fish: Too much data emitted by command substitution so it was discarded
+
+set b (string repeat -n 512 x)
+      ^
+# Command sub over the limit should fail.
+fish: Too much data emitted by command substitution so it was discarded
+
+set -l x (string repeat -n $argv x)
+         ^
+in function 'subme'
+	called on standard input
+	with parameter list '513'
+
+in command substitution
+	called on standard input
+
+# Same builtin in a command substitution is affected
+fish: Too much data emitted by command substitution so it was discarded
+
+echo this will fail (string repeat --max 513 b) to output anything
+                    ^

tests/test_cmdsub.in

@@ -0,0 +1,35 @@
+# This tests various corner cases involving command substitution. Most
+# importantly the limits on the amount of data we'll substitute.
+
+set FISH_READ_BYTE_LIMIT 512
+
+function subme
+    set -l x (string repeat -n $argv x)
+    echo $x
+end
+
+echo '# Command sub just under the limit should succeed.'
+set a (subme 511)
+show a
+
+echo '# Command sub at the limit should fail.'
+echo '# Command sub at the limit should fail.' >&2
+set b (string repeat -n 512 x)
+set saved_status $status
+test $saved_status -eq 122
+or echo expected status 122, saw $saved_status >&2
+show b
+
+echo '# Command sub over the limit should fail.'
+echo '# Command sub over the limit should fail.' >&2
+set c (subme 513)
+show c
+
+echo '# Make sure output from builtins outside of command substitution is not affected'
+string repeat --max 513 a
+
+echo '# Same builtin in a command substitution is affected' >&2
+echo this will fail (string repeat --max 513 b) to output anything
+set saved_status $status
+test $saved_status -eq 122
+or echo expected status 122, saw $saved_status >&2

tests/test_cmdsub.out

@@ -0,0 +1,10 @@
+# Command sub just under the limit should succeed.
+$a count=1
+$a[1]=|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|
+# Command sub at the limit should fail.
+$b is not set
+# Command sub over the limit should fail.
+$c count=1
+$c[1]=||
+# Make sure output from builtins outside of command substitution is not affected
+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

tests/test_functions/show.fish

@@ -1,18 +1,18 @@
 # Show information about the named var(s) passed to us.
 function show --no-scope-shadowing
-    for v in $argv
+    for _v in $argv
-        if set -q $v
+        if set -q $_v
-            set -l c (count $$v)
+            set -l _c (count $$_v)
-            printf '$%s count=%d\n' $v $c
+            printf '$%s count=%d\n' $_v $_c
             # This test is to work around a bogosity of the BSD `seq` command. If called with
             # `seq 0` it emits `1`, `0`. Whereas that GNU version outputs nothing.
-            if test $c -gt 0
+            if test $_c -gt 0
-                for i in (seq $c)
+                for i in (seq $_c)
-                    printf '$%s[%d]=|%s|\n' $v $i $$v[1][$i]
+                    printf '$%s[%d]=|%s|\n' $_v $i $$_v[1][$i]
                 end
             end
         else
-            echo \$$v is not set
+            echo \$$_v is not set
         end
     end
 end