possible misuse of posix_spawn API on non-linux OSes

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

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?

merge 61095 61079

quit

Hi Omar,

Apologies for the late reply.

Omar Polo <op@omarpolo.com> skribis:

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

https://issues.guix.gnu.org/61079

I came up with the following patch:

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!

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’.

Hi Ludo,

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

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?

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.

Isn't this unneeded given we call close_inherited_fds below?

See my comments above.

+1

Best,

--

Josselin Poiret

Hi!

Josselin Poiret <dev@jpoiret.xyz> skribis:

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

problem too. :-/

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. */

)

Oh, my bad.

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’.

Fixes https://bugs.gnu.org/61095.

Reported by Omar Polo <op@omarpolo.com>.

GNU/Linux, call 'addclose' only when 'fcntl' succeeds on MAX_FD.

---

libguile/posix.c | 19 ++++++++++++++++++-

1 file changed, 18 insertions(+), 1 deletion(-)

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.

(close_inherited_fds_slow): Rename to...

(close_inherited_fds): ... this.

---

libguile/posix.c | 30 +-----------------------------

1 file changed, 1 insertion(+), 29 deletions(-)

(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(-)

tags 61095 + patch

quit

Hi Ludo,

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

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

patchset?

Best,

--

Josselin Poiret

On 2023/03/30 22:21:28 +0200, Josselin Poiret <dev@jpoiret.xyz> wrote:

% 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

Hi!

Omar Polo <op@omarpolo.com> skribis:

Awesome! Pushed as 9cc85a4f52147fcdaa4c52a62bcc87bdb267d0a9.

Yeah, not great. :-/ I hope that function will be adopted by other

libcs, especially since ‘closefrom’ is already available.

There’s a long discussion at:

https://issues.guix.gnu.org/52835

Essentially, ‘fork’ is unusable in multi-threaded context, in addition

to being inefficient.

Thanks,

Ludo’.