[Chrome ELF] New NT registry API.

chrome_elf/blacklist/test/blacklist_test_main_dll.cc

Index: chrome_elf/blacklist/test/blacklist_test_main_dll.cc
diff --git a/chrome_elf/blacklist/test/blacklist_test_main_dll.cc b/chrome_elf/blacklist/test/blacklist_test_main_dll.cc
index 8c01d552975001e47c09f7c9af9239bbd5d5e81b..649b7ca9604f291c3b415ac4596ab6a0e1ad0d23 100644
--- a/chrome_elf/blacklist/test/blacklist_test_main_dll.cc
+++ b/chrome_elf/blacklist/test/blacklist_test_main_dll.cc
@@ -2,16 +2,88 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome_elf/blacklist/blacklist.h"
+
 #include <windows.h>
 
-#include "chrome_elf/blacklist/blacklist.h"
+#include "base/files/file.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/path_service.h"
+#include "chrome_elf/chrome_elf_reg.h"
 #include "chrome_elf/chrome_elf_util.h"
 
-extern "C" void InitBlacklistTestDll() {
+namespace {
+
+void GetIpcOverrides() {

[robertshield, 2016/04/20 15:45:47]

thinking about this, it looks like the test exe is writing stuff out to a file
that is then read in by the dll being loaded.

Any reason not to simply have a SetIpcOverrides() export on the dll, and have
the test call that? Is it because there is work done in DllMain that needs to
have this stuff already set up?

If so, how about sticking something in the environment block before loading the
DLL? Something like ::SetEnvironmentVariable (L"ipc_override", path) which the
DLL could read at init time.

[penny, 2016/05/28 01:34:22]

See main comment in reply.

(Excellent idea.  Works perfectly when we'll always be doing IPC within the same
process.  Done!)

+  wchar_t path[MAX_PATH] = {};
+  ::wsprintf(path, L"chrome_elf_test_%u\\", ::GetCurrentProcessId());

[robertshield, 2016/04/20 15:45:47]

PIDs get reused alarmingly frequently, please also stick the creation time here.
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce...
might be usable here.

[penny, 2016/05/28 01:34:22]

See main comment in reply.

(PID reuse isn't an issue here.  The ScopedTempDirs here are created only when
the individual test process is spawned, and they are destroyed when the test is
done and cleaning up.  No two processes will exist at the same time with the
same PID.)

+
+  base::FilePath ipc_file;
+  if (!base::PathService::Get(base::DIR_TEMP, &ipc_file))
+    return;
+  ipc_file = ipc_file.Append(path);
+  ipc_file = ipc_file.Append(L"chrome_elf_tests.txt");
+  base::File file(ipc_file, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!file.IsValid())
+    return;
+
+  DWORD size = static_cast<DWORD>(file.GetLength());
+  base::string16 content;
+  content.resize(size);
+  if (-1 == file.Read(0, (char*)content.c_str(), size)) {

[robertshield, 2016/04/20 15:45:47]

no c-style casts

also casting wchar_t* to char* should come with a comment about why this is
done. Here, I think there's a length confusion:
content.resize(size) is in terms of wchar_ts, whereas size passed to Read is
number of bytes. This won't explode, but the string will be twice as long as
needed.

[penny, 2016/05/28 01:34:22]

See main comment in reply.

Acknowledged.

+    file.Close();
+    return;
+  }
+  content.append(L"\0");
+
+  size_t index = content.find(L"hkcu=");
+  if (index != base::string16::npos) {
+    const wchar_t* ptr = content.c_str() + index;

[robertshield, 2016/04/20 15:45:47]

content.begin() + index

[penny, 2016/05/28 01:34:22]

Acknowledged.

+    ptr = ::wcschr(ptr, L'\"');

[robertshield, 2016/04/20 15:45:47]

why not use one of the find() variants that takes an offset:
http://www.cplusplus.com/reference/string/string/find/

[penny, 2016/05/28 01:34:22]

Acknowledged.

+    if (ptr != nullptr) {
+      ptr++;
+      std::vector<wchar_t>(str);
+      while (*ptr != L'\"') {
+        str.push_back(*ptr);
+        ptr++;
+      }

[robertshield, 2016/04/20 15:45:47]

I think you can replace most of this with:

size_t start_index = content.find(L"hkcu=");
size_t end_index = content.find(L"\", start_index);
base::string16 str(content.begin()+start_index, content.begin()+end_index));

you'll need to check the indexes are in range too.

[penny, 2016/05/28 01:34:22]

See main comment in reply.

Your idea would have looked cleaner.  Ack.  Noting that find returns the
position of the start of the match, so I'd have to increment past the length of
the match as well (dancing around the quotes as well).

Anyhow, this mess is fixed now.  No longer have to do string circus acts.

+      str.push_back(L'\0');
+      base::string16 substr(str.begin(), str.end());
+      nt::HKCU_override.assign(substr);
+    }
+  }
+
+  index = content.find(L"hklm=");
+  if (index != base::string16::npos) {
+    const wchar_t* ptr = content.c_str() + index;
+    ptr = ::wcschr(ptr, L'\"');
+    if (ptr != nullptr) {
+      ptr++;
+      std::vector<wchar_t>(str);
+      while (*ptr != L'\"') {
+        str.push_back(*ptr);
+        ptr++;
+      }

[robertshield, 2016/04/20 15:45:47]

ditto for simplification suggestion.

[penny, 2016/05/28 01:34:22]

Done.

+      str.push_back(L'\0');
+      base::string16 substr(str.begin(), str.end());
+      nt::HKLM_override.assign(substr);
+    }
+  }
+
+  file.Close();
+  return;
+}
+
+}  // namespace
+
+extern "C" __declspec(dllexport) void InitTestDll() {
+  // Make sure we've got the latest registry overrides.
+  GetIpcOverrides();
 }
 
 BOOL APIENTRY DllMain(HMODULE module, DWORD reason, LPVOID reserved) {
   if (reason == DLL_PROCESS_ATTACH) {
+    GetIpcOverrides();
     InitializeProcessType();
     blacklist::Initialize(true);  // force always on, no beacon
   }