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

sandbox/win/src/address_sanitizer_test.cc

+// Copyright (c) 2015 The Chromium Authors. All rights reserved.

[rvargas (doing something else), 2015/02/02 20:55:58]

nit: no (c)

[Timur Iskhodzhanov, 2015/02/03 13:25:22]

Thanks for catching this!
Self-for-the-record: I've missed
http://www.chromium.org/developers/coding-style#TOC-File-headers

+// 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/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) {
+  volatile int idx = 42;
+  int *blah = new int[42];
+  blah[idx] = 42;
+  return SBOX_TEST_FAILED;
+}
+
+#if defined(ADDRESS_SANITIZER)

[rvargas (doing something else), 2015/02/02 20:55:58]

Can we just have the whole test inside the ifdef? This pattern looks like there
is a problem with the test and something is temporarily disabled.

[Timur Iskhodzhanov, 2015/02/02 21:28:38]

The idea behind using MAYBE_ here was that I want to keep this test compileable
in all Windows builds to prevent typos from passing CQ even though ASan/Win is
not tree blocker [it will not be a blocker for a while].

What do you think?

[rvargas (doing something else), 2015/02/03 00:42:29]

That's a good point. Maybe we need another name for tests that are not really
disabled. I think we should try to have zero disabled tests, and if some tests
are in that category all the time it is pretty hard to notice that there is code
not being tested so it can regress without warnings.

So between breaking a bot because there was no compile try-bot and regressing
code because tests are disabled I'd vote for the former.

[Timur Iskhodzhanov, 2015/02/03 13:25:22]

Why do you think disabling tests leads to regressing code that much?
I assume if a test is [temporarily] disabled for a good reason.  If the reason
is not good, it doesn't matter if you disable or #ifdef, one should do neither. 
If the temporary reason for disabling is good, the test should be assigned a
crbug and the owner of the crbug should be responsible for fixing anything
caused by this exclusion.   Compiling even disabled tests [in contrast with
#ifdef'ing] might also be helpful in case you need to bisect to find a new bug
introduced while the test was disabled...

This is your codebase, not mine.  I'm #ifdef'ing the test as you said for now,
but willing to get back to MAYBE_ or ASAN_ONLY_ or whatever if you agree with my
arguments above.

[rvargas (doing something else), 2015/02/03 19:47:11]

What I've seen happening with disabled tests is that in practice they are not
prioritized accordingly with the effect (as in, the feature would have not been
checked in without the test). And that basically means that they just stay
disabled for months/years and filing a bug doesn't help (because we have
thousands of bugs open anyway).

I actually like your suggestion of leaving the test enabled and doing nothing
for a normal build (aka, minimizing the code that is not compiled by default).

+#define MAYBE_TestAddressSanitizer TestAddressSanitizer
+#else
+#define MAYBE_TestAddressSanitizer DISABLED_TestAddressSanitizer
+#endif
+TEST_F(AddressSanitizerTests, MAYBE_TestAddressSanitizer) {
+  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);
+
+  SECURITY_ATTRIBUTES attrs = {};
+  attrs.nLength = sizeof(attrs);
+  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);
+  ASSERT_NE(file_handle, INVALID_HANDLE_VALUE);
+
+  TestRunner runner;
+  ASSERT_EQ(SBOX_ALL_OK, runner.GetPolicy()->SetStderrHandle(file_handle));
+
+  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");
+  int result = runner.RunTest(L"AddressSanitizerTests_Report");
+  EXPECT_EQ(123, result);
+  EXPECT_TRUE(::CloseHandle(file_handle));
+
+  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"))
+        << "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 {
+    EXPECT_EQ("", data);
+  }
+
+  EXPECT_TRUE(::DeleteFile(temp_file_name));
+}
+
+}