Re: [musl] [PATCH 07/14] Emulate wait4 using waitid

[Rich Felker <dalias@libc.org>]

On Thu, Sep 03, 2020 at 12:25:19PM -0400, Stefan O'Rear wrote:
> 
> 
> On Thu, Sep 3, 2020, at 11:49 AM, Rich Felker wrote:
> > On Thu, Sep 03, 2020 at 07:23:02AM -0400, Stefan O'Rear wrote:
> > > riscv32 and future architectures lack wait4.
> > > ---
> > >  src/internal/wait4_waitid.h |  1 +
> > >  src/linux/__wait4_waitid.c  | 52 +++++++++++++++++++++++++++++++++++++
> > >  src/linux/wait4.c           |  5 ++++
> > >  src/process/waitpid.c       |  6 +++++
> > >  src/stdio/pclose.c          |  6 +++++
> > >  src/unistd/faccessat.c      |  6 ++++-
> > >  6 files changed, 75 insertions(+), 1 deletion(-)
> > >  create mode 100644 src/internal/wait4_waitid.h
> > >  create mode 100644 src/linux/__wait4_waitid.c
> > 
> > I really don't like introducing a new src/internal file for this. If
> 
> I agree.
> 
> > the code needs to be shared it should probably just be in wait4.c,
> > renamed to __wait4, declared in src/include/sys/wait.h according to
> > the usual pattern for exposing namespace-safe versions of
> > namespace-violating functions.
> 
> Currently waitpid and waitid are cancellation points (required by POSIX)
> but wait4 is not, which is why I did not have anything else call a function
> with exactly wait4 semantics.
> 
> > Ideally I'd like to just not need it, but I'm not sure that's
> > possible:
> > 
> > - faccessat throws away the status and doesn't have any need for the
> >   status or idtype emulation code. It could easily be just different
> >   syscalls in #ifdef/#else paths.
> 
> I think that is exactly what my patch does?

Yes, sorry I missed that. I think I saw the other three and just
assumed this one was the same since it was part of the same patch.

> > - pclose does need status but not the idtype part. but it's only
> >   listening for process termination not all the other weird stuff. I'm
> >   not sure if it would make sense to construct status inline here. It
> >   might if we had a macro like the glibc "inverse-W" macro (I forget
> >   its name) to construct a wait status from parts; if so this could
> >   later be considered for a public API (previously requested).
> 
> Earlier I had the wait status conversion code broken out separately
> but it seemed not worth it for just pclose.
> 
> > - waitpid and wait4 at least need the idtype and status construction.
> > 
> > Unfortunately, wait4 also needs the rusage conversion, which waitpid
> > doesn't want. This kinda defeats my idea of just exposing __wait4 from
> > wait4.c.
> > 
> > A side observation to keep in mind is that passing the argument cp
> > doesn't really help optimize anything; it only worked well in
> > c2feda4e2e because the function there is inline. Of the 4 callers you
> > have, faccessat and pclose have hard requirements not to be a
> > cancellation point and waitpid has a hard requirement to be one. But
> 
> pclose has a hard requirement to not be a cancellation point but it also
> has "If a thread is canceled during execution of pclose(), the behavior
> is undefined."  The implications of the combination are confusing.

It's an optional cancellation point, but if cancelled the side effects
must be correct. Since the FILE and underlying fd have already been
closed at this point, it's too late for cancellation to be acted upon;
any thing you do would leave the process in an inconsistent state,
where the caller has to assume the FILE is still valid and thus
performs UAF/DF if it does anything with it.

Note that you can't swap the order of the fd close and wait because
the child very well may not exit until the pipe is closed.

So, the only valid way pclose could act on cancellation is before
taking any action. This is almost surely not what was intended in the
standard, since it's useles, but the conclusion here is just that
nobody thought about this stuff enough to realize that allowing pclose
to be a cancellation point was fundamentally wrong.

> > if the function weren't used for the former, it could just always be
> > cancellable -- wait4 probably should have been cancellable all along,
> > and almost surely is on other implementations, so to use it safely
> > you'd have to not use, block, or handle cancellation.
> 
> I have no immediate objection to making wait4 a cancellation point but
> I would prefer to make that change consistently on all architectures.
> I don't have a good sense of who uses cancellation and wait4 and what
> compatibility constraints are imposed by code (as opposed to standards).

Yes, it should be done consistently and as a separate patch if we go
this way.

> > So I'm leaning towards sticking with something like what you've done,
> > but with declaration moved to src/include/sys/wait.h, or possibly
> > src/internal/syscall.h (since it's essentially emulation of a
> 
> I'd be very happy to not have a one-line header file.  I looked at syscall.h
> earlier but wasn't sure if it fit; I did not consider reserved namespace in
> a public header (would this preclude use of hidden?).

src/include/* are not public headers, but wrappers around the public
headers to add namespace-protected versions of functions etc. However
since this is really not a "version of wait4" but an "emulation of
SYS_wait4", it probably makes more sense to put it in
internal/syscall.h, just like __sys_open[23]. That doesn't mean it has
to be inline, just that the declaration could go there, and a macro to
just use __syscall(SYS_wait4...) directly if it exists which would
keep some small amount of #ifdef out of source files.

> Is src/linux/__exact_name_of_function.c the appropriate name for the file?

If there's an external source file still, I'd put it in
src/process/__function_name.c or src/internal/... like you did. The
existing naming here is not entirely consistent, but generally,
src/subsystem/__internal_function.c is an internal function for
implementing that subsystem/family, not an internal functon used by
other subsystems that just happens to mostly-match a public function
in that subsystem.

src/internal is a kinda sloppy mix of things, but is mostly internal
functions that are shared between two or more subsystems.

Rich