Use the libc clone wrapper in sys_clone.

Use the libc clone wrapper in sys_clone.

Previously, we directly invoked the syscall, which would not update
libc's PID cache in the child. Although the libc wrapper function
updates the PID cache, it unfortunately requires that the child run on a
different stack, even if CLONE_VM is not specified. We work around this
by briefly switching stacks in the child, then using longjmp to switch
back. This gives us a version of clone with fork-like behavior, which is
what we need for starting processes in new namespaces.

BUG=312380
Committed: https://crrev.com/d8a593bceaf0a4b38d06a8c13b948202d205f1b6
Cr-Commit-Position: refs/heads/master@{#308510}

[rickyz (no longer on Chrome), 2014-12-15 20:01:59]

rickyz@chromium.org changed reviewers:
+ jln@chromium.org, mdempsky@chromium.org

[rickyz (no longer on Chrome), 2014-12-15 20:02:00]

Here's the change for the clone setjmp/longjmp thing. It also changes the
interface to disallow non-null child_stack, since there's no way to reasonably
switch stacks from this C++ wrapper function. I ended up going with
PTHREAD_STACK_MIN for the stack size, which is 16k on my machine.

[mdempsky, 2014-12-15 20:30:08]

lgtm

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:67: return clone(&clone_helper, stack
+ sizeof(stack), flags, &env, ptid, tls,
"stack + sizeof(stack)" is technically wrong on stack-grows-down architectures
(i.e., HPPA), but not sure we care.

[mdempsky, 2014-12-15 20:30:26]

On 2014/12/15 20:30:08, mdempsky wrote:
> "stack + sizeof(stack)" is technically wrong on stack-grows-down architectures
> (i.e., HPPA), but not sure we care.

Er, stack-grows-up architectures.

[jln (very slow on Chromium), 2014-12-15 23:31:36]

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:32: int clone_helper(void* arg) {
Style: CloneHelper

Also add a comment to explain what it does.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:36: NOTREACHED();
RAW_CHECK(): probably a good idea to be very conservative with the execution
environment.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:53: const bool invalid_clone_vm =
flags & CLONE_VM;
maybe rename to clone_vm_used (to be consistent with CLONE_SETTLS above)?

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:67: return clone(&clone_helper, stack
+ sizeof(stack), flags, &env, ptid, tls,
On 2014/12/15 20:30:08, mdempsky wrote:
> "stack + sizeof(stack)" is technically wrong on stack-grows-down architectures
> (i.e., HPPA), but not sure we care.

You mean stack grows-up architecture.

How could we create a reasonable COMPILE_ASSERT at least? How horrible would you
think (&env < &stack) is?

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.h (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.h:19: // as caching the current pid
(c.f. getpid()).
Add: "unless specified otherwise"

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.h:29: // As a consequence, this function
does not support CLONE_VM.
Please, mention that under the hood, this is using GLIBC's clone and that
callers can expect getpid() etc to work as expected.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers_unittest.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers_unittest.cc:74: ASSERT_EQ(ctid,
getpid());
Let's not use ASSERT in there, it's confusing. I would just _exit(1) like the
test above.

I would actually even merge these tests.

[mdempsky, 2014-12-15 23:55:01]

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:67: return clone(&clone_helper, stack
+ sizeof(stack), flags, &env, ptid, tls,
On 2014/12/15 23:31:35, jln wrote:
> On 2014/12/15 20:30:08, mdempsky wrote:
> > "stack + sizeof(stack)" is technically wrong on stack-grows-down
architectures
> > (i.e., HPPA), but not sure we care.
> 
> You mean stack grows-up architecture.

Yep, oops.

> How could we create a reasonable COMPILE_ASSERT at least? How horrible would
you
> think (&env < &stack) is?

Pretty horrible, TBH. :-/  (The compiler might rearrange the stack layout of
objects to make that fail even on grows-down architectures.)

I'd be fine with a comment pointing out "stack + sizeof(stack)" is wrong for
grows-up architectures.  Better would be something like:

    #if X86 || ARM || MIPS
    // Grows down architectures.
    char *stack_pointer = stack + sizeof(stack);
    #else
    #error Port me.
    #endif

but that feels silly with how unlikely an HPPA port seems.

[rickyz (no longer on Chrome), 2014-12-16 00:35:09]

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:32: int clone_helper(void* arg) {
On 2014/12/15 23:31:35, jln wrote:
> Style: CloneHelper
> 
> Also add a comment to explain what it does.

Done.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:36: NOTREACHED();
On 2014/12/15 23:31:35, jln wrote:
> RAW_CHECK(): probably a good idea to be very conservative with the execution
> environment.

Done.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:53: const bool invalid_clone_vm =
flags & CLONE_VM;
On 2014/12/15 23:31:35, jln wrote:
> maybe rename to clone_vm_used (to be consistent with CLONE_SETTLS above)?

Done.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:67: return clone(&clone_helper, stack
+ sizeof(stack), flags, &env, ptid, tls,
On 2014/12/15 23:55:01, mdempsky wrote:
> On 2014/12/15 23:31:35, jln wrote:
> > On 2014/12/15 20:30:08, mdempsky wrote:
> > > "stack + sizeof(stack)" is technically wrong on stack-grows-down
> architectures
> > > (i.e., HPPA), but not sure we care.
> > 
> > You mean stack grows-up architecture.
> 
> Yep, oops.
> 
> > How could we create a reasonable COMPILE_ASSERT at least? How horrible would
> you
> > think (&env < &stack) is?
> 
> Pretty horrible, TBH. :-/  (The compiler might rearrange the stack layout of
> objects to make that fail even on grows-down architectures.)
> 
> I'd be fine with a comment pointing out "stack + sizeof(stack)" is wrong for
> grows-up architectures.  Better would be something like:
> 
>     #if X86 || ARM || MIPS
>     // Grows down architectures.
>     char *stack_pointer = stack + sizeof(stack);
>     #else
>     #error Port me.
>     #endif
> 
> but that feels silly with how unlikely an HPPA port seems.
Here's a version that detects the direction of stack growth at runtime, though I
guess it's a little hacky, so I'm also fine with just hardcoding the
architectures as well, what do you think?

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.h (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.h:19: // as caching the current pid
(c.f. getpid()).
On 2014/12/15 23:31:35, jln wrote:
> Add: "unless specified otherwise"

Done.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.h:29: // As a consequence, this function
does not support CLONE_VM.
On 2014/12/15 23:31:35, jln wrote:
> Please, mention that under the hood, this is using GLIBC's clone and that
> callers can expect getpid() etc to work as expected.

Done.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers_unittest.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers_unittest.cc:74: ASSERT_EQ(ctid,
getpid());
On 2014/12/15 23:31:35, jln wrote:
> Let's not use ASSERT in there, it's confusing. I would just _exit(1) like the
> test above.
> 
> I would actually even merge these tests.
I merged the tests, but actually, I'm curious as to why you prefer _exit(1) over
ASSERT. I prefer ASSERT because it writes a useful message to stderr, which
shows me the cause when a test fails due to an unexpected return code. With just
_exit(1), I can't tell precisely why the test failed from the test output.

[jln (very slow on Chromium), 2014-12-16 01:11:22]

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:67: return clone(&clone_helper, stack
+ sizeof(stack), flags, &env, ptid, tls,
On 2014/12/16 00:35:09, rickyz wrote:

> > but that feels silly with how unlikely an HPPA port seems.
> Here's a version that detects the direction of stack growth at runtime, though
I
> guess it's a little hacky, so I'm also fine with just hardcoding the
> architectures as well, what do you think?

Yes, I think Matthew's suggestion is best. It's faster, more reliable and less
hackish.

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers_unittest.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers_unittest.cc:74: ASSERT_EQ(ctid,
getpid());
On 2014/12/16 00:35:09, rickyz wrote:
> On 2014/12/15 23:31:35, jln wrote:
> > Let's not use ASSERT in there, it's confusing. I would just _exit(1) like
the
> > test above.
> > 
> > I would actually even merge these tests.
> I merged the tests, but actually, I'm curious as to why you prefer _exit(1)
over
> ASSERT. I prefer ASSERT because it writes a useful message to stderr, which
> shows me the cause when a test fails due to an unexpected return code. With
just
> _exit(1), I can't tell precisely why the test failed from the test output.

ASSERT is only guaranteed to return from the current function. It's not
guaranteed to terminate the process and if it does, it's not guaranteed to
terminate the process.

In general, it's really error prone to rely in implementation detail of the
testing framework in subprocesses (which are not supported by the testing
framework). Even doing exit() instead of _exit() in a subprocess would fail.

SANDBOX_TEST() have to go through some hoops to be "somewhat" reliable, by
failing if *anything* gets written to stderr and rely on very specific unlikely
exit codes to detect success.

If you need to do anything complicated in a subprocess of your test, I suggest
using SANDBOX_TEST.

Moreover: I think we'll want to move this code to base/. Unlikely sandbox/, in
base/ unittests are not careful with threads. So you could end-up forking with
other threads running here. So controlling tightly what we're doing in the
subprocess is important if we don't want to end-up with deadlocks when the
testing framework runs the test with existing threads.

[rickyz (no longer on Chrome), 2014-12-16 01:30:08]

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/20001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:67: return clone(&clone_helper, stack
+ sizeof(stack), flags, &env, ptid, tls,
On 2014/12/16 01:11:22, jln wrote:
> On 2014/12/16 00:35:09, rickyz wrote:
> 
> > > but that feels silly with how unlikely an HPPA port seems.
> > Here's a version that detects the direction of stack growth at runtime,
though
> I
> > guess it's a little hacky, so I'm also fine with just hardcoding the
> > architectures as well, what do you think?
> 
> Yes, I think Matthew's suggestion is best. It's faster, more reliable and less
> hackish.

Done.

[jln (very slow on Chromium), 2014-12-16 01:37:51]

lgtm

https://codereview.chromium.org/801033002/diff/80001/sandbox/linux/services/s...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/801033002/diff/80001/sandbox/linux/services/s...
sandbox/linux/services/syscall_wrappers.cc:71: #if defined(__x86_64__) ||
defined(__i386__) || defined(__arm__) || \
Nit: might be better to rely on what build_config.h defines for us instead.

[rickyz (no longer on Chrome), 2014-12-16 01:44:17]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2014-12-16 01:44:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801033002/100001

[commit-bot: I haz the power, 2014-12-16 02:40:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-16 02:41:30]

Patchset 6 (id:??) landed as
https://crrev.com/d8a593bceaf0a4b38d06a8c13b948202d205f1b6
Cr-Commit-Position: refs/heads/master@{#308510}