* Fix style issues in nacl_sandbox_test (http://codereview.chromium.org/1549046/show).

* Use base/native_library instead of dlopen, LoadLibrary, etc.
* Change error path so that it does not simply fail a CHECK.

BUG=none
TEST=Manually test success/fail case (force failure by passing -no-sandbox flag).

Review URL: http://codereview.chromium.org/3008024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@58090 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 112 insertions, 103 deletions

diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index d9d9ed3..7c838ec 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -567,7 +567,7 @@
       ],
       'conditions': [
         ['OS=="mac"', {
-          # only Mac is using gtest for now (linking issues on other plats).
+          # Only the Mac version uses gtest (linking issues on other platforms).
           'dependencies': [
             '../testing/gtest.gyp:gtest'
           ],
@@ -598,8 +598,9 @@
             'test/nacl_security_tests/nacl_security_tests_win.cc',
           ],
         },],
-        # set fPIC for linux in case it isn't set.
-        ['OS=="linux" and (target_arch=="x64" or target_arch=="arm") and linux_fpic!=1', {
+        # Set fPIC in case it isn't set.
+        ['(OS=="linux" or OS=="openbsd" or OS=="freebsd" or OS=="solaris")'
+         'and (target_arch=="x64" or target_arch=="arm") and linux_fpic!=1', {
           'cflags': ['-fPIC'],
         },],
       ],
diff --git a/chrome/nacl/nacl_main.cc b/chrome/nacl/nacl_main.cc
index 1ea5ae4d..24d8b1d 100644
--- a/chrome/nacl/nacl_main.cc
+++ b/chrome/nacl/nacl_main.cc
@@ -121,10 +121,17 @@ int NaClMain(const MainFunctionParams& parameters) {
   if (!no_sandbox) {
     platform.EnableSandbox();
   }
-  platform.RunSandboxTests();
-
-  LaunchNaClChildProcess();
-
+  bool sandbox_test_result = platform.RunSandboxTests();
+
+  if (sandbox_test_result) {
+    LaunchNaClChildProcess();
+  } else {
+    // This indirectly prevents the test-harness-success-cookie from being set,
+    // as a way of communicating test failure, because the nexe won't reply.
+    // TODO(jvoung): find a better way to indicate failure that doesn't
+    // require waiting for a timeout.
+    LOG(INFO) << "Sandbox test failed: Not launching NaCl process";
+  }
 #else
   NOTIMPLEMENTED() << " not implemented startup, plugin startup dialog etc.";
 #endif
diff --git a/chrome/nacl/nacl_main_platform_delegate.h b/chrome/nacl/nacl_main_platform_delegate.h
index 540fb95..007f346 100644
--- a/chrome/nacl/nacl_main_platform_delegate.h
+++ b/chrome/nacl/nacl_main_platform_delegate.h
@@ -6,12 +6,12 @@
 #define CHROME_NACL_NACL_MAIN_PLATFORM_DELEGATE_H_
 #pragma once
 
+#include "base/native_library.h"
 #include "chrome/common/main_function_params.h"
 
 typedef bool (*RunNaClLoaderTests)(void);
 const char kNaClLoaderTestCall[] = "RunNaClLoaderTests";
 
-
 class NaClMainPlatformDelegate {
  public:
   explicit NaClMainPlatformDelegate(const MainFunctionParams& parameters);
@@ -26,21 +26,17 @@ class NaClMainPlatformDelegate {
   // the sandbox.
   void InitSandboxTests(bool no_sandbox);
 
-  // Initiate Lockdown, returns true on success.
-  bool EnableSandbox();
+  // Initiate Lockdown.
+  void EnableSandbox();
 
   // Runs the sandbox tests for the NaCl Loader, if tests supplied.
   // Cannot run again, after this (resources freed).
-  void RunSandboxTests();
+  // Returns false if the tests are supplied and fail.
+  bool RunSandboxTests();
 
  private:
   const MainFunctionParams& parameters_;
-#if defined(OS_WIN)
-  HMODULE sandbox_test_module_;
-  // #elif defined(OS_POSIX) doesn't seem to work on Mac.
-#elif defined(OS_LINUX) || defined(OS_MACOSX)
-  void* sandbox_test_module_;
-#endif
+  base::NativeLibrary sandbox_test_module_;
 
   DISALLOW_COPY_AND_ASSIGN(NaClMainPlatformDelegate);
 };
diff --git a/chrome/nacl/nacl_main_platform_delegate_linux.cc b/chrome/nacl/nacl_main_platform_delegate_linux.cc
index 92613a8..341108f 100644
--- a/chrome/nacl/nacl_main_platform_delegate_linux.cc
+++ b/chrome/nacl/nacl_main_platform_delegate_linux.cc
@@ -5,14 +5,12 @@
 #include "chrome/nacl/nacl_main_platform_delegate.h"
 
 #include "base/command_line.h"
-#include "base/debug_util.h"
-#include "sandbox/linux/seccomp/sandbox.h"
-
 #include "chrome/common/chrome_switches.h"
+#include "sandbox/linux/seccomp/sandbox.h"
 
 NaClMainPlatformDelegate::NaClMainPlatformDelegate(
     const MainFunctionParams& parameters)
-        : parameters_(parameters), sandbox_test_module_(NULL) {
+    : parameters_(parameters), sandbox_test_module_(NULL) {
 }
 
 NaClMainPlatformDelegate::~NaClMainPlatformDelegate() {
@@ -30,7 +28,7 @@ void NaClMainPlatformDelegate::InitSandboxTests(bool no_sandbox) {
   return;
 }
 
-bool NaClMainPlatformDelegate::EnableSandbox() {
+void NaClMainPlatformDelegate::EnableSandbox() {
   // The setuid sandbox is started in the zygote process: zygote_main_linux.cc
   // http://code.google.com/p/chromium/wiki/LinuxSUIDSandbox
   //
@@ -43,11 +41,10 @@ bool NaClMainPlatformDelegate::EnableSandbox() {
   if (switches::SeccompSandboxEnabled() && SupportsSeccompSandbox(-1))
     StartSeccompSandbox();
 #endif
-  return true;
 }
 
-void NaClMainPlatformDelegate::RunSandboxTests() {
+bool NaClMainPlatformDelegate::RunSandboxTests() {
   // The sandbox is started in the zygote process: zygote_main_linux.cc
   // http://code.google.com/p/chromium/wiki/LinuxSUIDSandbox
+  return true;
 }
-
diff --git a/chrome/nacl/nacl_main_platform_delegate_mac.mm b/chrome/nacl/nacl_main_platform_delegate_mac.mm
index c2373cb..218252b 100644
--- a/chrome/nacl/nacl_main_platform_delegate_mac.mm
+++ b/chrome/nacl/nacl_main_platform_delegate_mac.mm
@@ -5,17 +5,18 @@
 #include "chrome/nacl/nacl_main_platform_delegate.h"
 
 #import <Cocoa/Cocoa.h>
-#include <dlfcn.h>
 #import "base/chrome_application_mac.h"
 #include "base/command_line.h"
+#include "base/file_path.h"
 #include "base/logging.h"
+#include "base/native_library.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/sandbox_mac.h"
 #include "third_party/WebKit/WebKit/mac/WebCoreSupport/WebSystemInterface.h"
 
 NaClMainPlatformDelegate::NaClMainPlatformDelegate(
     const MainFunctionParams& parameters)
-        : parameters_(parameters), sandbox_test_module_(NULL) {
+    : parameters_(parameters), sandbox_test_module_(NULL) {
 }
 
 NaClMainPlatformDelegate::~NaClMainPlatformDelegate() {
@@ -41,23 +42,21 @@ void NaClMainPlatformDelegate::InitSandboxTests(bool no_sandbox) {
        ii != argstrings.end(); ++ii) {
     DLOG(INFO) << *ii;
   }
-  DLOG(INFO) << std::endl;
 
   // Be sure not to load the sandbox test DLL if the sandbox isn't on.
   // Comment-out guard and recompile if you REALLY want to test w/out the SB.
   // TODO(jvoung): allow testing without sandbox, but change expected ret vals.
   if (!no_sandbox) {
-    std::string test_dll_name =
-      command_line.GetSwitchValueASCII(switches::kTestNaClSandbox);
+    FilePath test_dll_name =
+      command_line.GetSwitchValuePath(switches::kTestNaClSandbox);
     if (!test_dll_name.empty()) {
-      sandbox_test_module_ = dlopen(test_dll_name.c_str(), RTLD_LAZY);
+      sandbox_test_module_ = base::LoadNativeLibrary(test_dll_name);
       CHECK(sandbox_test_module_);
     }
   }
-  return;
 }
 
-bool NaClMainPlatformDelegate::EnableSandbox() {
+void NaClMainPlatformDelegate::EnableSandbox() {
   CommandLine* parsed_command_line = CommandLine::ForCurrentProcess();
   SandboxInitWrapper sandbox_wrapper;
   bool sandbox_initialized_ok =
@@ -65,22 +64,25 @@ bool NaClMainPlatformDelegate::EnableSandbox() {
                                          switches::kNaClLoaderProcess);
   CHECK(sandbox_initialized_ok) << "Error initializing sandbox for "
                                 << switches::kNaClLoaderProcess;
-  return sandbox_initialized_ok;
 }
 
-
-void NaClMainPlatformDelegate::RunSandboxTests() {
+bool NaClMainPlatformDelegate::RunSandboxTests() {
+  // TODO(jvoung): Win and mac should share this identical code.
+  bool result = true;
   if (sandbox_test_module_) {
-     RunNaClLoaderTests run_security_tests =
-         reinterpret_cast<RunNaClLoaderTests>(dlsym(sandbox_test_module_,
-                                                    kNaClLoaderTestCall));
-
-    CHECK(run_security_tests);
+    RunNaClLoaderTests run_security_tests =
+      reinterpret_cast<RunNaClLoaderTests>(
+        base::GetFunctionPointerFromNativeLibrary(sandbox_test_module_,
+                                                  kNaClLoaderTestCall));
     if (run_security_tests) {
       DLOG(INFO) << "Running NaCl Loader security tests";
-      CHECK((*run_security_tests)());
+      result = (*run_security_tests)();
+    } else {
+      LOG(INFO) << "Failed to get NaCl sandbox test function";
+      result = false;
     }
-    dlclose(sandbox_test_module_);
+    base::UnloadNativeLibrary(sandbox_test_module_);
+    sandbox_test_module_ = NULL;
   }
+  return result;
 }
-
diff --git a/chrome/nacl/nacl_main_platform_delegate_win.cc b/chrome/nacl/nacl_main_platform_delegate_win.cc
index 2d90ec9..f7625e75 100644
--- a/chrome/nacl/nacl_main_platform_delegate_win.cc
+++ b/chrome/nacl/nacl_main_platform_delegate_win.cc
@@ -5,14 +5,16 @@
 #include "chrome/nacl/nacl_main_platform_delegate.h"
 
 #include "base/command_line.h"
+#include "base/file_path.h"
 #include "base/logging.h"
+#include "base/native_library.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "sandbox/src/sandbox.h"
 
 NaClMainPlatformDelegate::NaClMainPlatformDelegate(
     const MainFunctionParams& parameters)
-        : parameters_(parameters), sandbox_test_module_(NULL) {
+    : parameters_(parameters), sandbox_test_module_(NULL) {
 }
 
 NaClMainPlatformDelegate::~NaClMainPlatformDelegate() {
@@ -35,53 +37,56 @@ void NaClMainPlatformDelegate::InitSandboxTests(bool no_sandbox) {
       parameters_.sandbox_info_.TargetServices();
 
   if (target_services && !no_sandbox) {
-    std::wstring test_dll_name =
-      command_line.GetSwitchValueNative(switches::kTestNaClSandbox);
+    FilePath test_dll_name =
+      command_line.GetSwitchValuePath(switches::kTestNaClSandbox);
     if (!test_dll_name.empty()) {
       // At this point, hack on the suffix according to with bitness
       // of your windows process.
 #if defined(_WIN64)
       DLOG(INFO) << "Using 64-bit test dll\n";
-      test_dll_name.append(L"64.dll");
+      test_dll_name = test_dll_name.InsertBeforeExtension(L"64");
+    test_dll_name = test_dll_name.ReplaceExtension(L"dll");
 #else
       DLOG(INFO) << "Using 32-bit test dll\n";
-      test_dll_name.append(L".dll");
+      test_dll_name = test_dll_name.ReplaceExtension(L"dll");
 #endif
-      DLOG(INFO) << "Loading test lib " << test_dll_name << "\n";
-      sandbox_test_module_ = LoadLibrary(test_dll_name.c_str());
+      DLOG(INFO) << "Loading test lib " << test_dll_name.value() << "\n";
+      sandbox_test_module_ = base::LoadNativeLibrary(test_dll_name);
       CHECK(sandbox_test_module_);
       LOG(INFO) << "Testing NaCl sandbox\n";
     }
   }
-  return;
 }
 
-bool NaClMainPlatformDelegate::EnableSandbox() {
+void NaClMainPlatformDelegate::EnableSandbox() {
   sandbox::TargetServices* target_services =
       parameters_.sandbox_info_.TargetServices();
 
-  if (target_services) {
-    // Cause advapi32 to load before the sandbox is turned on.
-    unsigned int dummy_rand;
-    rand_s(&dummy_rand);
-    // Turn the sandbox on.
-    target_services->LowerToken();
-    return true;
-  }
-  return false;
+  CHECK(target_services) << "NaCl-Win EnableSandbox: No Target Services!";
+  // Cause advapi32 to load before the sandbox is turned on.
+  unsigned int dummy_rand;
+  rand_s(&dummy_rand);
+  // Turn the sandbox on.
+  target_services->LowerToken();
 }
 
-void NaClMainPlatformDelegate::RunSandboxTests() {
+bool NaClMainPlatformDelegate::RunSandboxTests() {
+  // TODO(jvoung): Win and mac should share this code.
+  bool result = true;
   if (sandbox_test_module_) {
-    RunNaClLoaderTests run_security_tests = reinterpret_cast<RunNaClLoaderTests>
-      (GetProcAddress(sandbox_test_module_, kNaClLoaderTestCall));
-
-    CHECK(run_security_tests);
+    RunNaClLoaderTests run_security_tests =
+      reinterpret_cast<RunNaClLoaderTests>(
+        base::GetFunctionPointerFromNativeLibrary(sandbox_test_module_,
+                                                  kNaClLoaderTestCall));
     if (run_security_tests) {
       DLOG(INFO) << "Running NaCl Loader security tests";
-      CHECK((*run_security_tests)());
+      result = (*run_security_tests)();
+    } else {
+      LOG(INFO) << "Failed to get NaCl sandbox test function";
+      result = false;
     }
-    FreeLibrary(sandbox_test_module_);
+    base::UnloadNativeLibrary(sandbox_test_module_);
+    sandbox_test_module_ = NULL;
   }
+  return result;
 }
-
diff --git a/chrome/test/nacl/nacl_sandbox_test.cc b/chrome/test/nacl/nacl_sandbox_test.cc
index 2517403..87dc619 100644
--- a/chrome/test/nacl/nacl_sandbox_test.cc
+++ b/chrome/test/nacl/nacl_sandbox_test.cc
@@ -14,9 +14,9 @@ namespace {
 const FilePath::CharType kSrpcHwHtmlFileName[] =
     FILE_PATH_LITERAL("srpc_hw.html");
 
-}  // anonymous namespace
+}  // namespace
 
-NaClSandboxTest::NaClSandboxTest() : NaClTest() {
+NaClSandboxTest::NaClSandboxTest() {
   // Append the --test-nacl-sandbox=$TESTDLL flag before launching.
   FilePath dylib_dir;
   PathService::Get(base::DIR_EXE, &dylib_dir);
@@ -33,7 +33,8 @@ NaClSandboxTest::NaClSandboxTest() : NaClTest() {
 #endif
 }
 
-NaClSandboxTest::~NaClSandboxTest() {}
+NaClSandboxTest::~NaClSandboxTest() {
+}
 
 // http://crbug.com/50121
 TEST_F(NaClSandboxTest, FLAKY_NaClOuterSBTest) {
diff --git a/chrome/test/nacl/nacl_sandbox_test.h b/chrome/test/nacl/nacl_sandbox_test.h
index f5ed74a..432e2f9f8 100644
--- a/chrome/test/nacl/nacl_sandbox_test.h
+++ b/chrome/test/nacl/nacl_sandbox_test.h
@@ -20,6 +20,9 @@ class NaClSandboxTest : public NaClTest {
  protected:
   NaClSandboxTest();
   virtual ~NaClSandboxTest();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NaClSandboxTest);
 };
 
 #endif  // CHROME_TEST_NACL_NACL_SANDBOX_TEST_H_
diff --git a/chrome/test/nacl/nacl_test.cc b/chrome/test/nacl/nacl_test.cc
index f1c2b75..03feb4d 100644
--- a/chrome/test/nacl/nacl_test.cc
+++ b/chrome/test/nacl/nacl_test.cc
@@ -21,7 +21,7 @@ const char kTestCompleteSuccess[] = "OK";
 const FilePath::CharType kBaseUrl[] =
     FILE_PATH_LITERAL("http://localhost:5103/tests/prebuilt");
 
-}  // anonymous namespace
+}  // namespace
 
 NaClTest::NaClTest()
     : UITest(), use_x64_nexes_(false), multiarch_test_(false) {
@@ -41,8 +41,7 @@ NaClTest::~NaClTest() {}
 FilePath NaClTest::GetTestRootDir() {
   FilePath path;
   PathService::Get(base::DIR_SOURCE_ROOT, &path);
-  path = path.AppendASCII("native_client");
-  return path;
+  return path.AppendASCII("native_client");
 }
 
 GURL NaClTest::GetTestUrl(const FilePath& filename) {
@@ -58,7 +57,6 @@ GURL NaClTest::GetTestUrl(const FilePath& filename) {
   return GURL(path.value());
 }
 
-
 void NaClTest::WaitForFinish(const FilePath& filename,
                              int wait_time) {
   GURL url = GetTestUrl(filename);
@@ -83,7 +81,6 @@ void NaClTest::RunMultiarchTest(const FilePath& filename, int timeout) {
   RunTest(filename, timeout);
 }
 
-
 void NaClTest::SetUp() {
   FilePath nacl_test_dir = GetTestRootDir();
 #if defined(OS_WIN)
diff --git a/chrome/test/nacl/nacl_test.h b/chrome/test/nacl/nacl_test.h
index 00bcbb4..962c897 100644
--- a/chrome/test/nacl/nacl_test.h
+++ b/chrome/test/nacl/nacl_test.h
@@ -49,9 +49,12 @@ class NaClTest : public UITest {
   // Compute the path to the test binaries (prebuilt NaCL executables).
   FilePath GetTestBinariesDir();
   bool use_x64_nexes_;
-  // This test uses HTML file that lists nexes for different architectures
+
+  // This test uses an HTML file that lists nexes for different architectures
   // in the "nexes" property
   bool multiarch_test_;
+
+  DISALLOW_COPY_AND_ASSIGN(NaClTest);
 };
 
 #endif  // CHROME_TEST_NACL_NACL_TEST_H_
diff --git a/chrome/test/nacl/nacl_ui_test.cc b/chrome/test/nacl/nacl_ui_test.cc
index 84c8950..f6faf55 100644
--- a/chrome/test/nacl/nacl_ui_test.cc
+++ b/chrome/test/nacl/nacl_ui_test.cc
@@ -10,39 +10,30 @@
 #include "chrome/common/chrome_paths.h"
 
 namespace {
-
-  // base url specified in nacl_test
-
+// NOTE: The base URL of each HTML file is specified in nacl_test.
 const FilePath::CharType kSrpcHwHtmlFileName[] =
-    FILE_PATH_LITERAL("srpc_hw.html");
-
+  FILE_PATH_LITERAL("srpc_hw.html");
 const FilePath::CharType kSrpcBasicHtmlFileName[] =
-    FILE_PATH_LITERAL("srpc_basic.html");
-
+  FILE_PATH_LITERAL("srpc_basic.html");
 const FilePath::CharType kSrpcSockAddrHtmlFileName[] =
-    FILE_PATH_LITERAL("srpc_sockaddr.html");
-
+  FILE_PATH_LITERAL("srpc_sockaddr.html");
 const FilePath::CharType kSrpcShmHtmlFileName[] =
-    FILE_PATH_LITERAL("srpc_shm.html");
-
+  FILE_PATH_LITERAL("srpc_shm.html");
 const FilePath::CharType kSrpcPluginHtmlFileName[] =
-    FILE_PATH_LITERAL("srpc_plugin.html");
-
+  FILE_PATH_LITERAL("srpc_plugin.html");
 const FilePath::CharType kSrpcNrdXferHtmlFileName[] =
-    FILE_PATH_LITERAL("srpc_nrd_xfer.html");
-
+  FILE_PATH_LITERAL("srpc_nrd_xfer.html");
 const FilePath::CharType kServerHtmlFileName[] =
-    FILE_PATH_LITERAL("server_test.html");
-
+  FILE_PATH_LITERAL("server_test.html");
 const FilePath::CharType kNpapiHwHtmlFileName[] =
-    FILE_PATH_LITERAL("npapi_hw.html");
-}  // anonymous namespace
+  FILE_PATH_LITERAL("npapi_hw.html");
+}  // namespace
 
-NaClUITest::NaClUITest() : NaClTest() {
-  // NaClTest has all we need.
+NaClUITest::NaClUITest() {
 }
 
-NaClUITest::~NaClUITest() {}
+NaClUITest::~NaClUITest() {
+}
 
 TEST_F(NaClUITest, ServerTest) {
   FilePath test_file(kServerHtmlFileName);
diff --git a/chrome/test/nacl/nacl_ui_test.h b/chrome/test/nacl/nacl_ui_test.h
index 8e5b6d0..8c98687 100644
--- a/chrome/test/nacl/nacl_ui_test.h
+++ b/chrome/test/nacl/nacl_ui_test.h
@@ -14,6 +14,9 @@ class NaClUITest : public NaClTest {
  protected:
   NaClUITest();
   virtual ~NaClUITest();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NaClUITest);
 };
 
 #endif  // CHROME_TEST_NACL_NACL_UI_TEST_H_
diff --git a/chrome/test/nacl_security_tests/commands_posix.cc b/chrome/test/nacl_security_tests/commands_posix.cc
index 79bcde7..29ba436 100644
--- a/chrome/test/nacl_security_tests/commands_posix.cc
+++ b/chrome/test/nacl_security_tests/commands_posix.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/test/nacl_security_tests/commands_posix.h"
 
+#include <errno.h>
 #include <fcntl.h>
 #include <netdb.h>
 #include <stdio.h>
@@ -45,7 +46,6 @@ SboxTestResult TestOpenWriteFile(const char *path) {
 SboxTestResult TestCreateProcess(const char *path) {
   pid_t pid;
   int exec_res;
-  int child_stat;
 
   pid = fork();
   if (0 == pid) {
@@ -57,7 +57,10 @@ SboxTestResult TestCreateProcess(const char *path) {
     }
     return SBOX_TEST_SUCCEEDED;
   } else if (0 < pid) {
-    waitpid(pid, &child_stat, WNOHANG);
+    pid_t w_pid;
+    do {
+      w_pid = waitpid(pid, NULL, WNOHANG);
+    } while (w_pid != -1 && errno != EINTR);
     return SBOX_TEST_SUCCEEDED;
   } else {
     return SBOX_TEST_DENIED;