Re: 33/33: daemon: Workaround issues for the Hurd.

[Ludovic Courtès]

Hi!

Jan Nieuwenhuizen <address@hidden> skribis:

>>> +#if !__GNU__
>>>      int status = pid.wait(true);
>>>      if (status != 0)
>>>          throw Error(format("cannot kill processes for uid `%1%': %2%") % 
>>> uid % statusToString(status));
>>> +#endif
>>
>> Do you know what the rationale was?  It looks like it could leave
>> zombies behind us.
>
> No, maybe Manolis knows?  What I do know is why I used the patch: before
> applying this patch I could only build up to binutils-boot0.
> binutils-boot0 would always fail like so
>
>     ./pre-inst-env guix build -e '(@@ (gnu packages commencement) 
> binutils-boot0)' --no-offload
>     XXX fails: Workaround for nix daemon
> phase `compress-documentation' succeeded after 0.4 seconds
> error: cannot kill processes for uid `999': Operation not permitted
> guix build: error: cannot kill processes for uid `999': failed with exit code 
> 1

But is the build process actually running as UID 999?  If you pass
‘--disable-chroot’, then I think build users are not used at all, right?

> From 0307646b22fc488e6342f5814fdef336dd154be3 Mon Sep 17 00:00:00 2001
> From: Manolis Ragkousis <address@hidden>
> Date: Sun, 7 Aug 2016 17:48:30 +0300
> Subject: [PATCH 1/2] daemon: Break CHROOT_ENABLED into smaller macros.
>
> Checking for CLONE_NEWNS is only needed for using tha Linux specific clone(2),
> otherwise we can use fork(2).
>
> * nix/libstore/build.cc (CHROOT_ENABLED): Break into CHROOT_ENABLED
> and CLONE_ENABLED.
> (DerivationGoal::startBuilder): Replace CHROOT_ENABLED with CLONE_ENABLED.
> (DerivationGoal::runChild): Only define pivot_root() if SYS_pivot_root is
> defined.

[...]

> -#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIND) 
> && defined(MS_PRIVATE) && defined(CLONE_NEWNS) && defined(SYS_pivot_root)
> +#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIND) 
> && defined(MS_PRIVATE)
> +#define CLONE_ENABLED defined(CLONE_NEWNS)
> +
> +#if defined(SYS_pivot_root)
> +#define pivot_root(new_root, put_old) (syscall(SYS_pivot_root, 
> new_root,put_old))
> +#endif
>  
>  #if CHROOT_ENABLED
>  #include <sys/socket.h>
> @@ -2005,7 +2010,7 @@ void DerivationGoal::startBuilder()
>         - The UTS namespace ensures that builders see a hostname of
>           localhost rather than the actual hostname.
>      */
> -#if CHROOT_ENABLED
> +#if CLONE_ENABLED
>      if (useChroot) {
>       char stack[32 * 1024];
>       int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | 
> SIGCHLD;

I’m not sure this is correct.  Perhaps we rather need an “#ifdef
__linux__” around the use of clone(2)?

Other options:

  1. Implement clone(2) with CLONE_NEW* in libc on GNU/Hurd.

  2. Add a “sandbox” abstraction in the daemon, with OS-specific
     implementations of the abstraction (the Nix daemon did that at some
     point, with the goal of supporting proprietary macOS etc.)

     For GNU/Linux, it’d use chroot(2)+clone(NEWNS) etc. as root.

     On GNU/Hurd, it could spawn the process in a sub-Hurd, i.e., with
     its own proc server, root file system server, and without a pfinet
     server running.

Option #2 can be fun to implement and probably easier and less
controversial than Option #1.  However, it does mean adding more code of
the C++ code base, which is sad.

Either way, it’s a bit of work, so this can definitely come later.

Ludo’.