Subject: bug#15970: closed (Re: bug#15970: [Bug-tar] Crash in gettext() after fork() on Mac OS X)

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 26 Nov 2013 12:24:05 +0000

Your bug report

#15970: [Bug-tar] Crash in gettext() after fork() on Mac OS X

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
15970: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15970
GNU Bug Tracking System
Contact [email-masked] with problems

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

Subject: Re: [Bug-tar] Crash in gettext() after fork() on Mac OS X
From: Paul Eggert <[email-masked]>
To: [email-masked], [email-masked]
Date: Sun, 24 Nov 2013 16:10:16 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

Eric Blake wrote:
> I bet coreutils' sort has a similar bug

[Adding bug-coreutils to the CC:.]

Coreutils 'sort' runs into this problem only
if dup2 fails when called from move_fd_or_die
or if execlp fails.

dup2 should never fail when it's called
the way that 'sort' calls it.  So perhaps
'sort' should be simplified to call dup2
and discard its return value.

And when execlp fails, 'sort' could _exit
with a special value, so that the parent
could report the failure.