From: ulfvonbelow <striness@tilde.club>
To: 65221@debbugs.gnu.org
Cc: ulfvonbelow <striness@tilde.club>
Subject: [bug#65221] [PATCH 6/6] service: exec-command: close other file descriptors by default.
Date: Fri, 18 Aug 2023 15:22:39 -0500 [thread overview]
Message-ID: <20230818202239.21177-6-striness@tilde.club> (raw)
In-Reply-To: <20230818202239.21177-1-striness@tilde.club>
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
prev parent reply other threads:[~2023-08-18 20:24 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-11 9:03 [bug#65221] [PATCH 0/2] Fix EXTRA-PORTS edge cases ulfvonbelow
2023-08-11 9:06 ` [bug#65221] [PATCH 1/2] service: make EXTRA-PORTS work as advertised ulfvonbelow
2023-08-11 9:06 ` [bug#65221] [PATCH 2/2] service: use PRESERVE-PORTS for redirecting FDs 0-2 ulfvonbelow
2023-08-15 10:55 ` [bug#65221] [PATCH 0/2] Fix EXTRA-PORTS edge cases Ludovic Courtès
2023-08-18 20:21 ` Ulf Herrman
2023-08-18 20:22 ` [bug#65221] [PATCH 1/6] tests: add extra-ports.sh test ulfvonbelow
2023-08-18 20:22 ` [bug#65221] [PATCH 2/6] service: don't let earlier ports clobber later ones in EXTRA-PORTS ulfvonbelow
2023-08-18 20:22 ` [bug#65221] [PATCH 3/6] Makefile.am: enable extra-ports.sh test ulfvonbelow
2023-08-18 20:22 ` [bug#65221] [PATCH 4/6] service: honor EXTRA-PORTS regardless of log-port and log-file ulfvonbelow
2023-08-18 20:22 ` [bug#65221] [PATCH 5/6] service: use RECONFIGURE-FDS for redirecting FDs 0-2 ulfvonbelow
2023-08-18 20:22 ` ulfvonbelow [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230818202239.21177-6-striness@tilde.club \
--to=striness@tilde.club \
--cc=65221@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).