Seccomp-BPF: relax failure in probe process setup

Seccomp-BPF: relax failure in probe process setup

When we set-up the probe process to test seccomp-bpf availability, setting
a pipe on stderr can sometimes fail. Presumably if this descriptor is backed
by a file on a file system that will return an error on close().

We don't consider not being able to set-up the pipe on stderr as a fatal error
anymore.

BUG=152530
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164850

[Markus (顧孟勤), 2012-10-26 23:14:10]

https://codereview.chromium.org/11300014/diff/7001/sandbox/linux/seccomp-bpf/...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/11300014/diff/7001/sandbox/linux/seccomp-bpf/...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:17: HANDLE_EINTR(write(out_fd, "\n",
1))) {
After this change, do we still expect to see these messages in some place? If
so, should we consider logging a lot more data?

We could log the output from fstat(2) (we should probably collect this data
before trying to close stderr).

We could log the file descriptors returned by pipe() and we could log whether
they seem to be functional (e.g. do another fstat())

Instead of doing just a dup2(2), we could first attempt a close(2) and log it's
result.

There are probably a few other things that might be interesting but that I can't
think of right now.

In either case, since you can now call WriteFailedStderrSetupMessage() from
multiple locations, I would strongly suggest you add some data to disambiguate
why we returning this message. That could make debugging much easier.

https://codereview.chromium.org/11300014/diff/7001/sandbox/linux/seccomp-bpf/...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:114: if (HANDLE_EINTR(dup2(fds[1], 2))
!= 2) {
Try calling HANDLE_EINTR(close(2)) first and ignore the exit code. That seems
harmless and might just be good enough to make dup2() happy. Who knows.

https://codereview.chromium.org/11300014/diff/7001/sandbox/linux/seccomp-bpf/...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:121: // this child.
Do you still want this to be fatal in debug builds?

https://codereview.chromium.org/11300014/diff/7001/sandbox/linux/seccomp-bpf/...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:138: CodeInSandbox();
I'd prefer a blank line before the comment, as I feel it is easier to read
quickly. But it's your call, if you prefer the denser code without extra blank
lines.

[jln (very slow on Chromium), 2012-10-29 21:15:36]

https://chromiumcodereview.appspot.com/11300014/diff/7001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11300014/diff/7001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:17: HANDLE_EINTR(write(out_fd, "\n",
1))) {
On 2012/10/26 23:14:11, Markus (顧孟勤) wrote:
> After this change, do we still expect to see these messages in some place? If
> so, should we consider logging a lot more data?
> 
> We could log the output from fstat(2) (we should probably collect this data
> before trying to close stderr).
> 
> We could log the file descriptors returned by pipe() and we could log whether
> they seem to be functional (e.g. do another fstat())
>
> Instead of doing just a dup2(2), we could first attempt a close(2) and log
it's
> result.
> 
> There are probably a few other things that might be interesting but that I
can't
> think of right now.
> 
> In either case, since you can now call WriteFailedStderrSetupMessage() from
> multiple locations, I would strongly suggest you add some data to disambiguate
> why we returning this message. That could make debugging much easier.

That makes sense. But let's land a simple, easy to merge patch first, and we can
have something more audacious and add some logging information on the dev
channel afterwards. What do you think ?

https://chromiumcodereview.appspot.com/11300014/diff/7001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:121: // this child.
On 2012/10/26 23:14:11, Markus (顧孟勤) wrote:
> Do you still want this to be fatal in debug builds?

Yeah, the first iteration of this patch, as you can see on the review tool),
made it so, but this should wait for a refactor of sandbox_bpf.cc into a clean
interface, because we're not even including proper header files here (and are
relying on the .h file to do it instead).

Since the way it works is to check for NDEBUG (for non debug), I was scared that
we may have a bug of NDEBUG not being defined not because we are in a debug
build, but just because something strange happened with our headers.

https://chromiumcodereview.appspot.com/11300014/diff/7001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:138: CodeInSandbox();
On 2012/10/26 23:14:11, Markus (顧孟勤) wrote:
> I'd prefer a blank line before the comment, as I feel it is easier to read
> quickly. But it's your call, if you prefer the denser code without extra blank
> lines.

Done.

[Markus (顧孟勤), 2012-10-29 21:30:32]

lgtm

[commit-bot: I haz the power, 2012-10-29 22:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/11300014/12001

[commit-bot: I haz the power, 2012-10-30 01:29:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/11300014/12001

[commit-bot: I haz the power, 2012-10-30 01:40:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/11300014/12001

[commit-bot: I haz the power, 2012-10-30 05:01:26]

Change committed as 164850