Mirrors/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell synced 2025-01-16 15:04:05 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge branch 'cmdsub_inherit_pgroup'

Browse source 
This merges three fixes around propagating pgroups, and preventing pipe
deadlock. This fixes a class of hangs with fzf in particular.
This commit is contained in:
ridiculousfish 2020-04-26 11:07:04 -07:00
commit c034c2c99b
10 changed files with 110 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

46
src/builtin_eval.cpp
View file

@ -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);
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<io_bufferfill_t> stdout_fill =
io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO);
shared_ptr<io_bufferfill_t> 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<io_buffer_t> output = io_bufferfill_t::finish(std::move(stdout_fill));
std::shared_ptr<io_buffer_t> 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;
}

21
src/exec.cpp
View file

@ -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;
@ -1096,13 +1097,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<pid_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 +1128,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<io_buffer_t> buffer = io_bufferfill_t::finish(std::move(bufferfill));
if (buffer->buffer().discarded()) {
*break_expand = true;
@ -1144,21 +1148,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<pid_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);
}

6
src/exec.h
View file

@ -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<pid_t> parent_pgid,
wcstring_list_t &outputs);
/// Loops over close until the syscall was run without being interrupted.
void exec_close(int fd);

12
src/expand.cpp
View file

@ -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);
}
}

14
src/io.cpp
View file

@ -166,8 +166,10 @@ void io_buffer_t::complete_background_fillthread() {
fillthread_waiter_ = {};
}
shared_ptr<io_bufferfill_t> io_bufferfill_t::create(const fd_set_t &conflicts,
size_t buffer_limit) {
shared_ptr<io_bufferfill_t> 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> 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<io_buffer_t>(buffer_limit);
buffer->begin_filling(std::move(pipes->read));
return std::make_shared<io_bufferfill_t>(std::move(pipes->write), buffer);
return std::make_shared<io_bufferfill_t>(target, std::move(pipes->write), buffer);
}
std::shared_ptr<io_buffer_t> io_bufferfill_t::finish(std::shared_ptr<io_bufferfill_t> &&filler) {
@ -338,3 +340,9 @@ shared_ptr<const io_data_t> io_chain_t::io_for_fd(int fd) const {
}
return nullptr;
}
void output_stream_t::append_narrow_buffer(const separated_buffer_t<std::string> &buffer) {
for (const auto &rhs_elem : buffer.elements()) {
buffer_.append(str2wcstring(rhs_elem.contents), rhs_elem.separation);
}
}

17
src/io.h
View file

@ -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<io_buffer_t> 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<io_buffer_t> 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<io_bufferfill_t> create(const fd_set_t &conflicts, size_t buffer_limit = 0);
static shared_ptr<io_bufferfill_t> 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<std::string> &buffer);
void push_back(wchar_t c) { append(c); }
void append_format(const wchar_t *format, ...) {
@ -451,6 +455,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<pid_t> parent_pgid{};
// io_streams_t cannot be copied.
io_streams_t(const io_streams_t &) = delete;
void operator=(const io_streams_t &) = delete;

5
src/operation_context.h
View file

@ -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<pid_t> parent_pgid{};
// A function which may be used to poll for cancellation.
cancel_checker_t cancel_checker;

11
src/parser.cpp
View file

@ -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<pid_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<pid_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<grammar::job_list> 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<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<parse_execution_context_t>;
scoped_push<exc_ctx_ref_t> exc(&execution_context, make_unique<parse_execution_context_t>(

5
src/parser.h
View file

@ -282,19 +282,20 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
///
/// \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<pid_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<pid_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.

11
tests/checks/pipeline-pgroup.fish
View file

@ -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.
@ -38,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