Simplified unit testing of sandboxing code.

Simplified unit testing of sandboxing code. We now have helper methods that run all tests
inside their own processes. And we have another set of helpers that ensure we actually set
a sandboxing policy and don't forget to start the sandbox prior to running the tests.

Also simplified the handling of unexpected failure and termination of the sandbox'd process.

TODO: we still don't have a good story for testing fatal errors. We will eventually need
some form of exit tests.


BUG=n/a
TEST=sandbox_linux_unittests


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

[jln (very slow on Chromium), 2012-08-23 22:53:09]

Looks mostly good!

We can simplify die.h further to use a macro only where we *really* need to.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/die.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.cc:25: signal(SIGSEGV, SIG_DFL);
I think I would get rid of this complexity: realistically if we can't
exit_group, something is seriously wrong. I would just try to crash and then
spin.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.cc:35: syscall(__NR_exit_group, 1);
I'm a bit worried this would really *flood* logs. For instance on Ubuntu, if
exit_group is set to return an errno, this would flood the audit logs by
default.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.cc:42: sprintf(buf, "%d", line);
snprintf, please.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.cc:44: if (HANDLE_EINTR(write(2, s.c_str(),
s.length()))) { }
Depending on what stderr actually is, we may need a loop here since write could
return early. Is it overkill ? At least document it.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/die.h (right):

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.h:9: 
Nit: remove the extra line.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.h:37: } while (false)
Could you:
1. Inline the above macros.
2. Make this a SandboxDie(const string&, const string&, const string&) function.
3. Reduce the macro to a call to Die::SandboxDie(m, __FILE__, __LINE__)
4. Get rid of simple_exit() accessor because now you can read it directly from
the member function.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.h:52: static void enableSimpleExit() {
simple_exit_ = true; }
Nit: EnableSimpleExit(), capital E.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/tests/un...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/tests/un...
sandbox/linux/tests/unit_tests.cc:45: const int kCapacity = 256;
I think we want:

1. To make msg a std::string
2. Have a small char buffer.
3. read into the buffer.
4. Use msg.append(msg, rc).

That should be much nicer.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/tests/un...
sandbox/linux/tests/unit_tests.cc:55: HANDLE_EINTR(waitpid(pid, &status, 0));
We should check for error here. Maybe RunTestInProcess should even return a bool
?

[jln (very slow on Chromium), 2012-08-24 00:24:07]

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/bpf_tests.h (right):

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/bpf_tests.h:34: }                                     
                                   \
Please add a else branch here:

// TODO(markus): (crbug.com/141545) let us call the compiler from here.	
Sandbox::setSandboxPolicy(evaluator, NULL);

[jln (very slow on Chromium), 2012-08-24 00:49:06]

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/bpf_tests.h (right):

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/bpf_tests.h:11: #include <vector>
I think those includes are no longer necessary.

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/die.h (right):

https://chromiumcodereview.appspot.com/10878033/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/die.h:8: #include <vector>
I think this include can go.

[jln (very slow on Chromium), 2012-08-24 04:00:32]

Before this latest revision, we had a medium bug: sandbox tests could only fail
by writing to stderr (so a crash or exit() would not be a failure).

After the last batch of comment, you added a check for the return value, but
it's incorrect and it made things worse.

You use SANDBOX_ASSERT which will just exit the current process completely, so
the test will silently pass in all cases!

Also we're hitting some edge cases with the synthetic policy test since we
cannot write to stderr.

See the comments inline to correct this bug and a few other minor things.

All of these should be easy to fix, outside of the thing that I explicitly
mentioned I'd fix later.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/bpf_tests.h (right):

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/bpf_tests.h:33: /* TODO(markus): Call the compiler and
verify the policy. That's the */ \
I'm sorry I wasn't clear: I meant to call setSandboxPolicy() in the else branch
here.

The current re-order (calling setSandboxPolicy first) isn't great, most notably
because that's not what will happen in real code.

Also, I agree with you that having to pass policy here is an issue. I'll try to
address this later.

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

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:216:
BPF_ASSERT(syscall(syscall_number) == -1);
This won't output anything in this test to stderr because we can't write.
Right now this test will fail silently because of the additional bug that we
don't use the Gtest assert macro in unit_tests.cc.

