possible misuse of posix_spawn API on non-linux OSes

  • Done
  • quality assurance status badge
Details
3 participants
  • Josselin Poiret
  • Ludovic Courtès
  • Omar Polo
Owner
unassigned
Submitted by
Omar Polo
Severity
normal
Merged with
O
O
Omar Polo wrote on 27 Jan 2023 12:51
(address . bug-guile@gnu.org)
26OIN3L5D4V9L.2M0KM95K0YSNM@venera
Hello,

I've noticed that test-system-cmds fails on OpenBSD-CURRENT while
testing the update to guile 3.0.9:

test-system-cmds: system* exit status was 127 rather than 42
FAIL: test-system-cmds

Here's an excerpt of the ktrace of the child process while executing
that specific test: (the first fork() is the one implicitly done by
posix_spawn(3))

5590 guile RET fork 0
[...]
5590 guile CALL dup2(0,3)
5590 guile RET dup2 3
5590 guile CALL dup2(1,4)
5590 guile RET dup2 4
5590 guile CALL dup2(2,5)
5590 guile RET dup2 5
5590 guile CALL dup2(3,0)
5590 guile RET dup2 0
5590 guile CALL dup2(4,1)
5590 guile RET dup2 1
5590 guile CALL dup2(5,2)
5590 guile RET dup2 2
5590 guile CALL close(1023)
5590 guile RET close -1 errno 9 Bad file descriptor
5590 guile CALL kbind(0x7f7ffffd51f8,24,0x2b5c5ced59893fa9)
5590 guile RET kbind 0
5590 guile CALL exit(127)

(if you prefer I can provide a full ktrace of guile executing that
test case)

My interpretation is that the sequence of dup2(2) is from
posix_spawn_file_actions_adddup2 in do_spawn, while the strange
close(1023) is from close_inherited_fds_slow. Such file descriptor is
not open, so close(2) fails with EBADF and the posix_spawn machinery
exits prematurely. My current RLIMIT_NOFILE is 1024, so the number
would make sense.

On OpenBSD I've tried to use the following patch to work around the
issue:

[[[
Index: libguile/posix.c
--- libguile/posix.c.orig
+++ libguile/posix.c
@@ -1325,6 +1325,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
static void
close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
{
+ max_fd = getdtablecount();
while (--max_fd > 2)
posix_spawn_file_actions_addclose (actions, max_fd);
}
]]]

getdtablecount(2) returns the number of file descriptor currently open
by the process. unfortunately it doesn't seem to be portable. (well,
tbf /proc/self/fd is not portable too.)

However, while this pleases the system* test, it breaks the pipe
tests:

Running popen.test
FAIL: popen.test: open-input-pipe: echo hello
FAIL: popen.test: pipeline - arguments: (expected-value ("HELLO WORLD\n" (0 0)) actual-value ("" (127 0)))

the reason seem to be similar:

74865 guile CALL dup2(7,3)
74865 guile RET dup2 3
74865 guile CALL dup2(10,4)
74865 guile RET dup2 4
74865 guile CALL dup2(2,5)
74865 guile RET dup2 5
74865 guile CALL dup2(3,0)
74865 guile RET dup2 0
74865 guile CALL dup2(4,1)
74865 guile RET dup2 1
74865 guile CALL dup2(5,2)
74865 guile RET dup2 2
74865 guile CALL close(8)
74865 guile RET close -1 errno 9 Bad file descriptor
74865 guile CALL kbind(0x7f7ffffcfa88,24,0x2125923bdf2ca9e)
74865 guile RET kbind 0
74865 guile CALL exit(127)

I guess it's trying to close the fd of the pipe that was closed.

I'm not sure what to do from here, I'm not used to the posix_spawn_*
APIs. I'm happy to help testing diffs or by providing more info if
needed.


Thanks,

Omar Polo
O
O
Omar Polo wrote on 27 Jan 2023 13:25
(address . 61095@debbugs.gnu.org)
3F1FNOS0VFO9X.356V67A0RSKPT@venera
Actually I can avoid the EBADF by checking that the fd is 'live' with
something like fstat:

