Make chrome.exe built with ASan/Win work with sandbox enabled

Also add an ASan test to sbox_integration_tests on Windows

BUG=382867

Review URL: https://codereview.chromium.org/868253011

Cr-Commit-Position: refs/heads/master@{#314780}

components/nacl/loader/nacl_main_platform_delegate_win.cc

@@ -26,6 +26,13 @@ void NaClMainPlatformDelegate::EnableSandbox() {
   // Warm up language subsystems before the sandbox is turned on.
   ::GetUserDefaultLangID();
   ::GetUserDefaultLCID();
+
+#if defined(ADDRESS_SANITIZER)
+    // Bind and leak dbghelp.dll before the token is lowered, otherwise
+    // AddressSanitizer will crash when trying to symbolize a report.
+    CHECK(LoadLibraryA("dbghelp.dll"));
+#endif
+
   // Turn the sandbox on.
   target_services->LowerToken();
 }

content/common/sandbox_init_win.cc

@@ -26,11 +26,6 @@ bool InitializeSandbox(sandbox::SandboxInterfaceInfo* sandbox_info) {
     // process to swap its window station. During this time all the UI will be
     // broken. This has to run before threads and windows are created.
     if (!command_line.HasSwitch(switches::kNoSandbox)) {
-#if defined(ADDRESS_SANITIZER) && defined(OS_WIN)
-      LOG(FATAL) << "AddressSanitizer for Windows doesn't support sandboxing "
-                    "yet (http://crbug.com/382867).  "
-                    "Please rerun with sandbox disabled.";
-#endif
       // Precreate the desktop and window station used by the renderers.
       sandbox::TargetPolicy* policy = broker_services->CreatePolicy();
       sandbox::ResultCode result = policy->CreateAlternateDesktop(true);

content/common/sandbox_win.cc

@@ -339,6 +339,19 @@ bool AddGenericPolicy(sandbox::TargetPolicy* policy) {
     return false;
 #endif  // NDEBUG
 
+  // Add the policy for read-only PDB file access for AddressSanitizer.
+#if defined(ADDRESS_SANITIZER)
+  base::FilePath exe;
+  if (!PathService::Get(base::FILE_EXE, &exe))
+    return false;
+  base::FilePath pdb_path = exe.DirName().Append(L"*.pdb");
+  result = policy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
+                           sandbox::TargetPolicy::FILES_ALLOW_READONLY,
+                           pdb_path.value().c_str());
+  if (result != sandbox::SBOX_ALL_OK)
+    return false;
+#endif
+
   AddGenericDllEvictionPolicy(policy);
   return true;
 }

content/gpu/gpu_main.cc

@@ -521,6 +521,13 @@ bool StartSandboxWindows(const sandbox::SandboxInterfaceInfo* sandbox_info) {
   // content.
   sandbox::TargetServices* target_services = sandbox_info->target_services;
   if (target_services) {
+#if defined(ADDRESS_SANITIZER)
+    // Bind and leak dbghelp.dll before the token is lowered, otherwise
+    // AddressSanitizer will crash when trying to symbolize a report.
+    if (!LoadLibraryA("dbghelp.dll"))
+      return false;
+#endif
+
     target_services->LowerToken();
     return true;
   }

content/ppapi_plugin/ppapi_thread.cc

@@ -357,6 +357,12 @@ void PpapiThread::OnLoadPlugin(const base::FilePath& path,
 
     WarmupWindowsLocales(permissions);
 
+#if defined(ADDRESS_SANITIZER)
+    // Bind and leak dbghelp.dll before the token is lowered, otherwise
+    // AddressSanitizer will crash when trying to symbolize a report.
+    LoadLibraryA("dbghelp.dll");
+#endif
+
     g_target_services->LowerToken();
   }
 #endif

content/renderer/renderer_main_platform_delegate_win.cc

@@ -116,6 +116,13 @@ bool RendererMainPlatformDelegate::EnableSandbox() {
     ::GetUserDefaultLangID();
     ::GetUserDefaultLCID();
 
+#if defined(ADDRESS_SANITIZER)
+    // Bind and leak dbghelp.dll before the token is lowered, otherwise
+    // AddressSanitizer will crash when trying to symbolize a report.
+    if (!LoadLibraryA("dbghelp.dll"))
+      return false;
+#endif
+
     target_services->LowerToken();
     return true;
   }

content/utility/utility_main.cc

@@ -46,6 +46,12 @@ int UtilityMain(const MainFunctionParams& parameters) {
         parameters.sandbox_info->target_services;
     if (!target_services)
       return false;
+#if defined(ADDRESS_SANITIZER)
+    // Bind and leak dbghelp.dll before the token is lowered, otherwise
+    // AddressSanitizer will crash when trying to symbolize a report.
+    if (!LoadLibraryA("dbghelp.dll"))
+      return false;
+#endif
     target_services->LowerToken();
   }
 #endif

sandbox/win/BUILD.gn

@@ -185,6 +185,7 @@ if (cpu_arch == "x86") {
 
 test("sbox_integration_tests") {
   sources = [
+    "src/address_sanitizer_test.cc",
     "src/app_container_test.cc",
     "src/file_policy_test.cc",
     "src/handle_inheritance_test.cc",

sandbox/win/sandbox_win.gypi

@@ -218,6 +218,7 @@
         '../testing/gtest.gyp:gtest',
       ],
       'sources': [
+        'src/address_sanitizer_test.cc',
         'src/app_container_test.cc',
         'src/file_policy_test.cc',
         'src/handle_inheritance_test.cc',

sandbox/win/src/address_sanitizer_test.cc

+// Copyright 2015 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 <stdio.h>
+
+#include "base/environment.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/path_service.h"
+#include "base/win/scoped_handle.h"
+#include "base/win/windows_version.h"
+#include "sandbox/win/tests/common/controller.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sandbox {
+
+class AddressSanitizerTests : public ::testing::Test {
+ public:
+  void SetUp() {
+    env_.reset(base::Environment::Create());
+    had_asan_options_ = env_->GetVar("ASAN_OPTIONS", &old_asan_options_);
+  }
+
+  void TearDown() {
+    if (had_asan_options_)
+      ASSERT_TRUE(env_->SetVar("ASAN_OPTIONS", old_asan_options_));
+    else
+      env_->UnSetVar("ASAN_OPTIONS");
+  }
+
+ protected:
+  scoped_ptr<base::Environment> env_;
+  bool had_asan_options_;
+  std::string old_asan_options_;
+};
+
+SBOX_TESTS_COMMAND int AddressSanitizerTests_Report(int argc, wchar_t** argv) {
+  // AddressSanitizer should detect an out of bounds write (heap buffer
+  // overflow) in this code.
+  volatile int idx = 42;
+  int *blah = new int[42];
+  blah[idx] = 42;
+  delete [] blah;
+  return SBOX_TEST_FAILED;
+}
+
+TEST_F(AddressSanitizerTests, TestAddressSanitizer) {
+  // This test is only supposed to work when using AddressSanitizer.
+  // However, ASan/Win is not on the CQ yet, so compiler breakages may get into
+  // the code unnoticed.  To avoid that, we compile this test in all Windows
+  // builds, but only run the AddressSanitizer-specific part of the test when
+  // compiled with AddressSanitizer.
+#if defined(ADDRESS_SANITIZER)
+  bool asan_build = true;
+#else
+  bool asan_build = false;
+#endif
+  base::ScopedTempDir temp_directory;
+  base::FilePath temp_file_name;
+  ASSERT_TRUE(temp_directory.CreateUniqueTempDir());
+  ASSERT_TRUE(CreateTemporaryFileInDir(temp_directory.path(), &temp_file_name));
+
+  SECURITY_ATTRIBUTES attrs = {};
+  attrs.nLength = sizeof(attrs);
+  attrs.bInheritHandle = TRUE;
+  base::win::ScopedHandle tmp_handle(
+      CreateFile(temp_file_name.value().c_str(), GENERIC_WRITE,
+                 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+                 &attrs, OPEN_EXISTING, 0, NULL));
+  EXPECT_TRUE(tmp_handle.IsValid());
+
+  TestRunner runner;
+  ASSERT_EQ(SBOX_ALL_OK, runner.GetPolicy()->SetStderrHandle(tmp_handle.Get()));
+
+  base::FilePath exe;
+  ASSERT_TRUE(PathService::Get(base::FILE_EXE, &exe));
+  base::FilePath pdb_path = exe.DirName().Append(L"*.pdb");
+  ASSERT_TRUE(runner.AddFsRule(sandbox::TargetPolicy::FILES_ALLOW_READONLY,
+                               pdb_path.value().c_str()));
+
+  env_->SetVar("ASAN_OPTIONS", "exitcode=123");
+  if (asan_build) {
+    int result = runner.RunTest(L"AddressSanitizerTests_Report");
+    EXPECT_EQ(123, result);
+
+    std::string data;
+    ASSERT_TRUE(base::ReadFileToString(base::FilePath(temp_file_name), &data));
+    // Redirection uses a feature that was added in Windows Vista.
+    if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
+      ASSERT_TRUE(
+          strstr(data.c_str(), "ERROR: AddressSanitizer: heap-buffer-overflow"))
+          << "There doesn't seem to be an ASan report:\n" << data;
+      ASSERT_TRUE(strstr(data.c_str(), "AddressSanitizerTests_Report"))
+          << "The ASan report doesn't appear to be symbolized:\n" << data;
+      ASSERT_TRUE(strstr(data.c_str(), strrchr(__FILE__, '\\')))
+          << "The stack trace doesn't have a correct filename:\n" << data;
+    } else {
+      LOG(WARNING) << "Pre-Vista versions are not supported.";
+    }
+  } else {
+    LOG(WARNING) << "Not an AddressSanitizer build, skipping the run.";
+  }
+}
+
+}

sandbox/win/src/handle_inheritance_test.cc

@@ -5,6 +5,8 @@
 #include <stdio.h>
 
 #include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 #include "sandbox/win/tests/common/controller.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -18,37 +20,33 @@ SBOX_TESTS_COMMAND int HandleInheritanceTests_PrintToStdout(int argc,
 }
 
 TEST(HandleInheritanceTests, TestStdoutInheritance) {
-  wchar_t temp_directory[MAX_PATH];
-  wchar_t temp_file_name[MAX_PATH];
-  ASSERT_NE(::GetTempPath(MAX_PATH, temp_directory), 0u);
-  ASSERT_NE(::GetTempFileName(temp_directory, L"test", 0, temp_file_name), 0u);
+  base::ScopedTempDir temp_directory;
+  base::FilePath temp_file_name;
+  ASSERT_TRUE(temp_directory.CreateUniqueTempDir());
+  ASSERT_TRUE(CreateTemporaryFileInDir(temp_directory.path(), &temp_file_name));
 
   SECURITY_ATTRIBUTES attrs = {};
   attrs.nLength = sizeof(attrs);
-  attrs.lpSecurityDescriptor = NULL;
   attrs.bInheritHandle = TRUE;
-  HANDLE file_handle = CreateFile(
-      temp_file_name, GENERIC_WRITE,
-      FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
-      &attrs, OPEN_EXISTING, 0, NULL);
-  EXPECT_NE(file_handle, INVALID_HANDLE_VALUE);
+  base::win::ScopedHandle tmp_handle(
+      CreateFile(temp_file_name.value().c_str(), GENERIC_WRITE,
+                 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+                 &attrs, OPEN_EXISTING, 0, NULL));
+  ASSERT_TRUE(tmp_handle.IsValid());
 
   TestRunner runner;
-  EXPECT_EQ(SBOX_ALL_OK, runner.GetPolicy()->SetStdoutHandle(file_handle));
+  ASSERT_EQ(SBOX_ALL_OK, runner.GetPolicy()->SetStdoutHandle(tmp_handle.Get()));
   int result = runner.RunTest(L"HandleInheritanceTests_PrintToStdout");
-  EXPECT_EQ(SBOX_TEST_SUCCEEDED, result);
-  EXPECT_TRUE(::CloseHandle(file_handle));
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED, result);
 
   std::string data;
-  EXPECT_TRUE(base::ReadFileToString(base::FilePath(temp_file_name), &data));
+  ASSERT_TRUE(base::ReadFileToString(base::FilePath(temp_file_name), &data));
   // Redirection uses a feature that was added in Windows Vista.
   if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
-    EXPECT_EQ("Example output to stdout\r\n", data);
+    ASSERT_EQ("Example output to stdout\r\n", data);
   } else {
-    EXPECT_EQ("", data);
+    ASSERT_EQ("", data);
   }
-
-  EXPECT_TRUE(::DeleteFile(temp_file_name));
 }
 
 }

sandbox/win/tests/common/controller.cc

@@ -325,6 +325,13 @@ int DispatchCall(int argc, wchar_t **argv) {
     else if (EVERY_STATE == state)
       command(argc - 4, argv + 4);
 
+#if defined(ADDRESS_SANITIZER)
+    // Bind and leak dbghelp.dll before the token is lowered, otherwise
+    // AddressSanitizer will crash when trying to symbolize a report.
+    if (!LoadLibraryA("dbghelp.dll"))
+      return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND;
+#endif
+
     target->LowerToken();
   } else if (0 != _wcsicmp(argv[1], L"-child-no-sandbox")) {
     return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND;

tools/memory/asan/blacklist_win.txt

@@ -22,7 +22,6 @@ fun:*TargetNtMapViewOfSection@44
 fun:*AutoProtectMemory*sandbox*
 fun:*EatResolverThunk*sandbox*
 fun:*InterceptionAgent*sandbox*
-fun:*PolicyBase*sandbox*
 fun:*ResolverThunk*sandbox*
 fun:*Target*SandboxFactory*sandbox*
 src:*pe_image.h