Make sure Chrome_elf.dll imports are correct and that it the first import of chrome.exe

chrome_elf/elf_imports_unittest.cc

Index: chrome_elf/elf_imports_unittest.cc
diff --git a/chrome_elf/elf_imports_unittest.cc b/chrome_elf/elf_imports_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..36989b0bde37823c7030c1f511098c3f79b02a4e
--- /dev/null
+++ b/chrome_elf/elf_imports_unittest.cc
@@ -0,0 +1,110 @@
+// Copyright 2013 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 <stdint.h>
+#include <windows.h>
+
+#include <algorithm>
+#include <vector>
+
+#include "base/base_paths.h"
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "base/strings/string_util.h"
+#include "base/win/pe_image.h"
+#include "base/win/scoped_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const char* kPatterns[] = { "KERNEL32.dll", "MSVC*", "ADVAPI32.dll" };

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

nit: since this is used only once, I'd move it to the point of usage, and make
it a function static. 
Also, this is a non-const array of pointers to const char, which is perhaps not
exactly what you intended. 

const char* const kPatterns[]

is what you want - this'll make it impossible to (accidentally) stomp on the
expectations.

Also perhaps e.g. kValidFilePatterns for name.

And now I'm all nitted out on this one :).

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+
+bool ImportsCallback(const base::win::PEImage &image,

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

nit: might as well make this a static member of the fixture. For maximal
cleanliness, you could make it a privat member, though protected is fine in
tests.

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+                     LPCSTR module,
+                     PIMAGE_THUNK_DATA name_table,
+                     PIMAGE_THUNK_DATA iat,
+                     PVOID cookie) {
+  std::vector<std::string>* import_list =
+      reinterpret_cast<std::vector<std::string>*>(cookie);
+  LOG(INFO) << module;

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

did you intend to leave this in?

[Cait (Slow), 2013/12/19 19:52:00]

oops

+  import_list->push_back(module);
+  return true;
+}
+
+class ELFImportsTest : public testing::Test {
+ protected:
+  void GetImports(const wchar_t* module_name,

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

might as well pass a FilePath in here, it's more explicit.

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+                  std::vector<std::string>* imports) {
+    if (!imports)

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

ASSERT_TRUE(imports != NULL) or ASSERT_TRUE(imports) is more appropriate in a
test IMHO. I'd regard it a programming error to call this function with a NULL
imports pointer.

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+      return;
+
+    base::win::ScopedHandle module_file_handle(::CreateFile(module_name,

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

Is base::MemoryMappedFile (base/files/...) not suitable here?
I think it may take care of much of the grunge work to close mappings and
handles and stuff automatically?

[Cait (Slow), 2013/12/19 19:52:00]

Yes it is..and it simplifies things a lot -- thanks!

+        GENERIC_WRITE | GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL));
+
+    ASSERT_TRUE(module_file_handle.IsValid());
+
+    base::win::ScopedHandle module_file_mapping(::CreateFileMapping(
+        module_file_handle, NULL, PAGE_READWRITE, 0, 0, NULL));
+
+    ASSERT_TRUE(module_file_mapping.IsValid());
+
+    void* module_file_view = MapViewOfFile(

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

::MapViewOfFile?

[Cait (Slow), 2013/12/19 19:52:00]

Not needed any more

+        module_file_mapping, FILE_MAP_WRITE, 0, 0, 0);
+
+    if (module_file_view != NULL) {

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

Better to ASSERT consistently. This will ensure your test fails on weird
conditions, whereas you'd end up succeeding the test on what appears to be an
empty imports list if this condition hits.

+      base::win::PEImageAsData pe_image_data(
+          reinterpret_cast<HMODULE>(module_file_view));
+      pe_image_data.EnumImportChunks(ImportsCallback, imports);
+
+      EXPECT_TRUE(::UnmapViewOfFile(module_file_view));
+      module_file_mapping.Close();

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

This close operation is weirdly scoped, but it's also I think redundant. The
ScopedHandles will close when they go out of scope.

+    }
+
+    module_file_handle.Close();

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

also redundant, I think?

+  }
+};
+
+TEST_F(ELFImportsTest, ChromeElfSanityCheck) {
+  std::vector<std::string> elf_imports;
+
+  base::FilePath dll;
+  PathService::Get(base::DIR_MODULE, &dll);
+  dll = dll.Append(L"chrome_elf.dll");
+  GetImports(dll.value().c_str(), &elf_imports);
+
+  // Check that ELF has imports.
+  ASSERT_GT(elf_imports.size(), 0u);

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

nit: the ASSERT_XX macros want ASSERT_XX(expected, actual), and they format the
output accordingly on failure.

Here you want ASSERT_LT, I think?

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+
+  std::vector<std::string>::iterator it = elf_imports.begin();
+
+  // Make sure all of ELF's imports are in are valid imports list.
+  for (; it != elf_imports.end(); it++) {
+    bool match = false;
+    for (int i = 0; i < arraysize(kPatterns); i++) {
+      if (MatchPattern(*it, kPatterns[i]))
+        match = true;
+    }
+    if (!match)

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

hint; you can also

ASSERT_TRUE(match) << "....";

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+      FAIL() << "Illegal import in chrome_elf.dll.";
+  }
+}
+
+TEST_F(ELFImportsTest, ChromeExeSanityCheck) {
+  std::vector<std::string> exe_imports;
+
+  base::FilePath exe;
+  PathService::Get(base::DIR_EXE, &exe);

[Sigurður Ásgeirsson, 2013/12/18 21:30:48]

DIR_EXE and DIR_MODULE will produce the same results when you call them from an
EXE, as the EXE is the MODULE in this case. I'd go with DIR_EXE in both tests,
though I'm not sure what's common practice for tests to locate other test
executables. I'm sure CSharp will know, as he's worked on test configuration
involving isolates and such...

[Cait (Slow), 2013/12/19 19:52:00]

Done.

+  exe = exe.Append(L"chrome.exe");
+  GetImports(exe.value().c_str(), &exe_imports);
+
+  // Check that chrome.exe has imports.
+  ASSERT_GT(exe_imports.size(), 0u);
+
+  // Chrome.exe's first import must be ELF.
+  EXPECT_EQ("chrome_elf.dll", exe_imports[0]);
+}
+
+}  // namespace