[Win] Create ModuleWatcher.

chrome/common/conflicts/module_watcher_win.cc

Index: chrome/common/conflicts/module_watcher_win.cc
diff --git a/chrome/common/conflicts/module_watcher_win.cc b/chrome/common/conflicts/module_watcher_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..deef1486b618413907fe78406dc44686679c88b2
--- /dev/null
+++ b/chrome/common/conflicts/module_watcher_win.cc
@@ -0,0 +1,233 @@
+// Copyright 2016 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 "chrome/common/conflicts/module_watcher_win.h"
+
+#include <tlhelp32.h>
+#include <windows.h>

[grt (UTC plus 2), 2016/11/08 14:07:52]

nit: windows.h often needs to be included before other MS headers, so we usually
put it first.

[chrisha, 2016/11/08 21:45:21]

Done.

+#include <winternl.h>  // For UNICODE_STRING.
+
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/scoped_handle.h"
+
+namespace conflicts {
+
+namespace {
+
+// These structures and functions are documented in MSDN, see
+// http://msdn.microsoft.com/en-us/library/gg547638(v=vs.85).aspx
+// there are however no headers or import libraries available in the
+// Platform SDK.
+enum {
+  // The DLL was loaded. The NotificationData parameter points to a
+  // LDR_DLL_LOADED_NOTIFICATION_DATA structure.
+  LDR_DLL_NOTIFICATION_REASON_LOADED = 1,
+  // The DLL was unloaded. The NotificationData parameter points to a
+  // LDR_DLL_UNLOADED_NOTIFICATION_DATA structure.
+  LDR_DLL_NOTIFICATION_REASON_UNLOADED = 2,
+};
+
+// Structure that is used for module load notifications.
+typedef struct _LDR_DLL_LOADED_NOTIFICATION_DATA {
+  // Reserved.
+  ULONG Flags;
+  // The full path name of the DLL module.
+  PCUNICODE_STRING FullDllName;
+  // The base file name of the DLL module.
+  PCUNICODE_STRING BaseDllName;
+  // A pointer to the base address for the DLL in memory.
+  PVOID DllBase;
+  // The size of the DLL image, in bytes.
+  ULONG SizeOfImage;
+} LDR_DLL_LOADED_NOTIFICATION_DATA, *PLDR_DLL_LOADED_NOTIFICATION_DATA;
+
+// Structure that is used for module unload notifications.
+typedef struct _LDR_DLL_UNLOADED_NOTIFICATION_DATA {
+  // Reserved.
+  ULONG Flags;
+  // The full path name of the DLL module.
+  PCUNICODE_STRING FullDllName;
+  // The base file name of the DLL module.
+  PCUNICODE_STRING BaseDllName;
+  // A pointer to the base address for the DLL in memory.
+  PVOID DllBase;
+  // The size of the DLL image, in bytes.
+  ULONG SizeOfImage;
+} LDR_DLL_UNLOADED_NOTIFICATION_DATA, *PLDR_DLL_UNLOADED_NOTIFICATION_DATA;
+
+// Union that is used for notifications.
+typedef union _LDR_DLL_NOTIFICATION_DATA {
+  LDR_DLL_LOADED_NOTIFICATION_DATA Loaded;
+  LDR_DLL_UNLOADED_NOTIFICATION_DATA Unloaded;
+} LDR_DLL_NOTIFICATION_DATA, *PLDR_DLL_NOTIFICATION_DATA;
+
+// Signature of the otification callback function.
+typedef VOID(CALLBACK* PLDR_DLL_NOTIFICATION_FUNCTION)(

[grt (UTC plus 2), 2016/11/08 14:07:52]

nit: using rather than typedef?

[chrisha, 2016/11/08 21:45:21]

Done.

+    ULONG notification_reason,
+    const LDR_DLL_NOTIFICATION_DATA* notification_data,
+    PVOID context);
+
+// Signatures of the functions used for registering DLL notification callbacks.
+typedef NTSTATUS(NTAPI* LdrRegisterDllNotificationFunc)(
+    ULONG flags,
+    PLDR_DLL_NOTIFICATION_FUNCTION notification_function,
+    PVOID context,
+    PVOID* cookie);
+typedef NTSTATUS(NTAPI* LdrUnregisterDllNotificationFunc)(PVOID cookie);
+
+// Names of the DLL notification registration functions. These are exported by
+// ntdll.
+constexpr wchar_t kNtDll[] = L"ntdll.dll";
+constexpr char kLdrRegisterDllNotification[] = "LdrRegisterDllNotification";
+constexpr char kLdrUnregisterDllNotification[] = "LdrUnregisterDllNotification";
+
+// Helper function for converting a UNICODE_STRING to an 8-bit string piece.

[grt (UTC plus 2), 2016/11/08 14:07:52]

"an 8-bit string piece" -> "a UTF-8 string"

[chrisha, 2016/11/08 21:45:21]

Done.

+std::string ToString(const UNICODE_STRING* str) {
+  std::string s;
+  CHECK(base::WideToUTF8(str->Buffer, str->Length / sizeof(wchar_t), &s));

[grt (UTC plus 2), 2016/11/08 14:07:52]

why check?

[chrisha, 2016/11/08 21:45:21]

Good point. Removed.

+  return s;
+}
+
+}  // namespace
+
+// Static helper class for forwarding system notifications to the ModuleWatcher.
+// The notifications here will be called on the thread that is doing the DLL
+// load, which could be on a variety of threads in Chrome, including threads
+// injected by third parties.
+class ModuleWatcher::Bridge {

[grt (UTC plus 2), 2016/11/08 14:07:52]

The style guide more or less says not to make a class of only static member
functions
(https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...).

[chrisha, 2016/11/08 21:45:21]

I'm generally in favour of this, but I feel this case is a warranted exception:
it lets me hide more of the implementation details (I don't need to know the
callback function signature). I've rejigged it using a static member function.

+ public:
+  // This is invoked by the OS. This forwards to OnModuleLoaded or
+  // OnModuleUnloaded depending on |notification_reason|.
+  static void CALLBACK
+  LoaderNotificationCallback(ULONG notification_reason,
+                             const LDR_DLL_NOTIFICATION_DATA* notification_data,
+                             PVOID context);
+
+  // Invoked by LoaderNotification for LDR_DLL_NOTIFICATION_REASON_LOADED
+  // events.
+  static void OnModuleLoad(const LDR_DLL_LOADED_NOTIFICATION_DATA& load_data,
+                           ModuleWatcher* module_watcher);
+
+  // Invoked by LoaderNotification for LDR_DLL_NOTIFICATION_REASON_UNLOADED
+  // events.
+  static void OnModuleUnload(
+      const LDR_DLL_UNLOADED_NOTIFICATION_DATA& unload_data,
+      ModuleWatcher* module_watcher);
+};
+
+// static
+void ModuleWatcher::Bridge::LoaderNotificationCallback(
+    ULONG notification_reason,
+    const LDR_DLL_NOTIFICATION_DATA* notification_data,
+    PVOID context) {
+  ModuleWatcher* module_watcher = reinterpret_cast<ModuleWatcher*>(context);

[grt (UTC plus 2), 2016/11/08 14:07:52]

looks like this is racy since any thread could be getting notified of a load
while another thread is destroying the watcher. wdyt of having |context| hold a
ref on a TaskRunner to which the data is posted via a WeakPtr. there are
probably other safe ways of slicing it, too.

[chrisha, 2016/11/08 21:45:21]

I had assumed (terrible idea) that the Ldr* calls were under the loader lock, as
the callbacks themselves are. Turns out this is not true (just tried it out).

I'm not sure how your suggestion works? Who would own and clean up the ref that
is the |context|? This pushes some complexity to the creator/owner of the
ModuleWatcher, which isn't ideal IMO.

Since this thing is meant to be a singleton, I could make that officially true.
I've moved to using a singleton and a static lock to ensure synchronization.
This is no worse than the synchronization I was assuming was occurring, and
doesn't artificially bind the object to any particular task runner or thread.

[grt (UTC plus 2), 2016/11/09 08:37:52]

What I was thinking is that the context ptr passed to LdrRegisterDllNotification
would not be a pointer to the ModuleWatcher instance, but rather a simple
forwarder that holds a WeakPtr to the ModuleWatcher and a ref on the
SequencedTaskRunner to which the WeakPtr is bound. This forwarder would be
"owned" by the Ldr in the sense that it would be created in
RegisterDllNotificationCallback and destroyed in
UnregisterDllNotificationCallback. I don't think you would need any additional
locking. This does mean that ModuleWatcher would need to live and die on a
specific SequencedTaskRunner. Would this be a problem? I feel that a thread
bouncing approach is more Chrome-ish than a locking approach, but maybe you
really want/need the "any thread" aspect?

Another potential benefit to the bouncing approach is that you know you're doing
very little work in the dll notification function (only posting a task), whereas
who knows what happens if you directly run the consumer's callback from within
the notification function.

[chrisha, 2016/11/09 15:38:07]

Thanks for the detailed explanation.

The only issue I have with this is that we'll already be doing thread bouncing
in the ModuleDatabaseImpl, thus we'd be pushing thread-bouncing implementation
logic to two places. In the renderer process we'll be converting the
notifications directly to Mojo IPC messages, which means they'll immediately be
thread-bounced a second time.

I agree thread-bouncing is more Chrome-ish, but it also feels like unneeded
additional complexity in this case?

(This is along the lines of what sky@ was suggesting as well. I'm ambivalent
overall, but have a mild dislike of the additional complexity.)

[grt (UTC plus 2), 2016/11/10 08:52:12]

I think that's subjective. One could make the case that this impl is more
complex because it includes locking to resolve cross-thread lifetime issues
rather than using the Chromium style of binding object lifetime to a
SequencedTaskRunner. That said, I think it would be more lines of code with the
bouncing, so this is prolly fine.

+  switch (notification_reason) {
+    case LDR_DLL_NOTIFICATION_REASON_LOADED:
+      return OnModuleLoad(notification_data->Loaded, module_watcher);

[grt (UTC plus 2), 2016/11/08 14:07:52]

returning void is a bit odd. i don't think that's very idiomatic in Chromium.

[chrisha, 2016/11/08 21:45:21]

Saved me a "break" :)

Done.

+
+    case LDR_DLL_NOTIFICATION_REASON_UNLOADED:
+      return OnModuleUnload(notification_data->Unloaded, module_watcher);
+
+    default:
+      NOTREACHED() << "Unknown LDR_DLL_NOTIFICATION_REASON: "
+                   << notification_reason;
+  }
+}
+
+// static
+void ModuleWatcher::Bridge::OnModuleLoad(
+    const LDR_DLL_LOADED_NOTIFICATION_DATA& load_data,
+    ModuleWatcher* module_watcher) {
+  mojom::ModuleEvent event;
+  event.event_type = mojom::ModuleEventType::MODULE_LOADED;
+  event.module_path = ToString(load_data.FullDllName);
+  event.load_address = reinterpret_cast<uintptr_t>(load_data.DllBase);
+  event.size = load_data.SizeOfImage;
+  return module_watcher->Notify(event);
+}
+
+// static
+void ModuleWatcher::Bridge::OnModuleUnload(
+    const LDR_DLL_UNLOADED_NOTIFICATION_DATA& unload_data,
+    ModuleWatcher* module_watcher) {
+  /*mojom::ModuleEvent event = {
+      mojom::ModuleEventType::MODULE_UNLOADED,
+      ToString(unload_data.FullDllName),
+      reinterpret_cast<uintptr_t>(unload_data.DllBase),
+      unload_data.SizeOfImage};*/
+  mojom::ModuleEvent event;
+  event.event_type = mojom::ModuleEventType::MODULE_UNLOADED;
+  event.module_path = ToString(unload_data.FullDllName);
+  event.load_address = reinterpret_cast<uintptr_t>(unload_data.DllBase);
+  event.size = unload_data.SizeOfImage;
+  return module_watcher->Notify(event);
+}
+
+ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback)
+    : callback_(callback), dll_notification_cookie_(nullptr) {
+  CHECK(RegisterDllNotificationCallback());

[grt (UTC plus 2), 2016/11/08 14:07:52]

should a failure here really prevent Chrome from running at all?

[chrisha, 2016/11/08 21:45:21]

No, it shouldn't.

Done.

+  EnumerateAlreadyLoadedModules();
+}
+
+ModuleWatcher::~ModuleWatcher() {
+  CHECK(UnregisterDllNotificationCallback());

[grt (UTC plus 2), 2016/11/08 14:07:51]

same comment here

[chrisha, 2016/11/08 21:45:21]

Ditto.

+}
+
+bool ModuleWatcher::RegisterDllNotificationCallback() {
+  LdrRegisterDllNotificationFunc reg_fn =
+      reinterpret_cast<LdrRegisterDllNotificationFunc>(::GetProcAddress(
+          ::GetModuleHandle(kNtDll), kLdrRegisterDllNotification));
+
+  if (!reg_fn)
+    return false;
+
+  NTSTATUS status = reg_fn(0, &Bridge::LoaderNotificationCallback, this,
+                           &dll_notification_cookie_);
+  return status == 0;
+}
+
+bool ModuleWatcher::UnregisterDllNotificationCallback() {
+  LdrUnregisterDllNotificationFunc unreg_fn =
+      reinterpret_cast<LdrUnregisterDllNotificationFunc>(::GetProcAddress(
+          ::GetModuleHandle(kNtDll), kLdrUnregisterDllNotification));
+
+  if (!unreg_fn)
+    return false;
+
+  NTSTATUS status = unreg_fn(dll_notification_cookie_);
+  return status == 0;
+}
+
+bool ModuleWatcher::EnumerateAlreadyLoadedModules() {
+  // Get all modules in the current process.
+  base::win::ScopedHandle snap(
+      ::CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, ::GetCurrentProcessId()));

