On 11/25/2013 12:10 AM, Paul Eggert wrote: > Eric Blake wrote: >> I bet coreutils' sort has a similar bug > > [Adding bug-coreutils to the CC:.]
Referencing the original coreutils thread from the new bug: http://lists.gnu.org/archive/html/coreutils/2013-11/msg00083.html > Coreutils 'sort' runs into this problem only > if dup2 fails when called from move_fd_or_die > or if execlp fails. Right. > 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. For both cases we could _exit(errno), which the parent would get with waitpid(). Interestingly, currently the parent only notices an error after stdio eventually issues a write(), at which point the parent receives SIGPIPE and exits with status 141 (128 + SIGPIPE). If instead the parent caught sigchld it could waitpid() to get the status more directly. Though there are various gotchas with sigchld handling, and we'd have to handle the reap() error() calls outside of the signal handler. This would also be the case if we wanted to reap on SIGPIPE. So at first glance this seems to be a bit overcomplicated for what it's solving. How about the attached instead that just uses a somewhat degraded but simpler error() equivalent. thanks, Pádraig.
>From ecb2c10001a07f4ef0973353c6d963a068a56eee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com> 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_error): 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_error() rather than error(). (maybe_create_temp): Likewise. (open_temp): Likewise. Fixes http://bugs.gnu.org/15970 --- src/sort.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/sort.c b/src/sort.c index bb4e313..9fbb620 100644 --- a/src/sort.c +++ b/src/sort.c @@ -373,6 +373,62 @@ static bool debug; number are present, temp files will be used. */ static unsigned int nmerge = NMERGE_DEFAULT; +/* Used by async_safe_error(), initialized by async_safe_init(). */ +static int async_safe_fileno = 2; + +/* Initialize values unsafe to determine directly within async_safe_error(). */ + +static void +async_safe_init (void) +{ + async_safe_fileno = fileno (stderr); +} + +/* Output an error to stderr using async-signal-safe routines, + and _exit() if STATUS is non zero. + This can be used safely from signal handlers, + and between fork() and exec() of multithreaded processes. + async_safe_init() should be called prior to the first call, + to ensure the correct file descriptor for stderr is used. */ + +static void +async_safe_error (int status, int errnum, const char *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. */ + char errbuf[INT_BUFSIZE_BOUND (errnum)]; + + char *p = NULL; + + if (errnum) + { + p = errbuf + sizeof errbuf - 1; + + *--p = '\0'; + + do + *--p = '0' + (errnum % 10); + while ((errnum /= 10) != 0); + } + + ignore_value (write (async_safe_fileno, errstr, strlen (errstr))); + + if (p) + { + ignore_value (write (async_safe_fileno, ": errno ", 8)); + ignore_value (write (async_safe_fileno, p, strlen (p))); + } + + if (p || errnum) + ignore_value (write (async_safe_fileno, "\n", 1)); + + if (status) + _exit (status); +} + /* Report MESSAGE for FILE, then clean up and exit. If FILE is null, it represents standard output. */ @@ -982,8 +1038,9 @@ move_fd_or_die (int oldfd, int newfd) { if (oldfd != newfd) { + /* This should never fail for our usage. */ if (dup2 (oldfd, newfd) < 0) - error (SORT_FAILURE, errno, _("dup2 failed")); + async_safe_error (SORT_FAILURE, errno, "dup2 failed"); close (oldfd); } } @@ -1095,13 +1152,16 @@ 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_error (SORT_FAILURE, errno, + "couldn't execute compress program"); } } @@ -1153,13 +1213,16 @@ 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_error (SORT_FAILURE, errno, + "couldn't execute compress program (with -d option)"); default: temp->pid = child; @@ -4179,6 +4242,8 @@ main (int argc, char **argv) thousands_sep = -1; } + async_safe_init (); + have_read_stdin = false; inittables (); -- 1.7.7.6
