From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Josselin Poiret via "Bug reports for GUILE, GNU's Ubiquitous Extension Language" Newsgroups: gmane.lisp.guile.bugs Subject: bug#52835: [PATCH v4 4/4] Make start_child propagate the child errno to the parent. Date: Sat, 28 May 2022 14:46:34 +0200 Message-ID: <20220528124634.17353-5-dev@jpoiret.xyz> References: <20220207165543.12723-1-dev@jpoiret.xyz> <20220528124634.17353-1-dev@jpoiret.xyz> Reply-To: Josselin Poiret Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="30627"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 52835@debbugs.gnu.org To: Josselin Poiret , Timothy Sample Original-X-From: bug-guile-bounces+guile-bugs=m.gmane-mx.org@gnu.org Sat May 28 14:48:09 2022 Return-path: Envelope-to: guile-bugs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nuvrV-0007lc-4J for guile-bugs@m.gmane-mx.org; Sat, 28 May 2022 14:48:09 +0200 Original-Received: from localhost ([::1]:48006 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nuvrT-0004MT-Pt for guile-bugs@m.gmane-mx.org; Sat, 28 May 2022 08:48:07 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:35544) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nuvrO-0004L3-Gg for bug-guile@gnu.org; Sat, 28 May 2022 08:48:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:42725) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1nuvrO-0004EG-6m for bug-guile@gnu.org; Sat, 28 May 2022 08:48:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1nuvrO-00014e-5Q for bug-guile@gnu.org; Sat, 28 May 2022 08:48:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Josselin Poiret Original-Sender: "Debbugs-submit" Resent-CC: bug-guile@gnu.org Resent-Date: Sat, 28 May 2022 12:48:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 52835 X-GNU-PR-Package: guile X-GNU-PR-Keywords: patch Original-Received: via spool by 52835-submit@debbugs.gnu.org id=B52835.16537420403929 (code B ref 52835); Sat, 28 May 2022 12:48:02 +0000 Original-Received: (at 52835) by debbugs.gnu.org; 28 May 2022 12:47:20 +0000 Original-Received: from localhost ([127.0.0.1]:36621 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1nuvqh-00011D-5j for submit@debbugs.gnu.org; Sat, 28 May 2022 08:47:19 -0400 Original-Received: from jpoiret.xyz ([206.189.101.64]:34680) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1nuvqN-000109-Ca for 52835@debbugs.gnu.org; Sat, 28 May 2022 08:47:00 -0400 Original-Received: from authenticated-user (jpoiret.xyz [206.189.101.64]) by jpoiret.xyz (Postfix) with ESMTPA id 72C36184F6B; Sat, 28 May 2022 12:46:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jpoiret.xyz; s=dkim; t=1653742016; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ft8LuG4qiXeT1gYvoFLiOHBhudfMVvEZobNT06WyUR4=; b=voSIBD5nUiESr+ZaHCjvk9k08GAcS2XxA1NL4SW46DMm4/R8peeETsMSZZmaCi9SUgVzSr Q38vtEHxVUUr4eiPc3EB5H/0A+RcWK3LbUWzcCHgPGyLIK7jmFdczJpBkizanZ5JIrNezw 5ud7kPVNF7zRnOysoLgHwshBkGNmIEfZeF+LHQxyfvZJEde3aelvaOxhlVnuvQYBIZni8O tp6rrsxr/iDOp7EUolN3eRSuzcj3uU6LGQjDuJhFd/DiXUg5sg4kuZbMwa84IdZSsJmz95 zjbsgYTOMUJCZROK8R47SpbNqdCRzt82F9/lZmuBLqHi0ronCixua5nRDPj0qg== In-Reply-To: <20220528124634.17353-1-dev@jpoiret.xyz> Authentication-Results: jpoiret.xyz; auth=pass smtp.auth=jpoiret@jpoiret.xyz smtp.mailfrom=dev@jpoiret.xyz X-Spamd-Bar: / X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-guile@gnu.org List-Id: "Bug reports for GUILE, GNU's Ubiquitous Extension Language" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guile-bounces+guile-bugs=m.gmane-mx.org@gnu.org Original-Sender: "bug-guile" Xref: news.gmane.io gmane.lisp.guile.bugs:10287 Archived-At: * configure.ac: Add AC_CHECK_FUNCS for pipe2. * libguile/posix.c (start_child): Use a pipe to transmit the child's errno to the parent, which can then use it to signal an error instead of writing to its error file descriptor. This also avoids the use of async-signal unsafe functions inside the child before exec. Use pipe2 when available. (dup_handle_error,dup2_handle_error): Ditto. --- configure.ac | 3 +- libguile/posix.c | 113 ++++++++++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 27 deletions(-) diff --git a/configure.ac b/configure.ac index 827e1c09d..19441a52e 100644 --- a/configure.ac +++ b/configure.ac @@ -525,6 +525,7 @@ AC_CHECK_HEADERS([assert.h crt_externs.h]) # fork - unavailable on Windows # sched_getaffinity, sched_setaffinity - GNU extensions (glibc) # sendfile - non-POSIX, found in glibc +# pipe2 - non-POSIX, found on Linux # AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid \ fesetround ftime ftruncate fchown fchmod getcwd geteuid getsid \ @@ -536,7 +537,7 @@ AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid \ getgrent kill getppid getpgrp fork setitimer getitimer strchr strcmp \ index bcopy memcpy rindex truncate isblank _NSGetEnviron \ strcoll strcoll_l strtod_l strtol_l newlocale uselocale utimensat \ - sched_getaffinity sched_setaffinity sendfile]) + sched_getaffinity sched_setaffinity sendfile pipe2]) # The newlib C library uses _NL_ prefixed locale langinfo constants. AC_CHECK_DECLS([_NL_NUMERIC_GROUPING], [], [], [[#include ]]) diff --git a/libguile/posix.c b/libguile/posix.c index 94a043cca..79f097756 100644 --- a/libguile/posix.c +++ b/libguile/posix.c @@ -1284,8 +1284,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0, is specialized for that particular environment where it doesn't make sense to report errors via exceptions. It uses dup(2) to duplicate the file descriptor FD, does *not* close the original FD, and returns - the new descriptor. If dup(2) fails, print an error message to ERR - and abort. */ + the new descriptor. If dup(2) fails, send errno to ERR and abort. */ static int dup_handle_error (int fd, int err) { @@ -1299,9 +1298,12 @@ dup_handle_error (int fd, int err) { /* At this point we are in the child process before exec. We cannot safely raise an exception in this environment. */ - const char *msg = strerror (errno); - fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg); - _exit (127); /* Use exit status 127, as with other exec errors. */ + int errno_save = errno; + while (write (err, &errno_save, sizeof (errno)) == -1) + if (errno != EINTR) + break; + + _exit (127); } return new_fd; @@ -1311,8 +1313,8 @@ dup_handle_error (int fd, int err) is specialized for that particular environment where it doesn't make sense to report errors via exceptions. It uses dup2(2) to duplicate the file descriptor FD, does *not* close the original FD, and returns - the new descriptor. If dup2(2) fails, print an error message to ERR - and abort. */ + the new descriptor. If dup2(2) fails, send errno to ERR and + abort. */ static int dup2_handle_error (int fd, int to, int err) { @@ -1326,9 +1328,11 @@ dup2_handle_error (int fd, int to, int err) { /* At this point we are in the child process before exec. We cannot safely raise an exception in this environment. */ - const char *msg = strerror (errno); - fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg); - _exit (127); /* Use exit status 127, as with other exec errors. */ + int errno_save = errno; + while (write (err, &errno_save, sizeof (errno)) == -1) + if (errno != EINTR) + break; + _exit (127); } return new_fd; @@ -1347,6 +1351,8 @@ start_child (const char *exec_file, char **exec_argv, { int pid; int max_fd = 1024; + int errno_save; + int errpipefds[2]; #if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE) { @@ -1356,17 +1362,73 @@ start_child (const char *exec_file, char **exec_argv, } #endif +/* Setup a pipe to receive the errno of the child, which can't call + async-signal unsafe functions such as strerror or the printf family. */ +#ifdef HAVE_PIPE2 + if (pipe2 (errpipefds, O_CLOEXEC)) + return -1; +#else + if (pipe (errpipefds)) + return -1; + /* Set the writer end as close-on-exec, so that it automatically + closes on successful exec, and so that other threads that fork+exec + will not block it. + + XXX: There can potentially be a race issue between the pipe and + fcntl calls, such that another thread that forks in between + inherits the fd without CLOEXEC. There is no POSIXy way to make + the combination atomic, so just hope that any other fork would + release resources it doesn't need, like we do. */ + if (fcntl (errpipefds[1], F_SETFD, fcntl (errpipefds[1], F_GETFD) | FD_CLOEXEC)) + { + errno_save = errno; + close (errpipefds[0]); + close (errpipefds[1]); + errno = errno_save; + return -1; + } +#endif + pid = fork (); if (pid != 0) - /* The parent, with either and error (pid == -1), or the PID of the - child. Return directly in either case. */ - return pid; + { + /* We're in the parent process. */ + int read_count; + close (errpipefds[1]); + if (pid == -1) + { + /* Fork failed. */ + errno_save = errno; + close (errpipefds[0]); + errno = errno_save; + return -1; + } + + /* Fork successful, try to read a potential child errno from the pipe. */ + while ((read_count = read(errpipefds[0], &errno_save, sizeof (errno))) == -1) + if (errno != EAGAIN && errno != EINTR) + break; + if (read_count == -1) + errno_save = errno; + close (errpipefds[0]); + if (read_count != 0) + { + /* Either the read failed (-1) or the child sent us an errno (> 0) */ + errno = errno_save; + return -1; + } + return pid; + } + + /* From now on, we are single threaded because of fork, so double + closes should be a no-op. */ /* Close all file descriptors in ports inherited from the parent - except for in, out, and err. Heavy-handed, but robust. */ + except for in, out, err and the error pipe back to the parent. + Heavy-handed, but robust. */ while (max_fd--) - if (max_fd != in && max_fd != out && max_fd != err) + if (max_fd != in && max_fd != out && max_fd != err && max_fd != errpipefds[1]) close (max_fd); /* Ignore errors on these open() calls. */ @@ -1382,16 +1444,16 @@ start_child (const char *exec_file, char **exec_argv, in/out/err. Note that dup2 doesn't do anything if its arguments are equal. */ if (out == 0) - out = dup_handle_error (out, err); + out = dup_handle_error (out, errpipefds[1]); if (err == 0) - err = dup_handle_error (err, err); - dup2_handle_error (in, 0, err); + err = dup_handle_error (err, errpipefds[1]); + dup2_handle_error (in, 0, errpipefds[1]); if (err == 1) - err = dup_handle_error (err, err); - dup2_handle_error (out, 1, err); + err = dup_handle_error (err, errpipefds[1]); + dup2_handle_error (out, 1, errpipefds[1]); - dup2_handle_error (err, 2, err); + dup2_handle_error (err, 2, errpipefds[1]); if (in > 2) close (in); if (out > 2) close (out); @@ -1400,11 +1462,10 @@ start_child (const char *exec_file, char **exec_argv, execvp (exec_file, exec_argv); /* The exec failed! There is nothing sensible to do. */ - { - const char *msg = strerror (errno); - dprintf (2, "In execvp of %s: %s\n", - exec_file, msg); - } + errno_save = errno; + while (write (errpipefds[1], &errno_save, sizeof (errno)) == -1) + if (errno != EINTR) + break; /* Use exit status 127, like shells in this case, as per POSIX . */ -- 2.36.0