This is really an edge case, so maybe just add a comment ?

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:22: ASSERT_EQ(pipe(fds), 0);
The true syntax is strange, it's ASSERT_EQ(expected, actual). So you should
reverse the arguments.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:43: HANDLE_EINTR(close(fds[1]));
Add (void) to silence warnings.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:49: msg.resize(len + kCapacity);
I'll address my int overflow OCD in a later CL.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:53: HANDLE_EINTR(close(fds[0]));
Add (void) to silence warnings.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:57: // control
Did you forget a word here ? Also add a final dot.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:60: int status;
Please, initialize it.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:61: HANDLE_EINTR(waitpid(pid, &status, 0));
Add a SANDBOX_ASSERT(waitpid() >= 0 here ?

Right now it's especially bad because status is not initialized.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:62: SANDBOX_ASSERT(WIFEXITED(status));
We still have the same major bug as before: we're not testing anything at the
moment!
If we exit (which SANDBOX_ASSERT does), we'll fail silently.

This needs to be a GTEST macro: ASSERT_TRUE.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:63: SANDBOX_ASSERT(WEXITSTATUS(status) ==
kExpectedValue);
Same here, this needs to be ASSERT_EQ().
Be careful with the order: ASSERT_EQ(expected, actual).

[jln (very slow on Chromium), 2012-08-24 04:02:43]

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:61: HANDLE_EINTR(waitpid(pid, &status, 0));
On 2012/08/24 04:00:32, Julien Tinnes wrote:
> Add a SANDBOX_ASSERT(waitpid() >= 0 here ?
> 
> Right now it's especially bad because status is not initialized.

Err, I mean ASSERT_GE of course, don't use SANDBOX_ASSERT here!

[jln (very slow on Chromium), 2012-08-24 04:06:42]

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:58: EXPECT_EQ(msg.size(), 0u) <<
std::string(msg.begin(), msg.end());
Currently, this will result in something such as:

../../sandbox/linux/tests/unit_tests.cc:62: Failure
Value of: msg.size()
  Actual: 88
Expected: 0u
Which is: 0

This is really confusing. I would suggest to do:

bool sub_process_has_error_message = msg.size() == 0u;
EXPECT_FALSE(sub_process_has_error_message) << XXXXXXXXXXXXX

So that GTest's error message is more clear.

[jln (very slow on Chromium), 2012-08-24 04:26:21]

Ok, this time it's the last round of remarks :)

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:58: EXPECT_EQ(msg.size(), 0u) <<
std::string(msg.begin(), msg.end());
On 2012/08/24 04:06:42, Julien Tinnes wrote:
> Currently, this will result in something such as:
> 
> ../../sandbox/linux/tests/unit_tests.cc:62: Failure
> Value of: msg.size()
>   Actual: 88
> Expected: 0u
> Which is: 0
> 
> This is really confusing. I would suggest to do:
> 
> bool sub_process_has_error_message = msg.size() == 0u;
> EXPECT_FALSE(sub_process_has_error_message) << XXXXXXXXXXXXX
> 
> So that GTest's error message is more clear.

Something such as:

  bool subprocess_has_error_message = msg.size() > 0;
  ASSERT_FALSE(subprocess_has_error_message) << "Actual test failure: " 
                                             << std::string(msg.begin(),
                                                            msg.end());

Gives a reasonable result.

https://chromiumcodereview.appspot.com/10878033/diff/6002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:63: SANDBOX_ASSERT(WEXITSTATUS(status) ==
kExpectedValue);
On 2012/08/24 04:00:32, Julien Tinnes wrote:
> Same here, this needs to be ASSERT_EQ().
> Be careful with the order: ASSERT_EQ(expected, actual).

Because of how macro expansion works, this gives a really unreadable error
message.
Use:

int test_process_exit_status = WEXITSTATUS(status);
ASSERT_EQ(kExpectedValue, test_process_exit_status);

instead, so that gtest outputs:

Value of: test_process_exit_status
  Actual: 1
Expected: kExpectedValue
Which is: 42

[jln (very slow on Chromium), 2012-08-24 04:40:40]

I have sent you a diff for unit_tests.cc

[Markus (顧孟勤), 2012-08-24 17:34:28]

On 2012/08/24 04:40:40, Julien Tinnes wrote:
> I have sent you a diff for unit_tests.cc

PTAL

I manually worked through your comments so that I would better understand what
the issues are. My changes look very similar to your diff, so I think they are
probably correct -- but another pair of eyeballs wouldn't hurt. This CL seems to
make me stupid...

[jln (very slow on Chromium), 2012-08-24 18:30:07]

Note: this discusses unit_tests.cc there are 2 previous non addressed comments
in other files.

This looks ok in general outside of a few minor things.

But I am a little bit worried about this starting to fail on some bots
mysteriously because GLIBC or something else starts to print messages to stderr
(this happened before) in one of our inner tests which makes the test fails.

I'm tempted to have the check for msg.size() only happen when we didn't return
the correct error code.
What do you think ?

I could also just change this in another CL, where I would probably add a more
robust channel to specifically report test errors.

Also: please don't forget to change the commit message. You'll also have to
manually git try -b linux_rel_precise since this try bot is not a default.

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:25: ASSERT_LT(0, (pid = fork()));
For GE, there is no "expected" value, so the previous version was correct.

I personally find the previous version easier to read, but it's up to you!

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:60: EXPECT_TRUE(completed_successfully)
This variable name will be in the GTest output. It's nice if we can give a
casual reader (such as a Sheriff watching the build bots) a good hint about
what's going on. So something mentioning "subprocess_test" or similar might me
nice. Up to you.

More important: it should still be clear what this does:
child_has_stderr_messages or something like that maybe ?

More important: "EXPECT_TRUE" will not terminate. This may be what you want, but
since this error message is the only one likely to be intelligible, it may be
better to ASSERT here so that as casual reader is spared the strange messages
about error codes below.

But see the discussion on top of this email!

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:61: << "Actual test failure: " <<
std::string(msg.begin(), msg.end());
Nit: this should be indented 4 spaces. I personally prefer having the first <<
on the previous line and align the second << with the previous one, but it's of
course up to you.

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:64: EXPECT_EQ(pid, HANDLE_EINTR(waitpid(pid,
&status, 0)));
Given how macros work, this gives an absolutely impossible message to read
because of the HANDLE_EINTR expansion.

It's unlikely to happen, so I don't mind.

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:66: EXPECT_TRUE(terminated_normally);
This one should really be an ASSERT ?

https://chromiumcodereview.appspot.com/10878033/diff/9002/sandbox/linux/tests...
sandbox/linux/tests/unit_tests.cc:68: EXPECT_EQ(kExpectedValue, exit_status);
Same here, ASSERT?

[jln (very slow on Chromium), 2012-08-24 23:05:14]

https://chromiumcodereview.appspot.com/10878033/diff/2003/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/bpf_tests.h (right):

https://chromiumcodereview.appspot.com/10878033/diff/2003/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/bpf_tests.h:40: void (*test())() const { return test_;
}
It doesn't compile on Clang. I wonder why though, this seems correct.

