From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Ludovic =?UTF-8?Q?Court=C3=A8s?= Newsgroups: gmane.lisp.guile.bugs Subject: bug#61095: possible misuse of posix_spawn API on non-linux OSes Date: Tue, 28 Mar 2023 11:34:16 +0200 Message-ID: <87zg7xgqxz.fsf@gnu.org> References: <26OIN3L5D4V9L.2M0KM95K0YSNM@venera> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="15239"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) Cc: Josselin Poiret , Andrew Whatson , 61095@debbugs.gnu.org To: Omar Polo Original-X-From: bug-guile-bounces+guile-bugs=m.gmane-mx.org@gnu.org Tue Mar 28 11:35:19 2023 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 1ph5jb-0003f4-1q for guile-bugs@m.gmane-mx.org; Tue, 28 Mar 2023 11:35:19 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ph5jM-0002BB-9P; Tue, 28 Mar 2023 05:35:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ph5jL-0002Ax-9U for bug-guile@gnu.org; Tue, 28 Mar 2023 05:35:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1ph5jK-0000kj-UO for bug-guile@gnu.org; Tue, 28 Mar 2023 05:35:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ph5jK-0000KD-BF for bug-guile@gnu.org; Tue, 28 Mar 2023 05:35:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Ludovic =?UTF-8?Q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: bug-guile@gnu.org Resent-Date: Tue, 28 Mar 2023 09:35:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 61095 X-GNU-PR-Package: guile Original-Received: via spool by 61095-submit@debbugs.gnu.org id=B61095.16799960751209 (code B ref 61095); Tue, 28 Mar 2023 09:35:02 +0000 Original-Received: (at 61095) by debbugs.gnu.org; 28 Mar 2023 09:34:35 +0000 Original-Received: from localhost ([127.0.0.1]:48967 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ph5it-0000JP-5Z for submit@debbugs.gnu.org; Tue, 28 Mar 2023 05:34:35 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:41302) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ph5ir-0000JA-36 for 61095@debbugs.gnu.org; Tue, 28 Mar 2023 05:34:33 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ph5ie-0000f5-M1; Tue, 28 Mar 2023 05:34:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-Version:In-Reply-To:Date:References:Subject:To: From; bh=9WENLG4QvH8w/qU7PbrQPfjmc/cMmF76c1pHnomQgYc=; b=NT+FGUmw4hWLq/P/mvUY TVTpSu+GawufMYZvqWkGUaelhXWoC81WA6MHpRHFrZF5s0wimLsJfUS8/4M4ouPkYg3+gvCUTYK/p CwQ109BGpfiEwGl4r2xErNpmgBlZvZa2nKYsESQxVJxoNc9QnDM4q/ARH2N+ASaryCW2vAZmIrD0I JKYHNhmmBhilCaibSqSul14LAZxiYu5Qr2igXz5Zuzc8ol/JHBTiNj14bghZ02KkSu9GpXA5lX3dn Viqt3PFofSsjIB6Z/x02NcXBSDO0OOwlKJfOMViR8Pw+AW6LvQltMZ5/ih1vp2KHR+tXmOYcGH+bH 9Fv2JyhHzOzX1w==; Original-Received: from [2001:660:6102:320:e120:2c8f:8909:cdfe] (helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ph5id-0006f3-3p; Tue, 28 Mar 2023 05:34:19 -0400 In-Reply-To: <26OIN3L5D4V9L.2M0KM95K0YSNM@venera> (Omar Polo's message of "Fri, 27 Jan 2023 12:51:32 +0100") 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-bounces+guile-bugs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.lisp.guile.bugs:10580 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Omar, Apologies for the late reply. Omar Polo skribis: > I've noticed that test-system-cmds fails on OpenBSD-CURRENT while > testing the update to guile 3.0.9: > > test-system-cmds: system* exit status was 127 rather than 42 > FAIL: test-system-cmds We=E2=80=99re seeing the same failure on GNU/Hurd: https://issues.guix.gnu.org/61079 > Actually I can avoid the EBADF by checking that the fd is 'live' with > something like fstat: > > [[[ > > Index: libguile/posix.c > --- libguile/posix.c.orig > +++ libguile/posix.c > @@ -1325,8 +1325,12 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0, > static void > close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_f= d) > { > - while (--max_fd > 2) > - posix_spawn_file_actions_addclose (actions, max_fd); > + struct stat sb; > + max_fd =3D getdtablecount(); > + while (--max_fd > 2) { > + if (fstat(max_fd, &sb) !=3D -1) > + posix_spawn_file_actions_addclose (actions, max_fd); > + } > } I came up with the following patch: --=-=-= Content-Type: text/x-patch Content-Disposition: inline diff --git a/libguile/posix.c b/libguile/posix.c index 3a8be94e4..cde199888 100644 --- a/libguile/posix.c +++ b/libguile/posix.c @@ -1322,39 +1322,18 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0, #undef FUNC_NAME #endif /* HAVE_FORK */ -static void -close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd) -{ - while (--max_fd > 2) - posix_spawn_file_actions_addclose (actions, max_fd); -} - static void close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd) { - DIR *dirp; - struct dirent *d; - int fd; - - /* Try to use the platform-specific list of open file descriptors, so - we don't need to use the brute force approach. */ - dirp = opendir ("/proc/self/fd"); - - if (dirp == NULL) - return close_inherited_fds_slow (actions, max_fd); - - while ((d = readdir (dirp)) != NULL) + while (--max_fd > 2) { - fd = atoi (d->d_name); - - /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */ - if (fd <= 2) - continue; - - posix_spawn_file_actions_addclose (actions, fd); + /* Adding invalid file descriptors to an 'addclose' action leads + to 'posix_spawn' failures on some operating systems: + . Hence the extra check. */ + int flags = fcntl (max_fd, F_GETFD, NULL); + if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0)) + posix_spawn_file_actions_addclose (actions, max_fd); } - - closedir (dirp); } static pid_t @@ -1366,6 +1345,26 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env, posix_spawn_file_actions_t actions; posix_spawnattr_t *attrp = NULL; + posix_spawn_file_actions_init (&actions); + + /* Duplicate IN, OUT, and ERR unconditionally to clear their + FD_CLOEXEC flag, if any. */ + posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILENO); + posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILENO); + posix_spawn_file_actions_adddup2 (&actions, err, STDERR_FILENO); + + /* TODO: Use 'closefrom' where available. */ +#if 0 + /* Version 2.34 of the GNU libc provides this function. */ + posix_spawn_file_actions_addclosefrom_np (&actions, 3); +#else + if (in > 2) + posix_spawn_file_actions_addclose (&actions, in); + if (out > 2 && out != in) + posix_spawn_file_actions_addclose (&actions, out); + if (err > 2 && err != out && err != in) + posix_spawn_file_actions_addclose (&actions, err); + int max_fd = 1024; #if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE) @@ -1376,31 +1375,8 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env, } #endif - posix_spawn_file_actions_init (&actions); - - int free_fd_slots = 0; - int fd_slot[3]; - - for (int fdnum = 3; free_fd_slots < 3 && fdnum < max_fd; fdnum++) - { - if (fdnum != in && fdnum != out && fdnum != err) - { - fd_slot[free_fd_slots] = fdnum; - free_fd_slots++; - } - } - - /* Move the fds out of the way, so that duplicate fds or fds equal - to 0, 1, 2 don't trample each other */ - - posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]); - posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]); - posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]); - posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0); - posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1); - posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2); - close_inherited_fds (&actions, max_fd); +#endif int res = -1; if (spawnp) --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Could you confirm that it works on OpenBSD and that there=E2=80=99s no performance regression? Andrew: it removes the /proc/self/fd loop you added to fix , but it reduces the number of =E2=80=98close= =E2=80=99 calls in the child. Could you check whether that=E2=80=99s okay performance-wise? Eventually I plan to use =E2=80=98posix_spawn_file_actions_addclosefrom_np= =E2=80=99 on glibc >=3D 2.34, but I have yet to test it. That will be the best solution. Josselin: I simplified the =E2=80=98dup2=E2=80=99 logic somewhat. Feedback welcome! > The regress passes and while this workaround may be temporarly > acceptable I -personally- don't like it much. There's a reason guile > can't set CLOEXEC for all the file descriptors > 2 obtained via open, > socket, pipe, ... like perl -for example- does? Guile does that for file descriptors it opens internally, but applications using =E2=80=98open-file=E2=80=99 without the recently-added = =E2=80=9Ce=E2=80=9D flag, or =E2=80=98socket=E2=80=99 without =E2=80=98SOCK_CLOEXEC=E2=80=99, etc., end = up with more file descriptors that need to be taken care of. I wish the default were close-on-exec, but we=E2=80=99re not there yet. Thanks, Ludo=E2=80=99. --=-=-=--