Cleanup browser blacklist code

This change also involves some cleanup of how the blacklist is defined in the code.
This will make it easier to add new dll names at compile time.

The module name check is now also case-insensitive.


BUG=329023

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

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

4 files changed, 57 insertions, 26 deletions

diff --git a/chrome_elf/blacklist/blacklist.cc b/chrome_elf/blacklist/blacklist.cc
index eda611db..a90207e 100644
--- a/chrome_elf/blacklist/blacklist.cc
+++ b/chrome_elf/blacklist/blacklist.cc
@@ -19,8 +19,10 @@ extern "C" IMAGE_DOS_HEADER __ImageBase;
 
 namespace blacklist{
 
-const wchar_t* g_troublesome_dlls[kTroublesomeDllsMaxCount] = {};
-int g_troublesome_dlls_cur_index = 0;
+const wchar_t* g_troublesome_dlls[kTroublesomeDllsMaxCount] = {
+  // Keep this null pointer here to mark the end of the list.
+  NULL,
+};
 
 const wchar_t kRegistryBeaconPath[] = L"SOFTWARE\\Google\\Chrome\\BLBeacon";
 const wchar_t kBeaconVersion[] = L"version";
@@ -233,11 +235,20 @@ bool ResetBeacon() {
   return (result == ERROR_SUCCESS);
 }
 
+int BlacklistSize() {
+  int size = -1;
+  while(blacklist::g_troublesome_dlls[++size] != NULL);
+
+  return size;
+}
+
 bool AddDllToBlacklist(const wchar_t* dll_name) {
-  if (g_troublesome_dlls_cur_index >= kTroublesomeDllsMaxCount)
+  int blacklist_size = BlacklistSize();
+  // We need to leave one space at the end for the null pointer.
+  if (blacklist_size + 1 >= kTroublesomeDllsMaxCount)
     return false;
-  for (int i = 0; i < g_troublesome_dlls_cur_index; ++i) {
-    if (!wcscmp(g_troublesome_dlls[i], dll_name))
+  for (int i=0; i < blacklist_size; ++i) {
+    if (!_wcsicmp(g_troublesome_dlls[i], dll_name))
       return true;
   }
 
@@ -245,20 +256,19 @@ bool AddDllToBlacklist(const wchar_t* dll_name) {
   wchar_t* str_buffer = new wchar_t[wcslen(dll_name) + 1];
   wcscpy(str_buffer, dll_name);
 
-  g_troublesome_dlls[g_troublesome_dlls_cur_index] = str_buffer;
-  g_troublesome_dlls_cur_index++;
+  g_troublesome_dlls[blacklist_size] = str_buffer;
   return true;
 }
 
 bool RemoveDllFromBlacklist(const wchar_t* dll_name) {
-  for (int i = 0; i < g_troublesome_dlls_cur_index; ++i) {
-    if (!wcscmp(g_troublesome_dlls[i], dll_name)) {
+  int blacklist_size = BlacklistSize();
+  for (int i = 0; i < blacklist_size; ++i) {
+    if (!_wcsicmp(g_troublesome_dlls[i], dll_name)) {
       // Found the thing to remove. Delete it then replace it with the last
       // element.
-      g_troublesome_dlls_cur_index--;
       delete[] g_troublesome_dlls[i];
-      g_troublesome_dlls[i] = g_troublesome_dlls[g_troublesome_dlls_cur_index];
-      g_troublesome_dlls[g_troublesome_dlls_cur_index] = NULL;
+      g_troublesome_dlls[i] = g_troublesome_dlls[blacklist_size - 1];
+      g_troublesome_dlls[blacklist_size - 1] = NULL;
       return true;
     }
   }
diff --git a/chrome_elf/blacklist/blacklist.h b/chrome_elf/blacklist/blacklist.h
index 08ca572..17a511d 100644
--- a/chrome_elf/blacklist/blacklist.h
+++ b/chrome_elf/blacklist/blacklist.h
@@ -13,9 +13,6 @@ const int kTroublesomeDllsMaxCount = 64;
 // The DLL blacklist.
 extern const wchar_t* g_troublesome_dlls[kTroublesomeDllsMaxCount];
 
-// Cursor to the current last element in the blacklist.
-extern int g_troublesome_dlls_cur_index;
-
 // The registry path of the blacklist beacon.
 extern const wchar_t kRegistryBeaconPath[];
 
@@ -47,6 +44,9 @@ bool LeaveSetupBeacon();
 // Returns true if the beacon was successfully set to BLACKLIST_ENABLED.
 bool ResetBeacon();
 
+// Return the size of the current blacklist.
+int BlacklistSize();
+
 // Adds the given dll name to the blacklist. Returns true if the dll name is in
 // the blacklist when this returns, false on error. Note that this will copy
 // |dll_name| and will leak it on exit if the string is not subsequently removed
diff --git a/chrome_elf/blacklist/blacklist_interceptions.cc b/chrome_elf/blacklist/blacklist_interceptions.cc
index d0f64ea..6b01438 100644
--- a/chrome_elf/blacklist/blacklist_interceptions.cc
+++ b/chrome_elf/blacklist/blacklist_interceptions.cc
@@ -35,8 +35,8 @@ FARPROC GetNtDllExportByName(const char* export_name) {
 }
 
 bool DllMatch(const base::string16& module_name) {
-  for (int i = 0; i < blacklist::g_troublesome_dlls_cur_index; ++i) {
-    if (module_name == blacklist::g_troublesome_dlls[i])
+  for (int i = 0; blacklist::g_troublesome_dlls[i] != NULL; ++i) {
+    if (_wcsicmp(module_name.c_str(), blacklist::g_troublesome_dlls[i]) == 0)
       return true;
   }
   return false;
diff --git a/chrome_elf/blacklist/test/blacklist_test.cc b/chrome_elf/blacklist/test/blacklist_test.cc
index 90ce267..07623cd 100644
--- a/chrome_elf/blacklist/test/blacklist_test.cc
+++ b/chrome_elf/blacklist/test/blacklist_test.cc
@@ -5,6 +5,7 @@
 #include "base/environment.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/i18n/case_conversion.h"
 #include "base/path_service.h"
 #include "base/scoped_native_library.h"
 #include "base/strings/string16.h"
@@ -30,8 +31,8 @@ extern "C" {
 // When modifying the blacklist in the test process, use the exported test dll
 // functions on the test blacklist dll, not the ones linked into the test
 // executable itself.
-__declspec(dllimport) void TestDll_AddDllToBlacklist(const wchar_t* dll_name);
-__declspec(dllimport) void TestDll_RemoveDllFromBlacklist(
+__declspec(dllimport) bool TestDll_AddDllToBlacklist(const wchar_t* dll_name);
+__declspec(dllimport) bool TestDll_RemoveDllFromBlacklist(
     const wchar_t* dll_name);
 }
 
@@ -44,6 +45,7 @@ class BlacklistTest : public testing::Test {
   virtual void TearDown() {
     TestDll_RemoveDllFromBlacklist(kTestDllName1);
     TestDll_RemoveDllFromBlacklist(kTestDllName2);
+    TestDll_RemoveDllFromBlacklist(kTestDllName3);
   }
 };
 
@@ -95,18 +97,23 @@ TEST_F(BlacklistTest, AddAndRemoveModules) {
   EXPECT_TRUE(blacklist::RemoveDllFromBlacklist(L"foo.dll"));
   EXPECT_FALSE(blacklist::RemoveDllFromBlacklist(L"foo.dll"));
 
+  // Increase the blacklist size by 1 to include the NULL pointer
+  // that marks the end.
+  int empty_spaces = blacklist::kTroublesomeDllsMaxCount - (
+      blacklist::BlacklistSize() + 1);
   std::vector<base::string16> added_dlls;
-  added_dlls.reserve(blacklist::kTroublesomeDllsMaxCount);
-  for (int i = 0; i < blacklist::kTroublesomeDllsMaxCount; ++i) {
+  added_dlls.reserve(empty_spaces);
+  for (int i = 0; i < empty_spaces; ++i) {
     added_dlls.push_back(base::IntToString16(i) + L".dll");
     EXPECT_TRUE(blacklist::AddDllToBlacklist(added_dlls[i].c_str())) << i;
   }
   EXPECT_FALSE(blacklist::AddDllToBlacklist(L"overflow.dll"));
-  for (int i = 0; i < blacklist::kTroublesomeDllsMaxCount; ++i) {
+  for (int i = 0; i < empty_spaces; ++i) {
     EXPECT_TRUE(blacklist::RemoveDllFromBlacklist(added_dlls[i].c_str())) << i;
   }
-  EXPECT_FALSE(blacklist::RemoveDllFromBlacklist(L"0.dll"));
-  EXPECT_FALSE(blacklist::RemoveDllFromBlacklist(L"63.dll"));
+  EXPECT_FALSE(blacklist::RemoveDllFromBlacklist(added_dlls[0].c_str()));
+  EXPECT_FALSE(blacklist::RemoveDllFromBlacklist(
+    added_dlls[empty_spaces - 1].c_str()));
 }
 
 TEST_F(BlacklistTest, LoadBlacklistedLibrary) {
@@ -131,7 +138,7 @@ TEST_F(BlacklistTest, LoadBlacklistedLibrary) {
     // Add the DLL to the blacklist, ensure that it is not loaded both by
     // inspecting the handle returned by LoadLibrary and by looking for an
     // environment variable that is set when the DLL's entry point is called.
-    TestDll_AddDllToBlacklist(test_data[i].dll_name);
+    EXPECT_TRUE(TestDll_AddDllToBlacklist(test_data[i].dll_name));
     base::ScopedNativeLibrary dll_blacklisted(
         current_dir.Append(test_data[i].dll_name));
     EXPECT_FALSE(dll_blacklisted.is_valid());
@@ -140,11 +147,25 @@ TEST_F(BlacklistTest, LoadBlacklistedLibrary) {
 
     // Remove the DLL from the blacklist. Ensure that it loads and that its
     // entry point was called.
-    TestDll_RemoveDllFromBlacklist(test_data[i].dll_name);
+    EXPECT_TRUE(TestDll_RemoveDllFromBlacklist(test_data[i].dll_name));
     base::ScopedNativeLibrary dll(current_dir.Append(test_data[i].dll_name));
     EXPECT_TRUE(dll.is_valid());
     EXPECT_NE(0u, ::GetEnvironmentVariable(test_data[i].dll_beacon, NULL, 0));
     dll.Reset(NULL);
+
+    ::SetEnvironmentVariable(test_data[i].dll_beacon, NULL);
+
+    // Ensure that the dll won't load even if the name has different
+    // capitalization.
+    base::string16 uppercase_name = base::i18n::ToUpper(test_data[i].dll_name);
+    EXPECT_TRUE(TestDll_AddDllToBlacklist(uppercase_name.c_str()));
+    base::ScopedNativeLibrary dll_blacklisted_different_case(
+        current_dir.Append(test_data[i].dll_name));
+    EXPECT_FALSE(dll_blacklisted_different_case.is_valid());
+    EXPECT_EQ(0u, ::GetEnvironmentVariable(test_data[i].dll_beacon, NULL, 0));
+    dll_blacklisted_different_case.Reset(NULL);
+
+    EXPECT_TRUE(TestDll_RemoveDllFromBlacklist(uppercase_name.c_str()));
   }
 #endif
 }