diff mbox series

[bug#65221,6/6] service: exec-command: close other file descriptors by default.

Message ID 20230818202239.21177-6-striness@tilde.club
State New
Headers show
Series [bug#65221,1/6] tests: add extra-ports.sh test. | expand

Commit Message

ulfvonbelow Aug. 18, 2023, 8:22 p.m. UTC 
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 mbox series

Patch

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.