Add a platform-specific syscall number iterator.

Add a platform-specific syscall number iterator.

Avoid needlessly expensive scanning of system call ranges.
This CL improves how we deal with discontiguous ranges of system call numbers.

(Original CL by markus@chromium.org)

TEST=sandbox_linux_unittests on x86_64 and ARM
BUG=148856
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161943

[Jorge Lucangeli Obes, 2012-10-08 23:50:45]

If we can get this in then we can re-enable Seccomp-BPF for ARM.

[Jorge Lucangeli Obes, 2012-10-09 19:53:49]

On 2012/10/08 23:50:45, Jorge Lucangeli Obes wrote:
> If we can get this in then we can re-enable Seccomp-BPF for ARM.

Uploaded PS2 with some #define's cleanup and better isolated ARM private syscall
logic.

[Jorge Lucangeli Obes, 2012-10-11 16:38:18]

On 2012/10/09 19:53:49, Jorge Lucangeli Obes wrote:
> On 2012/10/08 23:50:45, Jorge Lucangeli Obes wrote:
> > If we can get this in then we can re-enable Seccomp-BPF for ARM.
> 
> Uploaded PS2 with some #define's cleanup and better isolated ARM private
syscall
> logic.

Friendly ping.

[jln (very slow on Chromium), 2012-10-11 22:41:18]

Sorry for the unacceptably long delay. Here are a few design-level remarks. I'll
go over the implementation and code style, but you can already work on this.

The big issue so far is that policies shouldn't have to be aware of what is a
valid or non valid syscall, this should all be provided by the API. The current
code duplication in every black-list policy is a symptom of this problem.

The even more high level concern is that this API is way too complex for no
benefit:

- Instead of trying to detect by "poking" what the user wants to do with invalid
syscalls, an ErrorCode should simply be passed to SetPolicy() (and we can have a
default that just does ENOSYS). Then an iterator can become something super
simple that goes through valid system calls, end of story. The current way of
doing things has reached a worrisome level of complexity because of the non
contiguous system call ranges.

- Similarly, X32 adds complexity even though it's 1. not used and 2. not tested.

Now I realize that you (Jorge) are modifying Markus' CL, and Markus should
probably take care of simplifying the API. It's ok if you want to just implement
the few changes I've mentioned and Markus could change the API later.

[jln (very slow on Chromium), 2012-10-11 22:42:00]

