From 938b6838951e24dff631a9dce8c5131ef5940f4b Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 11 Mar 2020 11:06:52 -0700 Subject: [PATCH 1/3] Thread pgroups into command substitutions Give string expansion an (optional) parent pgroup. This is threaded all the way into eval(). This ensures that in a mixed pipeline like: cmd | begin ; something (cmd2) ; end that cmd2 and cmd have the same pgroup. Add a test to ensure that command substitutions inherit pgroups properly. Fixes #6624 --- src/exec.cpp | 20 +++++++++++++------- src/exec.h | 6 ++++-- src/expand.cpp | 12 +++++++----- src/operation_context.h | 5 +++++ src/parser.cpp | 11 ++++++++--- src/parser.h | 5 +++-- tests/checks/pipeline-pgroup.fish | 6 ++++-- 7 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 0d2291c42..34c402fae 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -1096,13 +1096,15 @@ static void populate_subshell_output(wcstring_list_t *lst, const io_buffer_t &bu /// Execute \p cmd in a subshell in \p parser. If \p lst is not null, populate it with the output. /// Return $status in \p out_status. +/// If \p parent_pgid is set, any spawned commands should join that pgroup. /// If \p apply_exit_status is false, then reset $status back to its original value. /// \p is_subcmd controls whether we apply a read limit. /// \p break_expand is used to propagate whether the result should be "expansion breaking" in the /// sense that subshells used during string expansion should halt that expansion. \return the value /// of $status. -static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstring_list_t *lst, - bool *break_expand, bool apply_exit_status, bool is_subcmd) { +static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, + wcstring_list_t *lst, bool *break_expand, bool apply_exit_status, + bool is_subcmd) { ASSERT_IS_MAIN_THREAD(); auto &ld = parser.libdata(); @@ -1125,7 +1127,8 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin *break_expand = true; return STATUS_CMD_ERROR; } - eval_res_t eval_res = parser.eval(cmd, io_chain_t{bufferfill}, block_type_t::subst); + eval_res_t eval_res = + parser.eval(cmd, io_chain_t{bufferfill}, parent_pgid, block_type_t::subst); std::shared_ptr buffer = io_bufferfill_t::finish(std::move(bufferfill)); if (buffer->buffer().discarded()) { *break_expand = true; @@ -1144,21 +1147,24 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin return eval_res.status.status_value(); } -int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs) { +int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, + wcstring_list_t &outputs) { ASSERT_IS_MAIN_THREAD(); bool break_expand = false; - int ret = exec_subshell_internal(cmd, parser, &outputs, &break_expand, true, true); + int ret = exec_subshell_internal(cmd, parser, parent_pgid, &outputs, &break_expand, true, true); // Only return an error code if we should break expansion. return break_expand ? ret : STATUS_CMD_OK; } int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status) { bool break_expand = false; - return exec_subshell_internal(cmd, parser, nullptr, &break_expand, apply_exit_status, false); + return exec_subshell_internal(cmd, parser, none(), nullptr, &break_expand, apply_exit_status, + false); } int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs, bool apply_exit_status) { bool break_expand = false; - return exec_subshell_internal(cmd, parser, &outputs, &break_expand, apply_exit_status, false); + return exec_subshell_internal(cmd, parser, none(), &outputs, &break_expand, apply_exit_status, + false); } diff --git a/src/exec.h b/src/exec.h index 6c52be8db..7eb657209 100644 --- a/src/exec.h +++ b/src/exec.h @@ -29,8 +29,10 @@ int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &output /// Like exec_subshell, but only returns expansion-breaking errors. That is, a zero return means /// "success" (even though the command may have failed), a non-zero return means that we should -/// halt expansion. -int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs); +/// halt expansion. If the \p pgid is supplied, then any spawned external commands should join that +/// pgroup. +int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, + wcstring_list_t &outputs); /// Loops over close until the syscall was run without being interrupted. void exec_close(int fd); diff --git a/src/expand.cpp b/src/expand.cpp index 942461dff..8165645d1 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -591,10 +591,12 @@ static expand_result_t expand_braces(wcstring &&instr, expand_flags_t flags, com return expand_result_t::ok; } -/// Expand a command substitution \p input, executing on \p parser, and inserting the results into +/// Expand a command substitution \p input, executing on \p ctx, and inserting the results into /// \p out_list, or any errors into \p errors. \return an expand result. -static expand_result_t expand_cmdsubst(wcstring input, parser_t &parser, +static expand_result_t expand_cmdsubst(wcstring input, const operation_context_t &ctx, completion_list_t *out_list, parse_error_list_t *errors) { + assert(ctx.parser && "Cannot expand without a parser"); + wchar_t *paren_begin = nullptr, *paren_end = nullptr; wchar_t *tail_begin = nullptr; size_t i, j; @@ -620,7 +622,7 @@ static expand_result_t expand_cmdsubst(wcstring input, parser_t &parser, wcstring_list_t sub_res; const wcstring subcmd(paren_begin + 1, paren_end - paren_begin - 1); - int subshell_status = exec_subshell_for_expand(subcmd, parser, sub_res); + int subshell_status = exec_subshell_for_expand(subcmd, *ctx.parser, ctx.parent_pgid, sub_res); if (subshell_status != 0) { // TODO: Ad-hoc switch, how can we enumerate the possible errors more safely? const wchar_t *err; @@ -674,7 +676,7 @@ static expand_result_t expand_cmdsubst(wcstring input, parser_t &parser, // Recursively call ourselves to expand any remaining command substitutions. The result of this // recursive call using the tail of the string is inserted into the tail_expand array list completion_list_t tail_expand; - expand_cmdsubst(tail_begin, parser, &tail_expand, errors); // TODO: offset error locations + expand_cmdsubst(tail_begin, ctx, &tail_expand, errors); // TODO: offset error locations // Combine the result of the current command substitution with the result of the recursive tail // expansion. @@ -919,7 +921,7 @@ expand_result_t expander_t::stage_cmdsubst(wcstring input, completion_list_t *ou } } else { assert(ctx.parser && "Must have a parser to expand command substitutions"); - return expand_cmdsubst(std::move(input), *ctx.parser, out, errors); + return expand_cmdsubst(std::move(input), ctx, out, errors); } } diff --git a/src/operation_context.h b/src/operation_context.h index a230596a8..6eb79831e 100644 --- a/src/operation_context.h +++ b/src/operation_context.h @@ -29,6 +29,11 @@ class operation_context_t { // context itself. const environment_t &vars; + /// The pgid of the parental job. + /// This is used only when expanding command substitutions. If this is set, any jobs created by + /// the command substitions should use this pgid. + maybe_t parent_pgid{}; + // A function which may be used to poll for cancellation. cancel_checker_t cancel_checker; diff --git a/src/parser.cpp b/src/parser.cpp index 45250008b..87a000d0e 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -640,11 +640,12 @@ profile_item_t *parser_t::create_profile_item() { return result; } -eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t block_type) { +eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, maybe_t parent_pgid, + enum block_type_t block_type) { // Parse the source into a tree, if we can. parse_error_list_t error_list; if (parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list)) { - return this->eval(ps, io, block_type); + return this->eval(ps, io, parent_pgid, block_type); } else { // Get a backtrace. This includes the message. wcstring backtrace_and_desc; @@ -661,11 +662,12 @@ eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, enum block_ } eval_res_t parser_t::eval(const parsed_source_ref_t &ps, const io_chain_t &io, - enum block_type_t block_type) { + maybe_t parent_pgid, enum block_type_t block_type) { assert(block_type == block_type_t::top || block_type == block_type_t::subst); if (!ps->tree.empty()) { job_lineage_t lineage; lineage.block_io = io; + lineage.parent_pgid = parent_pgid; // Execute the first node. tnode_t start{&ps->tree, &ps->tree.front()}; return this->eval_node(ps, start, std::move(lineage), block_type); @@ -703,6 +705,9 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_t node, operation_context_t op_ctx = this->context(); block_t *scope_block = this->push_block(block_t::scope_block(block_type)); + // Propogate any parent pgid. + op_ctx.parent_pgid = lineage.parent_pgid; + // Create and set a new execution context. using exc_ctx_ref_t = std::unique_ptr; scoped_push exc(&execution_context, make_unique( diff --git a/src/parser.h b/src/parser.h index b93b031b4..6839158b3 100644 --- a/src/parser.h +++ b/src/parser.h @@ -282,19 +282,20 @@ class parser_t : public std::enable_shared_from_this { /// /// \param cmd the string to evaluate /// \param io io redirections to perform on all started jobs + /// \param parent_pgid if set, the pgid to give to spawned jobs /// \param block_type The type of block to push on the block stack, which must be either 'top' /// or 'subst'. /// \param break_expand If not null, return by reference whether the error ought to be an expand /// error. This includes nested expand errors, and command-not-found. /// /// \return the result of evaluation. - eval_res_t eval(const wcstring &cmd, const io_chain_t &io, + eval_res_t eval(const wcstring &cmd, const io_chain_t &io, maybe_t parent_pgid = {}, block_type_t block_type = block_type_t::top); /// Evaluate the parsed source ps. /// Because the source has been parsed, a syntax error is impossible. eval_res_t eval(const parsed_source_ref_t &ps, const io_chain_t &io, - block_type_t block_type = block_type_t::top); + maybe_t parent_pgid = {}, block_type_t block_type = block_type_t::top); /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. diff --git a/tests/checks/pipeline-pgroup.fish b/tests/checks/pipeline-pgroup.fish index 7973d45c8..4b7c86e8b 100644 --- a/tests/checks/pipeline-pgroup.fish +++ b/tests/checks/pipeline-pgroup.fish @@ -11,11 +11,13 @@ end # Here everything should live in the pgroup of the first fish_test_helper. $fth print_pgrp | read -g global_group | save_pgroup g1 | begin save_pgroup g2 +end | begin + echo (save_pgroup g3) >/dev/null end -[ "$global_group" -eq "$g1" ] && [ "$g1" -eq "$g2" ] +[ "$global_group" -eq "$g1" ] && [ "$g1" -eq "$g2" ] && [ "$g2" -eq "$g3" ] and echo "All pgroups agreed" -or echo "Pgroups disagreed. Should be in $global_group but found $g1 and $g2" +or echo "Pgroups disagreed. Should be in $global_group but found $g1, $g2, $g3" # CHECK: All pgroups agreed # Here everything should live in fish's pgroup. From 82f2d867181a9ec2a640f551752cb461d8ed9349 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 25 Apr 2020 17:11:36 -0700 Subject: [PATCH 2/3] Thread pgroups into builtin_eval Ensure that if eval is invoked as part of a pipeline, any jobs spawned by eval will have the same pgroup as the parent job. Partially fixes #6806 --- src/builtin_eval.cpp | 2 +- src/exec.cpp | 1 + src/io.h | 5 +++++ tests/checks/pipeline-pgroup.fish | 5 +++++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp index 41512e63f..8c4f9bb6e 100644 --- a/src/builtin_eval.cpp +++ b/src/builtin_eval.cpp @@ -26,7 +26,7 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) { int status = STATUS_CMD_OK; if (argc > 1) { - auto res = parser.eval(new_cmd, *streams.io_chain); + auto res = parser.eval(new_cmd, *streams.io_chain, streams.parent_pgid); if (res.was_empty) { // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc. // where we have an argument but nothing is executed. diff --git a/src/exec.cpp b/src/exec.cpp index 34c402fae..6b473f0eb 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -845,6 +845,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, const std::share case process_type_t::builtin: { io_streams_t builtin_io_streams{stdout_read_limit}; + if (j->pgid != INVALID_PID) builtin_io_streams.parent_pgid = j->pgid; if (!exec_internal_builtin_proc(parser, p, pipe_read.get(), process_net_io_chain, builtin_io_streams)) { return false; diff --git a/src/io.h b/src/io.h index 951271c14..5e724856c 100644 --- a/src/io.h +++ b/src/io.h @@ -451,6 +451,11 @@ struct io_streams_t { // Actual IO redirections. This is only used by the source builtin. Unowned. const io_chain_t *io_chain{nullptr}; + // The pgid of the job, if any. This enables builtins which run more code like eval() to share + // pgid. + // TODO: this is awkwardly placed, consider just embedding a lineage here. + maybe_t parent_pgid{}; + // io_streams_t cannot be copied. io_streams_t(const io_streams_t &) = delete; void operator=(const io_streams_t &) = delete; diff --git a/tests/checks/pipeline-pgroup.fish b/tests/checks/pipeline-pgroup.fish index 4b7c86e8b..34ddbdafd 100644 --- a/tests/checks/pipeline-pgroup.fish +++ b/tests/checks/pipeline-pgroup.fish @@ -40,6 +40,11 @@ and echo "All pgroups agreed" or echo "Pgroups disagreed. Found $a0 $a1 $a2, and $b0 $b1 $b2" # CHECK: All pgroups agreed +# Ensure that eval retains pgroups - #6806. +# Our regex will capture the first pgroup and use a positive lookahead on the second. +$fth print_pgrp | tr \n ' ' 1>&2 | eval '$fth print_pgrp' 1>&2 +# CHECKERR: {{(\d+) (?=\1)\d+}} + # Ensure that if a background job launches another background job, that they have different pgroups. # The pipeline here will arrange for the two pgroups to be printed on the same line, like: # 123 124 From a1f1b9c2d9de7d3f89d5fa4385439bfa85d1be7a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 25 Apr 2020 19:15:08 -0700 Subject: [PATCH 3/3] builtin_eval to direct output to its iostreams Prior to this fix, builtin_eval would direct output to the io_chain of the job. The problem is with pipes: `builtin_eval` might happily attempt to write unlimited output to the write end of a pipe, but the corresponding reading process has not yet been launched. This results in deadlock. The fix is to buffer all the output from `builtin_eval`. This is not fun but the best that can be done until we have real concurrent processes. Fixes #6806 --- src/builtin_eval.cpp | 46 ++++++++++++++++++++++++++++++++++---------- src/io.cpp | 14 +++++++++++--- src/io.h | 12 ++++++++---- 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp index 8c4f9bb6e..c9495a669 100644 --- a/src/builtin_eval.cpp +++ b/src/builtin_eval.cpp @@ -17,6 +17,9 @@ /// Implementation of eval builtin. int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) { int argc = builtin_count_args(argv); + if (argc <= 1) { + return STATUS_CMD_OK; + } wcstring new_cmd; for (int i = 1; i < argc; ++i) { @@ -24,16 +27,39 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) { new_cmd += argv[i]; } - int status = STATUS_CMD_OK; - if (argc > 1) { - auto res = parser.eval(new_cmd, *streams.io_chain, streams.parent_pgid); - if (res.was_empty) { - // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc. - // where we have an argument but nothing is executed. - status = STATUS_CMD_OK; - } else { - status = res.status.status_value(); - } + // The output and errput of eval must go to our streams, not to the io_chain in our streams, + // because they may contain a pipe which is intended to be consumed by a process which is not + // yet launched (#6806). Make bufferfills to capture it. + // TODO: we are sloppy and do not honor other redirections; eval'd code won't see for example a + // redirection of fd 3. + shared_ptr stdout_fill = + io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO); + shared_ptr stderr_fill = + io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDERR_FILENO); + if (!stdout_fill || !stderr_fill) { + // This can happen if we are unable to create a pipe. + return STATUS_CMD_ERROR; } + + int status = STATUS_CMD_OK; + auto res = parser.eval(new_cmd, io_chain_t{stdout_fill, stderr_fill}, streams.parent_pgid); + if (res.was_empty) { + // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc. + // where we have an argument but nothing is executed. + status = STATUS_CMD_OK; + } else { + status = res.status.status_value(); + } + + // Finish the bufferfills - exhaust and close our pipes. + // Note it is important that we hold no other references to the bufferfills here - they need to + // deallocate to close. + std::shared_ptr output = io_bufferfill_t::finish(std::move(stdout_fill)); + std::shared_ptr errput = io_bufferfill_t::finish(std::move(stderr_fill)); + + // Copy the output from the bufferfill back to the streams. + streams.out.append_narrow_buffer(output->buffer()); + streams.err.append_narrow_buffer(errput->buffer()); + return status; } diff --git a/src/io.cpp b/src/io.cpp index 60114fad6..f62bba75f 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -166,8 +166,10 @@ void io_buffer_t::complete_background_fillthread() { fillthread_waiter_ = {}; } -shared_ptr io_bufferfill_t::create(const fd_set_t &conflicts, - size_t buffer_limit) { +shared_ptr io_bufferfill_t::create(const fd_set_t &conflicts, size_t buffer_limit, + int target) { + assert(target >= 0 && "Invalid target fd"); + // Construct our pipes. auto pipes = make_autoclose_pipes(conflicts); if (!pipes) { @@ -184,7 +186,7 @@ shared_ptr io_bufferfill_t::create(const fd_set_t &conflicts, // Our fillthread gets the read end of the pipe; out_pipe gets the write end. auto buffer = std::make_shared(buffer_limit); buffer->begin_filling(std::move(pipes->read)); - return std::make_shared(std::move(pipes->write), buffer); + return std::make_shared(target, std::move(pipes->write), buffer); } std::shared_ptr io_bufferfill_t::finish(std::shared_ptr &&filler) { @@ -338,3 +340,9 @@ shared_ptr io_chain_t::io_for_fd(int fd) const { } return nullptr; } + +void output_stream_t::append_narrow_buffer(const separated_buffer_t &buffer) { + for (const auto &rhs_elem : buffer.elements()) { + buffer_.append(str2wcstring(rhs_elem.contents), rhs_elem.separation); + } +} diff --git a/src/io.h b/src/io.h index 5e724856c..d0f3d8e16 100644 --- a/src/io.h +++ b/src/io.h @@ -251,7 +251,6 @@ class io_buffer_t; class io_chain_t; /// Represents filling an io_buffer_t. Very similar to io_pipe_t. -/// Bufferfills always target stdout. class io_bufferfill_t : public io_data_t { /// Write end. The other end is connected to an io_buffer_t. const autoclose_fd_t write_fd_; @@ -264,8 +263,8 @@ class io_bufferfill_t : public io_data_t { // The ctor is public to support make_shared() in the static create function below. // Do not invoke this directly. - io_bufferfill_t(autoclose_fd_t write_fd, std::shared_ptr buffer) - : io_data_t(io_mode_t::bufferfill, STDOUT_FILENO, write_fd.fd()), + io_bufferfill_t(int target, autoclose_fd_t write_fd, std::shared_ptr buffer) + : io_data_t(io_mode_t::bufferfill, target, write_fd.fd()), write_fd_(std::move(write_fd)), buffer_(std::move(buffer)) { assert(write_fd_.valid() && "fd is not valid"); @@ -278,9 +277,11 @@ class io_bufferfill_t : public io_data_t { /// Create an io_bufferfill_t which, when written from, fills a buffer with the contents. /// \returns nullptr on failure, e.g. too many open fds. /// + /// \param target the fd which this will be dup2'd to - typically stdout. /// \param conflicts A set of fds. The function ensures that any pipe it makes does /// not conflict with an fd redirection in this list. - static shared_ptr create(const fd_set_t &conflicts, size_t buffer_limit = 0); + static shared_ptr create(const fd_set_t &conflicts, size_t buffer_limit = 0, + int target = STDOUT_FILENO); /// Reset the receiver (possibly closing the write end of the pipe), and complete the fillthread /// of the buffer. \return the buffer. @@ -418,6 +419,9 @@ class output_stream_t { void append(const wchar_t *s, size_t amt) { buffer_.append(s, s + amt); } + // Append data from a narrow buffer, widening it. + void append_narrow_buffer(const separated_buffer_t &buffer); + void push_back(wchar_t c) { append(c); } void append_format(const wchar_t *format, ...) {