[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20260727193024.GE27423@brightrain.aerifal.cx>
Date: Mon, 27 Jul 2026 15:30:27 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: popen issue analysis
I've diagnosed the popen issue discussed here:
https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/105685#note_633409
(Test case with fixes to remove C23 dependency attached as popen2.c)
The problem was created by the fix in e1a51185ceb4 to satisfy the
obscure historical requirement than popen not leak the pipe fds of
other popened FILEs to the child, even if they're not close-on-exec.
Because the open file list lock does not prevent closing the fd, only
removing the FILE from the list, it's possible that the fd we add for
closing to the file actions for the posix_spawn child has already been
closed in the parent.
I originally attempted to fix this by having popen take each FILE lock
while scanning the open file list for pipes and having __stdio_close
clear the fd when closing it (attached as popen-fix-try1.diff) and
this makes the test case stop failing, but it only catches the case
where the pclose happened early. It cannot catch a pclose that races
between the posix_spawn_file_actions_addclose in the parent and the
__clone that copies the file descriptor table to the posix_spawn
child.
At this point there are basically two possible fixes I see, plus a
sort of hybrid between them that could be considered a third choice.
Choice 1: The Big Hammer:
fclose currently performs the actual flush-and-close without holding
the open file list lock, before removing the FILE from the open file
list. This is to avoid a blocking flush or close operation serializing
fclose operations across the whole process. The big hammer would be
doing away with that, and closing the fd under lock.
Note that if both locks are to be held, the open file list lock must
be taken first by protocol to prevent deadlock (as fixed in
4a086030264f5cf423ea76453ef721e2c8e2e093) with fflush(0)/exit.
So if we wanted to keep the almost-surely-costly flush outside of the
open file list critical zone, we would have to first lock and unlock
to flush, then take the ofl lock. We don't actually need to lock
again, after this, since the only purpose of locking the FILE to begin
with was to block as long as any other thread may still be holding an
explicit flockfile lock. But this still leaves the close in the
critical section. Morally, close should be an instantaneous operation,
but badly behaved NFS implementations, weird devices like tape drives,
etc. can block for a long time on it, so it's somewhat unsatisfying to
serialize around it.
Choice 2: Finer-grained popen list:
Right now, popen theoretically wastes a lot of time scanning the
entire open file list for pipes that need to be closed in the child,
when in practice few if any other FILEs are actually popen pipes. We
could swap that time cost for a small space cost by putting extra
linked list fields in the FILE structure to track a list of popen
FILEs, and have a custom close method for popen FILEs that locks the
list during close. Then, popen would no longer need to touch the open
file list, only the popened-file list. And the lock order protocol
issues go away, except possibly for new considerations at fork time.
This solution is much more invasive. Normally this would make me like
it a lot less, since invasive changes as fixes to bugs make
backporting difficult. However, the bug here has only been present
since 1.2.3, and a patch would presumably apply unmodified to the 4
affected versions.
Hybrid choice:
The close operation is only costly on certain file types where the
underlying kernel close method might do something nontrivial. In
particular, it's always free on pipes.
So, we could have fclose conditionally take the ofl lock early, before
f->close(f), if f->pipe_pid is nonzero, and otherwise wait to take it
until the current point where it's needed.
This does not solve the additional problem choice 2 solves, that of
the gratuitous scan of the ofl for pipes, but it's much less invasive.
Since we can't take the ofl lock with the FILE lock held, we would
have to unlock between the fflush and f->close(f), but then the
FLOCK/FUNLOCK pair can just be removed, since fflush already does its
own locking internally.
I think my leaning is towards the hybrid choice at this time, with the
idea that choice 2 might be a good future direction. This is largely
because I like bug fixes that stick to directly fixing the bug rather
than making larger changes, even if larger changes are desirable; it's
a better factorization of history.
View attachment "popen2.c" of type "text/plain" (622 bytes)
View attachment "popen-fix-try1.diff" of type "text/plain" (1142 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