Sorry, I forgot to include the code comments.

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

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:285: for (SyscallIterator iter(true);
!iter.Done(); ) {
We still have the same problem as before on ARM.
We iterate over every invalid syscall number here.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:228: static bool isArmPrivateSyscall(int
sysnum);
This is not a good interface for the Sandbox class.

Instead the interface should be something more generic, such as
"IsValidSysCallNumber()" or something like this.

In turn, the implementation should request this to the syscall iterator class
(which could also expose a IsValidSyscallNumber() method), and in its
implementation, something such as IsArmPrivateSyscall could be used.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:50: if
(!Sandbox::isArmPrivateSyscall(sysno)) {
I don't understand why this would work. For instance, for EACCESS, which is not
a private syscall, wouldn't we return ENOSYS ?

Asides from that, it's a problem to require every black list-style policy to do
something so complex. Instead, export a IsValidSysCallNumber() method in the
Sandbox class to take care of what's above and here.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:192: // TODO(jorgelo): remove
this limit once crbug.com/141694 is fixed.
Do you want to go over this now?

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_iterator.h:16: // over ranges that can't
contain system calls. It iterates more slowly
It should be documented (and asserted from unit tests?) that we still get one
syscall from any range that doesn't contain system calls.

[jln (very slow on Chromium), 2012-10-11 22:59:07]

Ohh, and adding Markus on that CL.

[Jorge Lucangeli Obes, 2012-10-12 17:58:23]

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

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:285: for (SyscallIterator iter(true);
!iter.Done(); ) {
On 2012/10/11 22:42:00, Julien Tinnes wrote:
> We still have the same problem as before on ARM.
> We iterate over every invalid syscall number here.

Confirmed that we don't.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:228: static bool isArmPrivateSyscall(int
sysnum);
On 2012/10/11 22:42:00, Julien Tinnes wrote:
> This is not a good interface for the Sandbox class.
> 
> Instead the interface should be something more generic, such as
> "IsValidSysCallNumber()" or something like this.
> 
> In turn, the implementation should request this to the syscall iterator class
> (which could also expose a IsValidSyscallNumber() method), and in its
> implementation, something such as IsArmPrivateSyscall could be used.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:50: if
(!Sandbox::isArmPrivateSyscall(sysno)) {
On 2012/10/11 22:42:00, Julien Tinnes wrote:
> I don't understand why this would work. For instance, for EACCESS, which is
not
> a private syscall, wouldn't we return ENOSYS ?
> 
> Asides from that, it's a problem to require every black list-style policy to
do
> something so complex. Instead, export a IsValidSysCallNumber() method in the
> Sandbox class to take care of what's above and here.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:192: // TODO(jorgelo): remove
this limit once crbug.com/141694 is fixed.
On 2012/10/11 22:42:00, Julien Tinnes wrote:
> Do you want to go over this now?

This will still need the new compiler. We're still blowing up the jumptable.

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_iterator.h:16: // over ranges that can't
contain system calls. It iterates more slowly
On 2012/10/11 22:42:00, Julien Tinnes wrote:
> It should be documented (and asserted from unit tests?) that we still get one
> syscall from any range that doesn't contain system calls.

Invalid and InvalidOnly tests in syscall_iterator_unittests.cc

[Jorge Lucangeli Obes, 2012-10-12 18:17:56]

Fixed upload.

[jln (very slow on Chromium), 2012-10-12 20:26:52]

I can't believe the complexity brought by the syscall poking. This is madness.

I'll talk to you offline.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:227: // Checks whether a particular
system call number is valid.
Please remove the specific line about ARM above (or give use it as an example
only: "for instance, on ARM..")

I think the comment would be more clear this way: "Checks whether a particular
system call number is valid on the current architecture."

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:17: const int
kArmPublicSysnoCeiling = __NR_SYSCALL_BASE + 1024;
I believe this can now be removed?

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:201: // __ARM_NR_BASE plus 1
(to avoid NUL errno).
You mean offset from MIN_PRIVATE_SYSCALL, no ?

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:211: ErrorCode
ArmPrivatePolicy(int sysno) {
I'm a bit worried that this wouldn't be a viable policy. Does it work if you
blow it up on purpose ?

On error, BPF_ASSERT etc could end-up needing something such as set_tls or
cacheflush().

Perhaps you could start doing that after the last known private syscall only
(for which we don't have a good symbolic name, but since this is a test,
NR_set_tls + 1 wouldn't be horrifying) ?

You could even try calling cacheflush(), it should never do any harm.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:36: // for our loop to make some
progress.
This is another prime example of the sick complexity brought by trying to "poke"
instead of having the user define the default behavior.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:14: // Iterates over the entire
system call range from 0..0xFFFFFFFFu. This
Style: This class comment should be above the class declaration

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:23: // "impossible" system call
values are treated the same.
This should be made more clear: with invalid_only, the iterator will only
iterate over invalid syscall number, and is at liberty to skip some system
calls.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:23: // "impossible" system call
values are treated the same.
The way to use the iterator is really weird:

You have to call iter.Next() at the beginning of a for loop to get an actual
value. Please give a short example here on how to use it.

I don't understand why this couldn't be a normal C++-style iterator with classic
iterator semantics.   Would you mind adding a TODO for Markus: investigate
switching to classic iterator smeantics ?

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:25: : invalid_only_(invalid_only),
Style: the colon should be indented 4 spaces.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:39: };
DISALLOW_COPY_AND_ASSIGN ?

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:14: for (int i = 0; i <
2; ++i) {
Please add a comment: "Testing the following under two scenarios: invalid_only
is true or false."

Just add a "bool invalid_only = !i" inside the loop to make it more clear.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:17: if (i || MIN_SYSCALL
!= 0) {
Add a comment: we want to poke at 0, when we're explicitly looking for invalid
syscalls.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:24: SANDBOX_ASSERT(next
== 0xFFFFFFFFu);
This is testing for undocumented behavior: that 0xFFFFFFFFu is always returned.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:29: SyscallIterator
iter(false);
Please add comments, what is this trying to do ?

[Jorge Lucangeli Obes, 2012-10-13 01:39:29]

Addressing comments.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:227: // Checks whether a particular
system call number is valid.
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> Please remove the specific line about ARM above (or give use it as an example
> only: "for instance, on ARM..")
> 
> I think the comment would be more clear this way: "Checks whether a particular
> system call number is valid on the current architecture."

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:17: const int
kArmPublicSysnoCeiling = __NR_SYSCALL_BASE + 1024;
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> I believe this can now be removed?

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:201: // __ARM_NR_BASE plus 1
(to avoid NUL errno).
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> You mean offset from MIN_PRIVATE_SYSCALL, no ?

It's the same thing:
#define MIN_PRIVATE_SYSCALL ((unsigned int)__ARM_NR_BASE)

But I agree it's confusing.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:211: ErrorCode
ArmPrivatePolicy(int sysno) {
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> I'm a bit worried that this wouldn't be a viable policy. Does it work if you
> blow it up on purpose ?
> 
> On error, BPF_ASSERT etc could end-up needing something such as set_tls or
> cacheflush().
> 
> Perhaps you could start doing that after the last known private syscall only
> (for which we don't have a good symbolic name, but since this is a test,
> NR_set_tls + 1 wouldn't be horrifying) ?
> 
> You could even try calling cacheflush(), it should never do any harm.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:36: // for our loop to make some
progress.
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> This is another prime example of the sick complexity brought by trying to
"poke"
> instead of having the user define the default behavior.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:14: // Iterates over the entire
system call range from 0..0xFFFFFFFFu. This
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> Style: This class comment should be above the class declaration

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:23: // "impossible" system call
values are treated the same.
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> This should be made more clear: with invalid_only, the iterator will only
> iterate over invalid syscall number, and is at liberty to skip some system
> calls.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:23: // "impossible" system call
values are treated the same.
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> The way to use the iterator is really weird:
> 
> You have to call iter.Next() at the beginning of a for loop to get an actual
> value. Please give a short example here on how to use it.
> 
> I don't understand why this couldn't be a normal C++-style iterator with
classic
> iterator semantics.   Would you mind adding a TODO for Markus: investigate
> switching to classic iterator smeantics ?

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:25: : invalid_only_(invalid_only),
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> Style: the colon should be indented 4 spaces.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:39: };
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> DISALLOW_COPY_AND_ASSIGN ?

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:14: for (int i = 0; i <
2; ++i) {
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> Please add a comment: "Testing the following under two scenarios: invalid_only
> is true or false."
> 
> Just add a "bool invalid_only = !i" inside the loop to make it more clear.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:17: if (i || MIN_SYSCALL
!= 0) {
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> Add a comment: we want to poke at 0, when we're explicitly looking for invalid
> syscalls.

This is poking when invalid_only == false (invalid_only == !i).

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:24: SANDBOX_ASSERT(next
== 0xFFFFFFFFu);
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> This is testing for undocumented behavior: that 0xFFFFFFFFu is always
returned.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/26003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:29: SyscallIterator
iter(false);
On 2012/10/12 20:26:52, Julien Tinnes wrote:
> Please add comments, what is this trying to do ?

Done.

[jln (very slow on Chromium), 2012-10-13 02:44:22]

Mostly two general remarks:

- CHECK is a form of documentation of the programmer's assumption. It needs to
be clear why the assumption is important.

- Ideally, most unit tests could be written by someone who only knows the
interface, not the gory implementation details. It's usually not advisable to
ASSERT on things that aren't guaranteed in the interface.

In particular, I think the fact that syscall numbers around valid ranges (min -
1 / max + 1) will be returned should be documented (or the tests amended, as
explained in my comments).

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:17: // |num_| has been initialized
to MIN_SYSCALL.
This is where you shoul put the CHECK_EQ(MIN_SYSCALL, 0).

// |num_| has been initialized to 0, which we assume is also MIN_SYSCALL

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:33: : invalid_only_(invalid_only),
Style: You misread my previous comment. Colon should be indented 4 spaces, the
rest of the initializers should be indented below other initializers.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:36: CHECK_EQ(MIN_SYSCALL, 0u);
This needs to be localized where you are actually making this assumption, in the
implementation.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:53: }  // namespace
Style: namespace playground2

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:16:
SANDBOX_ASSERT(MIN_SYSCALL == 0u);
This assert is confusing, because I don't think anything here requires this to
be true. I'd just remove it.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:25: SANDBOX_ASSERT(next
== MIN_SYSCALL);
I think 0 was correct there. The contract (syscall_iterator.h) says that you'll
iterate from 0 to MAXUINT32, this function tests that.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:39:
SANDBOX_ASSERT(MIN_SYSCALL == 0u);
Assertions are nice in a place where they help to read the code (i.e. assert the
invariant for instance). This one should probably be moved below as indicated in
the comment below.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:46: SANDBOX_ASSERT(next
== MIN_SYSCALL);
It's not obvious why you'd assert that. You can assert next == 0, since this is
mandated in the interface.

But I think you may want to just add a comment, and probably regroup the two
asserts here to make it clear what the starting situation is and that you can
now iterate over public system calls.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:57: while (next <
MIN_PRIVATE_SYSCALL - 1) {
This is an implementation detail that we'll actually return MIN_PRIVATE_SYSCALL
- 1. Hopefully this will disappear soon with the "poking magic".

The test would be more clear if it just tested that we, in fact do cover the
private syscall range: from MIN_PRIVATE_SYSCALL to MAX_PRIVATE_SYSCALL.

That being said, we currently rely on that behavior, so perhaps the best course
of action is to document that +1 / -1 will be tested in syscall_iterator.h ?

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:72: while (next <
MIN_GHOST_SYSCALL - 1) {
The same remark as above applies.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:101: SANDBOX_ASSERT(next
== 0x7FFFFFFFu);
I don't really like that this is testing implementation details that are not
specified in the interface, but that's fine.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:115: // First invalid
syscall should be |MAX_PUBLIC_SYSCALL + 1|.
This is only true if 0 is a valid syscall, otherwise the first invalid syscall
would be 0.

So this time, you do need to assert that 0 is MIN_SYSCALL.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:123: next = iter.Next();
You're testing implementation details. In fact these details should change
(hopefully) soon as we can rid of the range testing.

I think the function was good as it was before. It really just tests that the
iterator only yields invalid syscalls.

Or, as before, you can document the +1/-1 behavior clearly in the interface.

[Jorge Lucangeli Obes, 2012-10-13 04:45:42]

I wanted to upload before the weekend so as to not lose all the context in my
head. I'm of course not expecting review until next week.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:17: // |num_| has been initialized
to MIN_SYSCALL.
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> This is where you shoul put the CHECK_EQ(MIN_SYSCALL, 0).
> 
> // |num_| has been initialized to 0, which we assume is also MIN_SYSCALL

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:33: : invalid_only_(invalid_only),
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> Style: You misread my previous comment. Colon should be indented 4 spaces, the
> rest of the initializers should be indented below other initializers.

I'm not sure what's wrong in this case. The colon is indented 4 spaces, and then
the initializers are aligned, e.g.
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/observer_list.h&type=...

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:36: CHECK_EQ(MIN_SYSCALL, 0u);
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> This needs to be localized where you are actually making this assumption, in
the
> implementation.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:53: }  // namespace
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> Style: namespace playground2

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:16:
SANDBOX_ASSERT(MIN_SYSCALL == 0u);
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> This assert is confusing, because I don't think anything here requires this to
> be true. I'd just remove it.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:25: SANDBOX_ASSERT(next
== MIN_SYSCALL);
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> I think 0 was correct there. The contract (syscall_iterator.h) says that
you'll
> iterate from 0 to MAXUINT32, this function tests that.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:39:
SANDBOX_ASSERT(MIN_SYSCALL == 0u);
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> Assertions are nice in a place where they help to read the code (i.e. assert
the
> invariant for instance). This one should probably be moved below as indicated
in
> the comment below.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:46: SANDBOX_ASSERT(next
== MIN_SYSCALL);
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> It's not obvious why you'd assert that. You can assert next == 0, since this
is
> mandated in the interface.
> 
> But I think you may want to just add a comment, and probably regroup the two
> asserts here to make it clear what the starting situation is and that you can
> now iterate over public system calls.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:57: while (next <
MIN_PRIVATE_SYSCALL - 1) {
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> This is an implementation detail that we'll actually return
MIN_PRIVATE_SYSCALL
> - 1. Hopefully this will disappear soon with the "poking magic".
> 
> The test would be more clear if it just tested that we, in fact do cover the
> private syscall range: from MIN_PRIVATE_SYSCALL to MAX_PRIVATE_SYSCALL.
> 
> That being said, we currently rely on that behavior, so perhaps the best
course
> of action is to document that +1 / -1 will be tested in syscall_iterator.h ?

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:72: while (next <
MIN_GHOST_SYSCALL - 1) {
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> The same remark as above applies.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:101: SANDBOX_ASSERT(next
== 0x7FFFFFFFu);
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> I don't really like that this is testing implementation details that are not
> specified in the interface, but that's fine.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:115: // First invalid
syscall should be |MAX_PUBLIC_SYSCALL + 1|.
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> This is only true if 0 is a valid syscall, otherwise the first invalid syscall
> would be 0.
> 
> So this time, you do need to assert that 0 is MIN_SYSCALL.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc:123: next = iter.Next();
On 2012/10/13 02:44:22, Julien Tinnes wrote:
> You're testing implementation details. In fact these details should change
> (hopefully) soon as we can rid of the range testing.
> 
> I think the function was good as it was before. It really just tests that the
> iterator only yields invalid syscalls.
> 
> Or, as before, you can document the +1/-1 behavior clearly in the interface.

The notion of an iterator and the wording in the interface suggest that
|invalid_only_| returns invalid syscalls in order, so we should test that. I'll
document the +/-1 behaviour in the interface.

[jln (very slow on Chromium), 2012-10-13 17:44:55]

LGTM with some nits.

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:17: // |num_| has been initialized
to MIN_SYSCALL, which we assume is 0.
See my comment in the .h file. I think the comment should look more like what I
had written before.

We first return 0, because this is what the interlace promises we'll do. Since 0
is also conveniently MIN_SYSCALL, this simplifies a bunch of things.

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:37: : invalid_only_(invalid_only),
Yeah the indent is right, for some reason the review tool was showing me 6
indents before for this line (but strangely not the others).

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:39: num_(MIN_SYSCALL) {}
I'm sorry I'm being picky about the details, but this should really be 0 here.

The interface states that we'll start at 0. The fact that 0 is also MIN_SYSCALL
is also a convenient detail that we take advantage off in the implementation.

[Jorge Lucangeli Obes, 2012-10-15 17:25:36]

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.cc (right):

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.cc:17: // |num_| has been initialized
to MIN_SYSCALL, which we assume is 0.
On 2012/10/13 17:44:55, Julien Tinnes wrote:
> See my comment in the .h file. I think the comment should look more like what
I
> had written before.
> 
> We first return 0, because this is what the interlace promises we'll do. Since
0
> is also conveniently MIN_SYSCALL, this simplifies a bunch of things.

Done.

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:37: : invalid_only_(invalid_only),
On 2012/10/13 17:44:55, Julien Tinnes wrote:
> Yeah the indent is right, for some reason the review tool was showing me 6
> indents before for this line (but strangely not the others).

Done.

https://chromiumcodereview.appspot.com/11096012/diff/41003/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/syscall_iterator.h:39: num_(MIN_SYSCALL) {}
On 2012/10/13 17:44:55, Julien Tinnes wrote:
> I'm sorry I'm being picky about the details, but this should really be 0 here.
> 
> The interface states that we'll start at 0. The fact that 0 is also
MIN_SYSCALL
> is also a convenient detail that we take advantage off in the implementation.

Done. It's clear now what you meant.

[commit-bot: I haz the power, 2012-10-15 18:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jorgelo@chromium.org/11096012/35002

[commit-bot: I haz the power, 2012-10-15 20:33:17]

Change committed as 161943