Move ForkWithFlags from sandbox/ to base/ and plug it into LaunchProcess.

Move ForkWithFlags from sandbox/ to base/ and plug it into LaunchProcess.

ForkWithFlags is a wrapper around the clone syscall that uses the libc clone
wrapper, and thus updates the libc's pid cache if it has one (using sys_clone
directly does not update the pid cache, so getpid may return an incorrect
result in the child). This exposes the ability to set clone flags, which is
needed to use Linux namespaces.

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

[rickyz (no longer on Chrome), 2015-01-07 22:39:04]

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

[rickyz (no longer on Chrome), 2015-01-07 22:39:05]

As discussed, added detection for namespaces support, and moved over the
ForkWithFlags test as well.

[rickyz (no longer on Chrome), 2015-01-12 20:13:53]

On 2015/01/07 22:39:05, rickyz wrote:
> As discussed, added detection for namespaces support, and moved over the
> ForkWithFlags test as well.

Friendly ping :-)

[jln (very slow on Chromium), 2015-01-12 20:26:51]

https://codereview.chromium.org/840893003/diff/20001/base/process/process_uni...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/840893003/diff/20001/base/process/process_uni...
base/process/process_unittest.cc:260: CHECK(syscall(__NR_getpid) == ctid);
I think it'd be more robust here to _exit with a kFail value.

Using CHECK in a forked subprocess is a little dangerous (the base/ test
frameworks differs from sandbox/ and in particular doesn't guarantee that
spurious threads don't exist if a previous test didn't clean up).

Since it doesn't provide much value anyways (given that you only have two
lines), I think simplicity may be the better route.

[rickyz (no longer on Chrome), 2015-01-12 21:37:51]

https://codereview.chromium.org/840893003/diff/20001/base/process/process_uni...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/840893003/diff/20001/base/process/process_uni...
base/process/process_unittest.cc:260: CHECK(syscall(__NR_getpid) == ctid);
On 2015/01/12 20:26:50, jln wrote:
> I think it'd be more robust here to _exit with a kFail value.
> 
> Using CHECK in a forked subprocess is a little dangerous (the base/ test
> frameworks differs from sandbox/ and in particular doesn't guarantee that
> spurious threads don't exist if a previous test didn't clean up).
> 
> Since it doesn't provide much value anyways (given that you only have two
> lines), I think simplicity may be the better route.

Is RAW_CHECK OK? It should be async signal safe, and I prefer it over exiting
since it writes the failed check on stderr.

[jln (very slow on Chromium), 2015-01-12 22:58:21]

lgtm

[rickyz (no longer on Chrome), 2015-01-12 23:40:38]

rickyz@chromium.org changed reviewers:
+ mark@chromium.org

[rickyz (no longer on Chrome), 2015-01-12 23:40:38]

Readding Mark for base/ OWNERS.

Some background on why the previous attempt's test failed: I was incorrectly
relying on errno to test for namespaces support. Since LaunchProcess calls
SetSignalMask even if the clone failed, errno was set to 0, so the test was not
being skipped on kernels without namespaces support. I replaced the fragile test
with explicit check for namespaces support.

[Mark Mentovai, 2015-01-13 21:51:03]

LGTM

[rickyz (no longer on Chrome), 2015-01-13 21:54:13]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-01-13 21:55:04]

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

[commit-bot: I haz the power, 2015-01-13 23:00:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-13 23:01:30]

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

[rvargas (doing something else), 2015-01-15 23:27:31]

rvargas@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2015-01-15 23:27:32]

https://codereview.chromium.org/840893003/diff/40001/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/840893003/diff/40001/base/process/process.h#n...
base/process/process.h:131: BASE_EXPORT pid_t ForkWithFlags(unsigned long flags,
pid_t* ptid, pid_t* ctid);
I don't think this function belongs to this file. Either it is rewritten so that
it returns a Process, or it is moved to launch.h (which looks like a more
natural place for it anyway)

[jln (very slow on Chromium), 2015-01-16 00:57:07]

On 2015/01/15 23:27:32, rvargas wrote:
> https://codereview.chromium.org/840893003/diff/40001/base/process/process.h
> File base/process/process.h (right):
> 
>
https://codereview.chromium.org/840893003/diff/40001/base/process/process.h#n...
> base/process/process.h:131: BASE_EXPORT pid_t ForkWithFlags(unsigned long
flags,
> pid_t* ptid, pid_t* ctid);
> I don't think this function belongs to this file. Either it is rewritten so
that
> it returns a Process, or it is moved to launch.h (which looks like a more
> natural place for it anyway)

Yeah, that seems reasonable to me. Ricky?

[rickyz (no longer on Chrome), 2015-01-16 01:00:44]

On 2015/01/16 00:57:07, jln wrote:
> On 2015/01/15 23:27:32, rvargas wrote:
> > https://codereview.chromium.org/840893003/diff/40001/base/process/process.h
> > File base/process/process.h (right):
> > 
> >
>
https://codereview.chromium.org/840893003/diff/40001/base/process/process.h#n...
> > base/process/process.h:131: BASE_EXPORT pid_t ForkWithFlags(unsigned long
> flags,
> > pid_t* ptid, pid_t* ctid);
> > I don't think this function belongs to this file. Either it is rewritten so
> that
> > it returns a Process, or it is moved to launch.h (which looks like a more
> > natural place for it anyway)
> 
> Yeah, that seems reasonable to me. Ricky?

Sure thing, will send a CL to move it in a bit (might be slightly slow due to
traveling).