From e0f62c178f84a93b002570d1abd20dec8d27757c Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 26 Feb 2017 21:46:15 -0800 Subject: [PATCH] make not blocking signals the default This is the next step in determining whether we can disable blocking signals without a good reason to do so. This makes not blocking signals the default behavior. If someone finds a problem they can add this to their ~/config/fish/config.fish file: set FISH_NO_SIGNAL_BLOCK 0 Alternatively set that env var before starting fish. I won't be surprised if people report problems. Till now we have relied on people opting in to this behavior to tell us whether it causes problems. This makes the experimental behavior the default that has to be opted out of. This will give us a lot more confidence this change doesn't cause problems before the next minor release. Note that there are still a few places where we force blocking of signals. Primarily to keep SIGTSTP from interfering with the shell in response to manipulating the controlling tty. Bash is more selective in the signals it blocks around the problematic syscalls (c.f., its `git_terminal_to()` function). However, I don't see any value in that refinement. --- src/common.cpp | 1 - src/exec.cpp | 1 - src/fish.cpp | 4 +++- src/postfork.cpp | 2 ++ src/proc.cpp | 55 ++++++++++++++++++++++++++++-------------------- src/signal.cpp | 10 ++++----- src/signal.h | 4 ++-- 7 files changed, 44 insertions(+), 33 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index 8e4285f0e..ea103fd08 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -109,7 +109,6 @@ demangled_backtrace(int max_frames, int skip_levels) { void __attribute__((noinline)) show_stackframe(const wchar_t msg_level, int frame_count, int skip_levels) { - ASSERT_IS_NOT_FORKED_CHILD(); if (frame_count < 1) return; // TODO: Decide if this is still needed. I'm commenting it out because it caused me some grief diff --git a/src/exec.cpp b/src/exec.cpp index 8b4d57528..3b6037a36 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -166,7 +166,6 @@ static void safe_launch_process(process_t *p, const char *actual_cmd, const char char *const *argv = const_cast(cargv); execve(actual_cmd, argv, envv); - err = errno; // Something went wrong with execve, check for a ":", and run /bin/sh if encountered. This is a diff --git a/src/fish.cpp b/src/fish.cpp index 3df48c738..572fbe3e0 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -368,7 +368,9 @@ int main(int argc, char **argv) { // 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()) ignore_signal_block = true; + 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_BUILTIN_OK); diff --git a/src/postfork.cpp b/src/postfork.cpp index 8b6c0c24a..6eb0f38de 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -108,7 +108,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(true); result = tcsetpgrp(STDIN_FILENO, j->pgid); + signal_unblock(true); } if (result == -1) { if (errno == ENOTTY) redirect_tty_output(); diff --git a/src/proc.cpp b/src/proc.cpp index 2a57e5ab1..7b070ce73 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -783,6 +783,14 @@ static void read_try(job_t *j) { /// \param cont If this variable is set, we are giving back control to a job that has previously /// been stopped. In that case, we need to set the terminal attributes to those saved in the job. static bool terminal_give_to_job(job_t *j, int cont) { + int s = 0; + errno = 0; + if (j->pgid == 0) { + debug(2, "terminal_give_to_job() returning early due to no process group"); + return true; + } + + signal_block(true); int result = -1; errno = EINTR; while (result == -1 && errno == EINTR) { @@ -792,11 +800,15 @@ 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(true); return false; } if (cont) { int result = -1; + // TODO: Remove this EINTR loop since we have blocked all signals and thus cannot be + // interrupted. I'm leaving it in place because all of the logic involving controlling + // terminal management is more than a little opaque and smacks of voodoo programming. errno = EINTR; while (result == -1 && errno == EINTR) { result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes); @@ -806,21 +818,32 @@ 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(true); return false; } } + signal_unblock(true); return true; } /// Returns control of the terminal to the shell, and saves the terminal attribute state to the job, /// so that we can restore the terminal ownership to the job at a later time. -static int terminal_return_from_job(job_t *j) { +static bool terminal_return_from_job(job_t *j) { + int s = 0; + errno = 0; + if (j->pgid == 0) { + debug(2, "terminal_return_from_job() returning early due to no process group"); + return true; + } + + 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"); - return 0; + signal_unblock(true); + return false; } // Save jobs terminal modes. @@ -828,7 +851,8 @@ static int 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"); - return 0; + signal_unblock(true); + return false; } // Disabling this per @@ -841,11 +865,12 @@ static int terminal_return_from_job(job_t *j) { if (errno == EIO) redirect_tty_output(); debug(1, _(L"Could not return shell to foreground")); wperror(L"tcsetattr"); - return 0; + return false; } #endif - return 1; + signal_unblock(true); + return true; } void job_continue(job_t *j, bool cont) { @@ -854,7 +879,6 @@ void job_continue(job_t *j, bool cont) { j->set_flag(JOB_NOTIFIED, false); CHECK_BLOCK(); - debug(4, L"Continue job %d, gid %d (%ls), %ls, %ls", j->job_id, j->pgid, j->command_wcstr(), job_is_completed(j) ? L"COMPLETED" : L"UNCOMPLETED", is_interactive ? L"INTERACTIVE" : L"NON-INTERACTIVE"); @@ -864,14 +888,7 @@ void job_continue(job_t *j, bool cont) { // Put the job into the foreground. Hack: ensure that stdin is marked as blocking first // (issue #176). make_fd_blocking(STDIN_FILENO); - - signal_block(); - - bool ok = terminal_give_to_job(j, cont); - - signal_unblock(); - - if (!ok) return; + if (!terminal_give_to_job(j, cont)) return; } // Send the job a continue signal, if necessary. @@ -955,15 +972,7 @@ void job_continue(job_t *j, bool cont) { // Put the shell back in the foreground. if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) { - int ok; - - signal_block(); - - ok = terminal_return_from_job(j); - - signal_unblock(); - - if (!ok) return; + terminal_return_from_job(j); } } } diff --git a/src/signal.cpp b/src/signal.cpp index 8cf5d01a3..93709a287 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -17,7 +17,7 @@ #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 = false; +bool ignore_signal_block = true; /// Struct describing an entry for the lookup table used to convert between signal names and signal /// ids, etc. @@ -372,8 +372,8 @@ void get_signals_with_handlers(sigset_t *set) { } } -void signal_block() { - if (ignore_signal_block) return; +void signal_block(bool force) { + if (!force && ignore_signal_block) return; ASSERT_IS_MAIN_THREAD(); sigset_t chldset; @@ -387,8 +387,8 @@ void signal_block() { // debug( 0, L"signal block level increased to %d", block_count ); } -void signal_unblock() { - if (ignore_signal_block) return; +void signal_unblock(bool force) { + if (!force && ignore_signal_block) return; ASSERT_IS_MAIN_THREAD(); sigset_t chldset; diff --git a/src/signal.h b/src/signal.h index 3c4da3990..436945105 100644 --- a/src/signal.h +++ b/src/signal.h @@ -27,10 +27,10 @@ void signal_set_handlers(); void signal_handle(int sig, int do_handle); /// 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();