From 35ee28ff24c7bca933d477029c7f3386cfbaf9a3 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 6 Aug 2017 14:38:25 -0700 Subject: [PATCH] Revert "finish cleanup of signal blocking code" This reverts commit fb08fe5f47ac08d9f623e892aee1d7ade680f61a. Needed to cleanly apply PR#4268. Will reapply after applying that change. --- src/exec.cpp | 28 +++++++++++++++++++++++++++- src/fish.cpp | 7 +++++++ src/history.cpp | 9 +++++++++ src/postfork.cpp | 6 ++++-- src/proc.cpp | 16 ++++++++-------- src/reader.cpp | 16 ++++++++++++++-- src/signal.cpp | 16 +++++++++++++--- src/signal.h | 6 ++++-- 8 files changed, 86 insertions(+), 18 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 5af251572..047df6f71 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -36,6 +36,7 @@ #include "postfork.h" #include "proc.h" #include "reader.h" +#include "signal.h" #include "wutil.h" // IWYU pragma: keep /// File descriptor redirection error message. @@ -321,12 +322,16 @@ static void internal_exec_helper(parser_t &parser, const wcstring &def, node_off return; } + signal_unblock(); + if (node_offset == NODE_OFFSET_INVALID) { parser.eval(def, morphed_chain, block_type); } else { parser.eval_block_node(node_offset, morphed_chain, block_type); } + signal_block(); + morphed_chain.clear(); io_cleanup_fds(opened_fds); job_reap(0); @@ -396,8 +401,10 @@ void exec_job(parser_t &parser, job_t *j) { if (j->processes.front()->type == INTERNAL_EXEC) { // Do a regular launch - but without forking first... + signal_block(); - // setup_child_process makes sure signals are properly set up. + // setup_child_process makes sure signals are properly set up. It will also call + // signal_unblock. // PCA This is for handling exec. Passing all_ios here matches what fish 2.0.0 and 1.x did. // It's known to be wrong - for example, it means that redirections bound for subsequent @@ -441,6 +448,8 @@ void exec_job(parser_t &parser, job_t *j) { } } + signal_block(); + // See if we need to create a group keepalive process. This is a process that we create to make // sure that the process group doesn't die accidentally, and is often needed when a // builtin/block/function is inside a pipeline, since that usually means we have to wait for one @@ -611,6 +620,9 @@ void exec_job(parser_t &parser, job_t *j) { switch (p->type) { case INTERNAL_FUNCTION: { + // Calls to function_get_definition might need to source a file as a part of + // autoloading, hence there must be no blocks. + signal_unblock(); const wcstring func_name = p->argv0(); wcstring def; bool function_exists = function_get_definition(func_name, &def); @@ -618,6 +630,8 @@ void exec_job(parser_t &parser, job_t *j) { const std::map inherit_vars = function_get_inherit_vars(func_name); + signal_block(); + if (!function_exists) { debug(0, _(L"Unknown function '%ls'"), p->argv0()); break; @@ -625,7 +639,13 @@ void exec_job(parser_t &parser, job_t *j) { function_block_t *fb = parser.push_block(p, func_name, shadow_scope); + + // Setting variables might trigger an event handler, hence we need to unblock + // signals. + signal_unblock(); function_prepare_environment(func_name, p->get_argv() + 1, inherit_vars); + signal_block(); + parser.forbid_function(func_name); if (!p->is_last_in_job) { @@ -775,8 +795,12 @@ void exec_job(parser_t &parser, job_t *j) { const int fg = j->get_flag(JOB_FOREGROUND); j->set_flag(JOB_FOREGROUND, false); + signal_unblock(); + p->status = builtin_run(parser, p->get_argv(), *builtin_io_streams); + signal_block(); + // Restore the fg flag, which is temporarily set to false during builtin // execution so as not to confuse some job-handling builtins. j->set_flag(JOB_FOREGROUND, fg); @@ -1089,7 +1113,9 @@ void exec_job(parser_t &parser, job_t *j) { kill(keepalive.pid, SIGKILL); } + signal_unblock(); debug(3, L"Job is constructed"); + j->set_flag(JOB_CONSTRUCTED, true); if (!j->get_flag(JOB_FOREGROUND)) { proc_last_bg_pid = j->pgid; diff --git a/src/fish.cpp b/src/fish.cpp index 096805a6e..273c85ab8 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -387,6 +387,13 @@ int main(int argc, char **argv) { const io_chain_t empty_ios; if (read_init(paths)) { + // TODO: Remove this once we're confident that not blocking/unblocking every signal around + // some critical sections is no longer necessary. + env_var_t fish_no_signal_block = env_get_string(L"FISH_NO_SIGNAL_BLOCK"); + if (!fish_no_signal_block.missing_or_empty() && !from_string(fish_no_signal_block)) { + ignore_signal_block = false; + } + // Stomp the exit status of any initialization commands (issue #635). proc_set_last_status(STATUS_CMD_OK); diff --git a/src/history.cpp b/src/history.cpp index a293933b9..5b748c8ce 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -39,6 +39,7 @@ #include "parse_util.h" #include "path.h" #include "reader.h" +#include "signal.h" #include "wutil.h" // IWYU pragma: keep // Our history format is intended to be valid YAML. Here it is: @@ -997,6 +998,9 @@ bool history_t::load_old_if_needed(void) { if (loaded_old) return true; loaded_old = true; + // PCA not sure why signals were blocked here + // signal_block(); + bool ok = false; if (map_file(name, &mmap_start, &mmap_length, &mmap_file_id)) { // Here we've mapped the file. @@ -1005,6 +1009,7 @@ bool history_t::load_old_if_needed(void) { this->populate_from_mmap(); } + // signal_unblock(); return ok; } @@ -1404,6 +1409,8 @@ bool history_t::save_internal_via_appending() { return true; } + signal_block(); + // We are going to open the file, lock it, append to it, and then close it // After locking it, we need to stat the file at the path; if there is a new file there, it // means @@ -1491,6 +1498,8 @@ bool history_t::save_internal_via_appending() { close(history_fd); } + signal_unblock(); + // If someone has replaced the file, forget our file state. if (file_changed) { this->clear_file_state(); diff --git a/src/postfork.cpp b/src/postfork.cpp index 8dc49b298..9f29963b0 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -109,9 +109,9 @@ bool set_child_group(job_t *j, process_t *p, int print_errors) { int result = -1; errno = EINTR; while (result == -1 && errno == EINTR) { - signal_block(); + signal_block(true); result = tcsetpgrp(STDIN_FILENO, j->pgid); - signal_unblock(); + signal_unblock(true); } if (result == -1) { if (errno == ENOTTY) redirect_tty_output(); @@ -251,6 +251,8 @@ int setup_child_process(job_t *j, process_t *p, const io_chain_t &io_chain) { signal_reset_handlers(); } + signal_unblock(); // remove all signal blocks + return ok ? 0 : -1; } diff --git a/src/proc.cpp b/src/proc.cpp index cdbea292b..063f01feb 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -789,7 +789,7 @@ static bool terminal_give_to_job(job_t *j, int cont) { return true; } - signal_block(); + signal_block(true); int result = -1; errno = EINTR; while (result == -1 && errno == EINTR) { @@ -799,7 +799,7 @@ static bool terminal_give_to_job(job_t *j, int cont) { if (errno == ENOTTY) redirect_tty_output(); debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr()); wperror(L"tcsetpgrp"); - signal_unblock(); + signal_unblock(true); return false; } @@ -817,12 +817,12 @@ static bool terminal_give_to_job(job_t *j, int cont) { debug(1, _(L"terminal_give_to_job(): Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr()); wperror(L"tcsetattr"); - signal_unblock(); + signal_unblock(true); return false; } } - signal_unblock(); + signal_unblock(true); return true; } @@ -835,12 +835,12 @@ static bool terminal_return_from_job(job_t *j) { return true; } - signal_block(); + signal_block(true); if (tcsetpgrp(STDIN_FILENO, getpgrp()) == -1) { if (errno == ENOTTY) redirect_tty_output(); debug(1, _(L"Could not return shell to foreground")); wperror(L"tcsetpgrp"); - signal_unblock(); + signal_unblock(true); return false; } @@ -849,7 +849,7 @@ static bool terminal_return_from_job(job_t *j) { if (errno == EIO) redirect_tty_output(); debug(1, _(L"Could not return shell to foreground")); wperror(L"tcgetattr"); - signal_unblock(); + signal_unblock(true); return false; } @@ -867,7 +867,7 @@ static bool terminal_return_from_job(job_t *j) { } #endif - signal_unblock(); + signal_unblock(true); return true; } diff --git a/src/reader.cpp b/src/reader.cpp index 89a0d767d..28dcbcf0e 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -1575,12 +1575,20 @@ static void reader_interactive_init() { // Check if we are in control of the terminal, so that we don't do semi-expensive things like // reset signal handlers unless we really have to, which we often don't. if (tcgetpgrp(STDIN_FILENO) != shell_pgid) { + int block_count = 0; + int i; + // Bummer, we are not in control of the terminal. Stop until parent has given us control of - // it. + // it. Stopping in fish is a bit of a challange, what with all the signal fidgeting, we need + // to reset a bunch of signal state, making this coda a but unobvious. // // In theory, reseting signal handlers could cause us to miss signal deliveries. In - // practice, this code should only be run during startup, when we're not waiting for any + // practice, this code should only be run suring startup, when we're not waiting for any // signals. + while (signal_is_blocked()) { + signal_unblock(); + block_count++; + } signal_reset_handlers(); // Ok, signal handlers are taken out of the picture. Stop ourself in a loop until we are in @@ -1624,6 +1632,10 @@ static void reader_interactive_init() { } signal_set_handlers(); + + for (i = 0; i < block_count; i++) { + signal_block(); + } } // Put ourselves in our own process group. diff --git a/src/signal.cpp b/src/signal.cpp index f36847b59..1b937825c 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -16,6 +16,9 @@ #include "reader.h" #include "wutil.h" // IWYU pragma: keep +// This is a temporary var while we explore whether signal_block() and friends is needed. +bool ignore_signal_block = true; + /// Struct describing an entry for the lookup table used to convert between signal names and signal /// ids, etc. struct lookup_entry { @@ -380,7 +383,9 @@ void get_signals_with_handlers(sigset_t *set) { } } -void signal_block() { +void signal_block(bool force) { + if (!force && ignore_signal_block) return; + ASSERT_IS_MAIN_THREAD(); sigset_t chldset; @@ -400,10 +405,14 @@ void signal_unblock_all() { sigprocmask(SIG_SETMASK, &iset, NULL); } -void signal_unblock() { +void signal_unblock(bool force) { + if (!force && ignore_signal_block) return; + ASSERT_IS_MAIN_THREAD(); + sigset_t chldset; block_count--; + if (block_count < 0) { debug(0, _(L"Signal block mismatch")); bugreport(); @@ -411,12 +420,13 @@ void signal_unblock() { } if (!block_count) { - sigset_t chldset; sigfillset(&chldset); DIE_ON_FAILURE(pthread_sigmask(SIG_UNBLOCK, &chldset, 0)); } + // debug( 0, L"signal block level decreased to %d", block_count ); } bool signal_is_blocked() { + if (ignore_signal_block) return false; return static_cast(block_count); } diff --git a/src/signal.h b/src/signal.h index dcbc3be75..06fe37cf9 100644 --- a/src/signal.h +++ b/src/signal.h @@ -30,14 +30,16 @@ void signal_handle(int sig, int do_handle); void signal_unblock_all(); /// Block all signals. -void signal_block(); +void signal_block(bool force = false); /// Unblock all signals. -void signal_unblock(); +void signal_unblock(bool force = false); /// Returns true if signals are being blocked. bool signal_is_blocked(); /// Returns signals with non-default handlers. void get_signals_with_handlers(sigset_t *set); + +extern bool ignore_signal_block; #endif