Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576843002:
[win] Create ModuleDatabase and ModuleEventSinkImpl. (Closed)
|
Created:
4 years ago by chrisha Modified:
3 years, 11 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[win] Create ModuleDatabase and ModuleEventSinkImpl.
This creates the browser-side components of the ModuleDatabase. A
future CL will hook up ModuleWatchers in each process to a singleton
ModuleDatabase in the browser process.
BUG=662084
Review-Url: https://codereview.chromium.org/2576843002
Cr-Original-Commit-Position: refs/heads/master@{#443651}
Committed: https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2dd7160c45331
Review-Url: https://codereview.chromium.org/2576843002
Cr-Commit-Position: refs/heads/master@{#444241}
Committed: https://chromium.googlesource.com/chromium/src/+/6de20f5d8a70cf65a5346eff0adcef7df500dde5
Patch Set 1 #
Total comments: 63
Patch Set 2 : Address grt's comments, merge CL 2579143002 #Patch Set 3 : git cl format #Patch Set 4 : Rework OnProcessStarted. #
Total comments: 119
Patch Set 5 : Address grt's comments. #
Total comments: 10
Patch Set 6 : Address comments for patchset 5. #Patch Set 7 : Moar comments, fix typos. #
Total comments: 55
Patch Set 8 : Address grt's comments on patchset 7. #
Total comments: 7
Patch Set 9 : Fix grt's nits on patchset 8. #
Total comments: 32
Patch Set 10 : Address sky's comments. #Patch Set 11 : Address sky@'s comments. #
Total comments: 5
Patch Set 12 : Fix clang warning. #Patch Set 13 : Fix win-clang error. #Patch Set 14 : Fix another win-clang error. #Patch Set 15 : Yet another win-clang error. #Patch Set 16 : Fix small bug. #Dependent Patchsets: Messages
Total messages: 65 (27 generated)
chrisha@chromium.org changed reviewers: + grt@chromium.org, pmonette@chromium.org, sky@chromium.org
pmonette: PTAL? grt: If you have time? sky: As an OWNER?
You'll also need a security reviewer. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:34: DCHECK_EQ(0u, process_id_); Won't a misbehaving client cause problems here? Whoever you have review this should consider this from a security perspective. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:35: process_id_ = process_id; DCHECK_NE(0u, process_id_); https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:37: process_id, static_cast<content::ProcessType>(process_type)); You need to valid process_type. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:16: class ModuleDatabaseImpl : public mojom::ModuleDatabase { optional: I've been encouraging people to use chrome:: as the namespace for mojoms. We don't appear to be consistent though, so optional. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:28: static void Create(::ModuleDatabase* module_database, How do you ensure ModuleDatabase outlives this? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:41: ::ModuleDatabase* module_database() const { return module_database_; } Either have this return a const ModuleDatabase* or remove const. optional: I would move this into the private section and friend the test, but that isn't a standard so I leave it to you to decide. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:51: }; DISALLOW... https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win_unittest.cc:43: std::unique_ptr<base::MessageLoop> message_loop_; protected: https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win_unittest.cc:46: }; private: DISALLOW... https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:45: DCHECK(process_info); Doesn't this really depend upon how fast messages are picked up? If a process id is reused rapidly then I could see this check being racy, but perhaps OSs don't recycle that quickly? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:44: // Converts |process_type| to the corresponding bit. Exposed in the header for Without looking at the implementation it isn't clear what 'corresponding bit' means here. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:50: static content::ProcessType BitIndexToProcessType(uint32_t bit_index); Same comment about bit index here. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:133: base::WeakPtrFactory<ModuleDatabase> weak_ptr_factory_; Make this the last members (I'm surprised you didn't hit a warning when uploading).
some comments for you, sir! https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:21: base::WrapUnique(new ModuleDatabaseImpl(module_database)); base::MakeUnique<ModuleDatabaseImpl>(module_database) and #include "base/memory/ptr_util.h" https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:23: base::Bind(&ModuleDatabaseImpl::OnProcessEnded, #include "base/bind.h" #include "base/callback.h" https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:21: explicit ModuleDatabaseImpl(::ModuleDatabase* module_database); is it common practice to have colliding names like this (one in mojom and one out of it)? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:47: // The cached process ID of the remote process on the other end of this pipe. nit: blank line before this https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:50: uint32_t process_id_; pid + creation time are needed to uniquely identify a process on windows. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:65: // worth crashing if this data is slightly out of sync as this is purely if it's not worth crashing, why is there a DCHECK? will the DCHECK do anything other than annoy/confuse some other random developer who happens to trip on this? see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... for guidance on DCHECK https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:73: uint32_t process_type_bit = ProcessTypeToBit(process_info->process_type); nit: omit variable https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:86: default: remove this so that compilation will fail if a new type is ever added https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:110: DCHECK_LE(0u, bit_index); nit: i find these more natural the other way around since that's the way we write general comparisons (i.e., "if (0 <= bit_index)" is backwards). https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:123: ModuleDatabase::ModuleInfo::ModuleInfo(const base::FilePath& module_path, nit: keep class function definitions grouped by class (so either move these above or below the ModuleDatabase functions). i'm partial to adding comments like: // ModuleDatabase::ModuleInfo -------------------------------------------------- to separate the blocks of functions. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:46: static uint32_t ProcessTypeToBit(content::ProcessType process_type); nit: move these function decls down below the types as per https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:67: const uint32_t module_id; int is the type of choice for things that don't explicitly need to be otherwise (https://google.github.io/styleguide/cppguide.html#Integer_Types). also, since the sets in ProcessInfo contain these, i mildly suggest making an explicit type for this (e.g., "using ModuleId = int;" in this struct, or somesuch). https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:70: // this struct will have their constness casted away at runtime, allowing use "mutable" keyword rather than casting? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:80: // Key comparator for ModuleInfo, allowing it to be used in a set. is it relevant to document that it compares by module_path? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:103: std::unordered_set<uint32_t> loaded_module_ids; will these be large enough that std::vector (sorted or unsorted) won't be better in all regards (less memory overhead, better cache performance, etc)? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win_unittest.cc:56: public: nit: protected https://codereview.chromium.org/2576843002/diff/1/chrome/common/conflicts/mod... File chrome/common/conflicts/module_database_win.mojom (right): https://codereview.chromium.org/2576843002/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_database_win.mojom:15: OnProcessStarted(uint32 process_id, uint32 process_type); does |process_id| come across the wire from the child proc? if yes, why? does the browser not know its children's pids?
Description was changed from ========== [win] Create ModuleDatabase and ModuleDatabaseImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 ========== to ========== [win] Create ModuleDatabase and ModuleDatabaseImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 ==========
wfh@chromium.org changed reviewers: + wfh@chromium.org
Much rework after discussions with wfh@ regarding security. Please take another look at your leisure :) https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:21: base::WrapUnique(new ModuleDatabaseImpl(module_database)); On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > base::MakeUnique<ModuleDatabaseImpl>(module_database) > > and #include "base/memory/ptr_util.h" Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:23: base::Bind(&ModuleDatabaseImpl::OnProcessEnded, On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > #include "base/bind.h" > #include "base/callback.h" bind.h I get, but why callback.h? I don't make use of base::Callback anywhere in this file? https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:34: DCHECK_EQ(0u, process_id_); On 2016/12/14 21:58:08, sky wrote: > Won't a misbehaving client cause problems here? Whoever you have review this > should consider this from a security perspective. The ModuleDatabase itself is robust to misuse, as noted in its comments. I was torn between making this class itself robust (as it simply forwards existing messages, although with some extra adornment), or simply documenting the expected use via DCHECKs. I'll talk to security about best practices here. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:35: process_id_ = process_id; On 2016/12/14 21:58:08, sky wrote: > DCHECK_NE(0u, process_id_); Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.cc:37: process_id, static_cast<content::ProcessType>(process_type)); On 2016/12/14 21:58:08, sky wrote: > You need to valid process_type. Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:16: class ModuleDatabaseImpl : public mojom::ModuleDatabase { On 2016/12/14 21:58:08, sky wrote: > optional: I've been encouraging people to use chrome:: as the namespace for > mojoms. We don't appear to be consistent though, so optional. Yeah, I've noticed both. A quick check on cs/ shows an order of magnitude more usage of mojom:: without the chrome::. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:21: explicit ModuleDatabaseImpl(::ModuleDatabase* module_database); On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > is it common practice to have colliding names like this (one in mojom and one > out of it)? I've seen all things. The most common is a mojom::Foo interface, and a FooImpl implementation of that interface on the other end of the pipe. In this case, the FooImpl is a thin facade to a singleton Foo. I was torn about naming, but in the end preferred the colliding namespaced names. I've also seen: mojom::FooControllerClient (interface definition) FooController (implementation of interface) Foo (the central thing they all talk to) I'm open to suggestions here! https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:28: static void Create(::ModuleDatabase* module_database, On 2016/12/14 21:58:08, sky wrote: > How do you ensure ModuleDatabase outlives this? My plan was for ModuleDatabase to be a leaky singleton. It's needed for the lifetime of the entire browser process. However, I'd be just as happy using shared_ptr if you have a strong preference. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:41: ::ModuleDatabase* module_database() const { return module_database_; } On 2016/12/14 21:58:08, sky wrote: > Either have this return a const ModuleDatabase* or remove const. > optional: I would move this into the private section and friend the test, but > that isn't a standard so I leave it to you to decide. I'll drop the accessors entirely and use a friend declaration. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:47: // The cached process ID of the remote process on the other end of this pipe. On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > nit: blank line before this Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:50: uint32_t process_id_; On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > pid + creation time are needed to uniquely identify a process on windows. This object is bound in lifetime to the corresponding process, and lives and dies with it (guaranteed by Mojo). So I was worried about colliding process IDs. There is a very minor potential for collision in the ::ModuleDatabase itself, if the OS reuses a PID before the ProcessEnd message gets posted, and a ProcessStart ends up getting posted before it due to a race. But the chances of this are *very* small given that the OS tends to avoid PID reuse on short timescales. If you feel strongly about this I could augment with the creation time. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win.h:51: }; On 2016/12/14 21:58:08, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_impl_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win_unittest.cc:43: std::unique_ptr<base::MessageLoop> message_loop_; On 2016/12/14 21:58:08, sky wrote: > protected: Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_impl_win_unittest.cc:46: }; On 2016/12/14 21:58:08, sky wrote: > private: DISALLOW... Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:45: DCHECK(process_info); On 2016/12/14 21:58:08, sky (OOO) wrote: > Doesn't this really depend upon how fast messages are picked up? If a process id > is reused rapidly then I could see this check being racy, but perhaps OSs don't > recycle that quickly? Yes, this indeed could depend on the order in which the messages are picked up. I was betting on process ID reuse being sufficiently spaced out that you'd never have two at the same time. If we think this is going to be an issue then we could move to using the (process_id, process_creation_time) pair, as suggested by grt. In the meantime, I've reworked the logic to be resistant to out of order messages. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:65: // worth crashing if this data is slightly out of sync as this is purely On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > if it's not worth crashing, why is there a DCHECK? will the DCHECK do anything > other than annoy/confuse some other random developer who happens to trip on > this? see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > for guidance on DCHECK Not worth crashing at all. Removed. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:73: uint32_t process_type_bit = ProcessTypeToBit(process_info->process_type); On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > nit: omit variable Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:86: default: On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > remove this so that compilation will fail if a new type is ever added Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:110: DCHECK_LE(0u, bit_index); On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > nit: i find these more natural the other way around since that's the way we > write general comparisons (i.e., "if (0 <= bit_index)" is backwards). I'm impartial, but over the years had reviewers drill into me this "backwards" way of doing things (that the constant should be first). https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:123: ModuleDatabase::ModuleInfo::ModuleInfo(const base::FilePath& module_path, On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > nit: keep class function definitions grouped by class (so either move these > above or below the ModuleDatabase functions). i'm partial to adding comments > like: > > // ModuleDatabase::ModuleInfo -------------------------------------------------- > > to separate the blocks of functions. I had kept the declarations and definitions in the same order. I'll move the inline declarations out of line (move to forward declares) in the header, and also maintain a consistent order of definitions. (So, done.) https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:44: // Converts |process_type| to the corresponding bit. Exposed in the header for On 2016/12/14 21:58:08, sky wrote: > Without looking at the implementation it isn't clear what 'corresponding bit' > means here. Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:46: static uint32_t ProcessTypeToBit(content::ProcessType process_type); On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > nit: move these function decls down below the types as per > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:50: static content::ProcessType BitIndexToProcessType(uint32_t bit_index); On 2016/12/14 21:58:08, sky wrote: > Same comment about bit index here. Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:67: const uint32_t module_id; On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > int is the type of choice for things that don't explicitly need to be otherwise > (https://google.github.io/styleguide/cppguide.html#Integer_Types). > > also, since the sets in ProcessInfo contain these, i mildly suggest making an > explicit type for this (e.g., "using ModuleId = int;" in this struct, or > somesuch). Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:70: // this struct will have their constness casted away at runtime, allowing On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > use "mutable" keyword rather than casting? I get the ability to have const-correct accessors by not using mutable. For example, with "CreateProcessInfo". (I don't feel strongly about this though.) https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:80: // Key comparator for ModuleInfo, allowing it to be used in a set. On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > is it relevant to document that it compares by module_path? Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:103: std::unordered_set<uint32_t> loaded_module_ids; On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > will these be large enough that std::vector (sorted or unsorted) won't be better > in all regards (less memory overhead, better cache performance, etc)? These won't be enormous... the long tail sees something like ~200 modules in a process. I dislike having to iterate in order to modify these, but I hate the fragmented storage and high overhead of a std::set. I chose unordered_set as a compromise (uses linear storage, O(1) access, etc). But, looking briefly into this it looks like the overhead is actually pretty atrocious. Given that loads/unloads are actually relatively rare events, I'll move to using a vector. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.h:133: base::WeakPtrFactory<ModuleDatabase> weak_ptr_factory_; On 2016/12/14 21:58:08, sky (OOO) wrote: > Make this the last members (I'm surprised you didn't hit a warning when > uploading). Done. https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win_unittest.cc:56: public: On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > nit: protected Done. https://codereview.chromium.org/2576843002/diff/1/chrome/common/conflicts/mod... File chrome/common/conflicts/module_database_win.mojom (right): https://codereview.chromium.org/2576843002/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_database_win.mojom:15: OnProcessStarted(uint32 process_id, uint32 process_type); On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > does |process_id| come across the wire from the child proc? if yes, why? does > the browser not know its children's pids? Yup, this was silly. Reworked this so that it's taken directly from the process handle in the browser process. https://codereview.chromium.org/2576843002/diff/1/chrome/common/conflicts/mod... chrome/common/conflicts/module_database_win.mojom:17: OnModuleEvent(ModuleEvent module_event); After talking to security folks (wfh@), also reworked this so that only the load address is transmitted (which is verifiable in the browser process). All of the other information can then be queried directly from the OS in the browser process, without having to trust any data from the child process. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:178: void ModuleDatabase::InsertLoadAddress(ModuleId module_id, grt: I was bored, so went a little overboard here to try to guarantee O(1) insertion in the usual case of a module being inserted for the first time. Let me know if you wish me to simplify.
whew! https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:65: // worth crashing if this data is slightly out of sync as this is purely On 2016/12/19 20:15:36, chrisha (slow) wrote: > On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > > if it's not worth crashing, why is there a DCHECK? will the DCHECK do anything > > other than annoy/confuse some other random developer who happens to trip on > > this? see > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > for guidance on DCHECK > > Not worth crashing at all. Removed. Maybe, maybe not.... :-) https://codereview.chromium.org/2576843002/diff/1/chrome/browser/conflicts/mo... chrome/browser/conflicts/module_database_win.cc:110: DCHECK_LE(0u, bit_index); On 2016/12/19 20:15:36, chrisha (slow) wrote: > On 2016/12/15 09:07:12, grt (UTC plus 1) wrote: > > nit: i find these more natural the other way around since that's the way we > > write general comparisons (i.e., "if (0 <= bit_index)" is backwards). > > I'm impartial, but over the years had reviewers drill into me this "backwards" > way of doing things (that the constant should be first). I apologize if I was a driller in the past, but I believe that you were improperly drilled. The guidance for general comparisons in C++ (let's forget about GTest for the time being) is that the constant comes last, for example: if (bit_index > 31) // woah there, cowboy! Then Google Test came around with a quad of {EXPECT,ASSERT}_{EQ,NE} macros where the expected value came first (https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?q=%2...). This put a wrench in the works, but we adapted by following this convention for these four macros (and their {,D}CHECK counterparts for consistency). We didn't carry this over to the use of the other EXPECT/ASSERT macros. Here's one thread where this was discussed: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/QuN9McxZpts/dis.... Also worth mentioning: GTest no longer has this convention: https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb..., but Chromium hasn't picked that up yet. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:8: #include "base/debug/leak_annotations.h" unused? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:9: #include "base/lazy_instance.h" unused https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:10: #include "base/strings/utf_string_conversions.h" unused https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:16: static_assert(1 == content::PROCESS_TYPE_UNKNOWN, reverse these! https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:20: static constexpr uint32_t kMinProcessType = content::PROCESS_TYPE_BROWSER; omit "static" in an unnamed namespace https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:26: : task_runner_(task_runner), weak_ptr_factory_(this) {} std::move(task_runner)? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:52: // to misbehave and sent out-of-order messages. It is easy to be tolerant of sent -> send https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:67: reinterpret_cast<uintptr_t>(event.module_load_address); wdyt of making module_load_address a uintptr_t throughout so that casting isn't needed? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:78: case mojom::ModuleEventType::MODULE_UNLOADED: i find it a bit confusing that unloads are handled here and in OnModuleUnload. can there be only one codepath? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:102: DCHECK(process_info); remove this one, too? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:156: int ModuleDatabase::FindLoadAddressIndexById( looks like using size_t for the index would be much more convenient than int. this is allowed by my interpretation of the style guide... https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:186: if (load_addresses->size() == 0 || module_id > load_addresses->back().first) { load_addresses->size() == 0 -> load_addresses->empty() https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:187: load_addresses->push_back(std::make_pair(module_id, load_address)); load_addresses->emplace_back(module_id, load_address); https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:195: load_addresses->at(i).second = load_address; curious: why would a module's load address change? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:202: load_addresses->push_back(load_addresses->back()); is this the same thing? load_addresses->emplace(--load_addresses->end(), module_id, load_address); https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:218: // Special case: removing the maximum index module. Need to find the new silly idea: how about changing FindLoadAddressIndexById so that it goes back to front. then this special case is handled automagically, no? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:244: max_id = load_addresses->at(i).first; avoid at() -- it does range checking and throws exceptions https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:255: std::swap((*load_addresses)[max_index], (*load_addresses)[last_index]); #include <algorithm> https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:260: // If the element to be removed is second last then a single swap is swap -> copy https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:265: // In the general case two swaps are necessary. swaps -> copies https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:279: return const_cast<ModuleInfo*>(&(*result.first)); is this cast really needed? |this| isn't const, and ModuleSet is not a set of const ModuleInfos. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:288: return const_cast<ProcessInfo*>(&(*it)); same comment about cast https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:291: const ModuleDatabase::ProcessInfo* ModuleDatabase::CreateProcessInfo( the only caller DCHECKS the result. how about doing that in here and making this a void func? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:304: bool ModuleDatabase::DeleteProcessInfo(uint32_t process_id) { similar comment: why not DCHECK in here? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:9: #include <unordered_map> unused https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:66: using ModuleLoadAddresses = std::vector<std::pair<ModuleId, uintptr_t>>; #include <utility> #include <vector> https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:136: struct ModuleDatabase::ModuleInfo { move into .cc file unless this is needed by tests or something https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:165: struct ModuleDatabase::ProcessInfo { this, too https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:169: bool operator<(const ProcessInfo& pi) const; q for me: see what Meyers says about this in Effective C++ vs putting the operator outside of the class. MAC has the book. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win_unittest.cc:75: message_loop_(new base::MessageLoop), MakeUnique? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win_unittest.cc:79: base::RunLoop run_loop; nit: base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:8: #include <psapi.h> should we set PSAPI_VERSION=2 explicitly somewhere? do we already? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:34: std::unique_ptr<ModuleEventSinkImpl> module_event_sink_impl = nit: i think "auto" is appropriate here since the type is clear https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:37: base::Closure error_handler = base::Bind(&ModuleDatabase::OnProcessEnded, #include "base/callback.h" for base::Closure (which is actually forward decl'd in callback_forward.h, but you need the full definition here). https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:61: default: remove default case so that compilation fails if/when a new type is added https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:76: WCHAR temp_path[MAX_PATH] = {}; nit: wchar_t consider not zero-initialzing the whole buffer. i don't feel it's needed. if you want, you could use "temp_path[0] = L'\0';" below to make it a valid zero-length string before using it. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:77: if (!::GetModuleFileNameEx(process_, module, temp_path, MAX_PATH)) MAX_PATH -> arraysize(temp_path) (and #include "base/macros.h") capture the return value for use below https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:87: ModuleWatcher::ModuleEvent event(event_type, base::FilePath(temp_path), temp_path -> base::StringPiece16(temp_path, len-from-GetModuleFileNameEx) https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.h:22: explicit ModuleEventSinkImpl(base::ProcessHandle process, nit: no explicit https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.h:43: // OnModuleEvent reroutes to these two functions depending on the event reroutes or just routes? how about dispatches or calls? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc:27: : message_loop_(new base::MessageLoop), base::MakeUnique? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc:50: std::unique_ptr<base::MessageLoop> message_loop_; #include <memory> https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:102: base::string16(str->Buffer, str->Length / sizeof(wchar_t))); nit: base::StringPiece16(str->Buffer, str->Length / sizeof(wchar_t)) rather than base::string16 (and #include "base/strings/string_piece.h") https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:106: void OnModuleEvent(const mojom::ModuleEventType& event_type, passing an enum by constref? https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:117: ModuleWatcher::ModuleEvent::ModuleEvent(mojom::ModuleEventType event_type, is this sufficiently trivial that it could be inline? https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:188: base::FilePath(base::UTF8ToWide(path)), while i loves me a good conversion, how about using module.szExePath directly rather than Wide->UTF8->Wide? https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:190: callback_.Run(event); nit: i'd be inclined to inline the ModuleEvent, but that's just me. do as you will. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:232: ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback) i just recently read this guidance on callbacks "Pass Callback objects by value if ownership is transferred; otherwise, pass it by const-reference." (https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). should this (or others be by-value (with std::move where appropriate) rather than by-ref? how about other places in this CL?
Description was changed from ========== [win] Create ModuleDatabase and ModuleDatabaseImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 ========== to ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 ==========
Thanks for the awesomely detailed review Greg. Addressed most issues, politely declined a few. Detailed responses inline. Also looked up Meyer's answer to your question :) https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:8: #include "base/debug/leak_annotations.h" On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > unused? Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:9: #include "base/lazy_instance.h" On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > unused Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:10: #include "base/strings/utf_string_conversions.h" On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > unused Done. (I really wish we had automated analysis for this kind of thing... why oh why do we need to manually do this kind of crap in 2016!?!) https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:16: static_assert(1 == content::PROCESS_TYPE_UNKNOWN, On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > reverse these! Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:20: static constexpr uint32_t kMinProcessType = content::PROCESS_TYPE_BROWSER; On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > omit "static" in an unnamed namespace Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:26: : task_runner_(task_runner), weak_ptr_factory_(this) {} On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > std::move(task_runner)? Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:52: // to misbehave and sent out-of-order messages. It is easy to be tolerant of On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > sent -> send Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:67: reinterpret_cast<uintptr_t>(event.module_load_address); On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > wdyt of making module_load_address a uintptr_t throughout so that casting isn't > needed? There's one boundary I can't get rid of: the IPC mechanism uses a uint64, as there's no "uintptr_t" or naked pointer equivalent in Mojo. IMO, ModuleWatcher makes sense to use pointers because in the context of it, these are real pointers that can be dereferenced, as well as fed into various OS functions as HMODULEs. However, in the ModuleDatabase, these aren't semantically pointers anymore, hence the desire to not store them as void*. I felt this mechanism slightly more externally consistent from the point of view of each class and its individual role. I don't find the casts too onerous, but if you feel strongly about this let me know. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:78: case mojom::ModuleEventType::MODULE_UNLOADED: On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > i find it a bit confusing that unloads are handled here and in OnModuleUnload. > can there be only one codepath? OnModuleUnload is expensive, costing an O(n) search. It is meant to handle untrusted data from out of process that has to be validated. OnModuleEvent handles trusted data from in process, and can be handled in O(lg n) without additional lookup. So, slightly annoying, but both more efficient and more secure. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:102: DCHECK(process_info); On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > remove this one, too? Oops, should have been deleted. Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:156: int ModuleDatabase::FindLoadAddressIndexById( On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > looks like using size_t for the index would be much more convenient than int. > this is allowed by my interpretation of the style guide... Indeed it would be. Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:186: if (load_addresses->size() == 0 || module_id > load_addresses->back().first) { On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > load_addresses->size() == 0 -> load_addresses->empty() Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:187: load_addresses->push_back(std::make_pair(module_id, load_address)); On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > load_addresses->emplace_back(module_id, load_address); Holy jebus batman, I've never seen "emplace". TIL... Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:195: load_addresses->at(i).second = load_address; On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > curious: why would a module's load address change? A vain attempt at maintaining long term consistency? This could happen if a module was loaded, unloaded and reloaded, and forced to load at a different address because something else occupied it's previous address. And if the unload/reload were processed out of order. This is admittedly very very unlikely. I think there are 3 options: 1. Keep the existing address. If a reload really did occur before the unload was processed, then this means that the module will appear to be unloaded when it is in fact in memory. Unlikely, but strictly possible. 2. Update the address. If the same race happens, this is slightly more consistent in that the module will appear to be loaded, and the out-of-order unload will be silently ignored. 3. Expect things to be strictly in order and explode. I've already decided this isn't realistic, and it's not worth tearing down the browser if this "informational" service is slightly incorrect. So I think (1) is the best option (what I've gone with). I've added more comments. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:202: load_addresses->push_back(load_addresses->back()); On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > is this the same thing? > load_addresses->emplace(--load_addresses->end(), module_id, load_address); It is indeed. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:218: // Special case: removing the maximum index module. Need to find the new On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > silly idea: how about changing FindLoadAddressIndexById so that it goes back to > front. then this special case is handled automagically, no? No, not silly at all! Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:244: max_id = load_addresses->at(i).first; On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > avoid at() -- it does range checking and throws exceptions Another TIL... Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:255: std::swap((*load_addresses)[max_index], (*load_addresses)[last_index]); On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > #include <algorithm> Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:260: // If the element to be removed is second last then a single swap is On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > swap -> copy Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:265: // In the general case two swaps are necessary. On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > swaps -> copies Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:279: return const_cast<ModuleInfo*>(&(*result.first)); On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > is this cast really needed? |this| isn't const, and ModuleSet is not a set of > const ModuleInfos. std::set always returns elements as "const", as they are considered immutable, so yes, this is needed. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:288: return const_cast<ProcessInfo*>(&(*it)); On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > same comment about cast Ditto. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:291: const ModuleDatabase::ProcessInfo* ModuleDatabase::CreateProcessInfo( On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > the only caller DCHECKS the result. how about doing that in here and making this > a void func? Actually, that DCHECK is meant to be elided. Made this void and removed it altogether. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:304: bool ModuleDatabase::DeleteProcessInfo(uint32_t process_id) { On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > similar comment: why not DCHECK in here? Made void, removed the unneeded DCHECK. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:9: #include <unordered_map> On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > unused Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:66: using ModuleLoadAddresses = std::vector<std::pair<ModuleId, uintptr_t>>; On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > #include <utility> > #include <vector> Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:136: struct ModuleDatabase::ModuleInfo { On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > move into .cc file unless this is needed by tests or something Yeah, they're all exposed for tests. To ensure that things are being added as expected. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:165: struct ModuleDatabase::ProcessInfo { On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > this, too Ditto. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:169: bool operator<(const ProcessInfo& pi) const; On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > q for me: see what Meyers says about this in Effective C++ vs putting the > operator outside of the class. MAC has the book. Meyers specifically mentions keeping it out of the class when you want implicit type conversions. The example he uses is operator* for a Rational class. Making it a member functions means that: Rational() * 2 works 2 * Rational() doesn't work Making it an external functions means they both work, provided Rational() has an implicit conversion from int. He follows that with a general preference to "prefer non-member non-friend functions to member functions". But in this case it means that ModuleInfo and ProcessInfo have to be externally visible from ModuleDatabase (ie, public instead of protected). Minor catch-22, IMO. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win_unittest.cc:75: message_loop_(new base::MessageLoop), On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > MakeUnique? How is that any shorter or better? I can see that argument when doing assignment, as it avoid using "reset" and "new", but it actually makes things longer and less clear, IMO, in the constructor. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win_unittest.cc:79: base::RunLoop run_loop; On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > nit: > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:8: #include <psapi.h> On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > should we set PSAPI_VERSION=2 explicitly somewhere? do we already? We want PSAPI_VERSION=1, and we set it globally in our build config. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:34: std::unique_ptr<ModuleEventSinkImpl> module_event_sink_impl = On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > nit: i think "auto" is appropriate here since the type is clear Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:37: base::Closure error_handler = base::Bind(&ModuleDatabase::OnProcessEnded, On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > #include "base/callback.h" for base::Closure (which is actually forward decl'd > in callback_forward.h, but you need the full definition here). Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:61: default: On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > remove default case so that compilation fails if/when a new type is added Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:76: WCHAR temp_path[MAX_PATH] = {}; On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > nit: wchar_t > consider not zero-initialzing the whole buffer. i don't feel it's needed. if you > want, you could use "temp_path[0] = L'\0';" below to make it a valid zero-length > string before using it. Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:77: if (!::GetModuleFileNameEx(process_, module, temp_path, MAX_PATH)) On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > MAX_PATH -> arraysize(temp_path) (and #include "base/macros.h") > capture the return value for use below Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:87: ModuleWatcher::ModuleEvent event(event_type, base::FilePath(temp_path), On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > temp_path -> base::StringPiece16(temp_path, len-from-GetModuleFileNameEx) Good catch with the potentially missing trailing nul. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.h:22: explicit ModuleEventSinkImpl(base::ProcessHandle process, On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > nit: no explicit Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.h:43: // OnModuleEvent reroutes to these two functions depending on the event On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > reroutes or just routes? how about dispatches or calls? Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc:27: : message_loop_(new base::MessageLoop), On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > base::MakeUnique? See previous response. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc:50: std::unique_ptr<base::MessageLoop> message_loop_; On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > #include <memory> Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:102: base::string16(str->Buffer, str->Length / sizeof(wchar_t))); On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > nit: base::StringPiece16(str->Buffer, str->Length / sizeof(wchar_t)) rather than > base::string16 (and #include "base/strings/string_piece.h") Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:106: void OnModuleEvent(const mojom::ModuleEventType& event_type, On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > passing an enum by constref? Errr.... oops? Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:117: ModuleWatcher::ModuleEvent::ModuleEvent(mojom::ModuleEventType event_type, On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > is this sufficiently trivial that it could be inline? Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:188: base::FilePath(base::UTF8ToWide(path)), On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > while i loves me a good conversion, how about using module.szExePath directly > rather than Wide->UTF8->Wide? Oops, remnants of when this was being stored as as std::string. Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:190: callback_.Run(event); On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > nit: i'd be inclined to inline the ModuleEvent, but that's just me. do as you > will. I lean towards verbosity, as I personally find it easier to read. Doing too much in one statement irks me :) https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:232: ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback) On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > i just recently read this guidance on callbacks "Pass Callback objects by value > if ownership is transferred; otherwise, pass it by const-reference." > (https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > should this (or others be by-value (with std::move where appropriate) rather > than by-ref? how about other places in this CL? Interesting. In the context of ModuleWatcher ownership is clearly maintained by the ModuleWatcher, as the callback is synchronous. IMO, a ref makes sense here. (FWIW, this is the only callback that passes around an object; everything is effectively ints, and is already passed by value.)
https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:10: #include "base/strings/utf_string_conversions.h" On 2016/12/20 19:46:23, chrisha (slow) wrote: > On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > > unused > > Done. > > (I really wish we had automated analysis for this kind of thing... why oh why do > we need to manually do this kind of crap in 2016!?!) Amen to that, brother. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:67: reinterpret_cast<uintptr_t>(event.module_load_address); On 2016/12/20 19:46:23, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > wdyt of making module_load_address a uintptr_t throughout so that casting > isn't > > needed? > > There's one boundary I can't get rid of: the IPC mechanism uses a uint64, as > there's no "uintptr_t" or naked pointer equivalent in Mojo. > > IMO, ModuleWatcher makes sense to use pointers because in the context of it, > these are real pointers that can be dereferenced, as well as fed into various OS > functions as HMODULEs. However, in the ModuleDatabase, these aren't semantically > pointers anymore, hence the desire to not store them as void*. > > I felt this mechanism slightly more externally consistent from the point of view > of each class and its individual role. I don't find the casts too onerous, but > if you feel strongly about this let me know. Naah. Thanks for clarifying the reasoning. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:78: case mojom::ModuleEventType::MODULE_UNLOADED: On 2016/12/20 19:46:23, chrisha (slow) wrote: > On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > > i find it a bit confusing that unloads are handled here and in OnModuleUnload. > > can there be only one codepath? > > OnModuleUnload is expensive, costing an O(n) search. It is meant to handle > untrusted data from out of process that has to be validated. > > OnModuleEvent handles trusted data from in process, and can be handled in O(lg > n) without additional lookup. > > So, slightly annoying, but both more efficient and more secure. Can you clarify in comments which is trusted and which isn't? As it is, each function has a comment saying that messages can come over IPC, so it's not really clear to me without looking into how they're called. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:187: load_addresses->push_back(std::make_pair(module_id, load_address)); On 2016/12/20 19:46:23, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > load_addresses->emplace_back(module_id, load_address); > > Holy jebus batman, I've never seen "emplace". TIL... emplace was the toy I got on my birthday from the overwhelming amount of "please can I create an object in a vector's memory" wishes I silent sent to the standards gods. I've wanted this for almost as long as I've touched the STL. > Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:195: load_addresses->at(i).second = load_address; On 2016/12/20 19:46:23, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > curious: why would a module's load address change? > > A vain attempt at maintaining long term consistency? > > This could happen if a module was loaded, unloaded and reloaded, and forced to > load at a different address because something else occupied it's previous > address. And if the unload/reload were processed out of order. If unload/reload could potentially be processed out of order, could process destroy/create also be processed out of order? I'm still thinking about PID reuse. I don't think the OS makes any guarantee beyond "a PID is valid as long as a process is alive", so I'm scared about aggressive reuse. Is there a statement somewhere saying "I swear I will never reuse a PID for the next N CreateProcess's"? > This is admittedly very very unlikely. > > I think there are 3 options: > > 1. Keep the existing address. If a reload really did occur before the unload was > processed, then this means that the module will appear to be unloaded when it is > in fact in memory. Unlikely, but strictly possible. Is it possible for a module to be loaded into process twice? LoadLibrary on a symlink? I suppose the paths would be different in that case. Could a module be loaded twice with the same path? UwS/malware? > 2. Update the address. If the same race happens, this is slightly more > consistent in that the module will appear to be loaded, and the out-of-order > unload will be silently ignored. > 3. Expect things to be strictly in order and explode. I've already decided this > isn't realistic, and it's not worth tearing down the browser if this > "informational" service is slightly incorrect. > > So I think (1) is the best option (what I've gone with). Ack. > I've added more comments. Tak. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:279: return const_cast<ModuleInfo*>(&(*result.first)); On 2016/12/20 19:46:23, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > is this cast really needed? |this| isn't const, and ModuleSet is not a set of > > const ModuleInfos. > > std::set always returns elements as "const", as they are considered immutable, > so yes, this is needed. Ah. Grn. The critical piece, then, is that the object not be mutated in a way that would change the ordering in the container, correct? Could you make this clear in the struct's doc comments so that an unwitting future committer doesn't modify the path/pid (comment applies to both containers)? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:136: struct ModuleDatabase::ModuleInfo { On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > move into .cc file unless this is needed by tests or something > > Yeah, they're all exposed for tests. To ensure that things are being added as > expected. Acknowledged. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:165: struct ModuleDatabase::ProcessInfo { On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > this, too > > Ditto. Acknowledged. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:169: bool operator<(const ProcessInfo& pi) const; On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > q for me: see what Meyers says about this in Effective C++ vs putting the > > operator outside of the class. MAC has the book. > > Meyers specifically mentions keeping it out of the class when you want implicit > type conversions. The example he uses is operator* for a Rational class. Making > it a member functions means that: > > Rational() * 2 works > 2 * Rational() doesn't work > > Making it an external functions means they both work, provided Rational() has an > implicit conversion from int. > > He follows that with a general preference to "prefer non-member non-friend > functions to member functions". > > But in this case it means that ModuleInfo and ProcessInfo have to be externally > visible from ModuleDatabase (ie, public instead of protected). > > Minor catch-22, IMO. Thanks for checking! https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win_unittest.cc:75: message_loop_(new base::MessageLoop), On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > MakeUnique? > > How is that any shorter or better? I can see that argument when doing > assignment, as it avoid using "reset" and "new", but it actually makes things > longer and less clear, IMO, in the constructor. With naked "new", I have to check the type of |message_loop_| to be sure it's a smart pointer. With MakeUnique, I know it must be. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:8: #include <psapi.h> On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > should we set PSAPI_VERSION=2 explicitly somewhere? do we already? > > We want PSAPI_VERSION=1, and we set it globally in our build config. Why 1? My brief read of MSDN made me think that version 2 means that PSAPI is implemented in kernel32, so we avoid loading an extra dll that just jumps from elsewhere into kernel32. I don't know what the other implications are, but that seemed like a good thing to me. What does version 1 do for us now that we only care about Win7+? https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:87: ModuleWatcher::ModuleEvent event(event_type, base::FilePath(temp_path), On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > temp_path -> base::StringPiece16(temp_path, len-from-GetModuleFileNameEx) > > Good catch with the potentially missing trailing nul. I didn't check whether |length| would include the string terminator or not. MSDN didn't really say. Is it not included? https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:190: callback_.Run(event); On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > > nit: i'd be inclined to inline the ModuleEvent, but that's just me. do as you > > will. > > I lean towards verbosity, as I personally find it easier to read. Doing too much > in one statement irks me :) Acknowledged. https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:232: ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback) On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > > i just recently read this guidance on callbacks "Pass Callback objects by > value > > if ownership is transferred; otherwise, pass it by const-reference." > > > (https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > > should this (or others be by-value (with std::move where appropriate) rather > > than by-ref? how about other places in this CL? > > Interesting. In the context of ModuleWatcher ownership is clearly maintained by > the ModuleWatcher Are you saying that ModuleWatcher takes ownership of |callback|? If so, this should be pass-by-value. > , as the callback is synchronous. IMO, a ref makes sense here. > > (FWIW, this is the only callback that passes around an object; everything is > effectively ints, and is already passed by value.) https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:285: processes_.insert(key); https://youtu.be/Aharq3PFX54 processes_.emplace(process_id, process_type); https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:291: if (it == processes_.end()) nit: flip logic and move the "else" into the "if" part so that "return;" becomes unnecessary? https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:59: static constexpr size_t kInvalidIndex = 0xFFFFFFFFu; hmm. still use -1 so that it's "all bits on" even for x64 builds? https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:77: if (!::GetModuleFileNameEx(process_, module, temp_path, arraysize(temp_path))) MSDN says this truncates the path if the buffer is too small. for the sake of paranoia, do this in a loop w/ a resize? https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:82: size_t length = GetModuleInformation(process_, module, &info, sizeof(info)); DWORD |length| should be captured from GetModuleFileNameEx, above, not this. :-)
Okay. Bit the bullet and moved to identifying processes by (process_id, creation_time). Also made modules identified by a strict triplet (path, size, timestamp). Simplified interface to ModuleDatabase. Added many more comments. I've tried to address everything else inline. Hopefully we're converging on something you like... and then it gets to be Will's turn to tear it apart from a security POV. PTAnotherL? (No rush... I'm now on vacation until Jan 2.) https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:78: case mojom::ModuleEventType::MODULE_UNLOADED: On 2016/12/20 21:09:52, grt (UTC plus 1) wrote: > On 2016/12/20 19:46:23, chrisha (slow) wrote: > > On 2016/12/20 11:11:33, grt (UTC plus 1) wrote: > > > i find it a bit confusing that unloads are handled here and in > OnModuleUnload. > > > can there be only one codepath? > > > > OnModuleUnload is expensive, costing an O(n) search. It is meant to handle > > untrusted data from out of process that has to be validated. > > > > OnModuleEvent handles trusted data from in process, and can be handled in O(lg > > n) without additional lookup. > > > > So, slightly annoying, but both more efficient and more secure. > > Can you clarify in comments which is trusted and which isn't? As it is, each > function has a comment saying that messages can come over IPC, so it's not > really clear to me without looking into how they're called. I've reworked the API so there's only one way to load, and one way to unload. This is slightly less efficient for "local" module events where the metadata is fully trusted and could be used for guaranteed O(1) lookup, but easier to reason about. I've also tried to rework the comments. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:195: load_addresses->at(i).second = load_address; > Is it possible for a module to be loaded into process twice? LoadLibrary on a > symlink? I suppose the paths would be different in that case. Could a module be > loaded twice with the same path? UwS/malware? I did some experiments here. 1. LoadLibrary("foo.dll") twice in a row. Get the same HMODULE both times. Enumerating and there's one occurrence. 2. LoadLibrary("foo.dll"). Rename "foo.dll" to "bar.dll", and copy another module to "foo.dll". LoadLibrary("foo.dll") again. I get the same HMODULE back. Enumerating and there's one occurrence. So modules are unique up to their filename as far as the loader is concerned. 3. LoadLibrary("foo.dll"). Rename "foo.dll" to "bar.dll". LoadLibrary("bar.dll"). I get the same HMODULE back, and only "foo.dll" is listed when enumerating modules. So modules are unique up to their "inode" equivalent in the filesystem. Malware may have ways of working around this, but we can't possibly be robust to them all. The best I can do is work correctly for expected use, and be resilient otherwise. Since you can load different multiple modules (not at the same time) with the same name, I've also added module_size and module_time_date_stamp to the key that uniquely identifies a module. This should reduce the probability of collisions, and is the same data that MS uses for things like symbol servers. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:279: return const_cast<ModuleInfo*>(&(*result.first)); On 2016/12/20 21:09:52, grt (UTC plus 1) wrote: > On 2016/12/20 19:46:23, chrisha (slow) wrote: > > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > > is this cast really needed? |this| isn't const, and ModuleSet is not a set > of > > > const ModuleInfos. > > > > std::set always returns elements as "const", as they are considered immutable, > > so yes, this is needed. > > Ah. Grn. The critical piece, then, is that the object not be mutated in a way > that would change the ordering in the container, correct? Could you make this > clear in the struct's doc comments so that an unwitting future committer doesn't > modify the path/pid (comment applies to both containers)? Tried to make this more clear. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win_unittest.cc:75: message_loop_(new base::MessageLoop), On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > On 2016/12/20 19:46:24, chrisha (slow) wrote: > > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > > MakeUnique? > > > > How is that any shorter or better? I can see that argument when doing > > assignment, as it avoid using "reset" and "new", but it actually makes things > > longer and less clear, IMO, in the constructor. > > With naked "new", I have to check the type of |message_loop_| to be sure it's a > smart pointer. With MakeUnique, I know it must be. Okay, fair enough. Done. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:8: #include <psapi.h> On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > On 2016/12/20 19:46:24, chrisha (slow) wrote: > > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > > should we set PSAPI_VERSION=2 explicitly somewhere? do we already? > > > > We want PSAPI_VERSION=1, and we set it globally in our build config. > > Why 1? My brief read of MSDN made me think that version 2 means that PSAPI is > implemented in kernel32, so we avoid loading an extra dll that just jumps from > elsewhere into kernel32. I don't know what the other implications are, but that > seemed like a good thing to me. What does version 1 do for us now that we only > care about Win7+? I think this is a discussion for the mailing list. We may well want to update to PSAPI_VERSION=2 now that we've dropped older OSes. I'm not going to touch that in this CL, however. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:87: ModuleWatcher::ModuleEvent event(event_type, base::FilePath(temp_path), On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > On 2016/12/20 19:46:24, chrisha (slow) wrote: > > On 2016/12/20 11:11:34, grt (UTC plus 1) wrote: > > > temp_path -> base::StringPiece16(temp_path, len-from-GetModuleFileNameEx) > > > > Good catch with the potentially missing trailing nul. > > I didn't check whether |length| would include the string terminator or not. MSDN > didn't really say. Is it not included? It could be omitted in the case that the buffer is too short. But otherwise, it's not clear. Better safe than sorry. https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc:27: : message_loop_(new base::MessageLoop), On 2016/12/20 19:46:24, chrisha (slow) wrote: > On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > > base::MakeUnique? > > See previous response. (Done.) https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2576843002/diff/60001/chrome/common/conflicts... chrome/common/conflicts/module_watcher_win.cc:232: ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback) On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > On 2016/12/20 19:46:24, chrisha (slow) wrote: > > On 2016/12/20 11:11:35, grt (UTC plus 1) wrote: > > > i just recently read this guidance on callbacks "Pass Callback objects by > > value > > > if ownership is transferred; otherwise, pass it by const-reference." > > > > > > (https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...). > > > should this (or others be by-value (with std::move where appropriate) rather > > > than by-ref? how about other places in this CL? > > > > Interesting. In the context of ModuleWatcher ownership is clearly maintained > by > > the ModuleWatcher > > Are you saying that ModuleWatcher takes ownership of |callback|? If so, this > should be pass-by-value. > > > , as the callback is synchronous. IMO, a ref makes sense here. > > > > (FWIW, this is the only callback that passes around an object; everything is > > effectively ints, and is already passed by value.) Err, no. I was walking about ownership of the arguments passed to the callback. My reading comprehension is clearly amazing. Moved to pass by value. https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:285: processes_.insert(key); On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > https://youtu.be/Aharq3PFX54 > processes_.emplace(process_id, process_type); Done. https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.cc:291: if (it == processes_.end()) On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > nit: flip logic and move the "else" into the "if" part so that "return;" becomes > unnecessary? Done. https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_database_win.h:59: static constexpr size_t kInvalidIndex = 0xFFFFFFFFu; On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > hmm. still use -1 so that it's "all bits on" even for x64 builds? Good point. https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:77: if (!::GetModuleFileNameEx(process_, module, temp_path, arraysize(temp_path))) On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > MSDN says this truncates the path if the buffer is too small. for the sake of > paranoia, do this in a loop w/ a resize? Done. https://codereview.chromium.org/2576843002/diff/80001/chrome/browser/conflict... chrome/browser/conflicts/module_event_sink_impl_win.cc:82: size_t length = GetModuleInformation(process_, module, &info, sizeof(info)); On 2016/12/20 21:09:53, grt (UTC plus 1) wrote: > DWORD |length| should be captured from GetModuleFileNameEx, above, not this. :-) Err... oops. Done.
i'm pretty satisfied. your turn, will! https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:198: return; nit: omit this and previous line https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:218: DCHECK_LE(0u, index); index is unsigned, so isn't this unconditionally true? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:219: DCHECK_GT(load_addresses->size(), index); i think this is more natural the other way: index < size https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:235: load_addresses->resize(load_addresses->size() - 1); nit: it's been established on line 223 that index == load_addresses->size() - 1, so you could use index here. maybe that's too subtle, though. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:266: auto result = modules_.emplace(key); nit: get rid of |key| so that only one instance is created in the right place. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:290: processes_.emplace(key); nit: get rid of |key| so that only one instance is created in the right place. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:314: // The key consists of the triplet of i think std::tuple implements operator< for lexicographical ordering, so maybe this could be simplified to: return std::make_tuple(module_path, module_size, module_time_date_stamp) < std::make_tuple(mi.module_path, mi.module_size, mi.module_time_date_stamp); which should be inherently correct? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:337: // The key consists of the pair of (process_id, creation_time). maybe use tuple or pair here, too? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:30: // same thread as |task_runner_|. nit: "on the same thread" -> "in the same sequence" https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:54: // thread and will be bounced to the |task_runner|. In practice it will be nit: |task_runner_| for consistency with comment above https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:160: const uint32_t module_size; maybe mention that this is SizeOfImage from the module's optional header? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:163: const uint32_t module_time_date_stamp; maybe mention that this is TimeDateStamp from the module's file header? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:22: explicit SingleTaskRunner(const base::Closure& task) pass by value and std::move below? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:34: void RunTask(const base::Closure& task) { pass by value and std::move below? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:40: const uint32_t kPid1 = 1234u; nit: constexpr all of these https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:105: size_t count = 0; nit: std::count_if(load_addresses.begin(), load_addresses.end(), lambda); https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:117: std::unique_ptr<base::MessageLoop> message_loop_; iwyu <memory> https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:279: std::vector<bool> inserted(n); iwyu <vector> https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:35: static_cast<uint64_t>(creation_ft.dwLowDateTime); nit: "git cl format" https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:68: if (!GetModuleInformation(process, module, &info, sizeof(info))) nit: ::GetModuleInformation for consistency with other Win32 calls https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:80: SIZE_T bytes_read = 0; nit: size_t https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:100: address = load_address + e_lfanew + offsetof(IMAGE_NT_HEADERS, FileHeader) + this structure differs for 32-bit and 64-bit modules. will this code ever be run for modules that don't match the bitness of the build (i.e., 32-bit dlls for 64-bit Chrome, or vice versa)? 32-bit procs running in WoW can have 64-bit modules loaded, no? https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.h:38: uint64_t load_address) override; #include <stdint.h> https://codereview.chromium.org/2576843002/diff/120001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:127: g_module_watcher_instance = new ModuleWatcher(callback); std::move the callback (and #include <utility>) https://codereview.chromium.org/2576843002/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:223: : callback_(callback) { std::move
forgot to mention: nice CL!
Thanks grt. wfh, your turn! https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:198: return; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: omit this and previous line Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:218: DCHECK_LE(0u, index); On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > index is unsigned, so isn't this unconditionally true? Oops, left over from when it was as int. Removed. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:219: DCHECK_GT(load_addresses->size(), index); On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > i think this is more natural the other way: index < size Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:235: load_addresses->resize(load_addresses->size() - 1); On 2016/12/22 14:19:05, grt (UTC plus 1) wrote: > nit: it's been established on line 223 that index == load_addresses->size() - 1, > so you could use index here. maybe that's too subtle, though. Done. Also made the comment more verbose. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:266: auto result = modules_.emplace(key); On 2016/12/22 14:19:05, grt (UTC plus 1) wrote: > nit: get rid of |key| so that only one instance is created in the right place. Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:290: processes_.emplace(key); On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: get rid of |key| so that only one instance is created in the right place. Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:314: // The key consists of the triplet of On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > i think std::tuple implements operator< for lexicographical ordering, so maybe > this could be simplified to: > return std::make_tuple(module_path, module_size, module_time_date_stamp) < > std::make_tuple(mi.module_path, mi.module_size, mi.module_time_date_stamp); > which should be inherently correct? Great idea! Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:337: // The key consists of the pair of (process_id, creation_time). On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > maybe use tuple or pair here, too? Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:30: // same thread as |task_runner_|. On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: "on the same thread" -> "in the same sequence" Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:54: // thread and will be bounced to the |task_runner|. In practice it will be On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: |task_runner_| for consistency with comment above Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:160: const uint32_t module_size; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > maybe mention that this is SizeOfImage from the module's optional header? Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:163: const uint32_t module_time_date_stamp; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > maybe mention that this is TimeDateStamp from the module's file header? Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:22: explicit SingleTaskRunner(const base::Closure& task) On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > pass by value and std::move below? Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:34: void RunTask(const base::Closure& task) { On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > pass by value and std::move below? Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:40: const uint32_t kPid1 = 1234u; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: constexpr all of these Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:105: size_t count = 0; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: std::count_if(load_addresses.begin(), load_addresses.end(), lambda); Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:117: std::unique_ptr<base::MessageLoop> message_loop_; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > iwyu <memory> Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:279: std::vector<bool> inserted(n); On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > iwyu <vector> Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:35: static_cast<uint64_t>(creation_ft.dwLowDateTime); On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: "git cl format" Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:68: if (!GetModuleInformation(process, module, &info, sizeof(info))) On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: > ::GetModuleInformation > for consistency with other Win32 calls Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:80: SIZE_T bytes_read = 0; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > nit: size_t Nope, the type doesn't cleanly cast. Under the hood SIZE_T is an "unsigned long", and I'd need explicit casting to pass in a "size_t" as an output parameter. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:100: address = load_address + e_lfanew + offsetof(IMAGE_NT_HEADERS, FileHeader) + On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > this structure differs for 32-bit and 64-bit modules. will this code ever be run > for modules that don't match the bitness of the build (i.e., 32-bit dlls for > 64-bit Chrome, or vice versa)? 32-bit procs running in WoW can have 64-bit > modules loaded, no? The instrumentation only sees modules of the same bitness as the running process. We'd have to go through more contortions to see the 64-bit modules on the other side of the WoW boundary, and we're pretty much just interested in finding modules that inject directly into our address space (which are of the same bitness by definition). https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.h:38: uint64_t load_address) override; On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > #include <stdint.h> Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/common/conflict... File chrome/common/conflicts/module_watcher_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:127: g_module_watcher_instance = new ModuleWatcher(callback); On 2016/12/22 14:19:07, grt (UTC plus 1) wrote: > std::move the callback (and #include <utility>) Done. https://codereview.chromium.org/2576843002/diff/120001/chrome/common/conflict... chrome/common/conflicts/module_watcher_win.cc:223: : callback_(callback) { On 2016/12/22 14:19:07, grt (UTC plus 1) wrote: > std::move Done.
https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:35: static_cast<uint64_t>(creation_ft.dwLowDateTime); On 2017/01/03 21:34:48, chrisha (slow) wrote: > On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > > nit: "git cl format" > > Done. Ah, this was correct. Apologies for the misdirection. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:80: SIZE_T bytes_read = 0; On 2017/01/03 21:34:48, chrisha (slow) wrote: > On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > > nit: size_t > > Nope, the type doesn't cleanly cast. Under the hood SIZE_T is an "unsigned > long", and I'd need explicit casting to pass in a "size_t" as an output > parameter. Acknowledged. https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:100: address = load_address + e_lfanew + offsetof(IMAGE_NT_HEADERS, FileHeader) + On 2017/01/03 21:34:48, chrisha (slow) wrote: > On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > > this structure differs for 32-bit and 64-bit modules. will this code ever be > run > > for modules that don't match the bitness of the build (i.e., 32-bit dlls for > > 64-bit Chrome, or vice versa)? 32-bit procs running in WoW can have 64-bit > > modules loaded, no? > > The instrumentation only sees modules of the same bitness as the running > process. Are you sure about that? I thought ModuleWatcher::EnumerateAlreadyLoadedModules enumerates 32-bit and 64-bit modules. Does LdrRegisterDllNotification only notify for same-bitness DLLs? > We'd have to go through more contortions to see the 64-bit modules on > the other side of the WoW boundary, and we're pretty much just interested in > finding modules that inject directly into our address space (which are of the > same bitness by definition). https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:264: auto result = modules_.emplace(ModuleInfo( use emplace to its fullest potential and avoid constructing an instance that will be copied into the set: auto result = modules_.emplace(module_path, module_size, module_time_date_stamp, modules_.size()); https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:288: processes_.emplace(ProcessInfo(process_id, creation_time, process_type)); processes_.emplace(process_id, creation_time, process_type); https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:315: return std::make_tuple(module_path, module_size, module_time_date_stamp) < #include <tuple>
https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:100: address = load_address + e_lfanew + offsetof(IMAGE_NT_HEADERS, FileHeader) + On 2017/01/04 08:41:42, grt (UTC plus 1) wrote: > On 2017/01/03 21:34:48, chrisha (slow) wrote: > > On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > > > this structure differs for 32-bit and 64-bit modules. will this code ever be > > run > > > for modules that don't match the bitness of the build (i.e., 32-bit dlls for > > > 64-bit Chrome, or vice versa)? 32-bit procs running in WoW can have 64-bit > > > modules loaded, no? > > > > The instrumentation only sees modules of the same bitness as the running > > process. > > Are you sure about that? I thought ModuleWatcher::EnumerateAlreadyLoadedModules > enumerates 32-bit and 64-bit modules. Does LdrRegisterDllNotification only > notify for same-bitness DLLs? > > > We'd have to go through more contortions to see the 64-bit modules on > > the other side of the WoW boundary, and we're pretty much just interested in > > finding modules that inject directly into our address space (which are of the > > same bitness by definition). ModuleWatcher::EnumerateAlreadyLoadedModules uses CreateToolhelp32Snapshot. This only exposes 32-bit modules to 32-bit processes, and 64-bit modules to 64-bit processes. The confusion comes when calling the API from a 64-bit process, but inspecting a 32-bit WOW process. You can in that case ask to be given the 64-bit modules, or the 32-bit modules. Since we're always calling on our self, we don't have to worry about this. (Which means the TH32CS_SNAPMODULE32 flag is unnecessary as well.) https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:264: auto result = modules_.emplace(ModuleInfo( On 2017/01/04 08:41:42, grt (UTC plus 1) wrote: > use emplace to its fullest potential and avoid constructing an instance that > will be copied into the set: > auto result = modules_.emplace(module_path, module_size, > module_time_date_stamp, modules_.size()); Mind blown. Thanks. https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:288: processes_.emplace(ProcessInfo(process_id, creation_time, process_type)); On 2017/01/04 08:41:42, grt (UTC plus 1) wrote: > processes_.emplace(process_id, creation_time, process_type); Done. https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:315: return std::make_tuple(module_path, module_size, module_time_date_stamp) < On 2017/01/04 08:41:42, grt (UTC plus 1) wrote: > #include <tuple> Done.
LGTM! https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/120001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:100: address = load_address + e_lfanew + offsetof(IMAGE_NT_HEADERS, FileHeader) + On 2017/01/04 16:28:37, chrisha (slow) wrote: > On 2017/01/04 08:41:42, grt (UTC plus 1) wrote: > > On 2017/01/03 21:34:48, chrisha (slow) wrote: > > > On 2016/12/22 14:19:06, grt (UTC plus 1) wrote: > > > > this structure differs for 32-bit and 64-bit modules. will this code ever > be > > > run > > > > for modules that don't match the bitness of the build (i.e., 32-bit dlls > for > > > > 64-bit Chrome, or vice versa)? 32-bit procs running in WoW can have 64-bit > > > > modules loaded, no? > > > > > > The instrumentation only sees modules of the same bitness as the running > > > process. > > > > Are you sure about that? I thought > ModuleWatcher::EnumerateAlreadyLoadedModules > > enumerates 32-bit and 64-bit modules. Does LdrRegisterDllNotification only > > notify for same-bitness DLLs? > > > > > We'd have to go through more contortions to see the 64-bit modules on > > > the other side of the WoW boundary, and we're pretty much just interested in > > > finding modules that inject directly into our address space (which are of > the > > > same bitness by definition). > > ModuleWatcher::EnumerateAlreadyLoadedModules uses CreateToolhelp32Snapshot. This > only exposes 32-bit modules to 32-bit processes, and 64-bit modules to 64-bit > processes. > > The confusion comes when calling the API from a 64-bit process, but inspecting a > 32-bit WOW process. You can in that case ask to be given the 64-bit modules, or > the 32-bit modules. Since we're always calling on our self, we don't have to > worry about this. > > (Which means the TH32CS_SNAPMODULE32 flag is unnecessary as well.) Ah. I think I asked for that. Should it be removed? I suggested it because I thought I remembered reading that a 32-bit proc running in WoW could have 32-bit and 64-bit DLLs in process. I can't find anything to back up this crazy idea (including looking at the loaded modules of 32-bit cmd.exe in windbg with both types of .effmach), so I must have been mistaken. https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/140001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:264: auto result = modules_.emplace(ModuleInfo( On 2017/01/04 16:28:37, chrisha (slow) wrote: > On 2017/01/04 08:41:42, grt (UTC plus 1) wrote: > > use emplace to its fullest potential and avoid constructing an instance that > > will be copied into the set: > > auto result = modules_.emplace(module_path, module_size, > > module_time_date_stamp, modules_.size()); > > Mind blown. Thanks. perfect forwarding FTW!
https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:98: size_t i = FindLoadAddressIndexByAddress(module_load_address, Did you consider returning an iterator? It would mean you don't need kInvalidIndex, and I think work better with how you use it. I could be wrong though. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:133: DCHECK_LE(0u, bit_index); How about a compile time assert that PROCESS_TYPE_MAX in <= 31? Otherwise the failure may happen at a weird time. For the >= 0 you could add a PROCESS_TYPE_FIRST_VALUE or something. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:270: return const_cast<ModuleInfo*>(&(*result.first)); Similar comment has as with the other const_cast. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:283: return const_cast<ProcessInfo*>(&(*it)); IMO this is a good indication you shouldn't be using set. Once you resort to a const_cast you can't ensure you don't accidentally do the wrong thing. Did you consider separating out the key aspects from the data that changes and using a map? That way you wouldn't need a const_cast and you are better enforcing the key won't change. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:289: processes_.emplace(process_id, creation_time, process_type); Do you care if there was a ProcessInfo that matched in processes_ already? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:295: auto it = processes_.find(key); processes_.erase(key) ? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 now on these files. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:22: // All calls must be made in the context of this task runner, unless How do you guarantee a function can't be called on this class from another thread while in the destructor? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:40: uint64_t creation_time, Is there a reason not to use base::Time for creation_time and module_time_date_stamp. Also, how come one is 64 and the other 32 bit? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:66: static constexpr size_t kInvalidIndex = ~0u; Please comment where this is used. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:65: using ModuleDatabase::ModuleId; I'm surprised this works given ModuleId is private. Subclassing just to use the types is bizarre. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.h:65: uint64_t creation_time_; base::Time?
Responded to your comments inline, sky. Thanks for the review. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:98: size_t i = FindLoadAddressIndexByAddress(module_load_address, On 2017/01/05 00:30:44, sky wrote: > Did you consider returning an iterator? It would mean you don't need > kInvalidIndex, and I think work better with how you use it. I could be wrong > though. I did consider using an iterator. About the only thing that makes it slightly less convenient to use is the fact that I often want to directly address the last and second last element, which is slightly more awkward to do with iterators. I'm largely ambivalent here, but feel it reads more easily with indices. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:133: DCHECK_LE(0u, bit_index); On 2017/01/05 00:30:45, sky wrote: > How about a compile time assert that PROCESS_TYPE_MAX in <= 31? Otherwise the > failure may happen at a weird time. For the >= 0 you could add a > PROCESS_TYPE_FIRST_VALUE or something. The less than 0 check is actually completely bogus, as bit_index is unsigned. And I don't actually require that PROCESS_TYPE_MAX be <= 31, but rather that process_type - kMinProcessType (the first valid process type) be <= 31, which is already checked. Would renaming kMinProcessType to kFirstValidProcessType be better? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:270: return const_cast<ModuleInfo*>(&(*result.first)); On 2017/01/05 00:30:45, sky wrote: > Similar comment has as with the other const_cast. Acknowledged. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:283: return const_cast<ProcessInfo*>(&(*it)); On 2017/01/05 00:30:45, sky wrote: > IMO this is a good indication you shouldn't be using set. Once you resort to a > const_cast you can't ensure you don't accidentally do the wrong thing. Did you > consider separating out the key aspects from the data that changes and using a > map? That way you wouldn't need a const_cast and you are better enforcing the > key won't change. As you point out, I'm effectively manually doing what a std::map would do for me. Because my keys are actually tuples, I would need two structures for each entry: ProcessInfoKey, ProcessInfoData, ModuleInfoKey and ModuleInfoData. And then internally I'd be dealing with either iterators or *Info = std::pair<const *InfoKey, *InfoData>, with mildly opaque ->first.some_key_member and ->second.some_data_member accesses. Since the access to these things is behind two gateway functions, I figured the mental overhead of the const_cast was less than the overhead of dealing with std::pair, and two structs rather than one. grt@ had also suggested using "mutable" for the things that are able to be modified, but I dislike that as well as it means you can't have const-correctness for simple non-modifying accesses. So yes, I did consider it, and preferred the std::set approach, with liberal comments and use of "const" for members that are indeed part of the key. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:289: processes_.emplace(process_id, creation_time, process_type); On 2017/01/05 00:30:45, sky wrote: > Do you care if there was a ProcessInfo that matched in processes_ already? On windows a (pid, creation_time) pair uniquely identify a process for the uptime of the machine. A collision would be an indication that OnProcessStarted was called twice for the same process. This is a misuse of the class, strictly speaking, but not an irrecoverable error and not worth crashing for. I originally had DCHECKs (fail in a debug build to let you know that something is not right, but not in a release build). grt@ suggested this was less than ideal. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:295: auto it = processes_.find(key); On 2017/01/05 00:30:44, sky wrote: > processes_.erase(key) ? Errr... yeah. I'm sure there's some logical explanation for how this evolved to be this way... but dang if I can see it :) Done. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/05 00:30:45, sky wrote: > 2017 now on these files. Indeed! Done. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:22: // All calls must be made in the context of this task runner, unless On 2017/01/05 00:30:45, sky wrote: > How do you guarantee a function can't be called on this class from another > thread while in the destructor? That's a very good point. In practice, the ModuleDatabase is a singleton that lives until process death. But to make this explicitly safe, I guess the bouncing will have to be performed externally, with a weak ptr that's been drawn earlier from the ModuleDatabase. And the member functions would then advertise that they could only be called from the |task_runner|. That would be easy enough to do. Thoughts? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:40: uint64_t creation_time, On 2017/01/05 00:30:45, sky wrote: > Is there a reason not to use base::Time for creation_time and > module_time_date_stamp. Also, how come one is 64 and the other 32 bit? Because the times are being used as keys to distinguish processes/modules from each other. In both cases I don't actually care about the meaning of the value, but I want the representation to be precise as it is being used to distinguish one process/module from another. If I convert to base::Time, there's a potential for something to be lost in the translation, with no benefit to me as I don't use them as a "time" value anywhere. One is 64-bit and the other 32-bit because that's the bitness of their original representations (process creation time uses a 64-bit FILETIME, and module time-date-stamp uses a DWORD). https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:66: static constexpr size_t kInvalidIndex = ~0u; On 2017/01/05 00:30:45, sky wrote: > Please comment where this is used. Done. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:65: using ModuleDatabase::ModuleId; On 2017/01/05 00:30:45, sky wrote: > I'm surprised this works given ModuleId is private. Subclassing just to use the > types is bizarre. TestModuleDatabase is a friend as well. The subclassing lets me promote private types and functions to be visible publicly in the test, without requiring wrappers. I've always thought of this as a common test fixture trick. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.h (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.h:65: uint64_t creation_time_; On 2017/01/05 00:30:45, sky wrote: > base::Time? Responded earlier.
https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:133: DCHECK_LE(0u, bit_index); On 2017/01/06 20:35:47, chrisha (slow) wrote: > On 2017/01/05 00:30:45, sky wrote: > > How about a compile time assert that PROCESS_TYPE_MAX in <= 31? Otherwise the > > failure may happen at a weird time. For the >= 0 you could add a > > PROCESS_TYPE_FIRST_VALUE or something. > > The less than 0 check is actually completely bogus, as bit_index is unsigned. > And I don't actually require that PROCESS_TYPE_MAX be <= 31, but rather that > process_type - kMinProcessType (the first valid process type) be <= 31, which is > already checked. > > Would renaming kMinProcessType to kFirstValidProcessType be better? Good idea. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:283: return const_cast<ProcessInfo*>(&(*it)); On 2017/01/06 20:35:47, chrisha (slow) wrote: > On 2017/01/05 00:30:45, sky wrote: > > IMO this is a good indication you shouldn't be using set. Once you resort to a > > const_cast you can't ensure you don't accidentally do the wrong thing. Did you > > consider separating out the key aspects from the data that changes and using a > > map? That way you wouldn't need a const_cast and you are better enforcing the > > key won't change. > > As you point out, I'm effectively manually doing what a std::map would do for > me. Because my keys are actually tuples, I would need two structures for each > entry: ProcessInfoKey, ProcessInfoData, ModuleInfoKey and ModuleInfoData. And > then internally I'd be dealing with either iterators or *Info = std::pair<const > *InfoKey, *InfoData>, with mildly opaque ->first.some_key_member and > ->second.some_data_member accesses. > > Since the access to these things is behind two gateway functions, I figured the > mental overhead of the const_cast was less than the overhead of dealing with > std::pair, and two structs rather than one. > > grt@ had also suggested using "mutable" for the things that are able to be > modified, but I dislike that as well as it means you can't have > const-correctness for simple non-modifying accesses. > > So yes, I did consider it, and preferred the std::set approach, with liberal > comments and use of "const" for members that are indeed part of the key. The problem is this code is being clever and while it works now, subtlety like this is hard to maintain going forward. If you don't like ->first.some_key_member and ->second.some_other_member couldn't you define a struct? https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:22: // All calls must be made in the context of this task runner, unless On 2017/01/06 20:35:47, chrisha (slow) wrote: > On 2017/01/05 00:30:45, sky wrote: > > How do you guarantee a function can't be called on this class from another > > thread while in the destructor? > > That's a very good point. In practice, the ModuleDatabase is a singleton that > lives until process death. > > But to make this explicitly safe, I guess the bouncing will have to be performed > externally, with a weak ptr that's been drawn earlier from the ModuleDatabase. > And the member functions would then advertise that they could only be called > from the |task_runner|. That would be easy enough to do. Thoughts? I'm ok with how you have it, as long as you document the assumption, perhaps around where the singleton is defined. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:40: uint64_t creation_time, On 2017/01/06 20:35:47, chrisha (slow) wrote: > On 2017/01/05 00:30:45, sky wrote: > > Is there a reason not to use base::Time for creation_time and > > module_time_date_stamp. Also, how come one is 64 and the other 32 bit? > > Because the times are being used as keys to distinguish processes/modules from > each other. In both cases I don't actually care about the meaning of the value, > but I want the representation to be precise as it is being used to distinguish > one process/module from another. If I convert to base::Time, there's a potential > for something to be lost in the translation, with no benefit to me as I don't > use them as a "time" value anywhere. > > One is 64-bit and the other 32-bit because that's the bitness of their original > representations (process creation time uses a 64-bit FILETIME, and module > time-date-stamp uses a DWORD). Ok, thanks for the clarification. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win_unittest.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win_unittest.cc:65: using ModuleDatabase::ModuleId; On 2017/01/06 20:35:47, chrisha (slow) wrote: > On 2017/01/05 00:30:45, sky wrote: > > I'm surprised this works given ModuleId is private. Subclassing just to use > the > > types is bizarre. > > TestModuleDatabase is a friend as well. The subclassing lets me promote private > types and functions to be visible publicly in the test, without requiring > wrappers. I've always thought of this as a common test fixture trick. More commonly I've seen people friend the test, or friend some test helper class. Subclassing like this is weird, but if you say it's common go with it.
PTAL? sky: Bit the bullet and refactored to a std::map. wfh: For security review of Mojo code. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:283: return const_cast<ProcessInfo*>(&(*it)); On 2017/01/06 21:05:40, sky wrote: > On 2017/01/06 20:35:47, chrisha (slow) wrote: > > On 2017/01/05 00:30:45, sky wrote: > > > IMO this is a good indication you shouldn't be using set. Once you resort to > a > > > const_cast you can't ensure you don't accidentally do the wrong thing. Did > you > > > consider separating out the key aspects from the data that changes and using > a > > > map? That way you wouldn't need a const_cast and you are better enforcing > the > > > key won't change. > > > > As you point out, I'm effectively manually doing what a std::map would do for > > me. Because my keys are actually tuples, I would need two structures for each > > entry: ProcessInfoKey, ProcessInfoData, ModuleInfoKey and ModuleInfoData. And > > then internally I'd be dealing with either iterators or *Info = > std::pair<const > > *InfoKey, *InfoData>, with mildly opaque ->first.some_key_member and > > ->second.some_data_member accesses. > > > > Since the access to these things is behind two gateway functions, I figured > the > > mental overhead of the const_cast was less than the overhead of dealing with > > std::pair, and two structs rather than one. > > > > grt@ had also suggested using "mutable" for the things that are able to be > > modified, but I dislike that as well as it means you can't have > > const-correctness for simple non-modifying accesses. > > > > So yes, I did consider it, and preferred the std::set approach, with liberal > > comments and use of "const" for members that are indeed part of the key. > > The problem is this code is being clever and while it works now, subtlety like > this is hard to maintain going forward. If you don't like > ->first.some_key_member and ->second.some_other_member couldn't you define a > struct? The "first" and "second" refer to the std::map::value_type, so there's no way around it. I bit the bullet and refactored things this way. It's much less "clever", but slightly more onerous to read. The impact wasn't as bad as I as expecting, mostly confined to the unit tests. https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:22: // All calls must be made in the context of this task runner, unless On 2017/01/06 21:05:40, sky wrote: > On 2017/01/06 20:35:47, chrisha (slow) wrote: > > On 2017/01/05 00:30:45, sky wrote: > > > How do you guarantee a function can't be called on this class from another > > > thread while in the destructor? > > > > That's a very good point. In practice, the ModuleDatabase is a singleton that > > lives until process death. > > > > But to make this explicitly safe, I guess the bouncing will have to be > performed > > externally, with a weak ptr that's been drawn earlier from the ModuleDatabase. > > And the member functions would then advertise that they could only be called > > from the |task_runner|. That would be easy enough to do. Thoughts? > > I'm ok with how you have it, as long as you document the assumption, perhaps > around where the singleton is defined. This is done in the follow-up CL that actually defines the singleton: https://codereview.chromium.org/2613803005/
Thanks for converting to a map, it seems better to me, I hope you agree. LGTM https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.h (right): https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.h:169: base::FilePath module_path; optional (by which I mean I'm ok as is, up to you): having 'module_' in all these names is a bit redundant given the struct is named ModuleInfoKey. I would just go with path, size, but it's up to you.
security lgtm - with one question https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:49: length = ::GetModuleFileNameEx(process, module, temp_path.data(), there's some stuff in the MSDN about PSAPI_VERSION https://msdn.microsoft.com/en-us/library/windows/desktop/ms683198 just checking to see this is being handled here correctly and nothing needs to be set/unset. (We only support win7+, so I don't think we do need to)
Thanks wfh. https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... File chrome/browser/conflicts/module_event_sink_impl_win.cc (right): https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... chrome/browser/conflicts/module_event_sink_impl_win.cc:49: length = ::GetModuleFileNameEx(process, module, temp_path.data(), On 2017/01/11 21:25:38, Will Harris wrote: > there's some stuff in the MSDN about PSAPI_VERSION > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms683198 > > just checking to see this is being handled here correctly and nothing needs to > be set/unset. (We only support win7+, so I don't think we do need to) grt brought this up too. We are currently setting PSAPI_VERSION=1 in build/config/win/BUILD.gn. So we're doing right by the current setting. However, we should likely revisit this as we no longer need to support anything older than Win7. Worth filing a separate bug to look at this?
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2576843002/#ps200001 (title: "Address sky@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
still lgtm https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/160001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:283: return const_cast<ProcessInfo*>(&(*it)); On 2017/01/11 16:34:17, chrisha (slow) wrote: > On 2017/01/06 21:05:40, sky wrote: > > On 2017/01/06 20:35:47, chrisha (slow) wrote: > > > On 2017/01/05 00:30:45, sky wrote: > > > > IMO this is a good indication you shouldn't be using set. Once you resort > to > > a > > > > const_cast you can't ensure you don't accidentally do the wrong thing. Did > > you > > > > consider separating out the key aspects from the data that changes and > using > > a > > > > map? That way you wouldn't need a const_cast and you are better enforcing > > the > > > > key won't change. > > > > > > As you point out, I'm effectively manually doing what a std::map would do > for > > > me. Because my keys are actually tuples, I would need two structures for > each > > > entry: ProcessInfoKey, ProcessInfoData, ModuleInfoKey and ModuleInfoData. > And > > > then internally I'd be dealing with either iterators or *Info = > > std::pair<const > > > *InfoKey, *InfoData>, with mildly opaque ->first.some_key_member and > > > ->second.some_data_member accesses. > > > > > > Since the access to these things is behind two gateway functions, I figured > > the > > > mental overhead of the const_cast was less than the overhead of dealing with > > > std::pair, and two structs rather than one. > > > > > > grt@ had also suggested using "mutable" for the things that are able to be > > > modified, but I dislike that as well as it means you can't have > > > const-correctness for simple non-modifying accesses. > > > > > > So yes, I did consider it, and preferred the std::set approach, with liberal > > > comments and use of "const" for members that are indeed part of the key. > > > > The problem is this code is being clever and while it works now, subtlety like > > this is hard to maintain going forward. If you don't like > > ->first.some_key_member and ->second.some_other_member couldn't you define a > > struct? > > The "first" and "second" refer to the std::map::value_type, so there's no way > around it. I bit the bullet and refactored things this way. It's much less > "clever", but slightly more onerous to read. The impact wasn't as bad as I as > expecting, mostly confined to the unit tests. My $.02: the "first." and "second." sprinkled around to make this work are somewhat opaque -- it takes a far amount of scrolling around to understand what's being accessed. That said, I like that std::map is being used appropriately now. https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:265: auto result = modules_.emplace( to get perfect forwarding with map::emplace, we need to step a bit further out into la-la land: modules_.emplace(std::piecewise_construct, std::forward_as_tuple(ModuleInfoKey(...)), std::forward_as_tuple(ModuleInfoData())); i couldn't make this up if i wanted to! i don't think there's a world of difference between making the pair and using piecewise construct since temporary key/data instances will be created and moved into the container regardless.
https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... File chrome/browser/conflicts/module_database_win.cc (right): https://codereview.chromium.org/2576843002/diff/200001/chrome/browser/conflic... chrome/browser/conflicts/module_database_win.cc:265: auto result = modules_.emplace( On 2017/01/12 10:54:29, grt (UTC plus 1) wrote: > to get perfect forwarding with map::emplace, we need to step a bit further out > into la-la land: > modules_.emplace(std::piecewise_construct, > std::forward_as_tuple(ModuleInfoKey(...)), > std::forward_as_tuple(ModuleInfoData())); > i couldn't make this up if i wanted to! > > i don't think there's a world of difference between making the pair and using > piecewise construct since temporary key/data instances will be created and moved > into the container regardless. C++ makes me sad sometimes :(
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2576843002/#ps220001 (title: "Fix clang warning.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2576843002/#ps240001 (title: "Fix win-clang error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2576843002/#ps260001 (title: "Fix another win-clang error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2576843002/#ps280001 (title: "Yet another win-clang error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484330004700270,
"parent_rev": "4141f530d2d03c42cba3fb5c0949fd33fff0f36f", "commit_rev":
"d97de8e6f3e1beba791dfdd6d7f2dd7160c45331"}
Message was sent while issue was closed.
Description was changed from ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 ========== to ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 Review-Url: https://codereview.chromium.org/2576843002 Cr-Commit-Position: refs/heads/master@{#443651} Committed: https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2...
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2634073002/ by pkalinnikov@chromium.org. The reason for reverting is: Reverting because Win7 Tests (dbg)(1) builder failed on ModuleDatabaseTest.LoadAddressVectorOperations, which crashed: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....
Message was sent while issue was closed.
Description was changed from ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 Review-Url: https://codereview.chromium.org/2576843002 Cr-Commit-Position: refs/heads/master@{#443651} Committed: https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2... ========== to ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 Review-Url: https://codereview.chromium.org/2576843002 Cr-Commit-Position: refs/heads/master@{#443651} Committed: https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2... ==========
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, sky@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2576843002/#ps300001 (title: "Fix small bug.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1484700991954690,
"parent_rev": "8ffd7f1c09ec056e0aee132e95b40df60f5ff821", "commit_rev":
"6de20f5d8a70cf65a5346eff0adcef7df500dde5"}
Message was sent while issue was closed.
Description was changed from ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 Review-Url: https://codereview.chromium.org/2576843002 Cr-Commit-Position: refs/heads/master@{#443651} Committed: https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2... ========== to ========== [win] Create ModuleDatabase and ModuleEventSinkImpl. This creates the browser-side components of the ModuleDatabase. A future CL will hook up ModuleWatchers in each process to a singleton ModuleDatabase in the browser process. BUG=662084 Review-Url: https://codereview.chromium.org/2576843002 Cr-Original-Commit-Position: refs/heads/master@{#443651} Committed: https://chromium.googlesource.com/chromium/src/+/d97de8e6f3e1beba791dfdd6d7f2... Review-Url: https://codereview.chromium.org/2576843002 Cr-Commit-Position: refs/heads/master@{#444241} Committed: https://chromium.googlesource.com/chromium/src/+/6de20f5d8a70cf65a5346eff0adc... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/6de20f5d8a70cf65a5346eff0adc... |