[grt (UTC plus 2), 2016/11/08 14:07:52]

?TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32?

[chrisha, 2016/11/08 21:45:21]

Indeed... cut and paste code from XP days.

+  if (!snap.IsValid())

[grt (UTC plus 2), 2016/11/08 14:07:52]

msdn says to retry if it fails with ERROR_BAD_LENGTH. do you want to do that
here?

[chrisha, 2016/11/08 21:45:21]

Yeah, not a terrible idea. I'll put an upper bound on it.

+    return false;
+
+  // Walk the module list.
+  MODULEENTRY32 module = {sizeof(module)};
+  if (!::Module32First(snap.Get(), &module))
+    return false;
+
+  do {
+    std::string s;
+    CHECK(base::WideToUTF8(module.szExePath, ::wcslen(module.szExePath), &s));

[grt (UTC plus 2), 2016/11/08 14:07:52]

why crash?

[grt (UTC plus 2), 2016/11/08 14:07:52]

how about doing this directly into event.module_path rather than using |s|?

[chrisha, 2016/11/08 21:45:21]

Done.

[chrisha, 2016/11/08 21:45:21]

It's a mojo::String, and not a std::string :(

+
+    mojom::ModuleEvent event;
+    event.event_type = mojom::ModuleEventType::MODULE_ALREADY_LOADED;
+    event.module_path = s;
+    event.load_address = reinterpret_cast<uintptr_t>(module.modBaseAddr);
+    event.size = module.modBaseSize;
+    Notify(event);
+  } while (::Module32Next(snap.Get(), &module));
+
+  return true;
+}
+
+void ModuleWatcher::Notify(const mojom::ModuleEvent& event) {
+  callback_.Run(event);
+}
+
+}  // namespace conflicts