[[[
Index: libguile/posix.c
--- libguile/posix.c.orig
+++ libguile/posix.c
@@ -1325,8 +1325,12 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
static void
close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
{
- while (--max_fd > 2)
- posix_spawn_file_actions_addclose (actions, max_fd);
+ struct stat sb;
+ max_fd = getdtablecount();
+ while (--max_fd > 2) {
+ if (fstat(max_fd, &sb) != -1)
+ posix_spawn_file_actions_addclose (actions, max_fd);
+ }
}
static void

]]]

The regress passes and while this workaround may be temporarly
acceptable I -personally- don't like it much. There's a reason guile
can't set CLOEXEC for all the file descriptors > 2 obtained via open,
socket, pipe, ... like perl -for example- does?
L
L
Ludovic Courtès wrote on 27 Mar 2023 15:32
control message for bug #61095
(address . control@debbugs.gnu.org)
874jq6iak7.fsf@gnu.org
merge 61095 61079
quit
L
L
Ludovic Courtès wrote on 28 Mar 2023 11:34
Re: bug#61095: possible misuse of posix_spawn API on non-linux OSes
(name . Omar Polo)(address . op@omarpolo.com)
87zg7xgqxz.fsf@gnu.org
Hi Omar,

Apologies for the late reply.

Omar Polo <op@omarpolo.com> skribis:

Toggle quote (6 lines)
> I've noticed that test-system-cmds fails on OpenBSD-CURRENT while
> testing the update to guile 3.0.9:
>
> test-system-cmds: system* exit status was 127 rather than 42
> FAIL: test-system-cmds

We’re seeing the same failure on GNU/Hurd:


Toggle quote (22 lines)
> Actually I can avoid the EBADF by checking that the fd is 'live' with
> something like fstat:
>
> [[[
>
> Index: libguile/posix.c
> --- libguile/posix.c.orig
> +++ libguile/posix.c
> @@ -1325,8 +1325,12 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
> static void
> close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
> {
> - while (--max_fd > 2)
> - posix_spawn_file_actions_addclose (actions, max_fd);
> + struct stat sb;
> + max_fd = getdtablecount();
> + while (--max_fd > 2) {
> + if (fstat(max_fd, &sb) != -1)
> + posix_spawn_file_actions_addclose (actions, max_fd);
> + }
> }

I came up with the following patch:
Toggle diff (111 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3a8be94e4..cde199888 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1322,39 +1322,18 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-static void
-close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
-{
- while (--max_fd > 2)
- posix_spawn_file_actions_addclose (actions, max_fd);
-}
-
static void
close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
{
- DIR *dirp;
- struct dirent *d;
- int fd;
-
- /* Try to use the platform-specific list of open file descriptors, so
- we don't need to use the brute force approach. */
- dirp = opendir ("/proc/self/fd");
-
- if (dirp == NULL)
- return close_inherited_fds_slow (actions, max_fd);
-
- while ((d = readdir (dirp)) != NULL)
+ while (--max_fd > 2)
{
- fd = atoi (d->d_name);
-
- /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */
- if (fd <= 2)
- continue;
-
- posix_spawn_file_actions_addclose (actions, fd);
+ /* Adding invalid file descriptors to an 'addclose' action leads
+ to 'posix_spawn' failures on some operating systems:
+ <https://bugs.gnu.org/61095>. Hence the extra check. */
+ int flags = fcntl (max_fd, F_GETFD, NULL);
+ if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
+ posix_spawn_file_actions_addclose (actions, max_fd);
}
-
- closedir (dirp);
}
static pid_t
@@ -1366,6 +1345,26 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
posix_spawn_file_actions_t actions;
posix_spawnattr_t *attrp = NULL;
+ posix_spawn_file_actions_init (&actions);
+
+ /* Duplicate IN, OUT, and ERR unconditionally to clear their
+ FD_CLOEXEC flag, if any. */
+ posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILENO);
+ posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILENO);
+ posix_spawn_file_actions_adddup2 (&actions, err, STDERR_FILENO);
+
+ /* TODO: Use 'closefrom' where available. */
+#if 0
+ /* Version 2.34 of the GNU libc provides this function. */
+ posix_spawn_file_actions_addclosefrom_np (&actions, 3);
+#else
+ if (in > 2)
+ posix_spawn_file_actions_addclose (&actions, in);
+ if (out > 2 && out != in)
+ posix_spawn_file_actions_addclose (&actions, out);
+ if (err > 2 && err != out && err != in)
+ posix_spawn_file_actions_addclose (&actions, err);
+
int max_fd = 1024;
#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
@@ -1376,31 +1375,8 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
}
#endif
- posix_spawn_file_actions_init (&actions);
-
- int free_fd_slots = 0;
- int fd_slot[3];
-
- for (int fdnum = 3; free_fd_slots < 3 && fdnum < max_fd; fdnum++)
- {
- if (fdnum != in && fdnum != out && fdnum != err)
- {
- fd_slot[free_fd_slots] = fdnum;
- free_fd_slots++;
- }
- }
-
- /* Move the fds out of the way, so that duplicate fds or fds equal
- to 0, 1, 2 don't trample each other */
-
- posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]);
- posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]);
- posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]);
- posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0);
- posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
- posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
-
close_inherited_fds (&actions, max_fd);
+#endif
int res = -1;
if (spawnp)
Could you confirm that it works on OpenBSD and that there’s no
performance regression?