https://chromiumcodereview.appspot.com/10878033/diff/2003/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/bpf_tests.h:44: void (*test_)();
Would you mind renaming it to test_function_pointer or somethinig like this?

https://chromiumcodereview.appspot.com/10878033/diff/2003/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/bpf_tests.h:45: playground2::Sandbox::EvaluateSyscall
policy_;
Can you also add DISALLOW_COPY_AND_ASSIGN?

[jln (very slow on Chromium), 2012-08-24 23:11:16]

On 2012/08/24 23:05:14, Julien Tinnes wrote:
>
https://chromiumcodereview.appspot.com/10878033/diff/2003/sandbox/linux/secco...
> File sandbox/linux/seccomp-bpf/bpf_tests.h (right):
> 
>
https://chromiumcodereview.appspot.com/10878033/diff/2003/sandbox/linux/secco...
> sandbox/linux/seccomp-bpf/bpf_tests.h:40: void (*test())() const { return
test_;
> }
> It doesn't compile on Clang. I wonder why though, this seems correct.

Actually no, it's:  void (*test() const)() { return test_; }

[jln (very slow on Chromium), 2012-08-24 23:16:47]

LGTM

Please:
- amend the commit message, it's one giant line currently
- Don't forget to explicitly test on the linux_rel_precise bot.

[Markus (顧孟勤), 2012-08-25 00:20:28]

PTAL. And yes, I have officially gone crazy

[jln (very slow on Chromium), 2012-08-27 18:00:02]

On 2012/08/25 00:20:28, Markus (顧孟勤) wrote:
> PTAL. And yes, I have officially gone crazy

LGTM

[commit-bot: I haz the power, 2012-08-27 19:49:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markus@chromium.org/10878033/11005

[commit-bot: I haz the power, 2012-08-27 22:09:38]

Change committed as 153555