From unknown Sat Jun 21 10:33:55 2025 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Mailer: MIME-tools 5.509 (Entity 5.509) Content-Type: text/plain; charset=utf-8 From: bug#65221 <65221@debbugs.gnu.org> To: bug#65221 <65221@debbugs.gnu.org> Subject: Status: [PATCH 0/2] Fix EXTRA-PORTS edge cases Reply-To: bug#65221 <65221@debbugs.gnu.org> Date: Sat, 21 Jun 2025 17:33:55 +0000 retitle 65221 [PATCH 0/2] Fix EXTRA-PORTS edge cases reassign 65221 guix-patches submitter 65221 ulfvonbelow severity 65221 normal tag 65221 patch thanks From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 11 05:04:15 2023 Received: (at submit) by debbugs.gnu.org; 11 Aug 2023 09:04:15 +0000 Received: from localhost ([127.0.0.1]:44886 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qUO46-0005rN-V8 for submit@debbugs.gnu.org; Fri, 11 Aug 2023 05:04:15 -0400 Received: from lists.gnu.org ([2001:470:142::17]:34610) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qUO42-0005r5-BO for submit@debbugs.gnu.org; Fri, 11 Aug 2023 05:04:13 -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 1qUO3w-0000Zh-I3 for guix-patches@gnu.org; Fri, 11 Aug 2023 05:04:04 -0400 Received: from tilde.club ([142.44.150.184]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qUO3u-0005pR-Vl for guix-patches@gnu.org; Fri, 11 Aug 2023 05:04:04 -0400 Received: by tilde.club (Postfix, from userid 5378) id 0F9D9224F4692; Fri, 11 Aug 2023 09:04:00 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 0F9D9224F4692 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1691744640; bh=linGC3kD3douhiErQdIdFJMfzkwkn0WJIIfccp4fbVc=; h=From:To:Subject:Date:From; b=dwON6zgNyQwXwdwnspw4J2TQUKDP8GSSGXL//UYDq1IvnFF0vaQ+zvNSnpfgjDlkW taI5N3XqXZ3u0j89NrK2DoGaMk+G6jLvNGwMEs/NmxWO/qUptWOX7smGTyxXNspJhU 86RjfeQbVCBUI16KUsuRNEJYCfQ6vD5YZBYmxm+g= From: ulfvonbelow To: guix-patches@gnu.org Subject: [PATCH 0/2] Fix EXTRA-PORTS edge cases Date: Fri, 11 Aug 2023 04:03:52 -0500 Message-Id: <20230811090352.3572-1-striness@tilde.club> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=142.44.150.184; envelope-from=striness@tilde.club; helo=tilde.club X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-Spam-Score: 0.9 (/) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.1 (/) The #:extra-ports argument to exec-command and its users behaves quite strangely in certain circumstances, for example when multiple ports are supplied, and they are supplied in an order other than by ascending file descriptor number. This can cause file descriptors to be clobbered. ulfvonbelow (2): service: make EXTRA-PORTS work as advertised. service: use PRESERVE-PORTS for redirecting FDs 0-2. modules/shepherd/service.scm | 119 ++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 43 deletions(-) -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 11 05:06:25 2023 Received: (at 65221) by debbugs.gnu.org; 11 Aug 2023 09:06:25 +0000 Received: from localhost ([127.0.0.1]:44891 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qUO6C-0005v0-F7 for submit@debbugs.gnu.org; Fri, 11 Aug 2023 05:06:25 -0400 Received: from tilde.club ([142.44.150.184]:33070 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qUO6A-0005ur-9m for 65221@debbugs.gnu.org; Fri, 11 Aug 2023 05:06:23 -0400 Received: by tilde.club (Postfix, from userid 5378) id CE52D224F4692; Fri, 11 Aug 2023 09:06:21 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club CE52D224F4692 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1691744781; bh=Int3jw4rw+fZql4dVXkEmhtVkM5b/HBzmw/2N5nrKQA=; h=From:To:Subject:Date:From; b=LK8hy63tE3Y7RD7mLPwjk9/aDOQsxvo5uiYs8ZI/CB+capjO0Lj+GzmFEf5ez/AeO r+HwyCiqxo/eu7LIvftq9ajCrIwCiqPY6RXUSUq66FKi7gDrT90kSFUCRJQKThkPpR e4u7zDpUtGOL3xZaJTiGfxzh1qkdbyEpl3/vXvXQ= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 1/2] service: make EXTRA-PORTS work as advertised. Date: Fri, 11 Aug 2023 04:06:14 -0500 Message-Id: <20230811090615.3707-1-striness@tilde.club> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) EXEC-COMMAND (and, by extension, FORK+EXEC-COMMAND) has several issues: 1. Despite it being documented that "all other file descriptors are closed prior to yielding control to COMMAND", this is not currently the case - only other file descriptors that are already marked as FD_CLOEXEC are closed. For example, if user code happens to have a file descriptor open, for example with call-with-input-file, while EXEC-COMMAND is run, the new process image will inherit that file descriptor. This may cause some resource waste, but more importantly may cause security issues in certain situations. 2. EXTRA-PORTS is only honored when either LOG-PORT or LOG-FILE is passed. I have no idea why this is the case, it isn't documented anywhere, and it isn't intuitive. 3. Even when LOG-PORT or LOG-FILE is passed, EXTRA-PORTS may not work as described, because it copies file descriptor contents in an arbitrary order. For example, suppose that (map fileno EXTRA-PORTS) is (7 6 5 4 3). If the underlying file originally stored in fd N is represented by F(N), it will assign 3 <-- F(7) 4 <-- F(6) 5 <-- F(5) 6 <-- F(6) 7 <-- F(7) In other words, the copying of earlier FDs in EXTRA-PORTS may overwrite later FDs in EXTRA-PORTS. Because the process of properly and safely copying those FDs involves many steps, we've split it, along with marking all file descriptors not being preserved as FD_CLOEXEC, into a separate procedure named PRESERVE-PORTS. * modules/shepherd/service.scm (preserve-ports): new procedure. (exec-command): use it. --- modules/shepherd/service.scm | 119 +++++++++++++++++++++++------------ 1 file changed, 78 insertions(+), 41 deletions(-) diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm index 68553d4..ffbd03c 100644 --- a/modules/shepherd/service.scm +++ b/modules/shepherd/service.scm @@ -1434,6 +1434,52 @@ FILE." (list->vector (map (lambda (group) (group:gid (getgr group))) supplementary-groups))) +(define (preserve-ports extra-ports) + "Duplicate the FDs (fd1 fd2 ... fdN) corresponding to the N ports in +EXTRA-PORTS into the FD range (3 4 ... 3+N). This will work regardless of the +numeric values of fd1 ... fdN. Any open file descriptors not in EXTRA-PORTS +and numbered 3 or higher WILL be closed or marked FD_CLOEXEC." + ;; We employ the following strategy: copy FDs as high as possible, in + ;; descending order of FD, so as to avoid clobbering, then copy the high FDs + ;; to low FDs, in the order specified in EXTRA-PORTS. If more than half of + ;; the FD range is included in EXTRA-PORTS, this still won't work, and we + ;; may reach a point where copying low will require us to copy the + ;; still-uncopied FDs high again. This should be sufficiently rare as to + ;; not be a concern. + (let* ((max-fds-count (max-file-descriptors)) + (highest-fd (- max-fds-count 1)) + (extra-fds-count (length extra-ports)) + (preserved-fds-count (+ 3 extra-fds-count)) + (extra-fds (map fileno extra-ports)) + (index+fd (map cons + (iota extra-fds-count) + extra-fds)) + (index+fd-by-fileno (sort index+fd + (lambda (pair1 pair2) + (> (cdr pair1) + (cdr pair2))))) + (index2+fd-by-fileno (map cons + (iota extra-fds-count) + index+fd-by-fileno)) + (index2+fd (sort index2+fd-by-fileno + (lambda (spec1 spec2) + (< (second spec1) (second spec2)))))) + (for-each dup2 + (map cdr index+fd-by-fileno) + (iota extra-fds-count highest-fd -1)) + (for-each (match-lambda + ((by-fileno-index original-index . original-fd) + (dup2 (- highest-fd by-fileno-index) + (+ 3 original-index)))) + index2+fd) + (for-each (lambda (fd) + (catch-system-error + (let ((flags (fcntl fd F_GETFD))) + (when (zero? (logand flags FD_CLOEXEC)) + (fcntl fd F_SETFD (logior FD_CLOEXEC flags)))))) + (iota (- max-fds-count preserved-fds-count) + preserved-fds-count)))) + (define* (exec-command command #:key (user #f) @@ -1479,48 +1525,39 @@ false." (chdir directory) (environ environment-variables) - ;; Close all the file descriptors except stdout and stderr. - (let ((max-fd (max-file-descriptors))) + ;; Redirect stdin. + (catch-system-error (close-fdes 0)) + ;; Make sure file descriptor zero is used, so we don't end up reusing + ;; it for something unrelated, which can confuse some packages. + (dup2 (if input-port + (fileno input-port) + (open-fdes "/dev/null" O_RDONLY)) + 0) - ;; Redirect stdin. - (catch-system-error (close-fdes 0)) - ;; Make sure file descriptor zero is used, so we don't end up reusing - ;; it for something unrelated, which can confuse some packages. - (dup2 (if input-port - (fileno input-port) - (open-fdes "/dev/null" O_RDONLY)) - 0) + (when (or log-port log-file) + (catch #t + (lambda () + ;; Redirect stdout and stderr to use LOG-FILE. + (catch-system-error (close-fdes 1)) + (catch-system-error (close-fdes 2)) + (dup2 (if log-file + (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) + #o640) + (fileno log-port)) + 1) + (dup2 1 2)) - (when (or log-port log-file) - (catch #t - (lambda () - ;; Redirect stout and stderr to use LOG-FILE. - (catch-system-error (close-fdes 1)) - (catch-system-error (close-fdes 2)) - (dup2 (if log-file - (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) - #o640) - (fileno log-port)) - 1) - (dup2 1 2) - - ;; Make EXTRA-PORTS available starting from file descriptor 3. - ;; This clears their FD_CLOEXEC flag. - (let loop ((fd 3) - (ports extra-ports)) - (match ports - (() #t) - ((port rest ...) - (catch-system-error (close-fdes fd)) - (dup2 (fileno port) fd) - (loop (+ 1 fd) rest))))) - - (lambda (key . args) - (when log-file - (format (current-error-port) - "failed to open log-file ~s:~%" log-file)) - (print-exception (current-error-port) #f key args) - (primitive-exit 1)))) + (lambda (key . args) + (when log-file + (format (current-error-port) + "failed to open log-file ~s:~%" log-file)) + (print-exception (current-error-port) #f key args) + (primitive-exit 1)))) + + ;; Close all the file descriptors except stdout, stderr, and EXTRA-PORTS. + ;; Make EXTRA-PORTS available starting from file descriptor 3. + ;; This clears their FD_CLOEXEC flag. + (preserve-ports extra-ports) ;; setgid must be done *before* setuid, otherwise the user will ;; likely no longer have permissions to setgid. @@ -1558,7 +1595,7 @@ false." (format (current-error-port) "exec of ~s failed: ~a~%" program (strerror (system-error-errno args))) - (primitive-exit 1))))))) + (primitive-exit 1)))))) (define %precious-signals ;; Signals that the shepherd process handles. -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 11 05:06:31 2023 Received: (at 65221) by debbugs.gnu.org; 11 Aug 2023 09:06:31 +0000 Received: from localhost ([127.0.0.1]:44894 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qUO6J-0005vK-3a for submit@debbugs.gnu.org; Fri, 11 Aug 2023 05:06:31 -0400 Received: from tilde.club ([142.44.150.184]:33084 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qUO6G-0005vC-Ll for 65221@debbugs.gnu.org; Fri, 11 Aug 2023 05:06:29 -0400 Received: by tilde.club (Postfix, from userid 5378) id 68AEC224F468B; Fri, 11 Aug 2023 09:06:28 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 68AEC224F468B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1691744788; bh=zT+RsBzrdwyHo9JkTvV911XbenRJjrkflyhLBtnqsII=; h=From:To:Subject:Date:In-Reply-To:References:From; b=UkXPcBtjRYG/oW0cImwMh3XsOKQeRLKp4PtyxHFIP+kVZE/ZWU8N5Pz1dMk3o1/8l cuQAmxsHOWy0ZN4WSqtuGgDrQaSGNPN2Xn3oOHZq1uy3rg3p1/xdqZ8sot+nzlyLwe uiaJlxhXj5u9y6m7UevqV5LRrsV/FebvQd9nkb3A= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 2/2] service: use PRESERVE-PORTS for redirecting FDs 0-2. Date: Fri, 11 Aug 2023 04:06:15 -0500 Message-Id: <20230811090615.3707-2-striness@tilde.club> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230811090615.3707-1-striness@tilde.club> References: <20230811090615.3707-1-striness@tilde.club> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) There are currently some corner cases in how EXTRA-PORTS works due to it not managing FDs 0, 1, and 2. Specifically, if one were to include a port in EXTRA-PORTS with FD 0, 1, or 2, it would *not* be preserved, and would instead represent the file that EXEC-COMMAND assigned to that file descriptor. To avoid this, it's necessary to call PRESERVE-PORTS *before* redirecting the input, but this could clobber LOG-PORT or INPUT-PORT, so it would become necessary to include LOG-PORT and INPUT-PORT in the call to PRESERVE-PORTS, then do the redirection using the new FD assignment, then close them. This complication can be avoided if we simply let PRESERVE-PORTS itself do the redirection. This also solves other edge cases, like if LOG-PORT has fileno 0 or 1 (previously passing a LOG-PORT of (current-output-port) would cause an error, as the underlying file descriptor would be closed before dup2 was called to copy it), or if INPUT-PORT has fileno 0. To solve this, we modify PRESERVE-PORTS to allow both file descriptors and ports, and to start the range it copies into at 0 instead of 3. We then modify EXEC-COMMAND to explicitly pass the desired standard I/O FDs / ports at the front of the list it passes to PRESERVE-PORTS. * modules/shepherd/service.scm (preserve-ports): Allow elements of EXTRA-PORTS to be either ports or file descriptors. Start the range of FDs being duplicated into at 0 instead of 3. (exec-command): use PRESERVE-PORTS for redirecting FDs 0, 1, and 2. --- modules/shepherd/service.scm | 74 +++++++++++++++++------------------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm index ffbd03c..5f735fe 100644 --- a/modules/shepherd/service.scm +++ b/modules/shepherd/service.scm @@ -1435,10 +1435,10 @@ FILE." supplementary-groups))) (define (preserve-ports extra-ports) - "Duplicate the FDs (fd1 fd2 ... fdN) corresponding to the N ports in -EXTRA-PORTS into the FD range (3 4 ... 3+N). This will work regardless of the + "Duplicate the FDs (fd0 fd1 ... fdN) corresponding to the N+1 ports or FDs in +EXTRA-PORTS into the FD range (0 1 ... N). This will work regardless of the numeric values of fd1 ... fdN. Any open file descriptors not in EXTRA-PORTS -and numbered 3 or higher WILL be closed or marked FD_CLOEXEC." +WILL be closed or marked FD_CLOEXEC." ;; We employ the following strategy: copy FDs as high as possible, in ;; descending order of FD, so as to avoid clobbering, then copy the high FDs ;; to low FDs, in the order specified in EXTRA-PORTS. If more than half of @@ -1449,8 +1449,9 @@ and numbered 3 or higher WILL be closed or marked FD_CLOEXEC." (let* ((max-fds-count (max-file-descriptors)) (highest-fd (- max-fds-count 1)) (extra-fds-count (length extra-ports)) - (preserved-fds-count (+ 3 extra-fds-count)) - (extra-fds (map fileno extra-ports)) + (extra-fds (map (lambda (x) + (if (port? x) (fileno x) x)) + extra-ports)) (index+fd (map cons (iota extra-fds-count) extra-fds)) @@ -1470,15 +1471,15 @@ and numbered 3 or higher WILL be closed or marked FD_CLOEXEC." (for-each (match-lambda ((by-fileno-index original-index . original-fd) (dup2 (- highest-fd by-fileno-index) - (+ 3 original-index)))) + original-index))) index2+fd) (for-each (lambda (fd) (catch-system-error (let ((flags (fcntl fd F_GETFD))) (when (zero? (logand flags FD_CLOEXEC)) (fcntl fd F_SETFD (logior FD_CLOEXEC flags)))))) - (iota (- max-fds-count preserved-fds-count) - preserved-fds-count)))) + (iota (- max-fds-count extra-fds-count) + extra-fds-count)))) (define* (exec-command command #:key @@ -1525,39 +1526,34 @@ false." (chdir directory) (environ environment-variables) - ;; Redirect stdin. - (catch-system-error (close-fdes 0)) - ;; Make sure file descriptor zero is used, so we don't end up reusing - ;; it for something unrelated, which can confuse some packages. - (dup2 (if input-port - (fileno input-port) - (open-fdes "/dev/null" O_RDONLY)) - 0) - - (when (or log-port log-file) - (catch #t - (lambda () - ;; Redirect stdout and stderr to use LOG-FILE. - (catch-system-error (close-fdes 1)) - (catch-system-error (close-fdes 2)) - (dup2 (if log-file - (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) - #o640) - (fileno log-port)) - 1) - (dup2 1 2)) - - (lambda (key . args) - (when log-file - (format (current-error-port) - "failed to open log-file ~s:~%" log-file)) - (print-exception (current-error-port) #f key args) - (primitive-exit 1)))) - - ;; Close all the file descriptors except stdout, stderr, and EXTRA-PORTS. + ;; Close all the file descriptors except stdin, stdout, stderr, and + ;; EXTRA-PORTS. ;; Make EXTRA-PORTS available starting from file descriptor 3. ;; This clears their FD_CLOEXEC flag. - (preserve-ports extra-ports) + (let* ( ;; Make sure file descriptor zero is used, so we don't end up reusing + ;; it for something unrelated, which can confuse some packages. + (stdin (or input-port (open-fdes "/dev/null" O_RDONLY))) + (stdout (catch #t + (lambda () + (or log-port + (and log-file + (open-fdes log-file + (logior O_CREAT O_WRONLY O_APPEND) + #o640)) + 1)) + (lambda (key . args) + (when log-file + (format (current-error-port) + "failed to open log-file ~s:~%" log-file)) + (print-exception (current-error-port) #f key args) + (primitive-exit 1)))) + (stderr (if (or log-port log-file) + stdout + 2))) + (preserve-ports (cons* stdin + stdout + stderr + extra-ports))) ;; setgid must be done *before* setuid, otherwise the user will ;; likely no longer have permissions to setgid. -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Tue Aug 15 06:55:49 2023 Received: (at 65221) by debbugs.gnu.org; 15 Aug 2023 10:55:49 +0000 Received: from localhost ([127.0.0.1]:35050 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qVriH-00080Z-96 for submit@debbugs.gnu.org; Tue, 15 Aug 2023 06:55:49 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46728) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qVriE-00080L-TY for 65221@debbugs.gnu.org; Tue, 15 Aug 2023 06:55:48 -0400 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 1qVri6-00052B-Uq; Tue, 15 Aug 2023 06:55:39 -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=P/qIV2Gy8zoK25NIJXESficYaDo5tv7FVkwN31l+Iuo=; b=VsSRR30qivIwOKU4xTfg xd0qkDCh8omsvWgHzzDum4B9YV1iFSHney2JEHHKgoapMaP8p3LWGWhPq9eC7akLl46IEFAboWdGM NYPCZpK04kkjlubR6Pc23qR6ksL3zPxgPbOVtVy73VvzCP9NK5JZfz6GNcfcEKrLRDh/m07ZHKrTd XeQkixBU9Wemft0j+DyR6BJMuS4uSP9LnjuCrrtBWTJEkO6lKaGq5zeFkeBIQtLn7+2Y/znYC6KbN wBE6TxY7CCjSTvdcdGKevgBGRIWiD4vob1AEQJTN+qV+C4BVWeLCpIY7hZG6mBrdofAi+5/SidB3F 3Z4LGichRCnxQA==; From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: ulfvonbelow Subject: Re: bug#65221: [PATCH 0/2] Fix EXTRA-PORTS edge cases References: <20230811090352.3572-1-striness@tilde.club> <20230811090615.3707-1-striness@tilde.club> Date: Tue, 15 Aug 2023 12:55:03 +0200 In-Reply-To: <20230811090615.3707-1-striness@tilde.club> (ulfvonbelow's message of "Fri, 11 Aug 2023 04:06:14 -0500") Message-ID: <87y1ic8tiw.fsf_-_@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 65221 Cc: 65221@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) Hi, ulfvonbelow skribis: > EXEC-COMMAND (and, by extension, FORK+EXEC-COMMAND) has several issues: > 1. Despite it being documented that "all other file descriptors are closed > prior to yielding control to COMMAND", this is not currently the case - > only other file descriptors that are already marked as FD_CLOEXEC are > closed. For example, if user code happens to have a file descriptor o= pen, > for example with call-with-input-file, while EXEC-COMMAND is run, the = new > process image will inherit that file descriptor. This may cause some > resource waste, but more importantly may cause security issues in cert= ain > situations. Yes. This has been the case since 0.9.2, as noted in =E2=80=98NEWS=E2=80= =99: 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. Howev= er, services started with =E2=80=98make-system-constructor=E2=80=99 and processes created by s= ome other means, such as calling =E2=80=98system*=E2=80=99, would inherit some of those descrip= tors, giving them more authority than intended. The change here consists in marking all internally-used file descriptors = as =E2=80=9Cclose-on-exec=E2=80=9D (O_CLOEXEC), a feature that=E2=80=99s bee= n available on GNU/Linux and GNU/Hurd for years but that so far wasn=E2=80=99t used consistently in sh= epherd. This is now fixed. As a side-effect, the file-descriptor-closing loop in =E2=80=98exec-command=E2=80=99 is now gone. 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? > 2. EXTRA-PORTS is only honored when either LOG-PORT or LOG-FILE is passed= . I > have no idea why this is the case, it isn't documented anywhere, and it > isn't intuitive. #:extra-ports wasn=E2=80=99t really made to be exposed I guess; it was adde= d for use by systemd-style services in 965f6b61a473ee57a1fc6ec3ea1ad6e35d596031. > 3. Even when LOG-PORT or LOG-FILE is passed, EXTRA-PORTS may not work as > described, because it copies file descriptor contents in an arbitrary > order. For example, suppose that (map fileno EXTRA-PORTS) is (7 6 5 4= 3). > If the underlying file originally stored in fd N is represented by F(N= ), it > will assign > 3 <-- F(7) > 4 <-- F(6) > 5 <-- F(5) > 6 <-- F(6) > 7 <-- F(7) > > In other words, the copying of earlier FDs in EXTRA-PORTS may overwrite > 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? Thanks for your work! Ludo=E2=80=99. From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:21:58 2023 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 To: Ludovic =?utf-8?Q?Court=C3=A8s?= Subject: Re: bug#65221: [PATCH 0/2] Fix EXTRA-PORTS edge cases 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?= =?utf-8?Q?=22'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-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: 65221@debbugs.gnu.org, ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --=-=-= 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----- --=-=-=-- From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:23:02 2023 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:23:02 +0000 Received: from localhost ([127.0.0.1]:48881 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX5zq-0006cR-77 for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:02 -0400 Received: from tilde.club ([142.44.150.184]:46950 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX5zo-0006bv-EH for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:01 -0400 Received: by tilde.club (Postfix, from userid 5378) id CBC512250B9BD; Fri, 18 Aug 2023 20:22:58 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club CBC512250B9BD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390178; bh=M1guxpFVdxF/8sHdzbvplvOIyQlVxQ1fObnPTcoY/oE=; h=From:To:Cc:Subject:Date:From; b=sHP0JZ2b0S79xYhakbGDlJ5NgwE60Gy2ldDuRGbq19vCb25P81zD5eT8RMM7lXyt/ hKZ4yNWj5RpMGWiqB5nEtHngNGnZ7aCzW9uGPoheO77Tg1juYwx6z+Wqs+2jOlNobh xYG4+xJcns49yWZ8Cx4WL2I8cL6ItOoZfB8aFD8s= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 1/6] tests: add extra-ports.sh test. Date: Fri, 18 Aug 2023 15:22:34 -0500 Message-Id: <20230818202239.21177-1-striness@tilde.club> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) * tests/extra-ports.sh: new test. --- tests/extra-ports.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 tests/extra-ports.sh diff --git a/tests/extra-ports.sh b/tests/extra-ports.sh new file mode 100644 index 0000000..51b91b7 --- /dev/null +++ b/tests/extra-ports.sh @@ -0,0 +1,76 @@ +socket="t-socket-$$" +conf="t-conf-$$" +log="t-log-$$" +pid="t-pid-$$" +testfile1="t-testfile1-$$" +testfile2="t-testfile2-$$" +resultfile="t-resultfile-$$" + +herd="herd -s $socket" + +trap "cat $log || true; + rm -f $socket $conf $log $testfile1 $testfile2 $resultfile; + test -f $pid && kill \`cat $pid\` || true; rm -f $pid" EXIT + +printf "test1" > "$testfile1" +printf "test2" > "$testfile2" + +cat > "$conf"< (fileno a) + (fileno b)))))) + + (define command + (list + "sh" + "-c" + (string-append + "set -x;" + " cat >> ${resultfile}.tmp <&" (number->string + (fileno test1)) + "; cat >> ${resultfile}.tmp <&" (number->string + (fileno test2)) + "; mv ${resultfile}.tmp ${resultfile}"))) + + (fork+exec-command command + #:extra-ports + ports + #:directory + "$(pwd)")))))) + #:stop (const #f) + #:respawn? #f))) +EOF + +rm -f "$pid" +shepherd -I -s "$socket" -c "$conf" -l "$log" --pid="$pid" & + +while ! test -f "$pid" ; do sleep 0.3 ; done + +shepherd_pid="`cat $pid`" +kill -0 $shepherd_pid + +$herd start test-extra-ports + +while ! test -f "$resultfile" ; do sleep 0.3 ; done + +result="$(cat $resultfile)" +test "$result" = "test1test2" -o "$result" = "test2test1" -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:23:11 2023 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:23:11 +0000 Received: from localhost ([127.0.0.1]:48884 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX5zy-0006d4-JG for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:11 -0400 Received: from tilde.club ([2607:5300:204:4340::114]:42128 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX5zx-0006cx-5I for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:09 -0400 Received: by tilde.club (Postfix, from userid 5378) id 2C5C82250B9BD; Fri, 18 Aug 2023 20:23:08 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 2C5C82250B9BD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390188; bh=yVbrYz9EJGJTmXLpYaMy+ky6PEx2qQ28zUuECdILUpU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=GCuxMvwcbX7UAaYBTSJkbjretG+IqSNk8X4esm2PPmDkTbYMdwKykibyqXcE3BOLx xthTZh0BlZA1wF8mPTDRwfhgRyB/kismThypwkNPyXgNXDuYsBzDZFXFuZdjWTXKtX zh47eJ7EFikXFF7A4Ue5pyvLmwsXQQ8WOlq4RDyE= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 2/6] service: don't let earlier ports clobber later ones in EXTRA-PORTS. Date: Fri, 18 Aug 2023 15:22:35 -0500 Message-Id: <20230818202239.21177-2-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 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) In some situations, EXTRA-PORTS may not work as described, because it copies file descriptor contents in an arbitrary order. For example, suppose that (map fileno EXTRA-PORTS) is (7 6 5 4 3). If the underlying file originally stored in fd N is represented by F(N), it will assign 3 <-- F(7) 4 <-- F(6) 5 <-- F(5) 6 <-- F(6) 7 <-- F(7) In other words, the copying of earlier FDs in EXTRA-PORTS may overwrite later FDs in EXTRA-PORTS. Because the process of properly and safely copying those FDs involves some complexity, we've split it into a separate procedure named PRESERVE-PORTS. * modules/shepherd/service.scm (reconfigure-ports): new procedure. (exec-command): use it. --- modules/shepherd/service.scm | 154 +++++++++++++++++++++++++---------- 1 file changed, 113 insertions(+), 41 deletions(-) diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm index 68553d4..68993ac 100644 --- a/modules/shepherd/service.scm +++ b/modules/shepherd/service.scm @@ -1434,6 +1434,88 @@ FILE." (list->vector (map (lambda (group) (group:gid (getgr group))) supplementary-groups))) +(define* (reconfigure-fds ports-or-fds #:optional (base 0)) + "Duplicate the FDs (fd0 fd1 ... fdN) corresponding to the N ports or FDs in +EXTRA-PORTS into the FD range (0 1 ... N), clearing their FD_CLOEXEC flag at +the same time. This will work regardless of the numeric values of fd1 +... fdN. File descriptors outside of the range 0..N will not be affected. +This may fail if there are zero unused file descriptors." + ;; If we view each FD as a node, and fd n at index k of FDS as an edge from + ;; fd n to fd k, then we have a rather special type of graph. Because of + ;; how the edges must be specified, it has the property that no node can + ;; have more than one parent, like a tree, but unlike a tree it is possible + ;; to have cycles (combined with the prior restriction, this means a given + ;; node can be part of at most one cycle). I don't know of a good name for + ;; that kind of graph - maybe "rootless tree"? Anyway, our approach is to, + ;; for each unique FD in FDS, do a traversal that both finds the cyclic + ;; path, if it exists, and sets every FD that isn't part of the cycle, then + ;; finally resolve the cycle using a temporary fd. + + (define fds (map (lambda (x) + (if (port? x) (fileno x) x)) + ports-or-fds)) + (define max-fd (apply max 0 fds)) + (define fd->targets + (let ((vec (make-vector (+ 1 max-fd) '()))) + (for-each (lambda (source-fd dest-fd) + (vector-set! vec source-fd + (cons dest-fd + (vector-ref vec source-fd)))) + fds + (iota (length fds) base)) + vec)) + (define visited (make-vector (+ 1 max-fd) #f)) + + (define (rotate-fds! fds) + ;; (fdval1 fdval2 fdval3) --> (fdval3 fdval1 fdval2) + (match (reverse fds) + ((fd0) + ;; Clear close-on-exec flag as if it were dup2'ed. + (fcntl fd0 F_SETFD 0)) + ((fd0 . (and rest (fd1 . _))) + (let ((tmp-fd (dup->fdes fd0))) + (let loop ((fds rest) + (prev fd0)) + (match fds + ((fd . rest) + (dup2 fd prev) + (loop rest fd)) + (() + (dup2 tmp-fd prev) + (close-fdes tmp-fd)))))))) + + (define (top-visit fd) + (let ((cycle (visit fd fd))) + (when cycle + (rotate-fds! cycle)))) + + (define (visit fd cycle-start-fd) + (if (vector-ref visited fd) + #f + (begin + (vector-set! visited fd #t) + (let loop ((targets (vector-ref fd->targets fd)) + (cycle-tail #f)) + (match targets + ((target . rest) + (cond + ((= target cycle-start-fd) + (loop rest (list fd))) + ((> target max-fd) ;; Has no targets, no need to visit. + (dup2 fd target) + (loop rest cycle-tail)) + (else + (let ((maybe-cycle-tail (visit target cycle-start-fd))) + (if maybe-cycle-tail + (loop rest (cons fd maybe-cycle-tail)) + (begin + (dup2 fd target) + (loop rest cycle-tail))))))) + (() + cycle-tail)))))) + + (for-each top-visit fds)) + (define* (exec-command command #:key (user #f) @@ -1479,48 +1561,38 @@ false." (chdir directory) (environ environment-variables) - ;; Close all the file descriptors except stdout and stderr. - (let ((max-fd (max-file-descriptors))) + ;; Redirect stdin. + (catch-system-error (close-fdes 0)) + ;; Make sure file descriptor zero is used, so we don't end up reusing + ;; it for something unrelated, which can confuse some packages. + (dup2 (if input-port + (fileno input-port) + (open-fdes "/dev/null" O_RDONLY)) + 0) - ;; Redirect stdin. - (catch-system-error (close-fdes 0)) - ;; Make sure file descriptor zero is used, so we don't end up reusing - ;; it for something unrelated, which can confuse some packages. - (dup2 (if input-port - (fileno input-port) - (open-fdes "/dev/null" O_RDONLY)) - 0) + (when (or log-port log-file) + (catch #t + (lambda () + ;; Redirect stdout and stderr to use LOG-FILE. + (catch-system-error (close-fdes 1)) + (catch-system-error (close-fdes 2)) + (dup2 (if log-file + (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) + #o640) + (fileno log-port)) + 1) + (dup2 1 2) + + ;; Make EXTRA-PORTS available starting from file descriptor 3. + ;; This clears their FD_CLOEXEC flag. + (reconfigure-fds extra-ports 3)) - (when (or log-port log-file) - (catch #t - (lambda () - ;; Redirect stout and stderr to use LOG-FILE. - (catch-system-error (close-fdes 1)) - (catch-system-error (close-fdes 2)) - (dup2 (if log-file - (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) - #o640) - (fileno log-port)) - 1) - (dup2 1 2) - - ;; Make EXTRA-PORTS available starting from file descriptor 3. - ;; This clears their FD_CLOEXEC flag. - (let loop ((fd 3) - (ports extra-ports)) - (match ports - (() #t) - ((port rest ...) - (catch-system-error (close-fdes fd)) - (dup2 (fileno port) fd) - (loop (+ 1 fd) rest))))) - - (lambda (key . args) - (when log-file - (format (current-error-port) - "failed to open log-file ~s:~%" log-file)) - (print-exception (current-error-port) #f key args) - (primitive-exit 1)))) + (lambda (key . args) + (when log-file + (format (current-error-port) + "failed to open log-file ~s:~%" log-file)) + (print-exception (current-error-port) #f key args) + (primitive-exit 1)))) ;; setgid must be done *before* setuid, otherwise the user will ;; likely no longer have permissions to setgid. @@ -1558,7 +1630,7 @@ false." (format (current-error-port) "exec of ~s failed: ~a~%" program (strerror (system-error-errno args))) - (primitive-exit 1))))))) + (primitive-exit 1)))))) (define %precious-signals ;; Signals that the shepherd process handles. -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:23:20 2023 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:23:20 +0000 Received: from localhost ([127.0.0.1]:48887 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX608-0006dS-7W for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:20 -0400 Received: from tilde.club ([142.44.150.184]:37562 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX606-0006dJ-4p for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:18 -0400 Received: by tilde.club (Postfix, from userid 5378) id 2F3C52250B9BD; Fri, 18 Aug 2023 20:23:17 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 2F3C52250B9BD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390197; bh=15inucqCXJOM3Zr0PIMqzeh5NP35V+VYnLYgi8mrMqA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=IsnCzQHmKotYUuuvPzqbqA7H4YPN/Eooh9vCabhWelBO2cjHE6WiTsabq8o+nR4cc NlbtDmN3lX2BGpzpSWJgvVfsVovMjiiwhzHnughhzJj050ITh/cVuxxU0v5Oq8sBJI u4D24/9dqcPRATOVLIPkO5sd5C72SAfUdqDkVfZM= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 3/6] Makefile.am: enable extra-ports.sh test. Date: Fri, 18 Aug 2023 15:22:36 -0500 Message-Id: <20230818202239.21177-3-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 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) * Makefile.am (TESTS): add tests/extra-ports.sh --- Makefile.am | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index fdfcf3d..b2ce46b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -271,7 +271,8 @@ TESTS = \ tests/daemonize.sh \ tests/eval-load.sh \ tests/services/monitoring.sh \ - tests/services/repl.sh + tests/services/repl.sh \ + tests/extra-ports.sh TEST_EXTENSIONS = .sh EXTRA_DIST += $(TESTS) -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:23:28 2023 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:23:28 +0000 Received: from localhost ([127.0.0.1]:48890 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX60G-0006dm-I6 for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:28 -0400 Received: from tilde.club ([2607:5300:204:4340::114]:42482 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX60F-0006df-BH for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:27 -0400 Received: by tilde.club (Postfix, from userid 5378) id 5BAD12250B9BD; Fri, 18 Aug 2023 20:23:26 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club 5BAD12250B9BD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390206; bh=3rHs921OPchPGGiUp/5LSeroI6rTTXbVTIfdaf0X9S4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Oc3Fb9fvKg16A5ha2ggqgbRV4F+llMgXBck6nXxpOiwcIa104tVZINo5qE67b4at8 TRUn9btiFuYDCrU/HjN9lDymxqTuAyZOxtivvtsAJW+D60c5oTxO3pF3BhNvh0Dmib eN2pg8nlnJvjvImrJC3VWR96jVe4Knmnrvq7W280= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 4/6] service: honor EXTRA-PORTS regardless of log-port and log-file. Date: Fri, 18 Aug 2023 15:22:37 -0500 Message-Id: <20230818202239.21177-4-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 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) EXTRA-PORTS is only honored when either LOG-PORT or LOG-FILE is passed. I have no idea why this is the case, it isn't documented anywhere, and it isn't intuitive. * modules/shepherd/service.scm (exec-command): Move preserve-ports call outside of condition. --- modules/shepherd/service.scm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm index 68993ac..e816cd1 100644 --- a/modules/shepherd/service.scm +++ b/modules/shepherd/service.scm @@ -1581,11 +1581,7 @@ false." #o640) (fileno log-port)) 1) - (dup2 1 2) - - ;; Make EXTRA-PORTS available starting from file descriptor 3. - ;; This clears their FD_CLOEXEC flag. - (reconfigure-fds extra-ports 3)) + (dup2 1 2)) (lambda (key . args) (when log-file @@ -1594,6 +1590,10 @@ false." (print-exception (current-error-port) #f key args) (primitive-exit 1)))) + ;; Make EXTRA-PORTS available starting from file descriptor 3. + ;; This clears their FD_CLOEXEC flag. + (reconfigure-fds extra-ports 3) + ;; setgid must be done *before* setuid, otherwise the user will ;; likely no longer have permissions to setgid. (when group -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:23:39 2023 Received: (at 65221) by debbugs.gnu.org; 18 Aug 2023 20:23:39 +0000 Received: from localhost ([127.0.0.1]:48893 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX60Q-0006eA-TF for submit@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:39 -0400 Received: from tilde.club ([2607:5300:204:4340::114]:43580 ident=postfix) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qX60O-0006e2-MI for 65221@debbugs.gnu.org; Fri, 18 Aug 2023 16:23:37 -0400 Received: by tilde.club (Postfix, from userid 5378) id AD3822250B9BD; Fri, 18 Aug 2023 20:23:35 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 tilde.club AD3822250B9BD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tilde.club; s=mail; t=1692390215; bh=lh+5H7R4WVDYzZjyh6yO9P0SCUKXn1IQbukNEpklb18=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=in7GRTY2fnH2Mt9TX87ssXX89bcnGLRY7wOUV1xdvb3hKTYCbl9l6h6YerBvTPu8c c+nFiygjqIT6u0UYNF1n6xhZDujU+GtFCxrNgWM5cEFhkoie/u0bR/XkHymDwrOYv2 z0I7YHaEBgaGkqBcL/SH6zvpw9DaB4PAknGPsHhk= From: ulfvonbelow To: 65221@debbugs.gnu.org Subject: [PATCH 5/6] service: use RECONFIGURE-FDS for redirecting FDs 0-2. Date: Fri, 18 Aug 2023 15:22:38 -0500 Message-Id: <20230818202239.21177-5-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 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) There are currently some corner cases in how EXTRA-PORTS works due to it not managing FDs 0, 1, and 2. Specifically, if one were to include a port in EXTRA-PORTS with FD 0, 1, or 2, it would *not* be preserved, and would instead represent the file that EXEC-COMMAND assigned to that file descriptor. To avoid this, it's necessary to call RECONFIGURE-FDS *before* redirecting the input, but this could clobber LOG-PORT or INPUT-PORT, so it would become necessary to include LOG-PORT and INPUT-PORT in the call to RECONFIGURE-FDS, then do the redirection using the new FD assignment, then close them. This complication can be avoided if we simply let RECONFIGURE-FDS itself do the redirection. This also solves other edge cases, like if LOG-PORT has fileno 0 or 1 (previously passing a LOG-PORT of (current-output-port) would cause an error, as the underlying file descriptor would be closed before dup2 was called to copy it), or if INPUT-PORT has fileno 0. To solve this, we have RECONFIGURE-FDS start the range it copies into at 0 instead of 3. We then explicitly pass the desired standard I/O FDs / ports at the front of the list passed to RECONFIGURE-FDS. We also use O_CLOEXEC for opening /dev/null and the log file so that the file descriptors they are originally opened on don't hang around. * modules/shepherd/service.scm (exec-command): use RECONFIGURE-FDS for redirecting FDs 0, 1, and 2. --- modules/shepherd/service.scm | 62 +++++++++++++++++------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm index e816cd1..3008e31 100644 --- a/modules/shepherd/service.scm +++ b/modules/shepherd/service.scm @@ -1561,38 +1561,36 @@ false." (chdir directory) (environ environment-variables) - ;; Redirect stdin. - (catch-system-error (close-fdes 0)) - ;; Make sure file descriptor zero is used, so we don't end up reusing - ;; it for something unrelated, which can confuse some packages. - (dup2 (if input-port - (fileno input-port) - (open-fdes "/dev/null" O_RDONLY)) - 0) - - (when (or log-port log-file) - (catch #t - (lambda () - ;; Redirect stdout and stderr to use LOG-FILE. - (catch-system-error (close-fdes 1)) - (catch-system-error (close-fdes 2)) - (dup2 (if log-file - (open-fdes log-file (logior O_CREAT O_WRONLY O_APPEND) - #o640) - (fileno log-port)) - 1) - (dup2 1 2)) - - (lambda (key . args) - (when log-file - (format (current-error-port) - "failed to open log-file ~s:~%" log-file)) - (print-exception (current-error-port) #f key args) - (primitive-exit 1)))) - - ;; Make EXTRA-PORTS available starting from file descriptor 3. - ;; This clears their FD_CLOEXEC flag. - (reconfigure-fds extra-ports 3) + (let* ( ;; Make sure file descriptor zero is used, so we don't end up reusing + ;; it for something unrelated, which can confuse some packages. + (stdin (or input-port (open-fdes "/dev/null" + (logior O_RDONLY + O_CLOEXEC)))) + (stdout (catch #t + (lambda () + (or log-port + (and log-file + (open-fdes log-file + (logior O_CREAT O_WRONLY O_APPEND + O_CLOEXEC) + #o640)) + 1)) + (lambda (key . args) + (when log-file + (format (current-error-port) + "failed to open log-file ~s:~%" log-file)) + (print-exception (current-error-port) #f key args) + (primitive-exit 1)))) + (stderr (if (or log-port log-file) + stdout + 2)) + (all-fds (+ 3 (length extra-ports)))) + ;; Make EXTRA-PORTS available starting from file descriptor 3. + ;; This clears their FD_CLOEXEC flag. + (reconfigure-fds (cons* stdin + stdout + stderr + extra-ports))) ;; setgid must be done *before* setuid, otherwise the user will ;; likely no longer have permissions to setgid. -- 2.40.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 18 16:23:48 2023 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 To: 65221@debbugs.gnu.org Subject: [PATCH 6/6] service: exec-command: close other file descriptors by default. 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 Content-Transfer-Encoding: 8bit X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 65221 Cc: ulfvonbelow X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) 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. -- 2.40.1