Make second param for TestChildProcess optional.

sandbox/win/src/process_mitigations_test.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/strings/stringprintf.h"
@@ skipping 187 matching lines @@
   EXPECT_EQ((is_success_test ? sandbox::SBOX_TEST_SUCCEEDED
                              : sandbox::SBOX_TEST_FAILED),
             runner.RunTest(test.c_str()));
 }
 
 }  // namespace
 
 namespace sandbox {
 
 // A shared helper test command that will attempt to CreateProcess
-// with a given command line. The second parameter, if set to non-zero
+// with a given command line. The second optional parameter, if set to non-zero

[penny, 2016/04/01 17:20:30]

<whisper>comma after 'non-zero'?</whisper>

:)

[Will Harris, 2016/04/01 18:23:52]

actually it had a comma but it went over 80chars so I removed it. sigh. I'll
just add a new line... actually I'll just change the comment. and the code.

[penny, 2016/04/01 18:35:36]

ROFL

 // will cause the child process to return exit code STATUS_ACCESS_VIOLATION.
 //
 // ***Make sure you've enabled basic process creation in the
 // test sandbox settings via:
 // sandbox::TargetPolicy::SetJobLevel(),
 // sandbox::TargetPolicy::SetTokenLevel(),
 // and TestRunner::SetDisableCsrss().
 SBOX_TESTS_COMMAND int TestChildProcess(int argc, wchar_t** argv) {
-  if (argc < 2)
+  if (argc < 1)
     return SBOX_TEST_INVALID_PARAMETER;
 
-  int desired_exit_code = _wtoi(argv[1]);
-  if (desired_exit_code)
+  int desired_exit_code = 0;
+
+  if (argc == 2) {
+    desired_exit_code = _wtoi(argv[1]);
+    if (desired_exit_code)
       desired_exit_code = STATUS_ACCESS_VIOLATION;
+  }
 
   std::wstring cmd = argv[0];
   base::LaunchOptions options = base::LaunchOptionsForTest();
   base::Process setup_proc = base::LaunchProcess(cmd.c_str(), options);
 
   if (setup_proc.IsValid()) {
     setup_proc.Terminate(desired_exit_code, false);
     return SBOX_TEST_SUCCEEDED;
   }
   // Note: GetLastError from CreateProcess returns 5, "ERROR_ACCESS_DENIED".
@@ skipping 470 matching lines @@
   test_command += cmd.value().c_str();
   test_command += L" 0";
 
   EXPECT_EQ(SBOX_TEST_FAILED, runner.RunTest(test_command.c_str()));
 }
 
 // This test validates that we can spawn a child process if
 // MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED mitigation is
 // not set. This also tests that a crashing child process is correctly handled
 // by the broker.
 TEST(ProcessMitigationsTest, CheckChildProcessSuccessAbnormalExit) {

[penny, 2016/04/01 17:20:30]

In terms of process_mitigations_test.cc, this new test appears to behave exactly
the same as CheckChildProcessSuccess - they both result in an
SBOX_TEST_SUCCEEDED return from TestChildProcess.

I think I know that this test is actually meant to test the job messages Chrome
receives from the OS (ref crbug/584753), and that a "failure case" would show up
as a DCHECK in broker_services.cc, TargetEventsThread() (where we handle job
messages).

I'd recommend adding a detailed header & comment above this test, explaining
what it's testing, and point to the code where the DCHECK would happen in a
failure case (or link to the bug I mentioned above).  It would make it a lot
easier to understand what this test is actually doing, and why.

This is not really a test of our mitigations that prevent child process
creation.  It's a great new test to ensure we're handling job messages properly
- messages that we now know differ between versions of Windows.  In some ways,
this test belongs with any other Job-related tests (if they exist), but sbox
test environment is uniquely excellent for this test case, so I'm happy with
leaving it here (with a detailed comment).

[Will Harris, 2016/04/01 18:23:52]

yes good point this should maybe go into job tests but those aren't integration
tests (i.e. they don't spawn child processes) as they are in sbox_unittests, so
I think it's okay to leave them here.

Regarding comments, we don't usually link bugs in code because it's always easy
(in theory) to look at commit logs and commits will link to the bugs, otherwise
there would be crbug links everywhere in the code... I suppose that's just
chromium style.

I've added a more detailed comment.

   TestRunner runner;
   sandbox::TargetPolicy* policy = runner.GetPolicy();
 
   // Set a policy that would normally allow for process creation.
   policy->SetJobLevel(JOB_INTERACTIVE, 0);
   policy->SetTokenLevel(USER_UNPROTECTED, USER_UNPROTECTED);
   runner.SetDisableCsrss(false);
 
   base::FilePath cmd;
   EXPECT_TRUE(base::PathService::Get(base::DIR_SYSTEM, &cmd));
   cmd = cmd.Append(L"calc.exe");
 
   std::wstring test_command = L"TestChildProcess ";
   test_command += cmd.value().c_str();
   test_command += L" 1";
 
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(test_command.c_str()));
 }
 
 // This test validates that setting the
 // MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED mitigation prevents
 // the spawning of child processes.  This also tests that a crashing child
 // process is correctly handled by the broker.
 TEST(ProcessMitigationsTest, CheckChildProcessFailureAbnormalExit) {

[penny, 2016/04/01 17:20:30]

I'm fairly sure this new test has the exact same run-time flow as
CheckChildProcessFailure.  There is no abnormal exit involved here, as the
actual launch of the process in TestChildProcess function fails due to our
sandbox settings (JOB level).  You don't get to terminate the child process with
any abnormal failure, because it never got launched.  So these two process
failure tests are identical.  I'd recommend removing this one.

[Will Harris, 2016/04/01 18:23:52]

Done. Removed.

   TestRunner runner;
   sandbox::TargetPolicy* policy = runner.GetPolicy();
 
   // Now set the job level to be <= JOB_LIMITED_USER
   // and ensure we can no longer create a child process.
   policy->SetJobLevel(JOB_LIMITED_USER, 0);
   policy->SetTokenLevel(USER_UNPROTECTED, USER_UNPROTECTED);
   runner.SetDisableCsrss(false);
 
   base::FilePath cmd;
   EXPECT_TRUE(base::PathService::Get(base::DIR_SYSTEM, &cmd));
   cmd = cmd.Append(L"calc.exe");
 
   std::wstring test_command = L"TestChildProcess ";
   test_command += cmd.value().c_str();
   test_command += L" 1";
 
   EXPECT_EQ(SBOX_TEST_FAILED, runner.RunTest(test_command.c_str()));
 }
 
 }  // namespace sandbox