workaround glibc bug that can corrupt malloc arena

If an interactive shell has its tty invalidated attempts to write to
stdout or stderr can trigger this bug:

https://sourceware.org/bugzilla/show_bug.cgi?id=20632

Avoid that by reopening the stdio streams on /dev/null if we're getting
an ENOTTY error when trying to do things like give or take ownership of
the tty.

This includes some unrelated style cleanups but including them seems
reasonable.

Fixes #3644

src/common.cpp

@@ -5,6 +5,7 @@
 #include <cxxabi.h>
 #include <dlfcn.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <limits.h>
 #include <signal.h>
 #include <stdarg.h>
@@ -1935,12 +1936,12 @@ long convert_digit(wchar_t d, int base) {
     return res;
 }
 
-// Test if the specified character is in a range that fish uses interally to store special tokens.
+/// Test if the specified character is in a range that fish uses interally to store special tokens.
-//
+///
-// NOTE: This is used when tokenizing the input. It is also used when reading input, before
+/// NOTE: This is used when tokenizing the input. It is also used when reading input, before
-// tokenization, to replace such chars with REPLACEMENT_WCHAR if they're not part of a quoted
+/// tokenization, to replace such chars with REPLACEMENT_WCHAR if they're not part of a quoted
-// string. We don't want external input to be able to feed reserved characters into our lexer/parser
+/// string. We don't want external input to be able to feed reserved characters into our
-// or code evaluator.
+/// lexer/parser or code evaluator.
 //
 // TODO: Actually implement the replacement as documented above.
 bool fish_reserved_codepoint(wchar_t c) {
@@ -1948,3 +1949,14 @@ bool fish_reserved_codepoint(wchar_t c) {
            (c >= ENCODE_DIRECT_BASE && c < ENCODE_DIRECT_END) ||
            (c >= INPUT_COMMON_BASE && c < INPUT_COMMON_END);
 }
+
+/// Reopen stdout and/or stderr on /dev/null. This is invoked when we find that our tty has become
+/// invalid.
+void redirect_tty_output() {
+    struct termios t;
+    int fd = open("/dev/null", O_WRONLY);
+    if (tcgetattr(STDIN_FILENO, &t) == -1) dup2(fd, STDIN_FILENO);
+    if (tcgetattr(STDOUT_FILENO, &t) == -1) dup2(fd, STDOUT_FILENO);
+    if (tcgetattr(STDERR_FILENO, &t) == -1) dup2(fd, STDERR_FILENO);
+    close(fd);
+}

src/common.h

@@ -843,3 +843,5 @@ using std::wcsncasecmp;
 #endif
 
 #endif
+
+void redirect_tty_output(void);

src/parse_execution.cpp

@@ -1288,7 +1288,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(const parse_node_t
     j->tmodes = tmodes;
     job_set_flag(j, JOB_CONTROL,
                  (job_control_mode == JOB_CONTROL_ALL) ||
-                     ((job_control_mode == JOB_CONTROL_INTERACTIVE) && (shell_is_interactive())));
+                     ((job_control_mode == JOB_CONTROL_INTERACTIVE) && shell_is_interactive()));
 
     job_set_flag(j, JOB_FOREGROUND, !tree.job_should_be_backgrounded(job_node));
 

src/postfork.cpp

@@ -58,8 +58,15 @@ static void debug_safe_int(int level, const char *format, int val) {
     debug_safe(level, format, buff);
 }
 
-int set_child_group(job_t *j, process_t *p, int print_errors) {
+/// This function should be called by both the parent process and the child right after fork() has
-    int res = 0;
+/// been called. If job control is enabled, the child is put in the jobs group, and if the child is
+/// also in the foreground, it is also given control of the terminal. When called in the parent
+/// process, this function may fail, since the child might have already finished and called exit.
+/// The parent process may safely ignore the exit status of this call.
+///
+/// Returns true on sucess, false on failiure.
+bool set_child_group(job_t *j, process_t *p, int print_errors) {
+    bool retval = true;
 
     if (job_get_flag(j, JOB_CONTROL)) {
         if (!j->pgid) {
@@ -89,27 +96,31 @@ int set_child_group(job_t *j, process_t *p, int print_errors) {
                     pid_buff, argv0, job_id_buff, command, getpgid_buff, job_pgid_buff);
 
                 safe_perror("setpgid");
-                res = -1;
+                retval = false;
             }
         }
     } else {
         j->pgid = getpid();
     }
 
-    if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND)) {
+    if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND)) {  //!OCLINT(early exit)
-        int result = tcsetpgrp(0, j->pgid);
+        int result = tcsetpgrp(STDIN_FILENO, j->pgid);  // to avoid "collapsible if statements" warn
-        if (result == -1 && print_errors) {
+        if (result == -1) {
-            char job_id_buff[64];
+            if (errno == ENOTTY) redirect_tty_output();
-            char command_buff[64];
+            if (print_errors) {
-            format_long_safe(job_id_buff, j->job_id);
+                char job_id_buff[64];
-            narrow_string_safe(command_buff, j->command_wcstr());
+                char command_buff[64];
-            debug_safe(1, "Could not send job %s ('%s') to foreground", job_id_buff, command_buff);
+                format_long_safe(job_id_buff, j->job_id);
-            safe_perror("tcsetpgrp");
+                narrow_string_safe(command_buff, j->command_wcstr());
-            res = -1;
+                debug_safe(1, "Could not send job %s ('%s') to foreground", job_id_buff,
+                           command_buff);
+                safe_perror("tcsetpgrp");
+                retval = false;
+            }
         }
     }
 
-    return res;
+    return retval;
 }
 
 /// Set up a childs io redirections. Should only be called by setup_child_process(). Does the
