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#61095: possible misuse of posix_spawn API on non-linux OSes Date: Tue, 28 Mar 2023 18:10:27 +0200 Message-ID: <87tty4svpo.fsf@jpoiret.xyz> References: <26OIN3L5D4V9L.2M0KM95K0YSNM@venera> <87zg7xgqxz.fsf@gnu.org> Reply-To: Josselin Poiret Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23552"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Andrew Whatson , 61095@debbugs.gnu.org To: Ludovic =?UTF-8?Q?Court=C3=A8s?= , Omar Polo Original-X-From: bug-guile-bounces+guile-bugs=m.gmane-mx.org@gnu.org Tue Mar 28 18:11:27 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 1phBuw-0005sn-1q for guile-bugs@m.gmane-mx.org; Tue, 28 Mar 2023 18:11:26 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1phBua-0007t1-7f; Tue, 28 Mar 2023 12:11: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 1phBuZ-0007ss-8J for bug-guile@gnu.org; Tue, 28 Mar 2023 12:11: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 1phBuY-0006nN-WE for bug-guile@gnu.org; Tue, 28 Mar 2023 12:11:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1phBuY-0005Xz-E0 for bug-guile@gnu.org; Tue, 28 Mar 2023 12:11:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Josselin Poiret Original-Sender: "Debbugs-submit" Resent-CC: bug-guile@gnu.org Resent-Date: Tue, 28 Mar 2023 16:11: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.168001983421284 (code B ref 61095); Tue, 28 Mar 2023 16:11:02 +0000 Original-Received: (at 61095) by debbugs.gnu.org; 28 Mar 2023 16:10:34 +0000 Original-Received: from localhost ([127.0.0.1]:50563 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1phBu6-0005XE-Az for submit@debbugs.gnu.org; Tue, 28 Mar 2023 12:10:34 -0400 Original-Received: from jpoiret.xyz ([206.189.101.64]:33816) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1phBu4-0005X3-1Z for 61095@debbugs.gnu.org; Tue, 28 Mar 2023 12:10:32 -0400 Original-Received: from authenticated-user (jpoiret.xyz [206.189.101.64]) by jpoiret.xyz (Postfix) with ESMTPA id 4CC73184F1E; Tue, 28 Mar 2023 16:10:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jpoiret.xyz; s=dkim; t=1680019830; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=HEYvsizn9jAYcZaSuFRuHtVO8WhPWurC300y+rIfHXo=; b=LzhUFRYiQ7AhVFcVKgNLUBrjK75sYyi6YDTJuGSeAd5BQICKJn9aHHLko2ZILST4HIJuEs 8K+T1TwvPuRfOiwpr9uoAXfB1RAp5gmQBWAJxe1CYXp2V1gWruyTOXzvoyWMBB934zg1ak Y9vhTgR9gXoEoipErr1v/DA6L3X80aoC3byJDrVujb5kOFBoPMo+4oQHRt1hGWtz3rDtpW L7TRA4wVI8iVY7IM9ZS5vjWHbJwvMRssDP+0j9PomdrWMXp1FahCZXWlfwkhSPt9wFNxni rbZDJxQhRqi4jYyTCmy7Q/J9emED1/oq8UsV8wSQDB3Rw9gLKqCs+5IGLC0GMg== In-Reply-To: <87zg7xgqxz.fsf@gnu.org> 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-bounces+guile-bugs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.lisp.guile.bugs:10581 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Ludo, Ludovic Court=C3=A8s writes: > - 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 =3D fcntl (max_fd, F_GETFD, NULL); > + if ((flags >=3D 0) && ((flags & FD_CLOEXEC) =3D=3D 0)) > + posix_spawn_file_actions_addclose (actions, max_fd); > } I'm worried about TOCTOU in multi-threaded contexts here, which is why I opted for the heavy handed-approach here. In general I don't think we can know in advance which fdes to close :/ It's a shame that the posix_spawn actions fails on other kernels, since I don't really see a way to mitigate this problem (apart from the new posix_spawn_file_actions_addclosefrom_np as you mentioned). I don't know what we could do here. Maybe not provide spawn? Or provide it in spite of the broken fd closing? > - > - closedir (dirp); > } >=20=20 > 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 =3D NULL; >=20=20 > + 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); This won't work, and actually this was one of the original logic bugs I was trying to fix. If err is equal to, let's say, STDIN_FILENO, then the first call will overwrite the initial file descriptor at STDIN_FILENO, and the second call won't do what the caller intended. This is why I was moving them out of the way first, so that they would not overwrite each other. > + /* 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 !=3D in) > + posix_spawn_file_actions_addclose (&actions, out); > + if (err > 2 && err !=3D out && err !=3D in) > + posix_spawn_file_actions_addclose (&actions, err); Isn't this unneeded given we call close_inherited_fds below? > [...] > > Josselin: I simplified the =E2=80=98dup2=E2=80=99 logic somewhat. See my comments above. > 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., en= d 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. +1 Best, =2D-=20 Josselin Poiret --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQHEBAEBCAAuFiEEOSSM2EHGPMM23K8vUF5AuRYXGooFAmQjEXMQHGRldkBqcG9p cmV0Lnh5egAKCRBQXkC5Fhcail3ZC/4q3n0W2AOoRdNaXOzROGyxrfTd7dz3Ehx/ xEko14Ury/kPiV+x8MxIQXZ2Vo9m8LXTbuhhiLzeRNwS+fBwwnZQbEDDE4GvDkLd Vwh6IRmxKk3A5PxVqaX3EJypcwCABxMHzj1et3I/393na3UbPB/nVJaLAs27L/al XrFwd6k1aUGCPNYMPOVkulJXx6NNrOVcV1fSm23VlwpQzeeRUqVQBWWLPWi7MJ4n lnH+iwbLOAPIgywltXrCkf4rsook8KEBGWvQXSCgAqlCycdNIiBid7bTNaoitbeW HX5LP314uiMjSktOVVQrK+Q1+PhLJXUSGPxXRrxDTZf6/fXYAF4OrR343KjwxHJQ KWwC2yCENFkyHlh3RKkXkfmH41ezkaHQu2o64uD/1QcpFyv7Vx5Rde41pTPSEd8M 3umstB4vWNb2bzuBczeNXaoTFpAGKZHwXkGjfCWCZVEmyGBnbRm16VWxyPHtlvAp WizmvpOzWpM63lTajCdzwlPnckxPlfM= =pbCP -----END PGP SIGNATURE----- --=-=-=--