[PATCH] daemon: Break CHROOT_ENABLED into CHROOT_ENABLED and CLONE_ENABLED.

[PATCH] daemon: Break CHROOT_ENABLED into CHROOT_ENABLED and CLONE_ENABLED.

[Manolis Ragkousis]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

Hello everyone,

This patch breaks CHROOT_ENABLED into CHROOT_ENABLED and CLONE_ENABLED.

If you check the code below, you will see that in case clone() is not
available it will use fork(), which is the case on Hurd.

But because CHROOT_ENABLED checks for others things, like mount.h and
pivot_root(), it never actually got to the second part of the code
below. This is fixed with my patch.

#if CHROOT_ENABLED
    if (useChroot) {
	char stack[32 * 1024];
	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS |
SIGCHLD;
	if (!fixedOutput) flags |= CLONE_NEWNET;
	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
	if (pid == -1)
	    throw SysError("cloning builder process");
    } else
#endif
    {
        pid = fork();
        if (pid == 0) runChild();
    }

Thank you,
Manolis

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-daemon-Break-CHROOT_ENABLED-into-CHROOT_ENABLED-and-.patch --]
[-- Type: text/x-patch; name="0001-daemon-Break-CHROOT_ENABLED-into-CHROOT_ENABLED-and-.patch", Size: 1574 bytes --]

From 51d96cdea9aec679680c08add3a5ac03065760ba Mon Sep 17 00:00:00 2001
From: Manolis Ragkousis <manolis837@gmail.com>
Date: Sun, 7 Aug 2016 17:48:30 +0300
Subject: [PATCH] daemon: Break CHROOT_ENABLED into CHROOT_ENABLED and
 CLONE_ENABLED.

We need to check for CLONE_NEWNS only when we want to use the
Linux specific clone(). Otherwise we use fork().

* nix/libstore/build.cc (CHROOT_ENABLED): Break into CHROOT_ENABLED
and CLONE_ENABLED.
(DerivationGoal::startBuilder): Replace CHROOT_ENABLED with CLONE_ENABLED.
---
 nix/libstore/build.cc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index ae78e65..b8a5ce6 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -51,7 +51,8 @@
 #include <linux/fs.h>
 #endif
 
-#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) && defined(SYS_pivot_root)
+#define CLONE_ENABLED defined(CLONE_NEWNS)
 
 #if CHROOT_ENABLED
 #include <sys/socket.h>
@@ -1998,7 +1999,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;
-- 
2.9.2

Re: [PATCH] daemon: Break CHROOT_ENABLED into CHROOT_ENABLED and CLONE_ENABLED.

[Manolis Ragkousis]

Hello again,

I was looking at nix's git repo and Eelco's 8f67325 commit is a better
solution to the issue. I cherry picked it and modified it to apply to
our version of the daemon which I will send in another mail.

For this reason forget this patch.

Thank you,
Manolis

On 08/08/16 15:25, Manolis Ragkousis wrote:
> Hello everyone,
> 
> This patch breaks CHROOT_ENABLED into CHROOT_ENABLED and CLONE_ENABLED.
> 
> If you check the code below, you will see that in case clone() is not
> available it will use fork(), which is the case on Hurd.
> 
> But because CHROOT_ENABLED checks for others things, like mount.h and
> pivot_root(), it never actually got to the second part of the code
> below. This is fixed with my patch.
> 
> #if CHROOT_ENABLED
>     if (useChroot) {
> 	char stack[32 * 1024];
> 	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS |
> SIGCHLD;
> 	if (!fixedOutput) flags |= CLONE_NEWNET;
> 	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> 	if (pid == -1)
> 	    throw SysError("cloning builder process");
>     } else
> #endif
>     {
>         pid = fork();
>         if (pid == 0) runChild();
>     }
> 
> Thank you,
> Manolis
> 

Re: [PATCH] daemon: Break CHROOT_ENABLED into CHROOT_ENABLED and CLONE_ENABLED.

[Mark H Weaver]

Manolis Ragkousis <manolis837@gmail.com> writes:

> I was looking at nix's git repo and Eelco's 8f67325 commit is a better
> solution to the issue. I cherry picked it and modified it to apply to
> our version of the daemon which I will send in another mail.
>
> For this reason forget this patch.

Okay, thanks for looking into it!

     Mark