[Chrome ELF] New NT registry API.

[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children" (https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch" (https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE" (https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32 (useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list instead of lots of individual reg values.

Small changes to organize file structure and functional components under chrome_elf.  Old common code now sits under "hook_util", "nt_registry", "breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc, blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry redirection
(that tests use to keep the hive safe and isolated), I added in a way for the tests to access
the redirection path (and pass that information on to the test DLL).  This way the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc: ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43
Cr-Commit-Position: refs/heads/master@{#405307}

[penny, 2016-04-04 19:13:15]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry.
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
==========

to

==========
[Chrome ELF] New NT registry API (for Chrome_Elf).

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
==========

[penny, 2016-04-04 21:21:53]

Description was changed from

==========
[Chrome ELF] New NT registry API (for Chrome_Elf).

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
==========

[penny, 2016-04-12 17:23:28]

pennymac@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, robertshield@chromium.org, wfh@chromium.org

[penny, 2016-04-12 17:23:29]

Hello all!

See summary - this is one of five CLs.  This one in particular just introduces a
new ntdll registry API for chrome_elf (so we can bypass advapi32).  It also
adjusts the existing blacklist trial/finch to use REG_MULTI_SZ instead of lots
of REG_SZ.

1) base/test/*, phajdan.jr@chromium.org
Override function to allow for passing back the new registry redirection path.

2) sandbox/win/src/nt_internals.h, wfh@
Just some new nt defines.

3) chrome/browser/* & chrome_elf/*, robertshield@chromium.org (& csharp)
- New chrome_elf_reg files added to gyp and gn files.  Builds as part of Common.
- Updated blacklist.cc, breakpad.cc, and chrome_elf_util.cc to use the new NT
api.
- Updated blacklist to interact with a REG_MULTI_SZ instead of many values.
- blacklist_test_main.cc doesn't seem to be used at all.  I've set to delete -
please let me know what this is for!
- Lots of test changes.  Since the tests set up registry redirection, and the NT
apis bypass registry redirection, I had to make a way to let chrome_elf_reg know
where the redirection points to.  And in the case of blacklist tests, I make
sure the main test DLL can get access to that redirection path.  This way we
preserve the reg redirection while testing.  I've also split the main test DLL
away from the unit tests, so that it can be loaded on demand (not linked in).
- Robert, are there any clients in the wild actually using the trial blacklist
to pass extra DLL names to ELF?  I don't think so, but I would have to handle an
"upgrade scenario" to the new format if so.


Thanks for your time!  I welcome nits and any thoughts.  Feel free to ask me any
questions.

Cheers,
Penny

[Paweł Hajdan Jr., 2016-04-12 20:41:25]

https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_uti...
File base/test/test_reg_util_win.h (right):

https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_uti...
base/test/test_reg_util_win.h:43: void OverrideRegistry(HKEY override,
base::string16* override_path);
Could you convert all callers to this new signature?

In the long run, having both seems too superfluous, and I'm especially
opinionated about cleanliness of src/base.

Please let me know if the cleanup turns out to be too much of a burden for some
reason - make sure to provide some supporting data/details/rationale in that
case.

[penny, 2016-04-15 17:48:10]

On 2016/04/12 20:41:25, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_uti...
> File base/test/test_reg_util_win.h (right):
> 
>
https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_uti...
> base/test/test_reg_util_win.h:43: void OverrideRegistry(HKEY override,
> base::string16* override_path);
> Could you convert all callers to this new signature?
> 
> In the long run, having both seems too superfluous, and I'm especially
> opinionated about cleanliness of src/base.
> 
> Please let me know if the cleanup turns out to be too much of a burden for
some
> reason - make sure to provide some supporting data/details/rationale in that
> case.

Hi Pawel, thanks for the review.

I had a look at converting all existing callers to the new signature.  My
current thoughts are that there are a lot of places calling this function - 99%
of which do not need the path returned.  They would all just pass in nullptr for
the second argument.  Getting that path is only needed by the new chrome_elf
tests - which absolutely need the path as ntdll bypasses registry redirection.

https://code.google.com/p/chromium/codesearch#search/&q=OverrideRegistry&sq=p...

However, I also feel strongly about keeping our code clean and tidy.  I've
adjusted the .cc implementation of the function override, so that there is no
longer duplicate code.  The older version just wraps the new overload.

Let me know how you feel about all this.

[Paweł Hajdan Jr., 2016-04-18 13:42:00]

base/test LGTM

[robertshield, 2016-04-20 05:16:09]

Sorry for the delay on this, I'm half-way through and it's getting late - looks
good overall, here's some nits so far, will try and complete the pass through
tomorrow.

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_elf_init_unittest_win.cc:194:
EXPECT_TRUE((dlls.at(0)).compare(L"TestDll1.dll") == 0);
How about:
EXPECT_STREQ(L"TestDll.dll", dlls[0].c_str());
here and below?

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_elf_init_win.cc:136: std::wstring value_wcs =
base::UTF8ToWide(value);
prefer base::string16

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_elf_init_win.cc:141: std::vector<wchar_t>
reg_buffer(value_wcs.begin(), value_wcs.end());
I'm not sure you need to copy the dll names here. std::wstrings can contain
embedded null chars (with minimal gotchas, none of which apply to the code
below).

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/b...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/b...
chrome_elf/blacklist/blacklist.cc:369: dlls.size() == 0)
dlls.empty()

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:139: wchar_t* file_name =
L"chrome_elf_tests.txt";
static const

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

Some more thoughts, getting closer to done :-)

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:142: ::wsprintf(path,
L"chrome_elf_test_%u\\", ::GetCurrentProcessId());
Please also use process creation time when naming the file.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:148:
ASSERT_TRUE(reg_override_dir_.Set(temp_dir));
I think this will delete the user's %TEMP% folder, which isn't what is wanted.
Maybe you want to call CreateUniqueTempDir() instead, and have the file you
stash information in live in there instead?

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
File chrome_elf/blacklist/test/blacklist_test_main_dll.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:17: void GetIpcOverrides()
{
thinking about this, it looks like the test exe is writing stuff out to a file
that is then read in by the dll being loaded.

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

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:19: ::wsprintf(path,
L"chrome_elf_test_%u\\", ::GetCurrentProcessId());
PIDs get reused alarmingly frequently, please also stick the creation time here.
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce...
might be usable here.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:33: if (-1 == file.Read(0,
(char*)content.c_str(), size)) {
no c-style casts

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:41: const wchar_t* ptr =
content.c_str() + index;
content.begin() + index

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:42: ptr = ::wcschr(ptr,
L'\"');
why not use one of the find() variants that takes an offset:
http://www.cplusplus.com/reference/string/string/find/

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:49: }
I think you can replace most of this with:

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

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:66: }
ditto for simplification suggestion.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_reg.h (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:63: bool GetRegValue_SZ(HANDLE key,
Minor style nit: base's registry functions are named without underscores, I'd
mildly prefer the form GetRegValueSZ, GetRegValueMULTISZ, GetRegValueDWORD, etc.

[robertshield, 2016-04-20 16:55:53]

A few more quick notes, just the registry implementation left to go through.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_reg.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.cc:27: base::string16 override_path;
These are globals, please label them as g_override_path, etc. to make it
clearer.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_reg.h (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:3: // found in the LICENSE file.
Suggest having at the top of this file some explanation for why it exists
"advapi32-free registry access" and some notes about usage (e.g. the HANDLEs
passed to many of the GetXXX functions should be obtained via Open/CreateRegKey
in this file.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:20: typedef enum _ROOT_KEY { AUTO = 0, HKLM, HKCU }
ROOT_KEY, *PROOT_KEY;
We don't usually explicitly typedef for a pointer-to-type, I don't think you
need the PROOT_KEY bit.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:31: bool DeleteRegKey(HANDLE key);
please add separate comments for each function.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
For all of the functions in this file that take a HANDLE, it looks like it is
important to have obtained that handle from OpenRegKey or CreateRegKey in order
for the override stuff to take effect. 

It would be worth having a comment somewhere to this effect.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
Why is this function called QueryXXX when the others are called GetXXX? Consider
renaming it for consistency.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:56: // the registry key will be opened, queried,
then closed.
Please attach comments to the individual functions, the semantics of these two
are different enough to make it worth separating them.

micro nit: word wrapping looks off. If you're using Sublime, Wrap Plus is handy,
otherwise git cl format should take care of it.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:98: // Passing in pointer to string for efficiency.
Use const ref instead of pointer here and everywhere else non POD types are
being passed as input values. Don't need the efficiency comment here.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:121: // Does NOT use advapi32, only ntdll.
Suggest:

Returns the current user SID in string form.

The other part of the comment isn't likely to be useful to future readers.

[penny, 2016-05-11 21:40:49]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2016-05-11 21:55:37]

On 2016/04/20 16:55:53, robertshield wrote:
> A few more quick notes, just the registry implementation left to go through.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> File chrome_elf/chrome_elf_reg.cc (right):
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.cc:27: base::string16 override_path;
> These are globals, please label them as g_override_path, etc. to make it
> clearer.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> File chrome_elf/chrome_elf_reg.h (right):
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:3: // found in the LICENSE file.
> Suggest having at the top of this file some explanation for why it exists
> "advapi32-free registry access" and some notes about usage (e.g. the HANDLEs
> passed to many of the GetXXX functions should be obtained via
Open/CreateRegKey
> in this file.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:20: typedef enum _ROOT_KEY { AUTO = 0, HKLM, HKCU
}
> ROOT_KEY, *PROOT_KEY;
> We don't usually explicitly typedef for a pointer-to-type, I don't think you
> need the PROOT_KEY bit.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:31: bool DeleteRegKey(HANDLE key);
> please add separate comments for each function.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
> For all of the functions in this file that take a HANDLE, it looks like it is
> important to have obtained that handle from OpenRegKey or CreateRegKey in
order
> for the override stuff to take effect. 
> 
> It would be worth having a comment somewhere to this effect.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
> Why is this function called QueryXXX when the others are called GetXXX?
Consider
> renaming it for consistency.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:56: // the registry key will be opened, queried,
> then closed.
> Please attach comments to the individual functions, the semantics of these two
> are different enough to make it worth separating them.
> 
> micro nit: word wrapping looks off. If you're using Sublime, Wrap Plus is
handy,
> otherwise git cl format should take care of it.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:98: // Passing in pointer to string for
efficiency.
> Use const ref instead of pointer here and everywhere else non POD types are
> being passed as input values. Don't need the efficiency comment here.
> 
>
https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
> chrome_elf/chrome_elf_reg.h:121: // Does NOT use advapi32, only ntdll.
> Suggest:
> 
> Returns the current user SID in string form.
> 
> The other part of the comment isn't likely to be useful to future readers.

Robert, sorry I've been MIA for the last couple weeks.  It's been a personal
Armageddon.  I will fix all of your comments very soon now, and then upload!

I've updated just now with some changes required to pull in the latest trunk. 
There were some clashes with things Ananta did to prep for crashpad in
chrome_elf (before he knew I was greatly changing the elf).  This changelist
looks much bigger than it actually is, due to some renames.

- hook_util static lib now holds the little thunk code.
- nt_registry now holds the new ntdll api.
- install_util now holds what used to be chrome_elf_util -> which Ananta broke
out into Chrome and renamed.  I'm leaving it a separate static lib now, in it's
own directory, with a name specific to install utility function.  But it's back
under chrome_elf now (since it's dependent on nt_registry.  This is all in prep
for switching breakpad to crashpad in chrome_elf (will be next on my list).
- breakpad now in subdirectory.
- removed "common" build target.  everything organized now.

All the content is the same.

Oh, I've added myself to chrome_elf\OWNERS - just let me know if you'd rather
not add me.  I'd like to keep an eye on any elf activity in the near future.

[penny, 2016-05-23 04:56:03]

Patchset #9 (id:160001) has been deleted

[penny, 2016-05-23 04:56:46]

Patchset #9 (id:180001) has been deleted

[penny, 2016-05-23 04:57:54]

Patchset #10 (id:220001) has been deleted

[penny, 2016-05-23 05:24:47]

Patchset #5 (id:80001) has been deleted

[penny, 2016-05-23 05:25:19]

Patchset #3 (id:40001) has been deleted

[penny, 2016-05-23 05:25:47]

Patchset #5 (id:120001) has been deleted

[penny, 2016-05-26 03:35:37]

Patchset #8 (id:260001) has been deleted

[penny, 2016-05-26 03:36:39]

Patchset #8 (id:280001) has been deleted

[penny, 2016-05-26 03:37:14]

Patchset #8 (id:300001) has been deleted

[penny, 2016-05-28 01:34:23]

Hi Robert,

All the fixes for your comments are done.  Instead of using a temp file to pass
the registry redirection (IPC), I'm just using environment variables.  Since
test DLLs within the same process are the only targets at this point, that works
great.

This changelist looks much bigger than it actually is, due to some renames.

- hook_util static lib now holds the little thunk code.
- nt_registry now holds the new ntdll api.
- chrome/install_static/install_util now holds what used to be chrome_elf_util
-> which Ananta changed in prep
for switching breakpad to crashpad in chrome_elf.
- breakpad now in subdirectory.
- removed "common" build target.  everything organized now.

All the content is basically the same.

Thanks for your time Robert!  Let me know any follow-up comments you may have.

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_elf_init_unittest_win.cc:194:
EXPECT_TRUE((dlls.at(0)).compare(L"TestDll1.dll") == 0);
On 2016/04/20 05:16:09, robertshield wrote:
> How about:
> EXPECT_STREQ(L"TestDll.dll", dlls[0].c_str());
> here and below?

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_elf_init_win.cc:136: std::wstring value_wcs =
base::UTF8ToWide(value);
On 2016/04/20 05:16:09, robertshield wrote:
> prefer base::string16

Done.  This base API explicitly returns a std::wstring, so that's what I was
using.  Also, it's converted from a std::string type, so it seemed cleaner to
match them.  I'm happy to make this base::string16 though, if you think it's
better here.

https://codereview.chromium.org/1841573002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_elf_init_win.cc:141: std::vector<wchar_t>
reg_buffer(value_wcs.begin(), value_wcs.end());
On 2016/04/20 05:16:09, robertshield wrote:
> I'm not sure you need to copy the dll names here. std::wstrings can contain
> embedded null chars (with minimal gotchas, none of which apply to the code
> below).
> 

Done.  TIL!  I didn't know you could embed null chars in a std::string  (and
still get the right size, etc).  Thanks!

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/b...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/b...
chrome_elf/blacklist/blacklist.cc:369: dlls.size() == 0)
On 2016/04/20 05:16:09, robertshield wrote:
> dlls.empty()

Done.

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:139: wchar_t* file_name =
L"chrome_elf_tests.txt";
On 2016/04/20 05:16:09, robertshield wrote:
> static const

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:142: ::wsprintf(path,
L"chrome_elf_test_%u\\", ::GetCurrentProcessId());
On 2016/04/20 15:45:47, robertshield wrote:
> Please also use process creation time when naming the file.

See main comment in reply.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test.cc:148:
ASSERT_TRUE(reg_override_dir_.Set(temp_dir));
On 2016/04/20 15:45:47, robertshield wrote:
> I think this will delete the user's %TEMP% folder, which isn't what is wanted.
> Maybe you want to call CreateUniqueTempDir() instead, and have the file you
> stash information in live in there instead?

It's all good actually - doesn't touch %TEMP%.

reg_override_dir_ is a base::ScopedTempDir.

scoped_temp_dir.h:
  // Takes ownership of directory at |path|, creating it if necessary.
  // Don't call multiple times unless Take() has been called first.
  bool Set(const FilePath& path) WARN_UNUSED_RESULT;

The contents of the base::FilePath when set() is called is
DIR_TEMP\chrome_elf_test_PID\.  This directory is created by the call to Set(),
and is deleted during destruction of the BlacklistTest object (at end of scope).
 DIR_TEMP itself isn't touched.

Side note, I'm not using CreateUniqueTempDir, because I want to decide the name
of the temp directory (so that I can easily recreate that temp name in another
testing DLL, without having to communicate the temp dir location).

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
File chrome_elf/blacklist/test/blacklist_test_main_dll.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:17: void GetIpcOverrides()
{
On 2016/04/20 15:45:47, robertshield wrote:
> thinking about this, it looks like the test exe is writing stuff out to a file
> that is then read in by the dll being loaded.
> 
> Any reason not to simply have a SetIpcOverrides() export on the dll, and have
> the test call that? Is it because there is work done in DllMain that needs to
> have this stuff already set up?
> 
> If so, how about sticking something in the environment block before loading
the
> DLL? Something like ::SetEnvironmentVariable (L"ipc_override", path) which the
> DLL could read at init time.

See main comment in reply.

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:19: ::wsprintf(path,
L"chrome_elf_test_%u\\", ::GetCurrentProcessId());
On 2016/04/20 15:45:47, robertshield wrote:
> PIDs get reused alarmingly frequently, please also stick the creation time
here.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce...
> might be usable here.

See main comment in reply.

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:33: if (-1 == file.Read(0,
(char*)content.c_str(), size)) {
On 2016/04/20 15:45:47, robertshield wrote:
> no c-style casts
> 
> also casting wchar_t* to char* should come with a comment about why this is
> done. Here, I think there's a length confusion:
> content.resize(size) is in terms of wchar_ts, whereas size passed to Read is
> number of bytes. This won't explode, but the string will be twice as long as
> needed.

See main comment in reply.

Acknowledged.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:41: const wchar_t* ptr =
content.c_str() + index;
On 2016/04/20 15:45:47, robertshield wrote:
> content.begin() + index

Acknowledged.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:42: ptr = ::wcschr(ptr,
L'\"');
On 2016/04/20 15:45:47, robertshield wrote:
> why not use one of the find() variants that takes an offset:
> http://www.cplusplus.com/reference/string/string/find/

Acknowledged.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:49: }
On 2016/04/20 15:45:47, robertshield wrote:
> I think you can replace most of this with:
> 
> size_t start_index = content.find(L"hkcu=");
> size_t end_index = content.find(L"\", start_index);
> base::string16 str(content.begin()+start_index, content.begin()+end_index));
> 
> you'll need to check the indexes are in range too.

See main comment in reply.

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

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

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/t...
chrome_elf/blacklist/test/blacklist_test_main_dll.cc:66: }
On 2016/04/20 15:45:47, robertshield wrote:
> ditto for simplification suggestion.

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_reg.cc (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.cc:27: base::string16 override_path;
On 2016/04/20 16:55:53, robertshield wrote:
> These are globals, please label them as g_override_path, etc. to make it
> clearer.

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
File chrome_elf/chrome_elf_reg.h (right):

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:3: // found in the LICENSE file.
On 2016/04/20 16:55:53, robertshield wrote:
> Suggest having at the top of this file some explanation for why it exists
> "advapi32-free registry access" and some notes about usage (e.g. the HANDLEs
> passed to many of the GetXXX functions should be obtained via
Open/CreateRegKey
> in this file.

Done.  Comments about HANDLEs have been added to the function documentation.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:20: typedef enum _ROOT_KEY { AUTO = 0, HKLM, HKCU }
ROOT_KEY, *PROOT_KEY;
On 2016/04/20 16:55:53, robertshield wrote:
> We don't usually explicitly typedef for a pointer-to-type, I don't think you
> need the PROOT_KEY bit.

Done.  I just always do that as an old habit.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:31: bool DeleteRegKey(HANDLE key);
On 2016/04/20 16:55:53, robertshield wrote:
> please add separate comments for each function.

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
On 2016/04/20 16:55:53, robertshield wrote:
> Why is this function called QueryXXX when the others are called GetXXX?
Consider
> renaming it for consistency.

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
On 2016/04/20 16:55:53, robertshield wrote:
> For all of the functions in this file that take a HANDLE, it looks like it is
> important to have obtained that handle from OpenRegKey or CreateRegKey in
order
> for the override stuff to take effect. 
> 
> It would be worth having a comment somewhere to this effect.

Done.  I've changed to always use "Query".  This main query function is named
after the NTDLL API, which uses the term "query".  I'd like to keep this
consistent with the NTDLL function names.  I was using "Get" only for wrapper
functions (that call QueryRegKeyValue underneath).  But I'm happy to make them
"Query" for consistency.  It's a fair point.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:49: bool QueryRegKeyValue(HANDLE key,
On 2016/04/20 16:55:53, robertshield wrote:
> For all of the functions in this file that take a HANDLE, it looks like it is
> important to have obtained that handle from OpenRegKey or CreateRegKey in
order
> for the override stuff to take effect. 
> 
> It would be worth having a comment somewhere to this effect.

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:56: // the registry key will be opened, queried,
then closed.
On 2016/04/20 16:55:53, robertshield wrote:
> Please attach comments to the individual functions, the semantics of these two
> are different enough to make it worth separating them.
> 
> micro nit: word wrapping looks off. If you're using Sublime, Wrap Plus is
handy,
> otherwise git cl format should take care of it.

Done.  I always use "git cl format" before uploading.  I wonder if it doesn't
fix comment lines that are too short?  Maybe it only handles lines that are too
long?

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:63: bool GetRegValue_SZ(HANDLE key,
On 2016/04/20 15:45:47, robertshield wrote:
> Minor style nit: base's registry functions are named without underscores, I'd
> mildly prefer the form GetRegValueSZ, GetRegValueMULTISZ, GetRegValueDWORD,
etc.

Done.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:98: // Passing in pointer to string for efficiency.
On 2016/04/20 16:55:53, robertshield wrote:
> Use const ref instead of pointer here and everywhere else non POD types are
> being passed as input values. Don't need the efficiency comment here. 

Done.  I think done.  I'm new to the strange '&' refs in c++.  I've adjusted the
base::string16s and std::vectors that are being passed into
SetRegValueSZ/MULTISZ to be const refs.  Oh, and the BYTE* is now const for
SetRegKeyValue.

https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_...
chrome_elf/chrome_elf_reg.h:121: // Does NOT use advapi32, only ntdll.
On 2016/04/20 16:55:53, robertshield wrote:
> Suggest:
> 
> Returns the current user SID in string form.
> 
> The other part of the comment isn't likely to be useful to future readers.

Done.

[robertshield, 2016-06-17 13:40:03]

lgtm, minor nits

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf.gyp
File chrome_elf/chrome_elf.gyp (right):

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:15:
##------------------------------------------------------------------------------
nit: wrap

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:72:
##------------------------------------------------------------------------------
nit: wrap

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:115:
##------------------------------------------------------------------------------
nit: wrap

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:161:
##------------------------------------------------------------------------------
nit: wrap

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:470: if (std::numeric_limits<DWORD>::max()
< ::wcslen(string))
could use value.length() instead of all this

[penny, 2016-06-22 23:13:15]

Thanks very much Robert.  I've uploaded nit fixes, and also made CreateRegKey
behave recursively (so it can make multiple sub keys if needed).  Could you do a
quick review of this recursive change?

(Also updated branch with latest trunk.)

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf.gyp
File chrome_elf/chrome_elf.gyp (right):

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:15:
##------------------------------------------------------------------------------
On 2016/06/17 13:40:03, robertshield wrote:
> nit: wrap

Done.

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:72:
##------------------------------------------------------------------------------
On 2016/06/17 13:40:03, robertshield wrote:
> nit: wrap

Done.

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:115:
##------------------------------------------------------------------------------
On 2016/06/17 13:40:03, robertshield wrote:
> nit: wrap

Done.

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf....
chrome_elf/chrome_elf.gyp:161:
##------------------------------------------------------------------------------
On 2016/06/17 13:40:03, robertshield wrote:
> nit: wrap

Done.

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:470: if (std::numeric_limits<DWORD>::max()
< ::wcslen(string))
On 2016/06/17 13:40:03, robertshield wrote:
> could use value.length() instead of all this

Done.

[robertshield, 2016-06-23 02:29:36]

This looks good, but it really feels like this CL is way too big. 

Is there anyway to split the CL up into 
1) Implementation of new registry api + tests
2) Use of new registry api
3) Anything else the CL does

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/breakpad.h
File chrome_elf/breakpad.h (left):

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/breakpad.h#...
chrome_elf/breakpad.h:4: 
Why is this file moved in this CL? If intentional, please update CL description,
but it seems like this should be done in a separate CL.

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:136: ULONG* create_or_open OPTIONAL) {
I'm haven't seen the OPTIONAL annotation used before in Chromium code. Do you
need it here?

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:152: HANDLE* out_handle) {
fwiw, you can do this more simply (and more efficiently) without recursion by
looping over |sub_key_path| keeping iterators to the beginning and end of each
sub key name. This works too though so only change it if you feel like it :-)

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:158: size_t index =
sub_key_path.find_first_of(L"\\", 0);
you're searching for one character, so you can use the find() overload that
takes a wchar_t.

[penny, 2016-06-24 19:56:33]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Also adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated
list instead of lots of individual reg values.

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2016-06-25 21:13:44]

Thanks for your time Robert.  I've changed the recursion - and updated again
with latest master.  See comments below.

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/breakpad.h
File chrome_elf/breakpad.h (left):

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/breakpad.h#...
chrome_elf/breakpad.h:4: 
On 2016/06/23 02:29:36, robertshield wrote:
> Why is this file moved in this CL? If intentional, please update CL
description,
> but it seems like this should be done in a separate CL.

Yes.  I've updated the CL description with a mention.  I mentioned the rename in
my previous comments on this review:

This changelist looks much bigger than it actually is, due to some renames.

- hook_util static lib now holds the little thunk code.
- nt_registry now holds the new ntdll api.
- chrome/install_static/install_util now holds what used to be chrome_elf_util
-> which Ananta changed in prep
for switching breakpad to crashpad in chrome_elf.
- breakpad now in subdirectory.
- removed "common" build target.  everything organized now.

I hear you and appreciate your comments about the size of this CL.  Some of the
bloating came from trying to handle the clashing with concurrent changes to
slowly add crashpad.  At this point, I plan to land this CL as is.

There should have been better communications about efforts to touch the same
code for different work (at the same time).  I will work to ensure I don't end
up reacting to multiple clashes again in the future - it's definitely not
efficient.

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:136: ULONG* create_or_open OPTIONAL) {
On 2016/06/23 02:29:36, robertshield wrote:
> I'm haven't seen the OPTIONAL annotation used before in Chromium code. Do you
> need it here?

So, I started a little discussion with my fellow chrome-windows folks.  I think
these tags make function definitions nicely readable when there is a truly
optional argument.  This annotation has not been used in chromium, but it's also
not mentioned or denied in chromium or google coding standards.  It's definitely
a Windows annotation.

As my use of this is contained in the new nt_registry library (also used a
couple times in CreateRegKey and OpenRegKey), and this library/code is very much
Windows-specific, I'm feeling ok with leaving these annotations in this isolated
library.

It's really easy to remove in the future if anyone gets emotional about it.

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:152: HANDLE* out_handle) {
On 2016/06/23 02:29:36, robertshield wrote:
> fwiw, you can do this more simply (and more efficiently) without recursion by
> looping over |sub_key_path| keeping iterators to the beginning and end of each
> sub key name. This works too though so only change it if you feel like it :-)

Thanks Robert.  I was thinking it would be easier to support "unwinding" changes
on failure with recursion.  However, I've changed it now to use an iterator and
still manage to handle cleanup on failure.  A little more complexity, but no
function stack frames at least.  Let me know what you think of this version.

https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:158: size_t index =
sub_key_path.find_first_of(L"\\", 0);
On 2016/06/23 02:29:36, robertshield wrote:
> you're searching for one character, so you can use the find() overload that
> takes a wchar_t. 

Acknowledged.

[robertshield, 2016-06-26 02:10:00]

Couple more nits coming, overall looks great, some quick comments below:

On 2016/06/25 21:13:44, penny wrote:
> Thanks for your time Robert.  I've changed the recursion - and updated again
> with latest master.  See comments below.
> 
> https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/breakpad.h
> File chrome_elf/breakpad.h (left):
> 
>
https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/breakpad.h#...
> chrome_elf/breakpad.h:4: 
> On 2016/06/23 02:29:36, robertshield wrote:
> > Why is this file moved in this CL? If intentional, please update CL
> description,
> > but it seems like this should be done in a separate CL.
> 
> Yes.  I've updated the CL description with a mention.  I mentioned the rename
in
> my previous comments on this review:
> 
> This changelist looks much bigger than it actually is, due to some renames.
> 
> - hook_util static lib now holds the little thunk code.
> - nt_registry now holds the new ntdll api.
> - chrome/install_static/install_util now holds what used to be chrome_elf_util
> -> which Ananta changed in prep
> for switching breakpad to crashpad in chrome_elf.
> - breakpad now in subdirectory.
> - removed "common" build target.  everything organized now.
> 
> I hear you and appreciate your comments about the size of this CL.  Some of
the
> bloating came from trying to handle the clashing with concurrent changes to
> slowly add crashpad.  At this point, I plan to land this CL as is.

Sg, and I hear your pain on changing a moving target.

I guess I've been down the big-bang CL path before and have always regretted the
resulting merge conflicts. 

Landing as is sounds good, may merges be smooth :-)

> 
> There should have been better communications about efforts to touch the same
> code for different work (at the same time).  I will work to ensure I don't end
> up reacting to multiple clashes again in the future - it's definitely not
> efficient.
> 
>
https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
> File chrome_elf/nt_registry/nt_registry.cc (right):
> 
>
https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
> chrome_elf/nt_registry/nt_registry.cc:136: ULONG* create_or_open OPTIONAL) {
> On 2016/06/23 02:29:36, robertshield wrote:
> > I'm haven't seen the OPTIONAL annotation used before in Chromium code. Do
you
> > need it here?
> 
> So, I started a little discussion with my fellow chrome-windows folks.  I
think
> these tags make function definitions nicely readable when there is a truly
> optional argument.  This annotation has not been used in chromium, but it's
also
> not mentioned or denied in chromium or google coding standards.  It's
definitely
> a Windows annotation.
> 
> As my use of this is contained in the new nt_registry library (also used a
> couple times in CreateRegKey and OpenRegKey), and this library/code is very
much
> Windows-specific, I'm feeling ok with leaving these annotations in this
isolated
> library.
> 
> It's really easy to remove in the future if anyone gets emotional about it.

Ok, no worries. Yes, it's an MS convention, since this is Win-only code I
suppose that's fine.

> 
>
https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
> chrome_elf/nt_registry/nt_registry.cc:152: HANDLE* out_handle) {
> On 2016/06/23 02:29:36, robertshield wrote:
> > fwiw, you can do this more simply (and more efficiently) without recursion
by
> > looping over |sub_key_path| keeping iterators to the beginning and end of
each
> > sub key name. This works too though so only change it if you feel like it
:-)
> 
> Thanks Robert.  I was thinking it would be easier to support "unwinding"
changes
> on failure with recursion.  However, I've changed it now to use an iterator
and
> still manage to handle cleanup on failure.  A little more complexity, but no
> function stack frames at least.  Let me know what you think of this version.
> 
>
https://codereview.chromium.org/1841573002/diff/380001/chrome_elf/nt_registry...
> chrome_elf/nt_registry/nt_registry.cc:158: size_t index =
> sub_key_path.find_first_of(L"\\", 0);
> On 2016/06/23 02:29:36, robertshield wrote:
> > you're searching for one character, so you can use the find() overload that
> > takes a wchar_t. 
> 
> Acknowledged.

[robertshield, 2016-06-26 02:11:04]

Looks great, few quick nits on the createregkey implementation

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:24: std::wstring
g_current_user_sid_string;
I believe that static non-POD types are disallowed by the style guide due to
non-deterministic creation / destruction ordering:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

If possible, please make these wchar_t arrays as above.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:199: // Save any handle of subkey created,
in case of rollback.
micro-nit: s/handle of subkey/subkey handle/

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:213: g_nt_delete_key(handle);
This doesn't delete |root_handle| in case of failure, is that intended?

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:220: g_nt_close(root_handle);
If we do want to delete |root_handle| above in case of failure, we could
probably get rid of it as a separate variable and make it the first element in
the |rollback| list, which would simplify some of the clean up code.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:226: if (out_handle) {
slight optimization: could you check |out_handle| before closing the handles
above, pluck the last one off and close the rest, e.g.:


if (!success) {
  // Delete any subkeys created.
  for (auto handle : rollback) {
    g_nt_delete_key(handle);
  }
} else if (out_handle && !rollback.empty()) {
  *out_handle = rollback.back();
  rollback.pop_back();
}

for (auto handle : rollback) {
  g_nt_close(handle);
}
g_nt_close(root_handle);

return success;

[Will Harris, 2016-06-29 15:46:43]

sandbox/win rs lgtm.

-- looks like this needs a rebase: chrome/browser/chrome_elf* moved in
crrev.com/23c8fb7 and you'll need a friendly chrome/browser owner too.

[penny, 2016-07-10 22:45:24]

Patchset #13 (id:420001) has been deleted

[penny, 2016-07-11 18:58:23]

Patchset #13 (id:440001) has been deleted

[penny, 2016-07-11 19:59:04]

Hi Robert,

I've fixed the latest nits.  Static/global variables are now PoD arrays (makes
things a tiny bit messier).  Biggest change is the CreateRegKey recursion.  I'm
now parsing the full reg path into tokens once at the beginning of the function.
 This lets me easily pull off the first tokens that are part of the base hive,
and also easily know which is the last/final subkey.  Please let me know what
you think of the latest.

Many thanks!

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:24: std::wstring
g_current_user_sid_string;
On 2016/06/26 02:11:04, robertshield wrote:
> I believe that static non-POD types are disallowed by the style guide due to
> non-deterministic creation / destruction ordering:
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> 
> If possible, please make these wchar_t arrays as above.

Thanks for that.  I was unaware of this restriction!  I figured using an object
would be more space efficient since I don't know how much room will be needed. 
I've made them POD arrays anyhow.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:152: std::wstring HKLM_override;
Robert, I adjusted these to be POD arrays as well.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:199: // Save any handle of subkey created,
in case of rollback.
On 2016/06/26 02:11:04, robertshield wrote:
> micro-nit: s/handle of subkey/subkey handle/

Done.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:213: g_nt_delete_key(handle);
On 2016/06/26 02:11:03, robertshield wrote:
> This doesn't delete |root_handle| in case of failure, is that intended?

I've adjusted this a bit.  Now I only open the base hive first. 
"\Registry\Machine" or "\Registry\User\SID".  This will always exist already.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:220: g_nt_close(root_handle);
On 2016/06/26 02:11:04, robertshield wrote:
> If we do want to delete |root_handle| above in case of failure, we could
> probably get rid of it as a separate variable and make it the first element in
> the |rollback| list, which would simplify some of the clean up code.

Acknowledged.

https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:226: if (out_handle) {
On 2016/06/26 02:11:04, robertshield wrote:
> slight optimization: could you check |out_handle| before closing the handles
> above, pluck the last one off and close the rest, e.g.:
> 
> 
> if (!success) {
>   // Delete any subkeys created.
>   for (auto handle : rollback) {
>     g_nt_delete_key(handle);
>   }
> } else if (out_handle && !rollback.empty()) {
>   *out_handle = rollback.back();
>   rollback.pop_back();
> }
> 
> for (auto handle : rollback) {
>   g_nt_close(handle);
> }
> g_nt_close(root_handle);
> 
> return success;
> 

Unfortunately, not every handle is on the vector - only the ones that had to be
created.

But, I've changed this now, and know when I'm dealing with the last handle.  So
no need to "reopen" the final handle if the user wants it (which is most often
the case).

[robertshield, 2016-07-12 04:26:24]

still, lg. One question re. ParseFullRegPath since it's a bit late and I'm a
confused:

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:142: std::vector<std::wstring>* subkeys) {
I don't fully understand this function and feel like I'm missing something. 

It looks like it
1) takes two path substrings and concatenates them 
2) tokenizes them into a vector of path components
3) figures out how many path components made up the first path substring
4) rebuilds the same |converted_root| string that was passed in as input and
stuffs it in out_base
5) removes the subkeys that made up |converted_root|

If my reading is correct, could the whole function be replaced with: 

*out_base = converted_root;
subkeys = tokenize(subkey_path);

?

In the quite likely event that I'm misreading, something of this complexity
still demands a unit test.

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.h:44: // - Optional |out_handle|.  If
nullptr, function will close handle when done.
Specify that, if provided, out_handle will refer to the deepest subkey created
by this function.

[penny, 2016-07-13 00:27:55]

Thanks very much indeed for your careful reviews Robert.

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.cc (right):

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.cc:142: std::vector<std::wstring>* subkeys) {
On 2016/07/12 04:26:23, robertshield wrote:
> I don't fully understand this function and feel like I'm missing something. 
> 
> It looks like it
> 1) takes two path substrings and concatenates them 
> 2) tokenizes them into a vector of path components
> 3) figures out how many path components made up the first path substring
> 4) rebuilds the same |converted_root| string that was passed in as input and
> stuffs it in out_base
> 5) removes the subkeys that made up |converted_root|
> 
> If my reading is correct, could the whole function be replaced with: 
> 
> *out_base = converted_root;
> subkeys = tokenize(subkey_path);
> 
> ?
> 
> In the quite likely event that I'm misreading, something of this complexity
> still demands a unit test.

It's a fair question Robert.  It seems at first like the converted_root WILL BE
a base hive like HKLM or HKCU.  However, the reason I need to do some tap
dancing here, is that this function needs to handle the cases when registry
redirection is being done (tests).  HKLM and HKCU in |converted_root| will then
be a longer path, and deeper subkeys can easily not exist yet.

This is why I concat the two inputs first, and then pull off the first 2 or 3
keys for the base hive - leaving all further subkeys to be tokenized.

Example of an HKCU override during a test:
"\REGISTRY\USER\S-1-5-21-39260824-743453154-142223018-716772\Software\Chromium\TempTestKeys\13110669370890870$94c6ed9d-bc34-44f3-a0b3-9eee2d3f2f82\SOFTWARE\Google\Chrome\BrowserSec"

|converted_root| would be
"\REGISTRY\USER\S-1-5-21-39260824-743453154-142223018-716772\Software\Chromium\TempTestKeys\13110669370890870$94c6ed9d-bc34-44f3-a0b3-9eee2d3f2f82\"

|subkey_path| would be "SOFTWARE\Google\Chrome\BrowserSec".

I've added some new comments, and also added a new bool for if registry
redirection is in play.  This way ParseFullRegPath is more efficient for normal
run environments where there's no redirection happening.


WRT unit tests, the ChromeElfUtilTest.NTRegistry test in
chrome_elf_util_unittest.cc (chrome_elf_unittests) exercises this.  However,
I've added a few more tests for creation, with different |subkey_path| inputs.

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
File chrome_elf/nt_registry/nt_registry.h (right):

https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry...
chrome_elf/nt_registry/nt_registry.h:44: // - Optional |out_handle|.  If
nullptr, function will close handle when done.
On 2016/07/12 04:26:24, robertshield wrote:
> Specify that, if provided, out_handle will refer to the deepest subkey created
> by this function.

Done.

[penny, 2016-07-13 16:59:11]

The CQ bit was checked by pennymac@chromium.org

[penny, 2016-07-13 16:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, robertshield@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1841573002/#ps480001
(title: "Final nits.")

[commit-bot: I haz the power, 2016-07-13 16:59:58]

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

[commit-bot: I haz the power, 2016-07-13 17:10:36]

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

[commit-bot: I haz the power, 2016-07-13 17:10:38]

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_presub...)

[penny, 2016-07-13 17:55:08]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
CQ_EXCLUDE_TRYBOTS=tryserver.chromium.linux:chromium_presubmit

(*Note: bypassing presubmit cq bot.  Chrome_elf and NTRegistry do not use base. 
Using std::wstring instead of base::string16 - which presubmit does not like. 
All other presubmit checks pass.)
==========

[penny, 2016-07-13 17:55:27]

The CQ bit was checked by pennymac@chromium.org

[commit-bot: I haz the power, 2016-07-13 17:56:10]

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

[commit-bot: I haz the power, 2016-07-13 18:05:56]

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

[commit-bot: I haz the power, 2016-07-13 18:06:01]

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_presub...)

[penny, 2016-07-13 18:10:47]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
CQ_EXCLUDE_TRYBOTS=tryserver.chromium.linux:chromium_presubmit

(*Note: bypassing presubmit cq bot.  Chrome_elf and NTRegistry do not use base. 
Using std::wstring instead of base::string16 - which presubmit does not like. 
All other presubmit checks pass.)
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
CQ_EXCLUDE_TRYBOTS=master.tryserver.chromium.linux:chromium_presubmit

(*Note: bypassing presubmit cq bot.  Chrome_elf and NTRegistry do not use base. 
Using std::wstring instead of base::string16 - which presubmit does not like. 
All other presubmit checks pass.)
==========

[penny, 2016-07-13 18:12:06]

The CQ bit was checked by pennymac@chromium.org

[commit-bot: I haz the power, 2016-07-13 18:13:07]

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

[penny, 2016-07-13 18:22:24]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
CQ_EXCLUDE_TRYBOTS=master.tryserver.chromium.linux:chromium_presubmit

(*Note: bypassing presubmit cq bot.  Chrome_elf and NTRegistry do not use base. 
Using std::wstring instead of base::string16 - which presubmit does not like. 
All other presubmit checks pass.)
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
CQ_EXCLUDE_TRYBOTS=tryserver.chromium.linux:chromium_presubmit

(*Note: bypassing presubmit cq bot.  Chrome_elf and NTRegistry do not use base. 
Using std::wstring instead of base::string16 - which presubmit does not like. 
All other presubmit checks pass.)
==========

[commit-bot: I haz the power, 2016-07-13 18:24:03]

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

[commit-bot: I haz the power, 2016-07-13 18:24:05]

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_presub...)

[penny, 2016-07-13 18:32:00]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
CQ_EXCLUDE_TRYBOTS=tryserver.chromium.linux:chromium_presubmit

(*Note: bypassing presubmit cq bot.  Chrome_elf and NTRegistry do not use base. 
Using std::wstring instead of base::string16 - which presubmit does not like. 
All other presubmit checks pass.)
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[penny, 2016-07-13 21:09:41]

The CQ bit was checked by pennymac@chromium.org

[penny, 2016-07-13 21:09:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, robertshield@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1841573002/#ps500001
(title: "PRESUBMIT to allow chrome_elf directory files to use wstring.")

[commit-bot: I haz the power, 2016-07-13 21:11:11]

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

[commit-bot: I haz the power, 2016-07-13 21:11:14]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-13 22:35:50]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[commit-bot: I haz the power, 2016-07-13 22:35:52]

Committed patchset #15 (id:500001)

[commit-bot: I haz the power, 2016-07-13 22:36:31]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 22:37:11]

Description was changed from

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
[Chrome ELF] New NT registry API.

This CL is part of a chain of CLs:
1) "MITIGATION_EXTENSION_POINT_DISABLE support for children"
(https://codereview.chromium.org/1835003003)
2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch"
(https://codereview.chromium.org/1836523004/)
-> THIS
4) "Early browser security support" (https://codereview.chromium.org/1656453002)
5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE"
(https://codereview.chromium.org/1854323002)

Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32
(useful from DllMain).
Direct calls to ntdll.  Updated Chrome ELF to always use this new registry API.

Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list
instead of lots of individual reg values.

Small changes to organize file structure and functional components under
chrome_elf.  Old common code now sits under "hook_util", "nt_registry",
"breakpad", "dll_hash", and "blacklist".

Fairly big changes to the chrome_elf tests (blacklist_test.cc,
blacklist_test_main_dll.cc and
chrome_elf_util_unittest.cc) were needed.  Since ntdll bypasses any registry
redirection
(that tests use to keep the hive safe and isolated), I added in a way for the
tests to access
the redirection path (and pass that information on to the test DLL).  This way
the NT reg
API can work with redirection during tests.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc:
ChromeElfUtilTest.NTRegistry is new
(...but run all tests to exercise the new API being used by blacklist and
utils).
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*

BUG=557798
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43
Cr-Commit-Position: refs/heads/master@{#405307}
==========

[commit-bot: I haz the power, 2016-07-13 22:37:13]

Patchset 15 (id:??) landed as
https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43
Cr-Commit-Position: refs/heads/master@{#405307}

[Will Harris, 2016-07-13 23:01:16]

On 2016/07/13 22:37:13, commit-bot: I haz the power wrote:
> Patchset 15 (id:??) landed as
> https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43
> Cr-Commit-Position: refs/heads/master@{#405307}

🎆🎉!