Issue 2345913003: [chrome_elf] NTRegistry - added wow64 redirection support.

penny

Description was changed from ========== braces fix. Different DEPS needed for NtRegistry tests. formatting. Don't ...

4 years, 3 months ago (2016-09-16 16:44:51 UTC) #1

penny

Description was changed from ========== braces fix. Different DEPS needed for NtRegistry tests. formatting. Don't ...

4 years, 3 months ago (2016-09-16 16:46:59 UTC) #2

penny

The CQ bit was checked by pennymac@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-16 16:47:24 UTC) #3

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/1

4 years, 3 months ago (2016-09-16 16:47:51 UTC) #4

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-16 17:57:14 UTC) #5

commit-bot: I haz the power

Dry run: 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/87213)

4 years, 3 months ago (2016-09-16 17:57:15 UTC) #6

penny

Description was changed from ========== NTRegistry - added wow64 redirection support. BUG=641169 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ...

4 years, 3 months ago (2016-09-16 22:16:55 UTC) #7

penny

pennymac@chromium.org changed reviewers: + robertshield@chromium.org

4 years, 3 months ago (2016-09-16 22:16:55 UTC) #8

penny

Hello Robert! New wow64 redirection support in NTRegistry. Registry access redirection override flags are now ...

4 years, 3 months ago (2016-09-16 22:24:26 UTC) #9

penny

The CQ bit was checked by pennymac@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-16 22:25:36 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/20001

4 years, 3 months ago (2016-09-16 22:26:13 UTC) #11

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-16 23:51:27 UTC) #12

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-16 23:51:28 UTC) #13

grt (UTC plus 2)

grt@chromium.org changed reviewers: + grt@chromium.org

4 years, 3 months ago (2016-09-20 10:39:52 UTC) #14

grt (UTC plus 2)

Here are some comments to get you started. I haven't finished yet, but I need ...

4 years, 3 months ago (2016-09-20 10:39:55 UTC) #15

penny

Thanks for your time Greg! I'm working through these now - and I know Robert ...

4 years, 3 months ago (2016-09-20 23:25:40 UTC) #16

grt (UTC plus 2)

It took me a while to grok the redirection code. I think it merits a ...

4 years, 3 months ago (2016-09-21 10:14:06 UTC) #17

robertshield

Hey Penny, sorry for the delay, I'm still completely swamped and will be until tomorrow ...

4 years, 2 months ago (2016-09-21 14:49:31 UTC) #18

penny

The CQ bit was checked by pennymac@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-09-23 23:47:04 UTC) #19

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/40001

4 years, 2 months ago (2016-09-23 23:47:36 UTC) #20

penny

Thanks very much Greg and Robert, Fixes are up, manual tests pass clean, and I'm ...

4 years, 2 months ago (2016-09-23 23:50:59 UTC) #21

Thanks very much Greg and Robert,

Fixes are up, manual tests pass clean, and I'm doing another dry run now.

Much appreciated!

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/BUILD.gn (right):

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/BUILD.gn:29: "test/nt_registry_test.cc",
On 2016/09/20 10:39:53, grt (UTC plus 2) wrote:
> please rename nt_registry_test.cc to nt_registry_unittest.cc and move it up
one
> dir; tests should live next to the files they are testing
>
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...).
> 
> is there a technical reason why this needs to be a new test binary? if not,
> please add it to the existing elf unittest exe. if there is, it looks like you
> can delete run_all_unittests.cc and instead take a dependency on
> //base/test:run_all_unittests.

Done.

I was following the structure of chrome_elf\blacklist and chrome_elf\hook_util. 
I've now restructured, renamed, and moved NTRegistry tests back into
chrome_elf_unittests.  

The nt_registry project is really independent of chrome_elf, so I've been making
it as self-contained as possible.  For now though, it's easy enough to have the
tests all together - especially given where the project is located in the source
tree.