Andrew: it removes the /proc/self/fd loop you added to fix
https://bugs.gnu.org/59321, but it reduces the number of ‘close’ calls
in the child. Could you check whether that’s okay performance-wise?

Eventually I plan to use ‘posix_spawn_file_actions_addclosefrom_np’ on
glibc >= 2.34, but I have yet to test it. That will be the best
solution.

Josselin: I simplified the ‘dup2’ logic somewhat.

Feedback welcome!

Toggle quote (5 lines)
> The regress passes and while this workaround may be temporarly
> acceptable I -personally- don't like it much. There's a reason guile
> can't set CLOEXEC for all the file descriptors > 2 obtained via open,
> socket, pipe, ... like perl -for example- does?

Guile does that for file descriptors it opens internally, but
applications using ‘open-file’ without the recently-added “e” flag, or
‘socket’ without ‘SOCK_CLOEXEC’, etc., end up with more file descriptors
that need to be taken care of.

I wish the default were close-on-exec, but we’re not there yet.

Thanks,
Ludo’.
J
J
Josselin Poiret wrote on 28 Mar 2023 18:10
87tty4svpo.fsf@jpoiret.xyz
Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (9 lines)
> - posix_spawn_file_actions_addclose (actions, fd);
> + /* Adding invalid file descriptors to an 'addclose' action leads
> + to 'posix_spawn' failures on some operating systems:
> + <https://bugs.gnu.org/61095>. Hence the extra check. */
> + int flags = fcntl (max_fd, F_GETFD, NULL);
> + if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
> + posix_spawn_file_actions_addclose (actions, max_fd);
> }

I'm worried about TOCTOU in multi-threaded contexts here, which is why I
opted for the heavy handed-approach here. In general I don't think we
can know in advance which fdes to close :/ It's a shame that the
posix_spawn actions fails on other kernels, since I don't really see a
way to mitigate this problem (apart from the new
posix_spawn_file_actions_addclosefrom_np as you mentioned). I don't
know what we could do here. Maybe not provide spawn? Or provide it in
spite of the broken fd closing?

Toggle quote (17 lines)
> -
> - closedir (dirp);
> }
>
> static pid_t
> @@ -1366,6 +1345,26 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
> posix_spawn_file_actions_t actions;
> posix_spawnattr_t *attrp = NULL;
>
> + posix_spawn_file_actions_init (&actions);
> +
> + /* Duplicate IN, OUT, and ERR unconditionally to clear their
> + FD_CLOEXEC flag, if any. */
> + posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILENO);
> + posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILENO);
> + posix_spawn_file_actions_adddup2 (&actions, err, STDERR_FILENO);

This won't work, and actually this was one of the original logic bugs I
was trying to fix. If err is equal to, let's say, STDIN_FILENO, then
the first call will overwrite the initial file descriptor at
STDIN_FILENO, and the second call won't do what the caller intended.
This is why I was moving them out of the way first, so that they would
not overwrite each other.

Toggle quote (12 lines)
> + /* TODO: Use 'closefrom' where available. */
> +#if 0
> + /* Version 2.34 of the GNU libc provides this function. */
> + posix_spawn_file_actions_addclosefrom_np (&actions, 3);
> +#else
> + if (in > 2)
> + posix_spawn_file_actions_addclose (&actions, in);
> + if (out > 2 && out != in)
> + posix_spawn_file_actions_addclose (&actions, out);
> + if (err > 2 && err != out && err != in)
> + posix_spawn_file_actions_addclose (&actions, err);