@@ -217,7 +228,7 @@ int setup_child_process(job_t *j, process_t *p, const io_chain_t &io_chain) {
     bool ok = true;
 
     if (p) {
-        ok = (0 == set_child_group(j, p, 1));
+        ok = set_child_group(j, p, 1);
     }
 
     if (ok) {

src/postfork.h

@@ -17,14 +17,7 @@ class io_chain_t;
 class job_t;
 class process_t;
 
-/// This function should be called by both the parent process and the child right after fork() has
+bool set_child_group(job_t *j, process_t *p, int print_errors);
-/// been called. If job control is enabled, the child is put in the jobs group, and if the child is
-/// also in the foreground, it is also given control of the terminal. When called in the parent
-/// process, this function may fail, since the child might have already finished and called exit.
-/// The parent process may safely ignore the exit status of this call.
-///
-/// Returns 0 on sucess, -1 on failiure.
-int set_child_group(job_t *j, process_t *p, int print_errors);
 
 /// Initialize a new child process. This should be called right away after forking in the child
 /// process. If job control is enabled for this job, the process is put in the process group of the

src/proc.cpp

@@ -792,13 +792,15 @@ 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) {
-    if (tcsetpgrp(0, j->pgid)) {
+    if (tcsetpgrp(STDIN_FILENO, j->pgid) == -1) {
+        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");
         return false;
     }
 
-    if (cont && tcsetattr(0, TCSADRAIN, &j->tmodes)) {
+    if (cont && tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes)) {
+        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"tcsetattr");
         return false;
@@ -809,16 +811,16 @@ static bool terminal_give_to_job(job_t *j, int cont) {
 /// 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) {
-    if (tcsetpgrp(0, getpgrp())) {
+    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;
     }
 
-    /*
+    // Save jobs terminal modes.
-       Save jobs terminal modes.
+    if (tcgetattr(STDIN_FILENO, &j->tmodes)) {
-    */
+        if (errno == ENOTTY) redirect_tty_output();
-    if (tcgetattr(0, &j->tmodes)) {
         debug(1, _(L"Could not return shell to foreground"));
         wperror(L"tcgetattr");
         return 0;
@@ -830,7 +832,7 @@ static int terminal_return_from_job(job_t *j) {
 // https://github.com/fish-shell/fish-shell/issues/121
 #if 0
     // Restore the shell's terminal modes.
-    if (tcsetattr(0, TCSADRAIN, &shell_modes)) {
+    if (tcsetattr(STDIN_FILENO, TCSADRAIN, &shell_modes)) {
         debug(1, _(L"Could not return shell to foreground"));
         wperror(L"tcsetattr");
         return 0;

src/reader.cpp

@@ -312,7 +312,8 @@ static void term_donate() {
     set_color(rgb_color_t::normal(), rgb_color_t::normal());
 
     while (1) {
-        if (tcsetattr(0, TCSANOW, &tty_modes_for_external_cmds)) {
+        if (tcsetattr(STDIN_FILENO, TCSANOW, &tty_modes_for_external_cmds) == -1) {
+            if (errno == ENOTTY) redirect_tty_output();
             if (errno != EINTR) {
                 debug(1, _(L"Could not set terminal mode for new job"));
                 wperror(L"tcsetattr");
@@ -340,10 +341,11 @@ static void update_buff_pos(editable_line_t *el, size_t buff_pos) {
 /// Grab control of terminal.
 static void term_steal() {
     while (1) {
-        if (tcsetattr(0, TCSANOW, &shell_modes)) {
+        if (tcsetattr(STDIN_FILENO, TCSANOW, &shell_modes) == -1) {
+            if (errno == ENOTTY) redirect_tty_output();
             if (errno != EINTR) {
                 debug(1, _(L"Could not set terminal mode for shell"));
-                wperror(L"tcsetattr");
+                perror("tcsetattr");
                 break;
             }
         } else
@@ -808,7 +810,9 @@ void restore_term_mode() {
     // Restore the term mode if we own the terminal. It's important we do this before
     // restore_foreground_process_group, otherwise we won't think we own the terminal.
     if (getpid() == tcgetpgrp(STDIN_FILENO)) {
-        tcsetattr(STDIN_FILENO, TCSANOW, &terminal_mode_on_startup);
+        if (tcsetattr(STDIN_FILENO, TCSANOW, &terminal_mode_on_startup) == -1 && errno == ENOTTY) {
+            redirect_tty_output();
+        }
     }
 }
 
@@ -1638,22 +1642,24 @@ static void reader_interactive_init() {
     // Put ourselves in our own process group.
     shell_pgid = getpid();
     if (getpgrp() != shell_pgid && setpgid(shell_pgid, shell_pgid) < 0) {
-        debug(1, _(L"Couldn't put the shell in its own process group"));
+        debug(0, _(L"Couldn't put the shell in its own process group"));
         wperror(L"setpgid");
         exit_without_destructors(1);
     }
 
     // Grab control of the terminal.
-    if (tcsetpgrp(STDIN_FILENO, shell_pgid)) {
+    if (tcsetpgrp(STDIN_FILENO, shell_pgid) == -1) {
-        debug(1, _(L"Couldn't grab control of terminal"));
+        if (errno == ENOTTY) redirect_tty_output();
+        debug(0, _(L"Couldn't grab control of terminal"));
         wperror(L"tcsetpgrp");
         exit_without_destructors(1);
     }
 
     common_handle_winch(0);
 
-    if (tcsetattr(0, TCSANOW, &shell_modes))  // set the new modes
+    // Set the new modes.
-    {
+    if (tcsetattr(0, TCSANOW, &shell_modes) == -1) {
+        if (errno == ENOTTY) redirect_tty_output();
         wperror(L"tcsetattr");
     }
 
@@ -2401,9 +2407,10 @@ const wchar_t *reader_readline(int nchars) {
     reader_repaint();
 
     // Get the current terminal modes. These will be restored when the function returns.
-    tcgetattr(0, &old_modes);
+    tcgetattr(STDIN_FILENO, &old_modes);
     // Set the new modes.
-    if (tcsetattr(0, TCSANOW, &shell_modes)) {
+    if (tcsetattr(0, TCSANOW, &shell_modes) == -1) {
+        if (errno == ENOTTY) redirect_tty_output();
         wperror(L"tcsetattr");
     }
 
@@ -3269,7 +3276,8 @@ const wchar_t *reader_readline(int nchars) {
     }
 
     if (!reader_exit_forced()) {
-        if (tcsetattr(0, TCSANOW, &old_modes)) {
+        if (tcsetattr(0, TCSANOW, &old_modes) == -1) {
+            if (errno == ENOTTY) redirect_tty_output();
             wperror(L"tcsetattr");  // return to previous mode
         }
         set_color(rgb_color_t::reset(), rgb_color_t::reset());