[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20260727194510.GF27423@brightrain.aerifal.cx>
Date: Mon, 27 Jul 2026 15:45:11 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: popen issue analysis
On Mon, Jul 27, 2026 at 03:30:27PM -0400, Rich Felker wrote:
> 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.
OK, here's a reason option 1 is not workable at least not without
serious modification: f->close(f) may call back to application code in
the case of fopencookie FILEs. It is not valid to call application
code with the ofl locked; doing so produces deadlock conditions.
> 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.
It should be noted that this produces a change where f->close(f) is
called without the lock held. Conceptually this does not matter, since
the FILE is already closed at this point and nothing can inspect it
without invoking UB, and we were already unlocking it before anyway;
this is mostly just a slight difference in timing.
For fopencookie, it's a difference in whether the callback is called
with the lock held, but the callback only sees its own cookie anyway,
not the FILE, which is dead at this point.
We could re-take the lock or fake re-taking the lock just by setting
f->lock = __pthread_self()->tid, but I don't think this is useful;
it's just ugly and wasteful.
> 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.
So I wrote up a patch for this that seems to work (attached).
It should be noted (and will be, at least in the commit message if not
a comment, if this gets applied) that the line:
if (f->flags & F_PERM) return r;
is not a code path where fclose could wrongly return with the lock
held, despite looking sus. popen FILEs are never F_PERM; only
stdin/stdout/stderr are "permanent" ones and they are always normal
fopen-type FILEs regardless of the underlying fd type.
Rich
View attachment "popen-fix-hybrid1.diff" of type "text/plain" (735 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.