Isn't this unneeded given we call close_inherited_fds below?

Toggle quote (4 lines)
> [...]
>
> Josselin: I simplified the ‘dup2’ logic somewhat.

See my comments above.

Toggle quote (7 lines)
> Guile does that for file descriptors it opens internally, but
> applications using ‘open-file’ without the recently-added “e” flag, or
> ‘socket’ without ‘SOCK_CLOEXEC’, etc., end up with more file descriptors
> that need to be taken care of.
>
> I wish the default were close-on-exec, but we’re not there yet.

+1

Best,
--
Josselin Poiret
-----BEGIN PGP SIGNATURE-----

iQHEBAEBCAAuFiEEOSSM2EHGPMM23K8vUF5AuRYXGooFAmQjEXMQHGRldkBqcG9p
cmV0Lnh5egAKCRBQXkC5Fhcail3ZC/4q3n0W2AOoRdNaXOzROGyxrfTd7dz3Ehx/
xEko14Ury/kPiV+x8MxIQXZ2Vo9m8LXTbuhhiLzeRNwS+fBwwnZQbEDDE4GvDkLd
Vwh6IRmxKk3A5PxVqaX3EJypcwCABxMHzj1et3I/393na3UbPB/nVJaLAs27L/al
XrFwd6k1aUGCPNYMPOVkulJXx6NNrOVcV1fSm23VlwpQzeeRUqVQBWWLPWi7MJ4n
lnH+iwbLOAPIgywltXrCkf4rsook8KEBGWvQXSCgAqlCycdNIiBid7bTNaoitbeW
HX5LP314uiMjSktOVVQrK+Q1+PhLJXUSGPxXRrxDTZf6/fXYAF4OrR343KjwxHJQ
KWwC2yCENFkyHlh3RKkXkfmH41ezkaHQu2o64uD/1QcpFyv7Vx5Rde41pTPSEd8M
3umstB4vWNb2bzuBczeNXaoTFpAGKZHwXkGjfCWCZVEmyGBnbRm16VWxyPHtlvAp
WizmvpOzWpM63lTajCdzwlPnckxPlfM=
=pbCP
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 30 Mar 2023 00:30
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87zg7vjimr.fsf@inria.fr
Hi!

Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (13 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> - posix_spawn_file_actions_addclose (actions, fd);
>> + /* Adding invalid file descriptors to an 'addclose' action leads
>> + to 'posix_spawn' failures on some operating systems:
>> + <https://bugs.gnu.org/61095>. Hence the extra check. */
>> + int flags = fcntl (max_fd, F_GETFD, NULL);
>> + if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
>> + posix_spawn_file_actions_addclose (actions, max_fd);
>> }
>
> I'm worried about TOCTOU in multi-threaded contexts here,

Yes, that’s a problem. The current /proc/self/fd optimization has that
problem too. :-/

Toggle quote (8 lines)
> which is why I opted for the heavy handed-approach here. In general I
> don't think we can know in advance which fdes to close :/ It's a shame
> that the posix_spawn actions fails on other kernels, since I don't
> really see a way to mitigate this problem (apart from the new
> posix_spawn_file_actions_addclosefrom_np as you mentioned). I don't
> know what we could do here. Maybe not provide spawn? Or provide it
> in spite of the broken fd closing?

Not providing ‘spawn’ is no longer an option.

We can expect the problem to practically vanish “soon” on GNU variants
with ‘closefrom’ (glibc 2.34 was released in Aug. 2021).

On Linux with glibc < 2.34, we’d keep the current code (maybe without
the /proc/self/fd optimization?).

On other systems, we can have the racy code above as a last resort or
OS-specific tricks, like Omar was suggesting for OpenBSD. It sucks but
what else can we do?

(BTW,
reads:

It shall not be considered an error for the fildes argument passed to
these functions to specify a file descriptor for which the specified
operation could not be performed at the time of the call. Any such
error will be detected when the associated file actions object is
later used during a posix_spawn() or posix_spawnp() operation.

OpenBSD and GNU/Hurd follow this to the letter.

OTOH ‘linux/spawni.c’ in glibc is purposefully more liberal:

/* Signal errors only for file descriptors out of range. */
)