Thanks for the coding standard link on the naming, TIL.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:126: typedef struct Node {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: remove typedef

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:129: bool if_match_set_to;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> consider replacing these two bools with something like
> enum RedirectionType { SHARED, REDIRECTED_BEFORE, REDIRECTED_AFTER };
> ...
>   RedirectionType redirection_type;

Done.  Good idea, thanks.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:133: // |next| is nullptr or an array of
Nodes.
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit "...Nodes of length |array_len|."

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:134: size_t array_len;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: swap these two so that they are in the same order as the comment above.

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:138: const Node classes_subtree[5] =
{{L"CLSID", true, true, 0, nullptr},
On 2016/09/20 10:39:53, grt (UTC plus 2) wrote:
> constexpr Node...
> same for the other constants below

Done.  TIL, thanks!

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:138: const Node classes_subtree[5] =
{{L"CLSID", true, true, 0, nullptr},
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: remove "5" since the initializer makes the size explicit

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:145: {L"Classes", false, false,
sizeof(classes_subtree) / sizeof(Node),
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: #include <stdlib.h> and use _countof(classes_subtree) (here and
elsewhere)

Done. Excellent, thank you.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:228: const Node
g_redirectionDecisionTreeHKCU = {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> suggested comment:
> // HKCU is shared by default with the exception of certain direct subkeys of
> // SOFTWARE\Classes that are mapped to SOFTWARE\Classes\WOW6432Node\<subkey>.

Acknowledged.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:232: const Node
g_redirectionDecisionTreeHKLM = {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> suggested comment:
> // HKLM\SOFTWARE is redirected by default to SOFTWARE\WOW6432Node with the
> // exception of some shared subkeys that may, in turn, have redirected
subkeys.

Acknowledged.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:232: const Node
g_redirectionDecisionTreeHKLM = {
On 2016/09/21 10:14:06, grt (UTC plus 2) wrote:
> i think this is more accurate, no?
> // HKLM\SOFTWARE is redirected by default to SOFTWARE\WOW6432Node with the
> // exception of many shared subkeys. HKLM\SOFTWARE\Classes is shared by
default
> // with the exception of certain direct subkeys that are redirected to
> // SOFTWARE\Classes\WOW6432Node\<subkey>.

Acknowledged.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:241: // start nor end with a path
seperator.
On 2016/09/21 10:14:06, grt (UTC plus 2) wrote:
> DCHECK/assert this requirement? it seems like an easy one to forget about.

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:243: std::wstring* subkey_path,
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> swap the order of the last two args; in/out args should follow in args
>
(https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering)

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:245: const wchar_t* redirect_before =
L"WOW6432Node\\";
On 2016/09/20 10:39:53, grt (UTC plus 2) wrote:
>   static constexpr wchar_t kRedirectBefore[] = ...

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:252: // Get rid of nt::AUTO.
On 2016/09/20 10:39:53, grt (UTC plus 2) wrote:
> nit: "Convert nt::AUTO to the appropriate root key." or something like that.

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:254: if (!root) {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> be explicit rather than assume AUTO == 0, and densify:
>   if (root == nt::AUTO)
>     temp_root = g_system_install ? nt::HKLM : nt::HKCU

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:261: if (((temp_root == nt::HKCU) &&
(::wcslen(nt::HKCU_override) != 0)) ||
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> don't compute a string length if you just want to know whether the string is
> empty or not:
>   if ((temp_root == nt::HKCU && *nt::HKCU_OVERRIDE) ||

Done.  Good one, thank you.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:268: if
(system_info.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_INTEL) {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: omit braces for single-line conditional and body

Thank you - sometimes I slip back into old coding standards.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:273: BOOL use_wow64 = false;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> i think bool is more natural for this since that's how it is used/set below

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:276: typedef decltype(IsWow64Process)*
IsWow64ProcessFunc;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit:
>     using IsWow64ProcessFunction = decltype(&IsWow64Process);

Done.  This was a bit of a mind bend for my C mind.  Thanks!

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:277: IsWow64ProcessFunc is_wow64_process =
reinterpret_cast<IsWow64ProcessFunc>(
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: cache this since it won't change throughout the lifetime of the process.
> something like this should work:
>     // Get and cache the result of IsWow64Process the first time through this
> function.
>     static const bool is_wow64 = []() {
>       using IsWow64ProcessFunction = decltype(&IsWow64Process);
>       IsWow64ProcessFunction is_wow64_process_function = ...;
>       if (!is_wow64_process_function)
>         return false;
>       BOOL wow64_process = FALSE;
>       if (!is_wow64_process_function(::GetCurrentProcess, &wow64_process))
>         return false;
>       return !!wow64_process;
>     }();
>     use_wow64 = is_wow64;

Ack.  I've moved this out into IsThisProcWow64().  It is now checked once at lib
initialization, and stored in g_wow64_proc.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:296: const Node* current = nullptr;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> ... = temp_root == nt::HKCU ? &g_redirectionDecisionTreeHKCU :
> &g_redirectionDecisionTreeHKLM;

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:304: const wchar_t* position =
subkey_path->data();
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> use c_str() rather than data() since the code below assumes that |position|
> points to a null-terminated string.

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:316: size_t len =
::wcslen(current->to_match);
On 2016/09/21 10:14:06, grt (UTC plus 2) wrote:
> it should be possible to get rid of this length computation by constructing
the
> Nodes with the length of the string. i think template magic could accomplish
> this, where the string constant is a template argument so that its size can be
> deduced at compile time. just a thought.

wow.  So you pushed me out of my comfort zone there.  I finally got some
compile-time/template foo working.  I've adjusted the Node struct to auto
compute the string length.  It took all morning, but it's probably good for me. 
<whew>

I tried to auto compute the length of the |next| array of Nodes too... but
couldn't in the end because arrays of size 0 are not legal.  Let me know if you
know a way around that.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:319: if (::wcslen(position) >= len) {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> rather than computing the string length through each iteration of the outer
> loop, how about grabbing the end of the path above the loop (e.g., "const
> wchar_t* subkey_path_end = subkey_path->c_str() + subkey_path->length();") and
> then computing the length in the loop as "subkey_path_end - position;"?

Done. Did it a slightly different way - but no more calculating length every
loop.  Thanks!

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:356: current += 1;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> nit: ++current;

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:357: index++;
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> personal nit: prefer pre-increment even for scalar types for consistency with
> class and template types; see
>
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement.

Done.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:43: typedef enum _WOW64_OVERRIDE {
On 2016/09/20 10:39:54, grt (UTC plus 2) wrote:
> could you use modern "enum WOW64_OVERRIDE {...};" style rather than old-school
> typedef?

Done. I am old-school.  #sorrynotsorry, #thanksforcatching!  :D

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:236: void
ChangeDefaultWow64Redirection(bool default_on);
On 2016/09/20 10:39:55, grt (UTC plus 2) wrote:
> in keeping with the principle of least surprise, i think that the default
should
> be to mimic advapi32's redirection. i imagine most programmers using the api
> will expect it to work like the other registry api they're familiar with.

Done.  The "principle of least surprise" to me would be to have NtRegistry
behave the way you expect ntdll to behave.

See new comments in nt_registry.cc about wow64 redirection.

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/test/nt_registry_test.cc (right):

https://codereview.chromium.org/2345913003/diff/20001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/test/nt_registry_test.cc:342: // Note: Disabled for
automated testing (HKLM protection).  Local testing only.
On 2016/09/20 10:39:55, grt (UTC plus 2) wrote:
> the bots run tests as admin. can the HKLM tests run on the bots knowing that?
> 
> if the test is only valid from an elevated prompt, please keep it enabled and
do
> something like
>
https://cs.chromium.org/chromium/src/chrome/installer/util/advanced_firewall_...
> to have the test early-exit if it's not running elevated.

So I've previously been informed that we shouldn't be touching HKLM on try bots,
and in fact that any registry manipulation should have the
registry_util::RegistryOverrideManager engaged so that we're only touching a
temporary subkey.

Now, I'm not using the OverrideManager for my NTRegistry tests at all, because
otherwise wow64 registry redirection will never happen.

But this has been an issue for some of our newer sandbox tests as well - that
require changing HKLM - so they were disabled to be run manually only.

I don't think this will be good enough moving forward - especially with more
low-level security testing.  I think we'll need to come up with a way (or a
specific bot) to allow automated try testing that could impact the machine OS
image.  But that's for another day.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-24 00:33:30 UTC) #22

commit-bot: I haz the power

Dry run: 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_compile_dbg_ng/builds/263523)

4 years, 2 months ago (2016-09-24 00:33:31 UTC) #23

grt (UTC plus 2)

looking pretty good! https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/nt_registry.cc File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/nt_registry.cc#newcode203 chrome_elf/nt_registry/nt_registry.cc:203: // NOTE: Could not auto compute ...

4 years, 2 months ago (2016-09-26 11:49:19 UTC) #24

penny

Thanks a lot again Greg! Fixes are up. Manual tests have been run clean, and ...

4 years, 2 months ago (2016-09-28 22:36:52 UTC) #25

Thanks a lot again Greg!

Fixes are up.  Manual tests have been run clean, and I'm starting try bots now.

(Note the extra fun MSVC bug in our compiler that I managed to stumble across.
I've managed to work around for now.)

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:203: // NOTE: Could not auto compute this
array length like above, as arrays of
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> could you have a 2nd ctor for the leaf nodes that didn't have the last two
args
> (|n| and |n_len|)?

Thanks - I've added a second constructor, which is nice.  HOWEVER, I discovered
an MS compiler bug after making these changes.  I can't handle > 2 layers of
"nested" constexpr at compile time.
https://connect.microsoft.com/VisualStudio/feedback/details/3104499

I've found a way around this for now... not ideal, but not too dirty.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:321: #ifdef _DEBUG
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> nit: #ifndef NDEBUG (or, less common, #if !defined(NDEBUG)) is the normal way
we
> do this (LOVE that double negative!)
> 
> that said, you don't need this at all here as "assert" is a noop when NDEBUG
is
> defined, which it is in our release builds.
> 
> i'm curious: what happens if this assert is hit this early in process startup?
> the chromium style says not to do error handling around debug-only assertions
> like this since a release build will silently ignore the error rather than
> sending us a crash. in this case, there's no crash reporting, so perhaps this
is
> the right thing. what would happen in a release build if this condition were
> satisfied? what if it this did a __debugbreak(); for release builds?

We don't want any sort of break to happen in release mode, from chrome_elf at
startup.

Specifically here in ProcessRedirection() in nt_registry.cc, I've removed the
assert.  Instead, I've added a SanitizeSubkeyPath function that can be called
for externally-supplied subkey paths.  It is now called from OpenRegKey and
CreateRegKey to ensure no starting or trailing backslashes.

So no silent failures in NtRegistry lib - the APIs return bool.


Chrome_elf is a separate discussion.  It has a couple places that CAN fail
silently.  When I recently landed a CL to add some early browser security here,
it was agreed with owners and code reviewers to continue with failing silently
(in release) for now.  However, it was also agreed that we'd really like to add
some histograms into the ELF startup (and some other sandbox startup as well) to
detect if security features or anything is silently failing.  Then at least we
can have some data about if/when this is happening in the wild.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:364: // Hold a pointer to the end of
|subkey_path|, for calculations.
On 2016/09/26 11:49:17, grt (UTC plus 2) wrote:
> nit: update comment

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:389: if (redirect_state != SHARED) {
On 2016/09/26 11:49:17, grt (UTC plus 2) wrote:
> nit: remove this outer conditional and change the else on line 395 to "} else
if
> (redirect_state == REDIRECTED_AFTER) {"

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:442: if ((key == nt::HKCU) &&
(::wcslen(nt::HKCU_override) != 0)) {
On 2016/09/26 11:49:17, grt (UTC plus 2) wrote:
> nit: use * rather than wcslen here and below, too

Done.  Thanks!

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:23: wchar_t temp_path[MAX_PATH] =
L"SOFTWARE\\Chromium\\TempTestKeys\\";
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> the RegistryOverrideManager in base was problematic once upon a time, as it
> would leave data in the registry in case of test failures. over time, the
> registry on the bots got clogged with tons of old test keys. this was fixed in
> http://crrev.com/234367. today, the ROM will automatically delete stale values
> on a machine each time it is used. do we not need something similar here?

Sure.  I've added NtRegDeleteStaleTestKeys() that is run in the NtRegistryTest
class constructor.  It'll delete any keys not touched in 12 hours.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:28: if (key == nt::HKCU) {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> nit: omit braces

Apparently we have a precheck that asks for braces in an "if/else if" block. 
That's why I added these for my first upload.

Having said that - now they need to be there for multi-lines.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:33: // nt::AUTO should not be
passed into this function.
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> remove comment and ASSERT_NE(nt::AUTO, key); at the top of the fn

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:37: if (key == nt::HKCU) {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> nit: omit braces

Apparently we have a precheck that asks for braces in an "if/else if" block. 
That's why I added these for my first upload.

Having said that - now they need to be there for multi-lines.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:42: // nt::AUTO should not be
passed into this function.
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> same comment as above

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:45: class NtRegistryTest : public
testing::Test {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> prefer using TEST rather than TEST_F with an empty fixture.

Acknowledged.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:54: TEST_F(NtRegistryTest, API) {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> i this this function should be broken up into a number of small tests within a
> single test fixture that each exhaustively test one thing. this will make it
> easier to read and easier for another developer to understand future test
> failures. in so doing, move the redirection into SetUpTestCase and
> TearDownTestCase; otherwise, cancel will not be called in case of any test
> failure. if it makes sense to, creation/deletion of the temp key could be
moved
> into the ctor/dtor or SetUp/TearDown overrides (prefer the former unless
> restrictions require the latter; see
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...).

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:80: 
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> ?
>   EXPEC_NE(key_handle, INVALID_HANDLE_VALUE);

nt::CreateRegKey does not touch |key_handle| unless it returns true and is
successful.

This function ASSERTS if the function returns false... also none of the follow
up calls will succeed without a valid handle.

But I can throw some checks in there.

Done in a couple places.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:89: get_dword == dword_val);
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> break these up into two expectations so that the developer doesn't need to
guess
> which one failed

Done. Got lazy there.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:179: ACCESS_MASK access =
KEY_WRITE | DELETE;
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> nit: const-ipate this and root_key:
>   constexpr ACCESS_MASK kAccess = ...;  (kFoo since this is a compile-time
> constant)
>   const HKEY root_key = ...; (unix_hacker since this is a runtime constant)

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:184:
nt::DeleteRegKey(nt_root_key, nt::NONE, path);
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> wdyt of using advapi functions for all of the setup-and-verify code? with this
> help ensure consistency?

I don't actually think it'll make any difference.  I made a conscious decision
in this redirection test to only use advapi in these two CreateKey spots - where
I purposefully want to catch any MS behaviour changes.

I'd prefer to leave this as is.  The test will absolutely break if NTReg lib is
not behaving as expected.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:260: // Hard-coding array size to
match nt_registry.cc.  Easier to catch missing
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> please make this comment directly reference the thing it needs to match in
> nt_registry.cc, and put a corresponding comment there pointing the reader at
> this. i don't see how putting the size in here makes it any less likely that
> someone will edit one but not the other. (comment applies to
hklm_no_redirects,
> too)

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:262: const wchar_t*
classes_redirects[10] = {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> nit: constexpr const wchar_t* kClassesRedirects[] = {
> (similarly for hklm_no_redirects)

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:273: 
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> static_assert(_countof(kClassesRedirects) & 0x01 == 0, "must have an even
number
> of Classes redirects");

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:282: while (index < array_size) {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> ?
>   for (size_t index = 0; index < _countof(kClassesRedirects); index += 2)
> 
> (same on line 378)

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:287: }
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> also test at least one key in Classes that should be shared?

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:291: const wchar_t*
hklm_no_redirects[49] = {
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> constexpr const wchar_t*

Done.

https://codereview.chromium.org/2345913003/diff/40001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:385:
//----------------------------------------------------------------------------
On 2016/09/26 11:49:18, grt (UTC plus 2) wrote:
> nit: remove this

Done.

penny

The CQ bit was checked by pennymac@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-09-28 23:12:20 UTC) #26

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/60001

4 years, 2 months ago (2016-09-28 23:12:52 UTC) #27

penny

Description was changed from ========== NTRegistry - added wow64 redirection support. Also moved the NTRegistry ...

4 years, 2 months ago (2016-09-28 23:21:44 UTC) #28

robertshield

Some quick drive-by tidy up nits Could be done here or in a follow up ...

4 years, 2 months ago (2016-09-28 23:49:24 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-29 01:10:24 UTC) #30

commit-bot: I haz the power

Dry run: 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/93658)

4 years, 2 months ago (2016-09-29 01:10:25 UTC) #31

grt (UTC plus 2)

getting down to brass tacks. these are mostly nits that i think are an improvement. ...

4 years, 2 months ago (2016-09-29 10:30:23 UTC) #32

penny

The CQ bit was checked by pennymac@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-09-30 05:20:37 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/80001

4 years, 2 months ago (2016-09-30 05:20:54 UTC) #34

penny

Thanks again. :) I've just uploaded a bunch of fixes. I missed a few of ...

4 years, 2 months ago (2016-09-30 05:28:23 UTC) #35

Thanks again. :)

I've just uploaded a bunch of fixes.  I missed a few of Robert's comments in
nt_registry.h - which I will ponder tomorrow morning.

Wanted to get Greg's fixes up tonight.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:53: bool IsThisProcWow64() {
On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> This function looks an awful lot like
>
https://codesearch.chromium.org/chromium/src/chrome_elf/hook_util/hook_util.c...
> 
> Could this be commonalizified (totally a word) somehow?

Commonalizified.  I've exposed this information with a new nt_registry function
IsCurrentProcWow64().  hook_util now calls that API.  This is nice because the
info only should need to be checked once, and then can be stored.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:165: //   The HKLM and HKCU descision
trees represent the information at the MSDN
On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> nit: decision

Thank you.  I dislike spelling or grammatical errors!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:215: constexpr Node classes_subtree[] =
{{L"CLSID", REDIRECTED_BEFORE},
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: kClassesSubtree

Sorry - I should have changed these at the same time as the fixes in the test
file.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:223: constexpr Node
hklm_software_subtree[] = {
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: kHklmSoftwareSubtree

Sorry - I should have changed these at the same time as the fixes in the test
file.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:297: constexpr Node
g_redirectionDecisionTreeHKCU = {L"SOFTWARE\\Classes", SHARED,
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: kRedirectionDecisionTreeHkcu

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:302: constexpr Node
g_redirectionDecisionTreeHKLM = {L"SOFTWARE", REDIRECTED_AFTER,
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: kRedirectionDecisionTreeHklm

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:310: // - Also, |subkey_path| should be
passed to SanitizeSubkeyPath() before calling
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> how about enforcing this by adding to the top of the function:
>   assert(subkey_path->empty() || subkey_path->front() != L'\\');
>   assert(subkey_path->empty() || subkey_path->back() != L'\\');
> 
> better yet, can you make life easier on the callers by calling Sanitize in
here
> rather than making them do it?

My take on this is that external APIs should be responsible for verifying and
sanitizing externally-supplied input.  They should gracefully handle any bad
input in debug or release builds.  I feel that private functions are fine to
simply debug assert on broken expectations as a red flag.

ProcessRedirection() is an internal function that will only be called from
within this file.  I've added the asserts you mentioned - just to catch
developer mistakes within this file.

By only sanitizing external inputs, we won't end up doing the same run-time
checks more than once.  (Private functions can easily be called from many
places.)

**I've also just added a |value_name| input check to QueryRegKeyValue() and
SetRegKeyValue().

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:368: // The root of the tree is an array
of 1.
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> uber-nit: move the definition of |array_len| up so that it immediately
precedes
> the definition of |current| on line 352. this makes it more clear that it
refers
> to the length of |current|, and matches the order in which the two are updated
> on line 402.

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:369: size_t array_len = 1;
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: array_len -> node_array_len

I like this, thanks.  More clear.  I've also changed current -> current_node.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:370: size_t index = 0;
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: index -> node_index

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:372: size_t len = current->to_match_len;
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: len -> to_match_len

Done. current_to_match_len.  I don't want any confusion with the Node member
|to_match_len|.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:420: subkey_path->insert((insertion_point
- subkey_path->data()), insert_string);
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> nit: data -> c_str for consistency with the code above

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:430: if (!root) {
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> > should this be if (root == nt::AUTO)?
> 
> yes, please

Done.  Sorry - missed this one!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:463: if (input == nullptr ||
input->empty())
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> two comments here:
> 
> since this is a private function and you know the callers never pass in
nullptr,
> replace the first part of this condition with an assert.
> 
> if it is true that |input| is not expected to be empty in the general case,
> remove the second part of this and let empty inputs be handled below. this
> avoids having an extra branch for the ordinary case to optimize for the rare
> case.

I fully agree.  Thank you.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:467: while (input->back() == L'\\')
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> this provokes undefined behavior if the string contains only backslashes. how
> about:
>   size_t last_valid_pos = input->find_last_not_of(L'\\');
>   if (last_valid_pos == std::wstring::npos) {
>     // The string is all backslashes. Clear it and abort.
>     input->clear();
>     return;
>   }
>   // Chop of the trailing backslashes.
>   input->resize(last_valid_pos + 1);
> 
>   // Remove leading backslashes.
>   input->erase(0, input->find_first_not_of(L'\\'));
> 
> and then please add a unittest for this. :-)

TIL, don't call front/back on an empty std::wstring, and at throws exceptions! 
Thanks!
How's this change?

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:472: while (input->at(index) == L'\\')
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> fyi: at() does bounds checking and throws exceptions, so avoid it at all
costs.

TIL.  Thanks.  Reading the docs, I think [] operator will work fine.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:565: wchar_t
HKLM_override[g_kRegMaxPathLen] = L"";
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> wdyt of adding 1 to the length here and below so that g_kRegMaxPathLen doesn't
> include the terminator? then you can get rid of the " - 1" in the test.

Done.  Thanks.  I've adjusted this everywhere to be accurate for Windows
registry.  255 is max length without null character.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:588: if
(!ParseFullRegPath(ConvertRootKey(root), redirected_key_path.c_str(),
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> pass redirected_key_path directly as a const std::wstring&

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:7: #include <windows.h>
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> nit: ordinarily windows.h must be included before other windows headers.
unless
> there's a reason not to do that here, please move windows.h up.

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:9: #include
"chrome_elf/nt_registry/nt_registry.h"
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> move up above line 5 with a blank line between this and the system headers --
> the header under test should precede all other includes; see
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes.

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:118: constexpr wchar_t*
kClassesRedirects[] = {
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> constexpr const wchar_t* kClassesRedirects[] = ...
> "constexpr" makes the array itself a compile-time constant.
> "const" says that the pointers in the array point to constant strings.
> C++ is the awesome!

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:130: // kClassesRedirects must
have an even number of elements.
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> nit: remove superfluous comment since it adds nothing beyond the string in
line
> 132
(https://google.github.io/styleguide/cppguide.html#Implementation_Comments)

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:140: ::wcscpy(nt::HKCU_override,
L"");
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> won't this make sure that another test blows up spectacularly rather than
making
> sure that there isn't another one? how about the following so that this test
> will fail if another one is already running?
>   ASSERT_EQ(*nt::HKCU_override, L'\0') << "Parallel tests are not supported.";

OK - I've actually fixed some of the other tests now (blacklist), that weren't
cleaning up the testing override at the end of their test run.  This is what
would cause a problem.  I no longer need to do the "just in case the env is
dirty" cleanup here anymore.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:261: public:
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> nit: test harness style generally has these protected: rather than public:

Done. I just saw other ones set to public.  Thanks!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:286: wchar_t key_name[255] = {};
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> 256 to account for the terminator. please add a link to
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724872(v=vs.85).aspx.

Done. This is defined in nt_registry.h now.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:297: name_size = 255;
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> _countof(key_name)

Good one, thanks.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:298: status = RegEnumKeyExW(key,
index, key_name, &name_size, NULL, NULL, NULL,
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> NULL -> nullptr everywhere

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:299: &ft);
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> ft -> last_write_time
> (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules)

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:301: ULONGLONG key_ticks =
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> please use base::Time::FromFileTime for last_write_time, base::Time::Now for
> |current|, and base::TimeDelta::FromHours for the delta. this will be a
> bajillion times more clear. :-) here's one approach:
> 
>     const base::Time max_write_time = base::Time::Now() -
>         base::TimeDelta::FromHours(12);
> ...
>         if (base::Time::FromFileTime(last_write_time) < max_write_time)
>           to_delete.emplace_back(key_name, name_size);

Sorry, there's no base usage at all in nt_registry.  Enforced by DEPS in this
directory.  I was going to use base just in the unittests, when this file lived
in a different subdirectory (.\Test\) where I could have a different DEPS file. 
But it doesn't live there now.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:307: // Don't delete until after
the enumerating is done.
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> nit: remove this comment and place one above line 296 that says something
like:
>     // Collect the names of all subkeys that have not been modified in at
least
> 12 hours.

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:309: to_delete.push_back(temp);
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> nit: construct the string in place using the length you already have:
>           to_delete.emplace_back(key_name, name_size);

TIL emplace_back!!!  Didn't know this existed.  <fireworks>

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:316: for (std::wstring subkey :
to_delete)
On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> const auto& subkey_name : to_delete

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:376: 
On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> please also test that Query returns false before Set is called

Done.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-30 06:22:10 UTC) #36

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/165657)

4 years, 2 months ago (2016-09-30 06:22:10 UTC) #37

grt (UTC plus 2)

this looks good to me. i like the new testing hook interface. i'm down to ...

4 years, 2 months ago (2016-09-30 09:32:17 UTC) #38

this looks good to me. i like the new testing hook interface. i'm down to final
nits here. thanks for bearing with me!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:310: // - Also, |subkey_path| should be
passed to SanitizeSubkeyPath() before calling
On 2016/09/30 05:28:22, penny wrote:
> On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > how about enforcing this by adding to the top of the function:
> >   assert(subkey_path->empty() || subkey_path->front() != L'\\');
> >   assert(subkey_path->empty() || subkey_path->back() != L'\\');
> > 
> > better yet, can you make life easier on the callers by calling Sanitize in
> here
> > rather than making them do it?
> 
> My take on this is that external APIs should be responsible for verifying and
> sanitizing externally-supplied input.  They should gracefully handle any bad
> input in debug or release builds.  I feel that private functions are fine to
> simply debug assert on broken expectations as a red flag.
> 
> ProcessRedirection() is an internal function that will only be called from
> within this file.  I've added the asserts you mentioned - just to catch
> developer mistakes within this file.
> 
> By only sanitizing external inputs, we won't end up doing the same run-time
> checks more than once.  (Private functions can easily be called from many
> places.)
> 
> **I've also just added a |value_name| input check to QueryRegKeyValue() and
> SetRegKeyValue().

Your philosophy on when to sanitize external input makes total sense. My
observation was that (at the time of writing), SanitizeSubkeyPath was only
called immediately before ProcessRedirection, and ProcessRedirection was never
called without being immediately preceded by SanitizeSubkeyPath. Is sanitation
really needed, or can that step be removed by gently adjusting the redirector?

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:467: while (input->back() == L'\\')
On 2016/09/30 05:28:22, penny wrote:
> On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > this provokes undefined behavior if the string contains only backslashes.
how
> > about:
> >   size_t last_valid_pos = input->find_last_not_of(L'\\');
> >   if (last_valid_pos == std::wstring::npos) {
> >     // The string is all backslashes. Clear it and abort.
> >     input->clear();
> >     return;
> >   }
> >   // Chop of the trailing backslashes.
> >   input->resize(last_valid_pos + 1);
> > 
> >   // Remove leading backslashes.
> >   input->erase(0, input->find_first_not_of(L'\\'));
> > 
> > and then please add a unittest for this. :-)
> 
> TIL, don't call front/back on an empty std::wstring, and at throws exceptions!

> Thanks!
> How's this change?

I think it's much more clear and safe to use the string API rather than rolling
your own. I also think you should avoid substr since it will create a new
instance and make a whole new allocation. erase will effectively be a memmove,
which i think is better than malloc->memcpy->free.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:301: ULONGLONG key_ticks =
On 2016/09/30 05:28:23, penny wrote:
> On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > please use base::Time::FromFileTime for last_write_time, base::Time::Now for
> > |current|, and base::TimeDelta::FromHours for the delta. this will be a
> > bajillion times more clear. :-) here's one approach:
> > 
> >     const base::Time max_write_time = base::Time::Now() -
> >         base::TimeDelta::FromHours(12);
> > ...
> >         if (base::Time::FromFileTime(last_write_time) < max_write_time)
> >           to_delete.emplace_back(key_name, name_size);
> 
> Sorry, there's no base usage at all in nt_registry.  Enforced by DEPS in this
> directory.  I was going to use base just in the unittests, when this file
lived
> in a different subdirectory (.\Test\) where I could have a different DEPS
file. 
> But it doesn't live there now.

You can safely use base in the test. Add this to DEPS to allow it:

specific_include_rules = {
  ".*_unittest\.cc": [
    "+base",
  ],
}

See https://cs.chromium.org/chromium/src/buildtools/checkdeps/builddeps.py for
the DEPS format documentation.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:309: to_delete.push_back(temp);
On 2016/09/30 05:28:23, penny wrote:
> On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> > nit: construct the string in place using the length you already have:
> >           to_delete.emplace_back(key_name, name_size);
> 
> TIL emplace_back!!!  Didn't know this existed.  <fireworks>

Indeed! I knew about it but had forgotten that we're allowed to use it now.
Sweet!

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:138: if (temp.length() != 0) {
nit: if (!temp.empty())
nit: omit braces :-)

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:208: base::string16 temp = L"";
nit: omit the assignment. just let |temp| be default initialized to an empty
string.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test_main_dll.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:17: std::wstring content;
nit: construct the string with the desired size:
    std::wstring content(buffer_size, '\0');
and remove the resize

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util_unittest.cc:76: ASSERT_TRUE(key != nt::AUTO);
  ASSERT_NE(key, nt::AUTO);?

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util_unittest.cc:89: ASSERT_TRUE(key != nt::AUTO);
  ASSERT_NE

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util_unittest.cc:90: base::string16 temp = L"";
omit assignment

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:385: if (::wcsnicmp(position,
current_node->to_match, current_to_match_len) ==
nit: !::wcsnicmp to make this fit on one line

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:440: if (root == nt::AUTO) {
nit: omit braces now that the comment is out of the body

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:757: if (value_name == nullptr ||
*value_name == L'\0' ||
will g_nt_query_value_key already do this validation? if so, do we need to
repeat it here? i think it's better not to, since MS has been known to change
the sizes of things (MAX_PATH!!!111!?!!!). this comment applies to all of the
range checks you've added.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:1079: bool SetTestingOverride(nt::ROOT_KEY
root, const std::wstring& new_path) {
nit: omit "nt::" here and below since this is within namespace nt

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:1085: if (::wcsnlen(sani_new_path.c_str(),
nt::g_kRegMaxPathLen + 1) ==
why not sani_new_path.length() > g_kRegMaxPathLen?

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:1094: if (temp_root == nt::HKCU)
how about doing away with temp_root and changing this to:
  if (root == HKCU || root == AUTO && !g_system_install)

alternatively, are you opposed to modifying |root| in place rather than
introducing another local for it?

comment applies to GetTestingOverride as well

if this pattern is widespread, you could add a helper like this in the unnamed
namespace:
bool IsHKCU(nt::ROOT_KEY root) {
  return root == nt::HKCU || root == nt::AUTO && !g_system_install;
}

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:32: constexpr size_t g_kRegMaxPathLen =
255;
this is max name for a single key, not for a full path, no?

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:236: void GetTestingOverride(nt::ROOT_KEY
root, std::wstring* out_path);
nit: we generally return strings as values rather than take them as out params.
i think
std::wstring GetTestingOverride(nt::ROOT_KEY root);
is more convenient

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:300:
to_delete.emplace_back(key_name);
(key_name, name_size)? this avoids an extra strlen

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:313: ASSERT_TRUE(key !=
nt::AUTO);
ASSERT_NE

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:337: ASSERT_TRUE(key !=
nt::AUTO);
ASSERT_NE

penny

The CQ bit was checked by pennymac@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-10-01 01:40:15 UTC) #39

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/100001

4 years, 2 months ago (2016-10-01 01:40:44 UTC) #40

penny

"thanks for bearing with me!" My goodness no - thank you. I really appreciate the ...

4 years, 2 months ago (2016-10-01 01:44:26 UTC) #41

"thanks for bearing with me!"  My goodness no - thank you.  I really appreciate
the time it takes to do thorough code reviews.

I'm also trying to be "indoctrinated" into the Chromium coding standards, but a
lifetime of habits are against me.  :D  Never mind trying to get into the "C++"
mindset.

Robert knows!  You both are excellent code reviewers.

I feel like we're very close now...

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:310: // - Also, |subkey_path| should be
passed to SanitizeSubkeyPath() before calling
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> On 2016/09/30 05:28:22, penny wrote:
> > On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > > how about enforcing this by adding to the top of the function:
> > >   assert(subkey_path->empty() || subkey_path->front() != L'\\');
> > >   assert(subkey_path->empty() || subkey_path->back() != L'\\');
> > > 
> > > better yet, can you make life easier on the callers by calling Sanitize in
> > here
> > > rather than making them do it?
> > 
> > My take on this is that external APIs should be responsible for verifying
and
> > sanitizing externally-supplied input.  They should gracefully handle any bad
> > input in debug or release builds.  I feel that private functions are fine to
> > simply debug assert on broken expectations as a red flag.
> > 
> > ProcessRedirection() is an internal function that will only be called from
> > within this file.  I've added the asserts you mentioned - just to catch
> > developer mistakes within this file.
> > 
> > By only sanitizing external inputs, we won't end up doing the same run-time
> > checks more than once.  (Private functions can easily be called from many
> > places.)
> > 
> > **I've also just added a |value_name| input check to QueryRegKeyValue() and
> > SetRegKeyValue().
> 
> Your philosophy on when to sanitize external input makes total sense. My
> observation was that (at the time of writing), SanitizeSubkeyPath was only
> called immediately before ProcessRedirection, and ProcessRedirection was never
> called without being immediately preceded by SanitizeSubkeyPath. Is sanitation
> really needed, or can that step be removed by gently adjusting the redirector?

I hear ya, and considered just changing ProcessRedirection to handle it. 
However, now I've got another external API for setting the testing override that
will use this sanitize function.

The lib on a whole assumes that the subkey paths it is storing and dealing with
won't have starting or trailing slashes - and I think it's nice to have a
consistent format expectation.

I've added an extra bit in there to ensure there are no sequences of more than
one backslash in a row as well.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:467: while (input->back() == L'\\')
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> On 2016/09/30 05:28:22, penny wrote:
> > On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > > this provokes undefined behavior if the string contains only backslashes.
> how
> > > about:
> > >   size_t last_valid_pos = input->find_last_not_of(L'\\');
> > >   if (last_valid_pos == std::wstring::npos) {
> > >     // The string is all backslashes. Clear it and abort.
> > >     input->clear();
> > >     return;
> > >   }
> > >   // Chop of the trailing backslashes.
> > >   input->resize(last_valid_pos + 1);
> > > 
> > >   // Remove leading backslashes.
> > >   input->erase(0, input->find_first_not_of(L'\\'));
> > > 
> > > and then please add a unittest for this. :-)
> > 
> > TIL, don't call front/back on an empty std::wstring, and at throws
exceptions!
> 
> > Thanks!
> > How's this change?
> 
> I think it's much more clear and safe to use the string API rather than
rolling
> your own. I also think you should avoid substr since it will create a new
> instance and make a whole new allocation. erase will effectively be a memmove,
> which i think is better than malloc->memcpy->free.

I'm fully agreeing with you now.  This is much nicer and more efficient.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:39: typedef enum _ROOT_KEY { AUTO = 0,
HKLM, HKCU } ROOT_KEY;
On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> thoughts re: AUTO: it seems to be used in only once place (install_util.cc).
It
> minutely complicates some of the implementations and requires an extra global
> bool. 
> 
> Also, install_util.cc already has an implementation of IsSystemInstall()
>
(https://codesearch.chromium.org/chromium/src/chrome/install_static/install_ut...)
> which could be called at the place in install_util.cc where AUTO is now
passed. 
> 
> This would simplify the interface here and allow us to have a bit less code in
> the implementation.

Wow.  Way to swoop in with the profound thinking Robert.

It just makes sense to me for an NTDLL Registry library to offer the AUTO
feature.  I know install_static existed before this lib, and constantly checks
whether it's system or not (it should probably cache it once), so it doesn't
really need to use AUTO.  But I imagine if I wrote something new, I would want
to let NtRegistry handle whether I should be accessing HKLM or HKCU.  So I'm
inclined to leave it in for now.

But you've inspired me.  I've made sure to resolve nt::AUTO in the external API
functions, so that private internal functions do not have to consider AUTO.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:114: // - Use |wow64_override| to force
redirection behaviour, or pass 0/nt::NONE.
On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> nit: figure we should express a stronger opinion that the enum should be used:
> e.g.: "or pass nt::NONE".

Done.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:301: ULONGLONG key_ticks =
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> On 2016/09/30 05:28:23, penny wrote:
> > On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > > please use base::Time::FromFileTime for last_write_time, base::Time::Now
for
> > > |current|, and base::TimeDelta::FromHours for the delta. this will be a
> > > bajillion times more clear. :-) here's one approach:
> > > 
> > >     const base::Time max_write_time = base::Time::Now() -
> > >         base::TimeDelta::FromHours(12);
> > > ...
> > >         if (base::Time::FromFileTime(last_write_time) < max_write_time)
> > >           to_delete.emplace_back(key_name, name_size);
> > 
> > Sorry, there's no base usage at all in nt_registry.  Enforced by DEPS in
this
> > directory.  I was going to use base just in the unittests, when this file
> lived
> > in a different subdirectory (.\Test\) where I could have a different DEPS
> file. 
> > But it doesn't live there now.
> 
> You can safely use base in the test. Add this to DEPS to allow it:
> 
> specific_include_rules = {
>   ".*_unittest\.cc": [
>     "+base",
>   ],
> }
> 
> See https://cs.chromium.org/chromium/src/buildtools/checkdeps/builddeps.py for
> the DEPS format documentation.

MIND IS BLOWN.  Thanks for that link.

<sigh> I just deleted all the manual testing override code.  :)

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:309: to_delete.push_back(temp);
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> On 2016/09/30 05:28:23, penny wrote:
> > On 2016/09/29 10:30:23, grt (UTC plus 2) wrote:
> > > nit: construct the string in place using the length you already have:
> > >           to_delete.emplace_back(key_name, name_size);
> > 
> > TIL emplace_back!!!  Didn't know this existed.  <fireworks>
> 
> Indeed! I knew about it but had forgotten that we're allowed to use it now.
> Sweet!

I feel like my life has changed.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:138: if (temp.length() != 0) {
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: if (!temp.empty())
> nit: omit braces :-)

Can you tell what the coding standard was in my previous life?

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:208: base::string16 temp = L"";
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: omit the assignment. just let |temp| be default initialized to an empty
> string.

Done.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test_main_dll.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:17: std::wstring content;
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: construct the string with the desired size:
>     std::wstring content(buffer_size, '\0');
> and remove the resize

Nice one.  It's unfortunate there's no constructor with just the size.  I don't
need the contents to be zeroed. Oh well, I suppose I'm not really using the
object as intended here.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
File chrome_elf/chrome_elf_util_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util_unittest.cc:76: ASSERT_TRUE(key != nt::AUTO);
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
>   ASSERT_NE(key, nt::AUTO);?

Better, yes.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util_unittest.cc:89: ASSERT_TRUE(key != nt::AUTO);
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
>   ASSERT_NE

Done.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/chrome_elf_u...
chrome_elf/chrome_elf_util_unittest.cc:90: base::string16 temp = L"";
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> omit assignment

Done.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:385: if (::wcsnicmp(position,
current_node->to_match, current_to_match_len) ==
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: !::wcsnicmp to make this fit on one line

Done.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:440: if (root == nt::AUTO) {
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: omit braces now that the comment is out of the body

Done.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:757: if (value_name == nullptr ||
*value_name == L'\0' ||
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> will g_nt_query_value_key already do this validation? if so, do we need to
> repeat it here? i think it's better not to, since MS has been known to change
> the sizes of things (MAX_PATH!!!111!?!!!). this comment applies to all of the
> range checks you've added.

Right, so yes, the ntdll functions should fail out for us.  In the case of
CreateRegKey and OpenRegKey, I added the length check to provide a "quick bail
out".  So if the key length is already too long, it would be a waste to go
through all the string processing that happens before the ntdll API is called. 
There's still a chance after processing that the path is too long and will fail
- but slim.

So I was hoping for a fast exit on those.  What do you think of that?

(I've removed the value name checks - I agree there's no point here.)

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:1079: bool SetTestingOverride(nt::ROOT_KEY
root, const std::wstring& new_path) {
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: omit "nt::" here and below since this is within namespace nt

Done.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:1085: if (::wcsnlen(sani_new_path.c_str(),
nt::g_kRegMaxPathLen + 1) ==
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> why not sani_new_path.length() > g_kRegMaxPathLen?

Thank you.  And good question.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:1094: if (temp_root == nt::HKCU)
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> how about doing away with temp_root and changing this to:
>   if (root == HKCU || root == AUTO && !g_system_install)
> 
> alternatively, are you opposed to modifying |root| in place rather than
> introducing another local for it?
> 
> comment applies to GetTestingOverride as well
> 
> if this pattern is widespread, you could add a helper like this in the unnamed
> namespace:
> bool IsHKCU(nt::ROOT_KEY root) {
>   return root == nt::HKCU || root == nt::AUTO && !g_system_install;
> }

You know, for some reason I was thinking |root| argument was const.

Much better now - and if the function was longer I would just modify it in
place.  Thanks!

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:32: constexpr size_t g_kRegMaxPathLen =
255;
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> this is max name for a single key, not for a full path, no?

No, this is actually the max for the absolute key path - after the base key
(HKLM/HKCU).

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:236: void GetTestingOverride(nt::ROOT_KEY
root, std::wstring* out_path);
On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> nit: we generally return strings as values rather than take them as out
params.
> i think
> std::wstring GetTestingOverride(nt::ROOT_KEY root);
> is more convenient

Good point, well made.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-10-01 03:30:04 UTC) #42

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-01 03:30:05 UTC) #43

grt (UTC plus 2)

this looks good to me. final nits. i swear! https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/nt_registry.cc File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/nt_registry.cc#newcode310 chrome_elf/nt_registry/nt_registry.cc:310: ...

4 years, 2 months ago (2016-10-02 20:10:55 UTC) #44

this looks good to me. final nits. i swear!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:310: // - Also, |subkey_path| should be
passed to SanitizeSubkeyPath() before calling
On 2016/10/01 01:44:25, penny wrote:
> On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> > On 2016/09/30 05:28:22, penny wrote:
> > > On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > > > how about enforcing this by adding to the top of the function:
> > > >   assert(subkey_path->empty() || subkey_path->front() != L'\\');
> > > >   assert(subkey_path->empty() || subkey_path->back() != L'\\');
> > > > 
> > > > better yet, can you make life easier on the callers by calling Sanitize
in
> > > here
> > > > rather than making them do it?
> > > 
> > > My take on this is that external APIs should be responsible for verifying
> and
> > > sanitizing externally-supplied input.  They should gracefully handle any
bad
> > > input in debug or release builds.  I feel that private functions are fine
to
> > > simply debug assert on broken expectations as a red flag.
> > > 
> > > ProcessRedirection() is an internal function that will only be called from
> > > within this file.  I've added the asserts you mentioned - just to catch
> > > developer mistakes within this file.
> > > 
> > > By only sanitizing external inputs, we won't end up doing the same
run-time
> > > checks more than once.  (Private functions can easily be called from many
> > > places.)
> > > 
> > > **I've also just added a |value_name| input check to QueryRegKeyValue()
and
> > > SetRegKeyValue().
> > 
> > Your philosophy on when to sanitize external input makes total sense. My
> > observation was that (at the time of writing), SanitizeSubkeyPath was only
> > called immediately before ProcessRedirection, and ProcessRedirection was
never
> > called without being immediately preceded by SanitizeSubkeyPath. Is
sanitation
> > really needed, or can that step be removed by gently adjusting the
redirector?
> 
> I hear ya, and considered just changing ProcessRedirection to handle it. 
> However, now I've got another external API for setting the testing override
that
> will use this sanitize function.
> 
> The lib on a whole assumes that the subkey paths it is storing and dealing
with
> won't have starting or trailing slashes - and I think it's nice to have a
> consistent format expectation.
> 
> I've added an extra bit in there to ensure there are no sequences of more than
> one backslash in a row as well.

Do the underlying Nt registry functions choke on multiple \\\? Does advapi strip
them out?

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry_unittest.cc:301: ULONGLONG key_ticks =
On 2016/10/01 01:44:25, penny wrote:
> On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> > On 2016/09/30 05:28:23, penny wrote:
> > > On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > > > please use base::Time::FromFileTime for last_write_time, base::Time::Now
> for
> > > > |current|, and base::TimeDelta::FromHours for the delta. this will be a
> > > > bajillion times more clear. :-) here's one approach:
> > > > 
> > > >     const base::Time max_write_time = base::Time::Now() -
> > > >         base::TimeDelta::FromHours(12);
> > > > ...
> > > >         if (base::Time::FromFileTime(last_write_time) < max_write_time)
> > > >           to_delete.emplace_back(key_name, name_size);
> > > 
> > > Sorry, there's no base usage at all in nt_registry.  Enforced by DEPS in
> this
> > > directory.  I was going to use base just in the unittests, when this file
> > lived
> > > in a different subdirectory (.\Test\) where I could have a different DEPS
> > file. 
> > > But it doesn't live there now.
> > 
> > You can safely use base in the test. Add this to DEPS to allow it:
> > 
> > specific_include_rules = {
> >   ".*_unittest\.cc": [
> >     "+base",
> >   ],
> > }
> > 
> > See https://cs.chromium.org/chromium/src/buildtools/checkdeps/builddeps.py
for
> > the DEPS format documentation.
> 
> MIND IS BLOWN.  Thanks for that link.
> 
> <sigh> I just deleted all the manual testing override code.  :)

Doh! Sorry I didn't notice this earlier!

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:138: if (temp.length() != 0) {
On 2016/10/01 01:44:25, penny wrote:
> On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> > nit: if (!temp.empty())
> > nit: omit braces :-)
> 
> Can you tell what the coding standard was in my previous life?

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/4a_P314B-Os/ZhyCq....
Say no more. :-)

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test_main_dll.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:17: std::wstring content;
On 2016/10/01 01:44:25, penny wrote:
> On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> > nit: construct the string with the desired size:
> >     std::wstring content(buffer_size, '\0');
> > and remove the resize
> 
> Nice one.  It's unfortunate there's no constructor with just the size.  I
don't
> need the contents to be zeroed. Oh well, I suppose I'm not really using the
> object as intended here.

resize(buffer_size) zero-fills it as well, so the form i suggested is only
marginally better (construct with the right size rather than construct and then
alloc). reserve(buffer_size) would allocate the memory, but you still couldn't
touch it. meh.

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/80001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:757: if (value_name == nullptr ||
*value_name == L'\0' ||
On 2016/10/01 01:44:25, penny wrote:
> On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> > will g_nt_query_value_key already do this validation? if so, do we need to
> > repeat it here? i think it's better not to, since MS has been known to
change
> > the sizes of things (MAX_PATH!!!111!?!!!). this comment applies to all of
the
> > range checks you've added.
> 
> Right, so yes, the ntdll functions should fail out for us.  In the case of
> CreateRegKey and OpenRegKey, I added the length check to provide a "quick bail
> out".  So if the key length is already too long, it would be a waste to go
> through all the string processing that happens before the ntdll API is called.

> There's still a chance after processing that the path is too long and will
fail
> - but slim.
> 
> So I was hoping for a fast exit on those.  What do you think of that?

Seems okay to me. Thanks.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:136: base::string16 temp;
nit:
    base::string16 temp = nt::GetTestingOverride(nt::HKCU);

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:138: if (temp.length() != 0)
nit:
    if (!temp.empty())
below, too

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:207:
ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, temp));
nit:
    ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, base::string16()));

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_util_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_util_unittest.cc:92:
ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, temp));
nit:
    ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, base::string16()));
below, too

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:334: if ((root == nt::HKCU &&
*g_HKCU_override) ||
wdyt:
  if (root == nt::HKCU ? *g_HKCU_override : *g_HKLM_override)

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:448: if (root == nt::HKCU)
  return root == nt::HKCU ? g_kRegPathHKCU : g_kRegPathHKLM;

and then move the definition of |temp| into each of the branches above since
it's unused in this codepath

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:477: // Replace any occurances of more
than 2 backslashes in a row with just 1.
more than two in a row or more than one in a row?

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:480: std::wregex regex(L"\\\\{2,}");
std::regex is banned: http://chromium-cpp.appspot.com/#library-blacklist. could
you do this the old fashioned way with find? maybe something like:
  // Shrink each run of separators down to one.
  for (size_t i = input->find(L'\\'); i != std::wstring::npos;
       i = input->find(L'\\', i)) {
    // Advance to the character following the first separator. There will always
    // be at least one non-separator character following it since trailing
    // separators were removed above.
    ++i;
    // Erase through the end of this run, leaving behind the first separator.
    input->erase(i, input->find_first_not_of(L'\\', i) - i);
  }

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:506: temp_path.append(converted_root, 1);
why ", 1"? is this to omit the leading '\' in the root? please mention why in a
comment.

in C++11, the count is required when providing a position, so you'll need:
    temp_path.append(converted_root, 1, converted_root.size() - 1);
in C++14 the count is optional, but we need to stick with the letter of the
C++11 law for now.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:519: if (!temp_path.empty())
nit: need braces here since the comment makes the body span two lines.
alternatively, move the comment up above the conditional (e.g., "Get the last
token if there is one.").

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:759: if (value_name == nullptr ||
*value_name == L'\0')
in advapi, nullptr and the empty string name a key's default/unnamed value. how
does one do so in the nt api? (same q in SetRegKeyValue)

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:1100: std::wstring temp;
nit: get rid of temp and return the value directly:
  if (root == HKCU || (root == AUTO && !g_system_install))
    return g_HKCU_override;
  return g_HKLM_override;

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.h:34: constexpr size_t g_kRegMaxValueName =
16383;
is this unused now?

penny

Soooo close! https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/nt_registry.cc File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/nt_registry.cc#newcode310 chrome_elf/nt_registry/nt_registry.cc:310: // - Also, |subkey_path| should be passed ...

4 years, 2 months ago (2016-10-03 19:18:49 UTC) #45

Soooo close!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.cc:310: // - Also, |subkey_path| should be
passed to SanitizeSubkeyPath() before calling
On 2016/10/02 20:10:53, grt (UTC plus 2) wrote:
> On 2016/10/01 01:44:25, penny wrote:
> > On 2016/09/30 09:32:16, grt (UTC plus 2) wrote:
> > > On 2016/09/30 05:28:22, penny wrote:
> > > > On 2016/09/29 10:30:22, grt (UTC plus 2) wrote:
> > > > > how about enforcing this by adding to the top of the function:
> > > > >   assert(subkey_path->empty() || subkey_path->front() != L'\\');
> > > > >   assert(subkey_path->empty() || subkey_path->back() != L'\\');
> > > > > 
> > > > > better yet, can you make life easier on the callers by calling
Sanitize
> in
> > > > here
> > > > > rather than making them do it?
> > > > 
> > > > My take on this is that external APIs should be responsible for
verifying
> > and
> > > > sanitizing externally-supplied input.  They should gracefully handle any
> bad
> > > > input in debug or release builds.  I feel that private functions are
fine
> to
> > > > simply debug assert on broken expectations as a red flag.
> > > > 
> > > > ProcessRedirection() is an internal function that will only be called
from
> > > > within this file.  I've added the asserts you mentioned - just to catch
> > > > developer mistakes within this file.
> > > > 
> > > > By only sanitizing external inputs, we won't end up doing the same
> run-time
> > > > checks more than once.  (Private functions can easily be called from
many
> > > > places.)
> > > > 
> > > > **I've also just added a |value_name| input check to QueryRegKeyValue()
> and
> > > > SetRegKeyValue().
> > > 
> > > Your philosophy on when to sanitize external input makes total sense. My
> > > observation was that (at the time of writing), SanitizeSubkeyPath was only
> > > called immediately before ProcessRedirection, and ProcessRedirection was
> never
> > > called without being immediately preceded by SanitizeSubkeyPath. Is
> sanitation
> > > really needed, or can that step be removed by gently adjusting the
> redirector?
> > 
> > I hear ya, and considered just changing ProcessRedirection to handle it. 
> > However, now I've got another external API for setting the testing override
> that
> > will use this sanitize function.
> > 
> > The lib on a whole assumes that the subkey paths it is storing and dealing
> with
> > won't have starting or trailing slashes - and I think it's nice to have a
> > consistent format expectation.
> > 
> > I've added an extra bit in there to ensure there are no sequences of more
than
> > one backslash in a row as well.
> 
> Do the underlying Nt registry functions choke on multiple \\\? Does advapi
strip
> them out?

I'm not sure at the moment.  It (advapi or ntdll) certainly won't fail out - it
just handles them.  I resolved them in Sanitize() so that private internal
subkey string manipulation doesn't have to worry about this.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:136: base::string16 temp;
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> nit:
>     base::string16 temp = nt::GetTestingOverride(nt::HKCU);

Done.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:138: if (temp.length() != 0)
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> nit:
>     if (!temp.empty())
> below, too

Done.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:207:
ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, temp));
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> nit:
>     ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, base::string16()));

Done.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_util_unittest.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_util_unittest.cc:92:
ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, temp));
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> nit:
>     ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, base::string16()));
> below, too

Done.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:334: if ((root == nt::HKCU &&
*g_HKCU_override) ||
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> wdyt:
>   if (root == nt::HKCU ? *g_HKCU_override : *g_HKLM_override)

Nice one.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:448: if (root == nt::HKCU)
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
>   return root == nt::HKCU ? g_kRegPathHKCU : g_kRegPathHKLM;
> 
> and then move the definition of |temp| into each of the branches above since
> it's unused in this codepath

Except that the globals are wchar_t arrays, and I want to return a std::wstring.
 How's this?

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:477: // Replace any occurances of more
than 2 backslashes in a row with just 1.
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> more than two in a row or more than one in a row?

Thank you.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:480: std::wregex regex(L"\\\\{2,}");
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> std::regex is banned: http://chromium-cpp.appspot.com/#library-blacklist.
could
> you do this the old fashioned way with find? maybe something like:
>   // Shrink each run of separators down to one.
>   for (size_t i = input->find(L'\\'); i != std::wstring::npos;
>        i = input->find(L'\\', i)) {
>     // Advance to the character following the first separator. There will
always
>     // be at least one non-separator character following it since trailing
>     // separators were removed above.
>     ++i;
>     // Erase through the end of this run, leaving behind the first separator.
>     input->erase(i, input->find_first_not_of(L'\\', i) - i);
>   }

<SIGH>
I've removed the regex use, as it's not worth the hassle.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:506: temp_path.append(converted_root, 1);
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> why ", 1"? is this to omit the leading '\' in the root? please mention why in
a
> comment.
> 
> in C++11, the count is required when providing a position, so you'll need:
>     temp_path.append(converted_root, 1, converted_root.size() - 1);
> in C++14 the count is optional, but we need to stick with the letter of the
> C++11 law for now.

TIL

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:519: if (!temp_path.empty())
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> nit: need braces here since the comment makes the body span two lines.
> alternatively, move the comment up above the conditional (e.g., "Get the last
> token if there is one.").

Done. Is this comment case a Chromium standard?

(Obviously I think there should always be braces, but that's not our standard. 
Just curious.)

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:759: if (value_name == nullptr ||
*value_name == L'\0')
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> in advapi, nullptr and the empty string name a key's default/unnamed value.
how
> does one do so in the nt api? (same q in SetRegKeyValue)

It's the same in ntoskrnl and ntdll.  Thanks for noticing my over-zealous
argument checking.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:1100: std::wstring temp;
On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> nit: get rid of temp and return the value directly:
>   if (root == HKCU || (root == AUTO && !g_system_install))
>     return g_HKCU_override;
>   return g_HKLM_override;

Again, these globals are wchar_t arrays and I want to return a std::wstring. 
How's this?

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.h:34: constexpr size_t g_kRegMaxValueName =
16383;
On 2016/10/02 20:10:55, grt (UTC plus 2) wrote:
> is this unused now?

Yes, but I think it's a good utility to have defined in an NTDLL registry shim. 
Users of this lib can use these defines.  That's why I left them in.

I'd rather try to centralize these defines and prevent re-definition of these
max values everywhere that interacts with registry.  I'm always a little peeved
that MS doesn't have them defined in a main header file.

grt (UTC plus 2)

lgtm with nits! SHIP IT!11!! https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry/nt_registry.cc File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry/nt_registry.cc#newcode448 chrome_elf/nt_registry/nt_registry.cc:448: if (root == nt::HKCU) ...

4 years, 2 months ago (2016-10-03 20:17:45 UTC) #46

lgtm with nits!

SHIP IT!11!!

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:448: if (root == nt::HKCU)
On 2016/10/03 19:18:48, penny wrote:
> On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> >   return root == nt::HKCU ? g_kRegPathHKCU : g_kRegPathHKLM;
> > 
> > and then move the definition of |temp| into each of the branches above since
> > it's unused in this codepath
> 
> Except that the globals are wchar_t arrays, and I want to return a
std::wstring.
>  How's this?

That's fine. I thought wstring's ctor that takes a wchar_t* is implicit, so
it'll be done for you. Does it not compile without the explicit call to the
ctor?

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:519: if (!temp_path.empty())
On 2016/10/03 19:18:49, penny wrote:
> On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> > nit: need braces here since the comment makes the body span two lines.
> > alternatively, move the comment up above the conditional (e.g., "Get the
last
> > token if there is one.").
> 
> Done. Is this comment case a Chromium standard?
> 
> (Obviously I think there should always be braces, but that's not our standard.

> Just curious.)

Yup. It's been discussed here and there on chromium-dev. For example:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/4a_P314B-Os/67X38...

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:1100: std::wstring temp;
On 2016/10/03 19:18:48, penny wrote:
> On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> > nit: get rid of temp and return the value directly:
> >   if (root == HKCU || (root == AUTO && !g_system_install))
> >     return g_HKCU_override;
> >   return g_HKLM_override;
> 
> Again, these globals are wchar_t arrays and I want to return a std::wstring. 
> How's this?

I think the implicit ctor will kick in if you "return g_HKLM_override;"

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.h:34: constexpr size_t g_kRegMaxValueName =
16383;
On 2016/10/03 19:18:49, penny wrote:
> On 2016/10/02 20:10:55, grt (UTC plus 2) wrote:
> > is this unused now?
> 
> Yes, but I think it's a good utility to have defined in an NTDLL registry
shim. 
> Users of this lib can use these defines.  That's why I left them in.
> 
> I'd rather try to centralize these defines and prevent re-definition of these
> max values everywhere that interacts with registry.  I'm always a little
peeved
> that MS doesn't have them defined in a main header file.

Acknowledged.

https://codereview.chromium.org/2345913003/diff/120001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/120001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:1094: else
nit: no "else" after "return" in an "if" body

penny

Thanks very much again Greg. Excellent code review. Robert! It's time for an LGTM from ...

4 years, 2 months ago (2016-10-03 20:46:03 UTC) #48

Thanks very much again Greg.  Excellent code review.

Robert!  It's time for an LGTM from you. :)

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:448: if (root == nt::HKCU)
On 2016/10/03 20:17:44, grt (UTC plus 2) wrote:
> On 2016/10/03 19:18:48, penny wrote:
> > On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> > >   return root == nt::HKCU ? g_kRegPathHKCU : g_kRegPathHKLM;
> > > 
> > > and then move the definition of |temp| into each of the branches above
since
> > > it's unused in this codepath
> > 
> > Except that the globals are wchar_t arrays, and I want to return a
> std::wstring.
> >  How's this?
> 
> That's fine. I thought wstring's ctor that takes a wchar_t* is implicit, so
> it'll be done for you. Does it not compile without the explicit call to the
> ctor?

MIND BLOWN AGAIN.  Implicit constructors.  I don't know if I like this world
yet... seems very hand-wavy and unintentional.  <sigh>

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:519: if (!temp_path.empty())
On 2016/10/03 20:17:44, grt (UTC plus 2) wrote:
> On 2016/10/03 19:18:49, penny wrote:
> > On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> > > nit: need braces here since the comment makes the body span two lines.
> > > alternatively, move the comment up above the conditional (e.g., "Get the
> last
> > > token if there is one.").
> > 
> > Done. Is this comment case a Chromium standard?
> > 
> > (Obviously I think there should always be braces, but that's not our
standard.
> 
> > Just curious.)
> 
> Yup. It's been discussed here and there on chromium-dev. For example:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/4a_P314B-Os/67X38...

Acknowledged.

https://codereview.chromium.org/2345913003/diff/100001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:1100: std::wstring temp;
On 2016/10/03 20:17:44, grt (UTC plus 2) wrote:
> On 2016/10/03 19:18:48, penny wrote:
> > On 2016/10/02 20:10:54, grt (UTC plus 2) wrote:
> > > nit: get rid of temp and return the value directly:
> > >   if (root == HKCU || (root == AUTO && !g_system_install))
> > >     return g_HKCU_override;
> > >   return g_HKLM_override;
> > 
> > Again, these globals are wchar_t arrays and I want to return a std::wstring.

> > How's this?
> 
> I think the implicit ctor will kick in if you "return g_HKLM_override;"

CRAY-Z.

https://codereview.chromium.org/2345913003/diff/120001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/2345913003/diff/120001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:1094: else
On 2016/10/03 20:17:45, grt (UTC plus 2) wrote:
> nit: no "else" after "return" in an "if" body

oh, right.

robertshield

LGTM, very nice and nicely tested patch. https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/nt_registry.h File chrome_elf/nt_registry/nt_registry.h (right): https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/nt_registry.h#newcode39 chrome_elf/nt_registry/nt_registry.h:39: typedef enum ...

4 years, 2 months ago (2016-10-04 05:22:09 UTC) #49

LGTM, very nice and nicely tested patch.

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:39: typedef enum _ROOT_KEY { AUTO = 0,
HKLM, HKCU } ROOT_KEY;
On 2016/10/01 01:44:25, penny wrote:
> On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> > thoughts re: AUTO: it seems to be used in only once place (install_util.cc).
> It
> > minutely complicates some of the implementations and requires an extra
global
> > bool. 
> > 
> > Also, install_util.cc already has an implementation of IsSystemInstall()
> >
>
(https://codesearch.chromium.org/chromium/src/chrome/install_static/install_ut...)
> > which could be called at the place in install_util.cc where AUTO is now
> passed. 
> > 
> > This would simplify the interface here and allow us to have a bit less code
in
> > the implementation.
> 
> Wow.  Way to swoop in with the profound thinking Robert.
> 
> It just makes sense to me for an NTDLL Registry library to offer the AUTO
> feature.  I know install_static existed before this lib, and constantly checks
> whether it's system or not (it should probably cache it once), so it doesn't
> really need to use AUTO.  But I imagine if I wrote something new, I would want
> to let NtRegistry handle whether I should be accessing HKLM or HKCU.  So I'm
> inclined to leave it in for now.


Ok. My line of reasoning was based on separation of concerns. Modulo nt::AUTO,
this is a nice leafish drop-in replacement library for a subset of advapi32's
logic. 

With nt::AUTO, an arguably-Chromium-specific implementation detail (namely that
you might want to pick HKLM vs. HKCU depending on process image path) works its
way in.

You can make the case that automatic hive-selection based on image path is a
handy feature for a generic library but figured I'd explain where I was coming
from :-)

> 
> But you've inspired me.  I've made sure to resolve nt::AUTO in the external
API
> functions, so that private internal functions do not have to consider AUTO.

Awesome, thanks!

penny

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the ...

4 years, 2 months ago (2016-10-04 17:40:26 UTC) #51

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/160001

4 years, 2 months ago (2016-10-04 17:40:54 UTC) #52

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-10-04 17:49:39 UTC) #53

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/273252)

4 years, 2 months ago (2016-10-04 17:49:40 UTC) #54

penny

pennymac@chromium.org changed reviewers: + dcheng@chromium.org

4 years, 2 months ago (2016-10-04 17:54:01 UTC) #55

penny

Hello Daniel, There is a very unfortunate presubmit check that I need your LGTM for. ...

4 years, 2 months ago (2016-10-04 17:55:53 UTC) #56

Hello Daniel,

There is a very unfortunate presubmit check that I need your LGTM for.  My
nt_registry\DEPS file needs to add dependency on base, only for the unittest
file.

Thank you!

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/2345913003/diff/60001/chrome_elf/nt_registry/...
chrome_elf/nt_registry/nt_registry.h:39: typedef enum _ROOT_KEY { AUTO = 0,
HKLM, HKCU } ROOT_KEY;
On 2016/10/04 05:22:09, robertshield_slow_reviews wrote:
> On 2016/10/01 01:44:25, penny wrote:
> > On 2016/09/28 23:49:23, robertshield_slow_reviews wrote:
> > > thoughts re: AUTO: it seems to be used in only once place
(install_util.cc).
> > It
> > > minutely complicates some of the implementations and requires an extra
> global
> > > bool. 
> > > 
> > > Also, install_util.cc already has an implementation of IsSystemInstall()
> > >
> >
>
(https://codesearch.chromium.org/chromium/src/chrome/install_static/install_ut...)
> > > which could be called at the place in install_util.cc where AUTO is now
> > passed. 
> > > 
> > > This would simplify the interface here and allow us to have a bit less
code
> in
> > > the implementation.
> > 
> > Wow.  Way to swoop in with the profound thinking Robert.
> > 
> > It just makes sense to me for an NTDLL Registry library to offer the AUTO
> > feature.  I know install_static existed before this lib, and constantly
checks
> > whether it's system or not (it should probably cache it once), so it doesn't
> > really need to use AUTO.  But I imagine if I wrote something new, I would
want
> > to let NtRegistry handle whether I should be accessing HKLM or HKCU.  So I'm
> > inclined to leave it in for now.
> 
> 
> Ok. My line of reasoning was based on separation of concerns. Modulo nt::AUTO,
> this is a nice leafish drop-in replacement library for a subset of advapi32's
> logic. 
> 
> With nt::AUTO, an arguably-Chromium-specific implementation detail (namely
that
> you might want to pick HKLM vs. HKCU depending on process image path) works
its
> way in.
> 
> You can make the case that automatic hive-selection based on image path is a
> handy feature for a generic library but figured I'd explain where I was coming
> from :-)
> 
> > 
> > But you've inspired me.  I've made sure to resolve nt::AUTO in the external
> API
> > functions, so that private internal functions do not have to consider AUTO.
> 
> Awesome, thanks!

Thank you!  I hear you.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2345913003/160001

4 years, 2 months ago (2016-10-04 20:36:44 UTC) #59

commit-bot: I haz the power

Description was changed from ========== NTRegistry - added wow64 redirection support. TESTS=chrome_elf_unittests: NtRegistryTest* BUG=641169 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ...

4 years, 2 months ago (2016-10-04 20:44:21 UTC) #60

commit-bot: I haz the power

Description was changed from ========== NTRegistry - added wow64 redirection support. TESTS=chrome_elf_unittests: NtRegistryTest* BUG=641169 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ...

4 years, 2 months ago (2016-10-04 20:46:25 UTC) #62

commit-bot: I haz the power

Patchset 8 (id:??) landed as https://crrev.com/5379f1724fc3c193828e137faba13c8d59c3f228 Cr-Commit-Position: refs/heads/master@{#422919}

4 years, 2 months ago (2016-10-04 20:46:26 UTC) #63

Sébastien Marchand

sebmarchand@chromium.org changed reviewers: + sebmarchand@chromium.org

4 years, 2 months ago (2016-10-05 15:04:15 UTC) #64

Sébastien Marchand

4 years, 2 months ago (2016-10-05 15:04:16 UTC) #65

Message was sent while issue was closed.

FYI this CL is breaking the LTCG builders (see crbug.com/652973), the workaround
for the MS compiler bug doesn't work with WPO enable :(. Using "pragma
optimize("", off)" on this file doesn't solve the issue.

Issue 2345913003: [chrome_elf] NTRegistry - added wow64 redirection support. (Closed)

Description

Patch Set 1 #

Patch Set 2 : clang fixes. #

Patch Set 3 : Code review fixes, part 1. #

Patch Set 4 : Code review fixes, part 2. #

Patch Set 5 : Code review fixes, part 3. #

Patch Set 6 : Code review fixes, part 4. #

Patch Set 7 : Code review fixes, part 5. #

Patch Set 8 : Final nits. #

Messages