* 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

5 files changed, 74 insertions, 67 deletions

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;
 }
-