Re: [RFC v3 03/23] sysdeps/wait: Use waitid if avaliable

[Rich Felker <dalias at libc dot org>]

On Sun, Jul 21, 2019 at 06:59:09AM -0500, Eric W. Biederman wrote:
> Rich Felker <dalias@libc.org> writes:
> 
> > On Sun, Jul 21, 2019 at 12:03:10AM -0400, Rich Felker wrote:
> >> On Tue, Jul 16, 2019 at 05:08:48PM -0700, Alistair Francis wrote:
> >> > If the waitid syscall is avaliable let's use that as waitpid
> >> > and wait4 aren't always avaliable (they aren't avaliable on RV32).
> >> > 
> >> > Unfortunately waitid is substantially differnt to waitpid and wait4, so
> >> > the conversion ends up being complex.
> >> > 
> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > ---
> >> >  ChangeLog                                  |  3 ++
> >> >  sysdeps/unix/sysv/linux/wait.c             | 39 ++++++++++++++++--
> >> >  sysdeps/unix/sysv/linux/waitpid.c          | 46 ++++++++++++++++++++++
> >> >  sysdeps/unix/sysv/linux/waitpid_nocancel.c | 45 +++++++++++++++++++++
> >> >  4 files changed, 130 insertions(+), 3 deletions(-)
> >> > [...]
> >> >  
> >> >  weak_alias (__libc_wait, __wait)
> >> > diff --git a/sysdeps/unix/sysv/linux/waitpid.c b/sysdeps/unix/sysv/linux/waitpid.c
> >> > index f0897574c0..7d4e0bb77d 100644
> >> > --- a/sysdeps/unix/sysv/linux/waitpid.c
> >> > +++ b/sysdeps/unix/sysv/linux/waitpid.c
> >> > @@ -20,12 +20,58 @@
> >> >  #include <sysdep-cancel.h>
> >> >  #include <stdlib.h>
> >> >  #include <sys/wait.h>
> >> > +#include <unistd.h>
> >> >  
> >> >  __pid_t
> >> >  __waitpid (__pid_t pid, int *stat_loc, int options)
> >> >  {
> >> >  #ifdef __NR_waitpid
> >> >    return SYSCALL_CANCEL (waitpid, pid, stat_loc, options);
> >> > +#elif defined(__NR_waitid)
> >> > +  __pid_t ret;
> >> > +  idtype_t idtype = P_PID;
> >> > +  siginfo_t infop;
> >> > +
> >> > +  if (pid < -1) {
> >> > +    idtype = P_PGID;
> >> > +    pid *= -1;
> >> > +  } else if (pid == -1) {
> >> > +    idtype = P_ALL;
> >> > +  } else if (pid == 0) {
> >> > +    idtype = P_PGID;
> >> > +    pid = getpgrp();
> >> > +  }
> >> > +
> >> > +  options |= WEXITED;
> >> > +
> >> > +  ret = SYSCALL_CANCEL (waitid, idtype, pid, &infop, options, NULL);
> >> 
> >> This emulation has a fundamental race condition. Between getpgrp and
> >> waitid, a signal handler may perform setpgrp, setsid, and/or fork in
> >> ways that cause the wrong pgid to be passed to the waitid syscall.
> >> There is no way around this because you cannot block signals for the
> >> interval, since signals must be able to interrupt the waitid syscall.
> >> 
> >> Unless there's some trick I'm missing here, the kernel folks' removal
> >> of the wait4 syscall is just a bug in the kernel that they need to
> >> fix. It also makes it impossible to implement the wait4 function,
> >> since there's no way to get rusage for the exited process.
> >
> > Reportedly (via Stefan O'Rear just now) there was a similar kernel bug
> > introduced in 161550d74c07303ffa6187ba776f62df5a906a21 that makes
> > wait4 fail to honor pgrp changes that happen while already in the
> > syscall (e.g. performed on the caller by another thread or even
> > another process).
> 
> I could not find the report from Stefan O'Rear.

There's no report yet as far as I know; it came up on #musl when I
mentioned the issue there. I'll inquire about getting it properly
reported. I just wanted to give credit to the original source when
conveying it here.

> Does that result in actual problems for programs or is this a
> theoretical race noticed upon code review?

I noticed it while looking at the glibc rv32 port proposal, where the
emulation caught my eye as something ugly and undesirable we'd have to
do for musl too. As soon as I looked at it in more detail, the race
condition was apparent, and this is exactly the kind of subtle bug we
don't like to reproduct in musl. I'm not aware of any particular
application that would be broken, but it's a cleae violation of the
AS-safety requirements of the interfaces involved.

Regarding resolving it, I would be perfectly happy with enabling wait4
even for archs that don't want time32 syscalls. musl will be
translating the wait4 rusage results to 64-bit time_t anyway, so
there's utterly no advantage to having to do it in a gratuitously
different way on rv32. The only problem with the fields in the kernel
struct rusage being 32-bit is that you can't represent usage of more
than 68 years of cpu time by a process, which is rather
inconsequential -- but if a working extension to the waitid syscall is
provided, we can use that too.

Rich