Remove the open directory fd check.

Remove the open directory fd check.

Linux Zygote sandboxing code keeps an fd for /proc while starting the
BPF sandbox (but ensures that it is closed later). This moves the
responsibility to the caller to ensure that no directory fds are present
after sandboxing is enabled.

Also adds WARN_UNUSED_RESULT to some important functions that return a
bool indicating success or failure.

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

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

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

[jln (very slow on Chromium), 2015-01-05 22:51:02]

Could you explain why this is necessary?

A DCHECK inside Credentials::DropFileSystemAccess() seemed useful (even if the
responsibility is that of the caller).

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

On 2015/01/05 22:51:02, jln wrote:
> Could you explain why this is necessary?
> 
> A DCHECK inside Credentials::DropFileSystemAccess() seemed useful (even if the
> responsibility is that of the caller).

Err sorry, obvious we don't meet that requirement in the Zygote, which is why
it's necessary.

[jln (very slow on Chromium), 2015-01-05 23:02:32]

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
File sandbox/linux/services/credentials.cc (left):

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.cc:250: bool
Credentials::DropFileSystemAccess() {
We could also take a file descriptor to /proc here as an argument and pass it to
HasOpenDirectory() (which does ignore it from the list).

The function would still allow "-1" as an argument, so doesn't become harder to
use in the trivial case.

WDYT?

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
File sandbox/linux/services/credentials.h (right):

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.h:66: bool DropFileSystemAccess()
WARN_UNUSED_RESULT;
Could you point to ProcUtil::HasOpenDirectory() as a way to do that?

[rickyz (no longer on Chrome), 2015-01-06 08:01:20]

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
File sandbox/linux/services/credentials.cc (left):

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.cc:250: bool
Credentials::DropFileSystemAccess() {
On 2015/01/05 23:02:32, jln wrote:
> We could also take a file descriptor to /proc here as an argument and pass it
to
> HasOpenDirectory() (which does ignore it from the list).
> 
> The function would still allow "-1" as an argument, so doesn't become harder
to
> use in the trivial case.
> 
> WDYT?
Hm, having the check isn't as big of an extra guarantee as it was before, since
the caller would still needs to remember to close the /proc fd at some point in
the future - I think it would be more flexible to expect the caller to call
ProcUtil::HasOpenDirectory right before they close the /proc fd.

I could probably be convinced either way if you feel strongly about it though.

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
File sandbox/linux/services/credentials.h (right):

https://codereview.chromium.org/835623005/diff/1/sandbox/linux/services/crede...
sandbox/linux/services/credentials.h:66: bool DropFileSystemAccess()
WARN_UNUSED_RESULT;
On 2015/01/05 23:02:32, jln wrote:
> Could you point to ProcUtil::HasOpenDirectory() as a way to do that?

Done.

[jln (very slow on Chromium), 2015-01-06 17:19:39]

lgtm

[rickyz (no longer on Chrome), 2015-01-06 19:52:16]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-01-06 19:53:22]

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

[commit-bot: I haz the power, 2015-01-06 20:59:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-06 21:01:31]

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