Adding blacklisted dlls to safe browsing incident reports.

chrome_elf/blacklist/blacklist.cc

Index: chrome_elf/blacklist/blacklist.cc
diff --git a/chrome_elf/blacklist/blacklist.cc b/chrome_elf/blacklist/blacklist.cc
index 8107ffcab39abeaebde41f90c73a90beefc36ac4..fced32dcd0034ecf976c9946c154d053ebfd9e8d 100644
--- a/chrome_elf/blacklist/blacklist.cc
+++ b/chrome_elf/blacklist/blacklist.cc
@@ -371,7 +371,7 @@ bool AddDllsFromRegistryToBlacklist() {
   if (result != ERROR_SUCCESS)
     return false;
 
-  // We add dlls from the registry to the blacklist, and then clear registry.
+  // We add dlls from the registry to the blacklist.
   DWORD value_len;
   DWORD name_len = MAX_PATH;
   std::vector<wchar_t> name_buffer(name_len);
@@ -393,10 +393,7 @@ bool AddDllsFromRegistryToBlacklist() {
     }
   }
 
-  // Delete the finch registry key to clear the values.
-  result = ::RegDeleteKey(key, L"");
-
-  ::RegCloseKey(key);
+  result = ::RegCloseKey(key);
   return result == ERROR_SUCCESS;

[grt (UTC plus 2), 2014/06/19 20:06:59]

i don't understand the return value from this function. rather than returning
true/false based on the success or failure of adding items to the blacklist, it
returns true/false based on whether or not the key could be closed. since the
return value is ignored anyway, how about making it a void function?

[krstnmnlsn, 2014/06/19 22:13:15]

The return value is used by blacklist_test.cc. The way I've got the loop
condition I can't check for registry errors in that part (I think I saw this
format somewhere and copied it without thinking too much).  I could change the
condition to "result != ERROR_NO_MORE_ITEMS" and add a return false on the other
types failures inside?  Then at least it returns false on any failure   with the
registry.  I think for the test's purposes we'd still like to know if the key
failed to close right?  or no?

[grt (UTC plus 2), 2014/06/20 14:40:59]

since the return value isn't used by the production code, checking it in the
test is harmful since the test can potentially fail when the production code
would work fine (all values are read from the registry and used but false was
returned because the key couldn't be closed) or pass when the production code
would fail (all values couldn't be read but the key was closed).

if the call to CheckBlacklistedDllsNotLoaded in the test verifies that every
item in the registry is correctly blacklisted, then i think this can safely
become a void function and we'll have a more accurate test.

make sense?

[krstnmnlsn, 2014/06/20 18:02:58]

Okay I'll do that.

 }