Toggle quote (13 lines)
>> + /* Duplicate IN, OUT, and ERR unconditionally to clear their
>> + FD_CLOEXEC flag, if any. */
>> + posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILENO);
>> + posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILENO);
>> + posix_spawn_file_actions_adddup2 (&actions, err, STDERR_FILENO);
>
> This won't work, and actually this was one of the original logic bugs I
> was trying to fix. If err is equal to, let's say, STDIN_FILENO, then
> the first call will overwrite the initial file descriptor at
> STDIN_FILENO, and the second call won't do what the caller intended.
> This is why I was moving them out of the way first, so that they would
> not overwrite each other.

Oh, my bad.

Toggle quote (14 lines)
>> + /* TODO: Use 'closefrom' where available. */
>> +#if 0
>> + /* Version 2.34 of the GNU libc provides this function. */
>> + posix_spawn_file_actions_addclosefrom_np (&actions, 3);
>> +#else
>> + if (in > 2)
>> + posix_spawn_file_actions_addclose (&actions, in);
>> + if (out > 2 && out != in)
>> + posix_spawn_file_actions_addclose (&actions, out);
>> + if (err > 2 && err != out && err != in)
>> + posix_spawn_file_actions_addclose (&actions, err);
>
> Isn't this unneeded given we call close_inherited_fds below?

No, because of the FD_CLOEXEC selection.

Coming next is an updated patch series addressing this as proposed
above. Let me know what y’all think!

I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building in:

guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm

… which gives us glibc 2.35.

