Make do_builtin_io multi-fork safe, moved it to postfork.cpp

Addresses fish-shell#495

Author: ridiculousfish

common.cpp

@@ -105,6 +105,8 @@ static char *wcs2str_internal(const wchar_t *in, char *out);
 
 void show_stackframe()
 {
+    ASSERT_IS_NOT_FORKED_CHILD();
+    
     /* Hack to avoid showing backtraces in the tester */
     if (program_name && ! wcscmp(program_name, L"(ignore)"))
         return;

exec.cpp

@@ -497,32 +497,6 @@ static void internal_exec_helper(parser_t &parser,
     is_block=is_block_old;
 }
 
-/** Perform output from builtins. Called from a forked child, so don't do anything that may allocate memory, etc.. */
-static void do_builtin_io(const char *out, size_t outlen, const char *err, size_t errlen)
-{
-    if (out && outlen)
-    {
-
-        if (write_loop(STDOUT_FILENO, out, outlen) == -1)
-        {
-            debug(0, L"Error while writing to stdout");
-            wperror(L"write_loop");
-            show_stackframe();
-        }
-    }
-
-    if (err && errlen)
-    {
-        if (write_loop(STDERR_FILENO, err, errlen) == -1)
-        {
-            /*
-              Can't really show any error message here, since stderr is
-              dead.
-            */
-        }
-    }
-}
-
 /* Returns whether we can use posix spawn for a given process in a given job.
  Per https://github.com/fish-shell/fish-shell/issues/364 , error handling for file redirections is too difficult with posix_spawn
  So in that case we use fork/exec
@@ -1177,7 +1151,11 @@ void exec(parser_t &parser, job_t *j)
                     const wcstring &out = get_stdout_buffer(), &err = get_stderr_buffer();
                     const std::string outbuff = wcs2string(out);
                     const std::string errbuff = wcs2string(err);
-                    do_builtin_io(outbuff.data(), outbuff.size(), errbuff.data(), errbuff.size());
+                    bool builtin_io_done = do_builtin_io(outbuff.data(), outbuff.size(), errbuff.data(), errbuff.size());
+                    if (! builtin_io_done)
+                    {
+                        show_stackframe();
+                    }
                     skip_fork = true;
                 }
 
@@ -1239,7 +1217,6 @@ void exec(parser_t &parser, job_t *j)
                     setup_child_process(j, p);
                     do_builtin_io(outbuff, outbuff_len, errbuff, errbuff_len);
                     exit_without_destructors(p->status);
-
                 }
                 else
                 {

postfork.cpp

@@ -599,3 +599,28 @@ void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char *
         }
     }
 }
+
+/** Perform output from builtins. May be called from a forked child, so don't do anything that may allocate memory, etc.. */
+bool do_builtin_io(const char *out, size_t outlen, const char *err, size_t errlen)
+{
+    bool success = true;
+    if (out && outlen)
+    {
+
+        if (write_loop(STDOUT_FILENO, out, outlen) < 0)
+        {
+            debug(0, L"Error while writing to stdout");
+            safe_perror("write_loop");
+            success = false;
+        }
+    }
+
+    if (err && errlen)
+    {
+        if (write_loop(STDERR_FILENO, err, errlen) < 0)
+        {
+            success = false;
+        }
+    }
+    return success;
+}

postfork.h

@@ -65,6 +65,9 @@ int setup_child_process(job_t *j, process_t *p);
 */
 pid_t execute_fork(bool wait_for_threads_to_die);
 
+/* Perform output from builtins. Returns true on success. */
+bool do_builtin_io(const char *out, size_t outlen, const char *err, size_t errlen);
+
 /** Report an error from failing to exec or posix_spawn a command */
 void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char **envv);
 

wutil.cpp

@@ -455,15 +455,6 @@ const wchar_t *wgettext(const wchar_t *in)
     return val->c_str();
 }
 
-wcstring wgettext2(const wcstring &in)
-{
-    wgettext_init_if_necessary();
-    std::string mbs_in = wcs2string(in);
-    char *out = gettext(mbs_in.c_str());
-    wcstring result = format_string(L"%s", out);
-    return result;
-}
-
 const wchar_t *wgetenv(const wcstring &name)
 {
     ASSERT_IS_MAIN_THREAD();

wutil.h

@@ -135,7 +135,6 @@ std::wstring wbasename(const std::wstring &path);
    around gettext, like all other functions in this file.
 */
 const wchar_t *wgettext(const wchar_t *in);
-wcstring wgettext2(const wcstring &in);
 
 /**
    Wide character version of getenv