bpf_dsl: Change bpf_dsl_unittests into actual standalone unit tests

bpf_dsl: Change bpf_dsl_unittests into actual standalone unit tests

Previously they were more like integration tests that relied on
seccomp-bpf for installing the policies and executing them via live
system calls.

BUG=449357
Committed: https://crrev.com/fdb1befee90e6849a1743fcacbe01521261a6366
Cr-Commit-Position: refs/heads/master@{#317238}

[mdempsky, 2015-02-19 06:18:41]

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

[jln (very slow on Chromium), 2015-02-19 20:19:17]

lgtm, this is great!

The test are kind of funny-looking though, because we're using real syscall
numbers and do things that "look real" even though it doesn't matter.

It's especially striking with the socketpair() stuff which is disabled on
architectures that don't have socketpair() as a single syscall.

Ideally we could have the same test run once via the emulator and once for real
as part of the integration tests. But it's not likely that adding the complexity
to do that is warranted.

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
File sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc (right):

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc:60: ~FakeTrapRegistry() {}
Let's mark this as override (and virtual in the interface).

The usage here is safe, but the pattern in dangerous.

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc:80: PolicyEmulator(const Policy*
policy) : program_(), traps_() {
explicit :)

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc:254: // TODO(mdempsky): This is really
an integration test.
Should be easy to move, no?

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
File sandbox/linux/bpf_dsl/seccomp_macros.h (right):

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/seccomp_macros.h:8: #include
"sandbox/linux/system_headers/linux_seccomp.h"  // For AUDIT_ARCH_*
Nit: below to respect typical include order?

[mdempsky, 2015-02-20 02:12:57]

New patchsets have been uploaded after l-g-t-m from jln@chromium.org

[mdempsky, 2015-02-20 02:21:47]

On 2015/02/19 20:19:17, jln wrote:
> lgtm, this is great!

Thanks.

> The test are kind of funny-looking though, because we're using real syscall
> numbers and do things that "look real" even though it doesn't matter.
> 
> It's especially striking with the socketpair() stuff which is disabled on
> architectures that don't have socketpair() as a single syscall.
> 
> Ideally we could have the same test run once via the emulator and once for
real
> as part of the integration tests. But it's not likely that adding the
complexity
> to do that is warranted.

Yeah, agreed.  I couldn't decide yet which direction to take these tests next
(purely abstract tests that work on any platform vs. tests that could also run
against the kernel), so this just seemed like a reasonable checkpoint.

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
File sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc (right):

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc:60: ~FakeTrapRegistry() {}
On 2015/02/19 20:19:17, jln wrote:
> Let's mark this as override (and virtual in the interface).
> 
> The usage here is safe, but the pattern in dangerous.

Marked here as "virtual" and added comments to trap_registry.h to document the
situation.

In short, I wanted sandbox::Trap's destructor to be marked "= delete" (to make
any accidental attempt to destroy it into a compiler error), but you can't
delete a destructor if its parent class has a virtual destructor.  To prevent
someone from deleting via a TrapRegistry*, I mark TrapRegistry's destructor as
protected.

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc:80: PolicyEmulator(const Policy*
policy) : program_(), traps_() {
On 2015/02/19 20:19:17, jln wrote:
> explicit :)

Rawr, done.

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc:254: // TODO(mdempsky): This is really
an integration test.
On 2015/02/19 20:19:17, jln wrote:
> Should be easy to move, no?

Yeah, but there's already an integration test very similar to this.  I was
leaving this here while I figure out how to convert it to use PolicyEmulator.

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
File sandbox/linux/bpf_dsl/seccomp_macros.h (right):

https://chromiumcodereview.appspot.com/940693004/diff/20001/sandbox/linux/bpf...
sandbox/linux/bpf_dsl/seccomp_macros.h:8: #include
"sandbox/linux/system_headers/linux_seccomp.h"  // For AUDIT_ARCH_*
On 2015/02/19 20:19:17, jln wrote:
> Nit: below to respect typical include order?

Done.

[mdempsky, 2015-02-20 04:15:37]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2015-02-20 04:15:49]

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

[commit-bot: I haz the power, 2015-02-20 04:18:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-20 04:19:29]

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

[kaliamoorthi, 2015-02-20 11:07:27]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/944823002/ by kaliamoorthi@chromium.org.

The reason for reverting is: A related CL was reverted. Reverting this to ensure
that this does not cause a failure due to the revert..