Ludo’.
L
L
Ludovic Courtès wrote on 30 Mar 2023 00:30
[PATCH 1/3] 'spawn' closes only open file descriptors on non-GNU/Linux systems.
(address . 61095@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20230329223057.28100-1-ludo@gnu.org
Reported by Omar Polo <op@omarpolo.com>.

* libguile/posix.c (close_inherited_fds_slow): On systems other than
GNU/Linux, call 'addclose' only when 'fcntl' succeeds on MAX_FD.
---
libguile/posix.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

Toggle diff (34 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3a8be94e4..68e9bfade 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1326,7 +1326,24 @@ static void
close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
{
while (--max_fd > 2)
- posix_spawn_file_actions_addclose (actions, max_fd);
+ {
+ /* Adding a 'close' action for a file descriptor that is not open
+ causes 'posix_spawn' to fail on GNU/Hurd and on OpenBSD, but
+ not on GNU/Linux: <https://bugs.gnu.org/61095>. Hence this
+ strategy:
+
+ - On GNU/Linux, close every FD, since that's the only
+ race-free way to make sure the child doesn't inherit one.
+ - On other systems, only close FDs currently open in the
+ parent; it works, but it's racy (XXX).
+
+ The only reliable option is 'addclosefrom'. */
+#if ! (defined __GLIBC__ && defined __linux__)
+ int flags = fcntl (max_fd, F_GETFD, NULL);
+ if (flags >= 0)
+#endif
+ posix_spawn_file_actions_addclose (actions, max_fd);
+ }
}
static void

base-commit: e334e59589c3cbfc68d3f7d0d739000e0876b36d
--
2.39.2
L
L
Ludovic Courtès wrote on 30 Mar 2023 00:30
[PATCH 2/3] Remove racy optimized file descriptor closing loop in 'spawn'.
(address . 61095@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20230329223057.28100-2-ludo@gnu.org
This reverts 9332b632407894c2e1951cce1bc678f19e1fa8e4, thereby
reinstating the performance issue in https://bugs.gnu.org/59321.

This optimization was subject to race conditions in multi-threaded code:
new file descriptors could pop up at any time and thus leak in the
child.

* libguile/posix.c (close_inherited_fds): Remove.
(close_inherited_fds_slow): Rename to...
(close_inherited_fds): ... this.
---
libguile/posix.c | 30 +-----------------------------
1 file changed, 1 insertion(+), 29 deletions(-)

Toggle diff (50 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 68e9bfade..b5830c43b 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1323,7 +1323,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#endif /* HAVE_FORK */
static void
-close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
+close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
{
while (--max_fd > 2)
{
@@ -1346,34 +1346,6 @@ close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
}
}
-static void
-close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
-{
- DIR *dirp;
- struct dirent *d;
- int fd;
-
- /* Try to use the platform-specific list of open file descriptors, so
- we don't need to use the brute force approach. */
- dirp = opendir ("/proc/self/fd");
-
- if (dirp == NULL)
- return close_inherited_fds_slow (actions, max_fd);
-
- while ((d = readdir (dirp)) != NULL)
- {
- fd = atoi (d->d_name);
-
- /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */
- if (fd <= 2)
- continue;
-
- posix_spawn_file_actions_addclose (actions, fd);
- }
-
- closedir (dirp);
-}
-
static pid_t
do_spawn (char *exec_file, char **exec_argv, char **exec_env,
int in, int out, int err, int spawnp)
--
2.39.2
L
L
Ludovic Courtès wrote on 30 Mar 2023 00:30
[PATCH 3/3] Use 'posix_spawn_file_actions_addclosefrom_np' where available.
(address . 61095@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20230329223057.28100-3-ludo@gnu.org
* configure.ac: Check for 'posix_spawn_file_actions_addclosefrom_np'.
* libguile/posix.c (HAVE_ADDCLOSEFROM): New macro.
(close_inherited_fds): Wrap in #ifdef HAVE_ADDCLOSEFROM.
(do_spawn) [HAVE_ADDCLOSEFROM]: Use 'posix_spawn_file_actions_addclosefrom_np'.
---
configure.ac | 4 +++-
libguile/posix.c | 14 ++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)

Toggle diff (64 lines)
diff --git a/configure.ac b/configure.ac
index d5ce1c4ac..4a93be979 100644
--- a/configure.ac
+++ b/configure.ac
@@ -515,6 +515,7 @@ AC_CHECK_HEADERS([crt_externs.h])
# sched_getaffinity, sched_setaffinity - GNU extensions (glibc)
# sendfile - non-POSIX, found in glibc
# pipe2 - non-POSIX, found in glibc (GNU/Linux and GNU/Hurd)
+# posix_spawn_file_actions_addclosefrom_np - glibc >= 2.34
#
AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid \
fesetround ftime ftruncate fchown fchownat fchmod fchdir readlinkat \
@@ -528,7 +529,8 @@ AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid \
index bcopy rindex truncate isblank _NSGetEnviron \
strcoll_l strtod_l strtol_l newlocale uselocale utimensat \
fstatat futimens openat \
- sched_getaffinity sched_setaffinity sendfile pipe2])
+ sched_getaffinity sched_setaffinity sendfile pipe2
+ posix_spawn_file_actions_addclosefrom_np])
# The newlib C library uses _NL_ prefixed locale langinfo constants.
AC_CHECK_DECLS([_NL_NUMERIC_GROUPING], [], [], [[#include <langinfo.h>]])
diff --git a/libguile/posix.c b/libguile/posix.c
index b5830c43b..3adc743c4 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1322,6 +1322,12 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
+#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP
+# define HAVE_ADDCLOSEFROM 1
+#endif
+
+#ifndef HAVE_ADDCLOSEFROM
+
static void
close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
{
@@ -1346,6 +1352,8 @@ close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
}
}
+#endif
+
static pid_t
do_spawn (char *exec_file, char **exec_argv, char **exec_env,
int in, int out, int err, int spawnp)
@@ -1389,7 +1397,13 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
+#ifdef HAVE_ADDCLOSEFROM
+ /* This function appears in glibc 2.34. It's both free from race
+ conditions and more efficient than the alternative. */
+ posix_spawn_file_actions_addclosefrom_np (&actions, 3);
+#else
close_inherited_fds (&actions, max_fd);
+#endif
int res = -1;
if (spawnp)
--
2.39.2
L
L
Ludovic Courtès wrote on 30 Mar 2023 00:33
control message for bug #61095
(address . control@debbugs.gnu.org)
87v8ijjige.fsf@gnu.org
tags 61095 + patch
quit
J
J
Josselin Poiret wrote on 30 Mar 2023 22:21
Re: bug#61095: possible misuse of posix_spawn API on non-linux OSes
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ileirnw7.fsf@jpoiret.xyz
Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (7 lines)
> Coming next is an updated patch series addressing this as proposed
> above. Let me know what y’all think!
>
> I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building in:
>
> guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm

I didn't test, but this LGTM! Maybe someone on OpenBSD could test this
patchset?

Best,
--
Josselin Poiret
-----BEGIN PGP SIGNATURE-----

iQHEBAEBCAAuFiEEOSSM2EHGPMM23K8vUF5AuRYXGooFAmQl70gQHGRldkBqcG9p
cmV0Lnh5egAKCRBQXkC5Fhcaika2C/4vZu9E2Yhc94DQV0ptmwlmFlAWlwSdvvbH
+PPbHQwrY0rq5G1MqoxeegqVR2RU2o/mtP5edUcLlz7f6pbU8jPuPwDGAIaM5jLU
gxrWXyadvbMk6HjaGm6T4lXhFvt4OT2dl8DoEPHzd+eqX/Tv9Xnx2j3kynMsY389
+RqBhQcLtH+Qk6Z9qvEYRXUrBYQhR7OVoKpu3Vg8tM6T50a7bgc9Xtnnxai/aqQW
J3cJahq6oi1fkUaNiOAWsS7IN6XLJm01QZekkHdnAd9dJi1zJUdyP0/qIBjkXYXr
ge3AYL3kxDWIus6zO3x8aWm4ZM57G0dw++VKU/qFora1BShSQCwg5J7496T5Q3fc
sQ+zJzC27UnptMoTFZP2M3gJzNV/FH89KSVNSBGIe+8KVJY9INobr9fGNzUyR2mc
bDaNvn9d9rRaRsvqpdAhaKGMtb1IwA06tWl262beYs9CDO7Lnx4XSEt4xyCeFpwg
oU4RlegH8ChQtzf2O7J99CSjmcN7liU=
=xCrc
-----END PGP SIGNATURE-----

O
O
Omar Polo wrote on 31 Mar 2023 19:45
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
2KFIQJHFVA0GP.2GKMWUIGIVCMU@venera
On 2023/03/30 22:21:28 +0200, Josselin Poiret <dev@jpoiret.xyz> wrote:
Toggle quote (14 lines)
> Hi Ludo,
>
> Ludovic Courtès <ludo@gnu.org> writes:
>
> > Coming next is an updated patch series addressing this as proposed
> > above. Let me know what y’all think!
> >
> > I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building in:
> >
> > guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm
>
> I didn't test, but this LGTM! Maybe someone on OpenBSD could test this
> patchset?

% gmake check
<snip />
gmake[5]: Entering directory '/home/op/w/guile/test-suite/standalone'
PASS: test-system-cmds

it seems to work on OpenBSD 7.3 :)

but note that our libc doesn't have posix_spawn_file_actions_addclosefrom_np,
so this is using the "racy" code path.

Just for curiosity, as it's outside the scope of the bug, what's the
reason posix_spawn was used instead of a more classic fork() +
closefrom()?


Thanks,

Omar Polo
L
L
Ludovic Courtès wrote on 2 Apr 2023 15:44
(name . Omar Polo)(address . op@omarpolo.com)
87zg7qtn4u.fsf@gnu.org
Hi!

Omar Polo <op@omarpolo.com> skribis:

Toggle quote (22 lines)
> On 2023/03/30 22:21:28 +0200, Josselin Poiret <dev@jpoiret.xyz> wrote:
>> Hi Ludo,
>>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>> > Coming next is an updated patch series addressing this as proposed
>> > above. Let me know what y’all think!
>> >
>> > I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building in:
>> >
>> > guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm
>>
>> I didn't test, but this LGTM! Maybe someone on OpenBSD could test this
>> patchset?
>
> % gmake check
> <snip />
> gmake[5]: Entering directory '/home/op/w/guile/test-suite/standalone'
> PASS: test-system-cmds
>
> it seems to work on OpenBSD 7.3 :)

Awesome! Pushed as 9cc85a4f52147fcdaa4c52a62bcc87bdb267d0a9.

Toggle quote (3 lines)
> but note that our libc doesn't have posix_spawn_file_actions_addclosefrom_np,
> so this is using the "racy" code path.

Yeah, not great. :-/ I hope that function will be adopted by other
libcs, especially since ‘closefrom’ is already available.

Toggle quote (4 lines)
> Just for curiosity, as it's outside the scope of the bug, what's the
> reason posix_spawn was used instead of a more classic fork() +
> closefrom()?

There’s a long discussion at:


Essentially, ‘fork’ is unusable in multi-threaded context, in addition
to being inefficient.

Thanks,
Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 61095@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 61095
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch