Subject: Re: bug#15970: [Bug-tar] Crash in gettext() after fork() on Mac OS X
From: Pádraig Brady <[email-masked]>
To: Paul Eggert <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Tue, 26 Nov 2013 12:23:15 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 11/26/2013 04:55 AM, Paul Eggert wrote:
> Pádraig Brady wrote:
> 
>> How about the attached instead that just uses
>> a somewhat degraded but simpler error() equivalent.
> 
> That looks safe, though it could be simplified:
> use inttostr instead of repeating its body,

Hah, I was thinking this must be done somewhere else :)

> and since !!p == !!errnum there's no need
> to have those duplicate conditions.

I meant to use (p || errstr), but let's assume
errstr is present, so yes we can then remove condition.

> Plus, just use STDERR_FILENO rather than
> fiddling with fileno (stderr),

done

> and async_safe_error
> should be _Noreturn and should unconditionally call
> _exit.

fair enough. I changed the name so to async_safe_die()

> And, I wouldn't bother looking at dup2's
> return value, any more than we look at close's.

Updated patch attached.

thanks!
Pádraig.

From 02ac652957d0e21b0aa285dbcb718d8d60329ec7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Tue, 26 Nov 2013 02:47:36 +0000
Subject: [PATCH] sort: avoid issues when issuing diagnostics from child
 processes

* src/sort.c: (async_safe_die): A new limited version of error(),
that outputs fixed strings and unconverted errnos to stderr.
This is safe to call in the limited context of a signal handler,
or in this particular case, between the fork() and exec() of
a multithreaded process.
(move_fd_or_die): Use the async_safe_die() rather than error().
(maybe_create_temp): Likewise.
(open_temp): Likewise.
Fixes http://bugs.gnu.org/15970
---
 src/sort.c |   46 +++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index bb4e313..573d6cb 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -373,6 +373,34 @@ static bool debug;
    number are present, temp files will be used. */
 static unsigned int nmerge = NMERGE_DEFAULT;
 
+/* Output an error to stderr using async-signal-safe routines, and _exit().
+   This can be used safely from signal handlers,
+   and between fork() and exec() of multithreaded processes.  */
+
+static void async_safe_die (int, const char *) ATTRIBUTE_NORETURN;
+static void
+async_safe_die (int errnum, const char *errstr)
+{
+  ignore_value (write (STDERR_FILENO, errstr, strlen (errstr)));
+
+  /* Even if defined HAVE_STRERROR_R, we can't use it,
+     as it may return a translated string etc. and even if not
+     may malloc() which is unsafe.  We might improve this
+     by testing for sys_errlist and using that if available.
+     For now just report the error number.  */
+  if (errnum)
+    {
+      char errbuf[INT_BUFSIZE_BOUND (errnum)];
+      char *p = inttostr (errnum, errbuf);
+      ignore_value (write (STDERR_FILENO, ": errno ", 8));
+      ignore_value (write (STDERR_FILENO, p, strlen (p)));
+    }
+
+  ignore_value (write (STDERR_FILENO, "\n", 1));
+
+  _exit (SORT_FAILURE);
+}
+
 /* Report MESSAGE for FILE, then clean up and exit.
    If FILE is null, it represents standard output.  */
 
@@ -982,8 +1010,8 @@ move_fd_or_die (int oldfd, int newfd)
 {
   if (oldfd != newfd)
     {
-      if (dup2 (oldfd, newfd) < 0)
-        error (SORT_FAILURE, errno, _("dup2 failed"));
+      /* This should never fail for our usage.  */
+      dup2 (oldfd, newfd);
       close (oldfd);
     }
 }
@@ -1095,13 +1123,15 @@ maybe_create_temp (FILE **pfp, bool survive_fd_exhaustion)
         }
       else if (node->pid == 0)
         {
+          /* Being the child of a multithreaded program before exec(),
+             we're restricted to calling async-signal-safe routines here.  */
           close (pipefds[1]);
           move_fd_or_die (tempfd, STDOUT_FILENO);
           move_fd_or_die (pipefds[0], STDIN_FILENO);
 
-          if (execlp (compress_program, compress_program, (char *) NULL) < 0)
-            error (SORT_FAILURE, errno, _("couldn't execute %s"),
-                   compress_program);
+          execlp (compress_program, compress_program, (char *) NULL);
+
+          async_safe_die (errno, "couldn't execute compress program");
         }
     }
 
@@ -1153,13 +1183,15 @@ open_temp (struct tempnode *temp)
       break;
 
     case 0:
+      /* Being the child of a multithreaded program before exec(),
+         we're restricted to calling async-signal-safe routines here.  */
       close (pipefds[0]);
       move_fd_or_die (tempfd, STDIN_FILENO);
       move_fd_or_die (pipefds[1], STDOUT_FILENO);
 
       execlp (compress_program, compress_program, "-d", (char *) NULL);
-      error (SORT_FAILURE, errno, _("couldn't execute %s -d"),
-             compress_program);
+
+      async_safe_die (errno, "couldn't execute compress program (with -d)");
 
     default:
       temp->pid = child;
-- 
1.7.7.6