From patchwork Fri Aug 18 20:22:39 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: ulfvonbelow X-Patchwork-Id: 16043 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 4A77027BBEA; Fri, 18 Aug 2023 21:24:18 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 80AA627BBE2 for ; Fri, 18 Aug 2023 21:24:17 +0100 (BST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qX60q-0008SW-O7; Fri, 18 Aug 2023 16:24: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 1qX60p-0008Rw-Ft for guix-patches@gnu.org; Fri, 18 Aug 2023 16:24: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 1qX60p-0008GB-7p for guix-patches@gnu.org; Fri, 18 Aug 2023 16:24:03 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qX60p-0006fZ-QR for guix-patches@gnu.org; Fri, 18 Aug 2023 16:24:03 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#65221] [PATCH 6/6] service: exec-command: close other file descriptors by default. Resent-From: ulfvonbelow Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Fri, 18 Aug 2023 20:24:03 +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: 65221@debbugs.gnu.org Cc: ulfvonbelow Received: via spool by 65221-submit@debbugs.gnu.org id=B65221.169239022825581 (code B ref 65221); Fri, 18 Aug 2023 20:24:03 +0000 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:23:48 +0000 Received: from localhost ([127.0.0.1]:48896 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX60a-0006eX-D8 for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:48 -0400 Received: from tilde.club ([2607:5300:204:4340::114]:54574 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX60Y-0006eO-2x for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:46 -0400 Received: by tilde.club (Postfix, from userid 5378) id 1BFBD2250B9BD; Fri, 18 Aug 2023 20:23:45 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 1BFBD2250B9BD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390225; bh=ZgBbQs28uqvNpr/4pc3fFoWaeSblYKOh4Iz3s/RVLNc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=BnuTFdUqnnkLyM2Rg0Z1zHXgooAFR7OW1Et5Lk8PP4PWKcR8e6jzXvAkh6+Fk+v1M 8fLM2nrB+c/1fOY0bgqfc5+gv3h1xCvLCNrkIL6t7ofxPwoNli3k/tLiwmFc2y35dW /i0z6CJvzAowQlbo5/9PiRCv/MK+clR2rtVwoQoI= From: ulfvonbelow Date: Fri, 18 Aug 2023 15:22:39 -0500 Message-Id: <20230818202239.21177-6-striness@tilde.club> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230818202239.21177-1-striness@tilde.club> References: <20230818202239.21177-1-striness@tilde.club> MIME-Version: 1.0 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+patchwork=mira.cbaines.net@gnu.org Sender: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org X-getmail-retrieved-from-mailbox: Patches If EXTRA-PORTS is given, no strong guarantee about which, if any, other file descriptors will remain open can be made anyhow. Better to err on the side of caution in that case and close them. If EXTRA-PORTS isn't given, we can either close all non-standard file descriptors or none of them. The former I've decided to represent with the empty list, and the latter with #t (as in "which extra ports do you want? ... Yes"). We choose '() for the default because 1. It's already the default value. 2. It's hard to imagine a use case that depends on EXTRA-PORTS being explicitly given, but additional unspecified file descriptors also being available, since that has never worked and in the general case never can, short of manually duplicating ports to high file descriptors. 3. It's hard to imagine a use case that depends on EXTRA-PORTS not being given and additional unspecified file descriptors also being available, since until 0.9.2 this didn't work, and 4. It's the documented behavior, both in EXEC-COMMAND's docstring and in the manual. 5. It's how guile's system* behaves, and this makes our transparent replacement a closer match. 6. It errs on the side of security. While *_CLOEXEC is good practice and a quality second layer of defense against unintentional leaking of file descriptors, it requires all fd-opening to be done very carefully in a concurrent context. Understanding everything that can and can't be a yield point requires a nontrivial understanding of both shepherd and fibers. For example, at present, on systems without signalfd support, *anything* where asyncs can run can be a yield point, due to the fact that the SIGCHLD handler calls put-message. --- modules/shepherd/service.scm | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm index 3008e31..924cbfe 100644 --- a/modules/shepherd/service.scm +++ b/modules/shepherd/service.scm @@ -1537,7 +1537,8 @@ either LOG-PORT or LOG-FILE if it's true, whereas file descriptor 0 (standard input) points to INPUT-PORT or /dev/null. EXTRA-PORTS are made available starting from file descriptor 3 onwards; all -other file descriptors are closed prior to yielding control to COMMAND. When +other file descriptors are closed prior to yielding control to COMMAND, unless +EXTRA-PORTS is #t, in which case no file descriptors are closed. When CREATE-SESSION? is true, call 'setsid' first. Guile's SETRLIMIT procedure is applied on the entries in RESOURCE-LIMITS. For @@ -1590,7 +1591,17 @@ false." (reconfigure-fds (cons* stdin stdout stderr - extra-ports))) + (if (list? extra-ports) + extra-ports + '()))) + (unless (eq? extra-ports #t) + (let ((max-fds-count (max-file-descriptors))) + (let loop ((fd (+ 3 (length extra-ports)))) + (when (< fd max-fds-count) + ;; Use FD_CLOEXEC instead of close-fdes so fd finalizers don't + ;; run. + (catch-system-error (fcntl fd F_SETFD FD_CLOEXEC)) + (loop (+ fd 1))))))) ;; setgid must be done *before* setuid, otherwise the user will ;; likely no longer have permissions to setgid.