From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp10.migadu.com ([2001:41d0:403:478a::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms9.migadu.com with LMTPS id yCboLQPT32S/4AAASxT56A (envelope-from ) for ; Fri, 18 Aug 2023 22:22:27 +0200 Received: from aspmx1.migadu.com ([2001:41d0:403:478a::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp10.migadu.com with LMTPS id 2DDeLAPT32Q0FAEAG6o9tA (envelope-from ) for ; Fri, 18 Aug 2023 22:22:27 +0200 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id A35C44FED3 for ; Fri, 18 Aug 2023 22:22:26 +0200 (CEST) Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=tilde.club header.s=mail header.b=OdEE+KWE; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org"; dmarc=fail reason="SPF not aligned (relaxed)" header.from=tilde.club (policy=none) ARC-Seal: i=1; s=key1; d=yhetil.org; t=1692390147; a=rsa-sha256; cv=none; b=QT7oSaocL2fEYCHwcQOaE2TpnoS5y3IPcRQSiJhIZhWLZP0km8GT96FV1NttxDDMjum8lr UR8+4/b/vADmjiBgqPdM382wsCDJiRYx2AY/nSTdHqJqh63FkWquAr2l74iSmopGgeMqTX Xf5pdYIpp3AylKCnZYIgTACdTZPcoFeFZSTsOErqhfurZFZv2VnI0cxuBSeLb4C7qIh4ct SpQTv7+Ua2n/u8XMZ4NlPIb9S3Lvhtg18nqbhEvfGC5JEy/YgXKjpGNwcIJa1rAcdcgpsd bgLw+P7s/F/lpEBmswrQKV+dOZY8olr8mkYJrOt04VETU6/rfli7ueGot7OVog== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=tilde.club header.s=mail header.b=OdEE+KWE; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org"; dmarc=fail reason="SPF not aligned (relaxed)" header.from=tilde.club (policy=none) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1692390147; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post: dkim-signature; bh=9VPzMOHXZITVdc5z65F+872woH3ETuecutUDdiBFUIw=; b=CJ2FfGcbK2zXEV0vVYiAoRhaHNXPCJioMAvhpCPSqtvFNmgrJTTWUfc4oVaOiP9NvYH9du lnrp9rwcCBX+irTblCWl2W1/BlkPWf1b/OZXk7OosioxQZ/2nM8R6z5fh+Z7s7lNQVO+QT XH7ibKNO9cUHcsy7mEFu1Xx7jctxaA5wOUB1Fg/ucaSMhzgiMFI8FFZHLvmP2pi2aYDddu lO98EnFIzFF+IiI2xQu5fs3Zaw4j9u/37jbPQPh6Zc8v4RqeqfF0T/tPStF8t7M/4uYZr+ MmQrygiZU+X2tes/yXP4AN+ojShIkt0Lrj4bCKwU1hCeSZ7fIg8g1Q2TKIPoDQ== Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qX5yu-0007dI-Qk; Fri, 18 Aug 2023 16:22:04 -0400 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 1qX5yt-0007cq-28 for guix-patches@gnu.org; Fri, 18 Aug 2023 16:22:03 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qX5ys-00081O-B4 for guix-patches@gnu.org; Fri, 18 Aug 2023 16:22:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qX5ys-0006ag-DQ for guix-patches@gnu.org; Fri, 18 Aug 2023 16:22:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#65221] [PATCH 0/2] Fix EXTRA-PORTS edge cases Resent-From: Ulf Herrman Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Fri, 18 Aug 2023 20:22:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 65221 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 65221@debbugs.gnu.org, ulfvonbelow Received: via spool by 65221-submit@debbugs.gnu.org id=B65221.169239011825324 (code B ref 65221); Fri, 18 Aug 2023 20:22:02 +0000 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:21:58 +0000 Received: from localhost ([127.0.0.1]:48877 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX5yn-0006aO-JU for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:21:58 -0400 Received: from tilde.club ([2607:5300:204:4340::114]:56720 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX5yk-0006aE-PY for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:21:55 -0400 Received: by tilde.club (Postfix, from userid 5378) id 183D42250B9B8; Fri, 18 Aug 2023 20:21:52 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 183D42250B9B8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390112; bh=fxo6m6nFhij2UQ1qEeH+Zdau8O6OGZsZSZi0epXXfo0=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=OdEE+KWE2lFnSJFBJD/EdLQqPICHkb82zv0340Qt/w8uU52+zMR+joGV6l7xY7RsR irSZ2QedmiqJe7bJ40NJLyNAiRb9w4aKy5ItvVJKMNTX4OyLh+4qePN96yNed9Ssxw A04rQKRAQlw99RA5UJmEFGLfPtjaFw4RITphQrzc= From: Ulf Herrman References: <20230811090352.3572-1-striness@tilde.club> <20230811090615.3707-1-striness@tilde.club> <87y1ic8tiw.fsf_-_@gnu.org> Date: Fri, 18 Aug 2023 15:21:13 -0500 In-Reply-To: <87y1ic8tiw.fsf_-_@gnu.org> ("Ludovic =?UTF-8?Q?Court=C3=A8s?="'s message of "Tue, 15 Aug 2023 12:55:03 +0200") Message-ID: <87wmxsw18m.fsf@tilde.club> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: guix-patches-bounces+larch=yhetil.org@gnu.org X-Migadu-Flow: FLOW_IN X-Migadu-Country: US X-Migadu-Scanner: mx0.migadu.com X-Spam-Score: -7.19 X-Migadu-Queue-Id: A35C44FED3 X-Migadu-Spam-Score: -7.19 X-TUID: cah6cUjuEbKR --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Ludovic Court=C3=A8s writes: > Previously, services started indirectly with =E2=80=98exec-command=E2=80= =99 (which is usually > the case) would not inherit any file descriptor from shepherd because > =E2=80=98exec-command=E2=80=99 would explicitly close all of them. Howe= ver, services started > with =E2=80=98make-system-constructor=E2=80=99 and processes created by = some other means, such > as calling =E2=80=98system*=E2=80=99, would inherit some of those descri= ptors, giving them > more authority than intended. Interestingly enough, guile's system* closes all file descriptors aside from the first 3, and we now provide a replacement for system* that uses fork+exec-command and therefore does not. > The FD-closing loop was removed on purpose, in > 2c0354258047133db8b885bcc11afdf0def5d885. > > Now, as you write, it means that service writers must be careful now and > not leave any non-CLOEXEC file descriptor behind them. > > At the time I audited Guix System to check that this was a reasonable > thing to expect and that we could indeed ensure no file descriptors were > leaked. There=E2=80=99s also =E2=80=98tests/close-on-exec.sh=E2=80=99. > > If you found cases where it would be necessary, what we could do is have > =E2=80=98shepherd=E2=80=99 replace =E2=80=98call-with-input-file=E2=80=99= & co. with a variant that > opens files as O_CLOEXEC by default. WDYT? The problem is that there isn't a way for the user to replicate the old behavior of default-non-inheriting short of wrapping the desired command with another file-descriptor-closing program, by which point the user and group may already have been changed. O_CLOEXEC is good, but to use it to prevent leaks to any particular program is a matter of global correctness, while closing everything not explicitly allowed for a program is a matter of local correctness. For example, if I have a program whose quality (and, to some extent, trustworthiness) I am wary of, I would really prefer to be certain that it doesn't get any file descriptors it shouldn't. The only way to be certain of this currently is to examine the entire shepherd process - including all services - to be sure there aren't any leaks. Replacing 'call-with-input-file' and such - as I now see is done in guix's shepherd config file - would probably be a good idea (I see we use call-with-input-file in primitive-load* and read-pid-file, for example), but it's still no guarantee. For example, I didn't even know SOCK_CLOEXEC was a thing, and so didn't use it in one of my services that calls socketpair. I did use fcntl afterward, but the time between those two introduces all kinds of questions, like "have you called system, system*, spawn-command, sleep, done any I/O, or had the misfortune to receive a SIGCHLD on a system without signalfd support". And if I hadn't happened to have looked at the source of exec-command, I wouldn't have even tried fcntl, because the manual entry for fork+exec-command and exec-command still clearly states: File descriptors 1 and 2 are kept as is or redirected to either LOG-PORT or LOG-FILE if it=E2=80=99s true, whereas file descriptor 0 (sta= ndard input) points to INPUT-PORT or =E2=80=98/dev/null=E2=80=99; all other fil= e descriptors are closed prior to yielding control to COMMAND. Whatever course of action is taken, it is imperative that the documentation and code be aligned on this matter. Personally I'm inclined to bring the code in line with the documentation. >> In other words, the copying of earlier FDs in EXTRA-PORTS may overwri= te >> later FDs in EXTRA-PORTS. > > Good catch! > > Could you make a more minimal patch fixing this specific issue, also > adding a test reproducing the problem being fixed? I'm sending a more granular v2 series that adds the requested test, fixes the clobbering, makes extra-ports be honored regardless of the values of log-port and log-file, fixes edge cases around file descriptors 0-2, and finally adds support for closing all other file descriptors to exec-command. That last one also makes closing all other file descriptors the default, mostly because it makes the most sense for the default value of extra-ports, and otherwise I'd have to update the manual. It does include the option of #:extra-ports #t, though, in which case no fds are closed. One alternative would be to have the close-all-others behavior controlled by an independent argument; that way the file descriptor rearranging functionality can be independent of whether other file descriptors are closed. Speaking of file descriptor rearranging functionality, the v2 replaces 'preserve-ports' with 'reconfigure-fds', which uses a different, graph-based approach to avoiding clobbering while using fewer system calls. It's also more robust, and will function as long as there is even a single unused file descriptor, and won't touch any open file descriptors other than those in the target range. =2D ulfvonbelow --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQHIBAEBCAAyFiEEn6BUn0yca1D9JsMa1lV76sJM9mgFAmTf0sgUHHN0cmluZXNz QHRpbGRlLmNsdWIACgkQ1lV76sJM9mgRBgv/eE1D1m7a1aNVU/0MjkKYbHb+wtia fDmM5pXOB9WwWHH0O8tpu9Hev5WG0II6dYdPzZBuUFPNx3/LRfP+W+7+rDS89zu1 eialqQnT0Z0AY/iiDTTf7H/sI4sitG9VvlfT9/FDGb2EfN8BmrQEr8Anj3QKEPoL 8/y09VK9qlpOcbA+FuUaEYpQXhsDAwZWLhGCvBrPtGpegn4/rqMSEVP052ixRZkx wGIS6bZpoRLbWBgLBa7jLraRs3t+IShx9gxsFgoPZhTY0P061bRsVoaUJHcpqUq7 tBukua8TvGtmjVXBQuU+XcXOlqKMUNSTAuua0/OlLmRQA3mvFj3O0us2I/oRaxsz BDGAoSxEyb5/0ESOu6kIFM4BKGkPXfQwMgW0bgPVIq2g/uzZn3TxiRLahlHXTD6D AF0Nrg+xg+qc9OhSJOU2KxkImmg6DsNj3PVGH4nO5ioMa3hmh0jMRJfXm/C2qd/J twz4Cucav4ejHgou/1wJiTyHvXeESkc2cZ/M =8l4P -----END PGP SIGNATURE----- --=-=-=--