[chrome_elf] Big ELF cleanup. Part 1.

[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART 1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771
Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Committed: https://crrev.com/5446d89cd2b5e01f28c0b479eef46e2d9fd9385a
Cr-Original-Commit-Position: refs/heads/master@{#411982}
Cr-Commit-Position: refs/heads/master@{#414898}

[penny, 2016-07-27 21:25:36]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.

Moving all crash related APIs into one place.
Also started to clean up external dependencies - starting with hook_utils.

BUG=631771
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.

Moving all crash related APIs into one place.
Also started to clean up external dependencies - starting with hook_utils.

BUG=631771
==========

[penny, 2016-07-29 17:56:56]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.

Moving all crash related APIs into one place.
Also started to clean up external dependencies - starting with hook_utils.

BUG=631771
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.


BUG=631771
==========

[penny, 2016-07-29 20:22:26]

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

[commit-bot: I haz the power, 2016-07-29 20:23:08]

Dry run: 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-29 20:48:34]

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

[commit-bot: I haz the power, 2016-07-29 20:48:35]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[penny, 2016-07-29 23:28:52]

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

[penny, 2016-07-29 23:28:53]

Hello Robert!

Here's the first in a number of CLs I'll land to clean up ELF and remove the
dependencies that snuck in.  I'm trying to keep them in manageable sized chunks
(for your sanity mostly).

This is part 1:
- Moved the new crashpad stuff out into one place - removing it from
chrome_elf_main.  Easier to manage crash this way.
- Almost done making hook_util independent of base and sandbox dependencies. 
Will finish that completely in next CL (then on to the crash stuff).
- Removed new dependency on base/win/iat_patch_function completely.

I'm interested in your take on the global vector of crash reports.  In
crash_helper.cc, g_crash_reports.  Since we're not supposed to use any global
vectors (PoD only), I've changed this to be a raw pointer, which I currently
free up during DLL_PROCESS_DETACH.  I was hesitant to initialize a giant array
(guessing at what the max size should be) because that's just ugly.  Anyway, let
me know what you think.

Once I clean up the crash module, I might not need this global anymore anyhow. 
Might just catch it as a raw pointer from crashpad::GetReports(), pass it along
to chrome.dll (when it calls our GetCrashReportsImpl()).  But for this first CL,
I've done what you see here.


Thanks so much for your time Robert.  I'm going to try to do these couple CLs as
fast as possible, to get the dependencies that snuck in out of chrome_elf.

[penny, 2016-08-01 17:52:26]

On 2016/07/29 23:28:53, penny wrote:
> Hello Robert!
> 
> Here's the first in a number of CLs I'll land to clean up ELF and remove the
> dependencies that snuck in.  I'm trying to keep them in manageable sized
chunks
> (for your sanity mostly).
> 
> This is part 1:
> - Moved the new crashpad stuff out into one place - removing it from
> chrome_elf_main.  Easier to manage crash this way.
> - Almost done making hook_util independent of base and sandbox dependencies. 
> Will finish that completely in next CL (then on to the crash stuff).
> - Removed new dependency on base/win/iat_patch_function completely.
> 
> I'm interested in your take on the global vector of crash reports.  In
> crash_helper.cc, g_crash_reports.  Since we're not supposed to use any global
> vectors (PoD only), I've changed this to be a raw pointer, which I currently
> free up during DLL_PROCESS_DETACH.  I was hesitant to initialize a giant array
> (guessing at what the max size should be) because that's just ugly.  Anyway,
let
> me know what you think.
> 
> Once I clean up the crash module, I might not need this global anymore anyhow.

> Might just catch it as a raw pointer from crashpad::GetReports(), pass it
along
> to chrome.dll (when it calls our GetCrashReportsImpl()).  But for this first
CL,
> I've done what you see here.
> 
> 
> Thanks so much for your time Robert.  I'm going to try to do these couple CLs
as
> fast as possible, to get the dependencies that snuck in out of chrome_elf.

Hi Robert, I've adjusted the global pointer I mentioned above.  Realized it
doesn't need to be a void*.  It can be a typed pointer without violating our
coding standard for PoD globals.

[robertshield, 2016-08-02 02:38:41]

cool stuff, comments and thoughts inline

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
File chrome_elf/crash/crash_helper.cc (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:29: std::wstring file_name_string = file_path;
You can save a copy here like this:

std::wstring file_name;
file_name.resize(MAX_PATH);
if (!::GetModuleFileNameW(nullptr, &file_name[0], MAX_PATH)) {
...
}

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:85: 
Could you also assert in debug mode that g_crash_reports starts off null here?

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:94: // (while we have the loader lock), so do
not misbehave.
This comment also applies to InitializeCrashReportingForProcess(), should have
it in both places.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:102: // Please refer above to more information
about why we intercept the
specify to refer to the comment on g_set_unhandled_exception_filter

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
File chrome_elf/crash/crash_helper.h (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:15: void InitializeCrashReportingForProcess();
Is the ForProcess suffix needed?

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:18: void DllDetachCrashReportingCleanup();
This is a complementary function to InitializeCrashReportingForProcess(). They
should be named similarly:

InitializeCrashReportingForProcess()
ShutdownCrashReportingForProcess()

or

DllAttachCrashReportingSetup()
DllDetachCrashReportingCleanup()

.. 

I like the versions that don't implicitly encode the expected callsite in the
name best.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:20: // Used to control unhandled-exception
filters.
This is a crazy and awesome hack and should be documented a bit more. How about
something like 

"Permanently neuters subsequent calls to kernel32!SetUnhandledExceptionFilter(),
see comments in .cc for why this is needed."

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:25: // blacklist interception code, etc.
This comment should also mention that the handler takes a minidump.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
File chrome_elf/hook_util/hook_util.cc (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:169: DWORD ApplyIATHook(HMODULE module_handle,
Please document this function Are the return values standard system error codes?

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:177: return 1;
Along the same lines, should this be ERROR_INVALID_FUNCTION?

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:224: // No hooking unsupported OS versions.
nit: No hooking _on_ unsupported

[penny, 2016-08-02 21:07:44]

Thanks Robert.  Fixes are in!

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
File chrome_elf/crash/crash_helper.cc (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:29: std::wstring file_name_string = file_path;
On 2016/08/02 02:38:40, robertshield wrote:
> You can save a copy here like this:
> 
> std::wstring file_name;
> file_name.resize(MAX_PATH);
> if (!::GetModuleFileNameW(nullptr, &file_name[0], MAX_PATH)) {
> ...
> }

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:85: 
On 2016/08/02 02:38:40, robertshield wrote:
> Could you also assert in debug mode that g_crash_reports starts off null here?

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:94: // (while we have the loader lock), so do
not misbehave.
On 2016/08/02 02:38:40, robertshield wrote:
> This comment also applies to InitializeCrashReportingForProcess(), should have
> it in both places.

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.cc:102: // Please refer above to more information
about why we intercept the
On 2016/08/02 02:38:40, robertshield wrote:
> specify to refer to the comment on g_set_unhandled_exception_filter

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
File chrome_elf/crash/crash_helper.h (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:15: void InitializeCrashReportingForProcess();
On 2016/08/02 02:38:41, robertshield wrote:
> Is the ForProcess suffix needed?

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:18: void DllDetachCrashReportingCleanup();
On 2016/08/02 02:38:40, robertshield wrote:
> This is a complementary function to InitializeCrashReportingForProcess(). They
> should be named similarly:
> 
> InitializeCrashReportingForProcess()
> ShutdownCrashReportingForProcess()
> 
> or
> 
> DllAttachCrashReportingSetup()
> DllDetachCrashReportingCleanup()
> 
> .. 
> 
> I like the versions that don't implicitly encode the expected callsite in the
> name best.
> 

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:20: // Used to control unhandled-exception
filters.
On 2016/08/02 02:38:41, robertshield wrote:
> This is a crazy and awesome hack and should be documented a bit more. How
about
> something like 
> 
> "Permanently neuters subsequent calls to
kernel32!SetUnhandledExceptionFilter(),
> see comments in .cc for why this is needed."

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_...
chrome_elf/crash/crash_helper.h:25: // blacklist interception code, etc.
On 2016/08/02 02:38:40, robertshield wrote:
> This comment should also mention that the handler takes a minidump.

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
File chrome_elf/hook_util/hook_util.cc (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:169: DWORD ApplyIATHook(HMODULE module_handle,
On 2016/08/02 02:38:41, robertshield wrote:
> Please document this function Are the return values standard system error
codes?

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:177: return 1;
On 2016/08/02 02:38:41, robertshield wrote:
> Along the same lines, should this be ERROR_INVALID_FUNCTION?

Done.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:224: // No hooking unsupported OS versions.
On 2016/08/02 02:38:41, robertshield wrote:
> nit: No hooking _on_ unsupported

Well, technically we're hooking the operating system (when hooking a system
service).  Not just doing random hooking on the OS platform.

But I'm happy.  :)

[penny, 2016-08-02 21:07:52]

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

[commit-bot: I haz the power, 2016-08-02 21:08:25]

Dry run: 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-08-02 21:12:42]

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

[commit-bot: I haz the power, 2016-08-02 21:12:43]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[penny, 2016-08-02 22:32:33]

Patchset #5 (id:80001) has been deleted

[penny, 2016-08-02 22:32:47]

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

[commit-bot: I haz the power, 2016-08-02 22:33:31]

Dry run: 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-08-02 23:53:01]

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

[commit-bot: I haz the power, 2016-08-02 23:53:02]

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

[robertshield, 2016-08-03 04:52:46]

Looks really cool (this is fun code to review :-)). 

One or two thoughts about life-cycle stuff, and a suggestion for a test, but
overall this is really close. 

There are a couple of compile failure on some bots that need a quick look as
well.

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
File chrome_elf/hook_util/hook_util.cc (right):

https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/hook_util/ho...
chrome_elf/hook_util/hook_util.cc:224: // No hooking unsupported OS versions.
On 2016/08/02 21:07:43, penny wrote:
> On 2016/08/02 02:38:41, robertshield wrote:
> > nit: No hooking _on_ unsupported
> 
> Well, technically we're hooking the operating system (when hooking a system
> service).  Not just doing random hooking on the OS platform.
> 
> But I'm happy.  :)

Potato, po-tah-to I guess, 'twas merely a nit :-)

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/crash/crash...
File chrome_elf/crash/crash_helper.cc (right):

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/crash/crash...
chrome_elf/crash/crash_helper.cc:59: elf_hook::IATHook
g_set_unhandled_exception_filter;
Technically this violates
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables.

As is, this will be destroyed at some point after DLLMain with
dll_process_detach gets called. It would be cleaner / marginally hypothetically
safer to call Unhook explicitly during dll_process_detach instead of having it
happen during static destruction.

Following from that, you could make this a raw pointer and new / delete it
during process attach / detach.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/crash/crash...
chrome_elf/crash/crash_helper.cc:136: extern "C" __declspec(dllexport) void
GetCrashReportsImpl(
Thinking about lifecycle here, it is important to ensure that anyone who calls
this doesn't attempt to use the returned |reports| after chrome_elf.dll has
unloaded (since they'll be referencing memory that we free during DLL unload).
ELF *should* be the last thing unloaded, so this *should* be fine in chrome.exe.


Would need to be careful if this was called in unit tests that dynamically load
/ unload elf. 

This is more of an unactionable musing... only thing I might suggest is adding a
note to the comment here to be careful not to use the returned data after this
DLL has been unloaded.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
File chrome_elf/hook_util/hook_util.cc (right):

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.cc:279: void* new_function) {
It looks like this method shouldn't be called twice on the same object,
otherwise the first call won't get unhooked properly. If that's the case, please
add a DCHECK to guard against this.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.cc:290: if (original_function_ == new_function)
If this is an error condition, please consider adding to the documentation in
the header, that |new_function| should not be the same as the function
referenced in the import table.  Should Hook() return an error if this happens?

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
File chrome_elf/hook_util/hook_util.h (right):

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:19: // Systen Service hooking support
s/Systen/System/

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:29: class IATHook {
It would be pretty cool (and not too much work I think) to have a unit test for
this class.

A test that:
1) Loads a stub dll that has the exports Foo() { g_call_count++; } and
NumberOFTimesFooWasCalled() { return g_call_count; }
2) calls Foo(), EXPECTs NumberOFTimesFooWasCalled() == 1
3) Hooks Foo(), redirects to a no-op function, same as we do for the unhandled
exception handler.
4) calls Foo(), EXPECTs NumberOFTimesFooWasCalled() == 1
5) Unhooks Foo()
6) calls Foo(), EXPECTs NumberOFTimesFooWasCalled() == 2

would do the trick. Some of the other elf tests already have small test dlls
they build and load, could copy their approach to generate another one, this
link has the places to look to duplicate:
https://codesearch.chromium.org/search/?q=blacklist_test_dll_2&sq=package:chr....

What do you think, worth it?

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:40: // function_name          Name of the API
to be intercepted
new_function should be added here for completeness

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:43: DWORD Hook(HMODULE module,
From the implementation, it looks like Hook should only be called once for a
given instance of an IATHook, if that's the case, please document here.

[penny, 2016-08-04 18:45:59]

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

[commit-bot: I haz the power, 2016-08-04 18:46:53]

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

[penny, 2016-08-04 18:55:35]

I'm glad you're enjoying it Robert - there's more goodness to come.  ;D

Here's the next round of fixes!

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/crash/crash...
File chrome_elf/crash/crash_helper.cc (right):

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/crash/crash...
chrome_elf/crash/crash_helper.cc:59: elf_hook::IATHook
g_set_unhandled_exception_filter;
On 2016/08/03 04:52:45, robertshield wrote:
> Technically this violates
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables.
> 
> As is, this will be destroyed at some point after DLLMain with
> dll_process_detach gets called. It would be cleaner / marginally
hypothetically
> safer to call Unhook explicitly during dll_process_detach instead of having it
> happen during static destruction.
> 
> Following from that, you could make this a raw pointer and new / delete it
> during process attach / detach. 

I fully agree.  I was planning to change this to be like g_crash_reports in my
next CL.  Was trying split up many changes into multiple smaller CLs for your
sanity.  ;D  However, it's small to add this now, so done!  Fully agree.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/crash/crash...
chrome_elf/crash/crash_helper.cc:136: extern "C" __declspec(dllexport) void
GetCrashReportsImpl(
On 2016/08/03 04:52:45, robertshield wrote:
> Thinking about lifecycle here, it is important to ensure that anyone who calls
> this doesn't attempt to use the returned |reports| after chrome_elf.dll has
> unloaded (since they'll be referencing memory that we free during DLL unload).
> ELF *should* be the last thing unloaded, so this *should* be fine in
chrome.exe.
> 
> 
> Would need to be careful if this was called in unit tests that dynamically
load
> / unload elf. 
> 
> This is more of an unactionable musing... only thing I might suggest is adding
a
> note to the comment here to be careful not to use the returned data after this
> DLL has been unloaded. 

Done.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
File chrome_elf/hook_util/hook_util.cc (right):

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.cc:279: void* new_function) {
On 2016/08/03 04:52:45, robertshield wrote:
> It looks like this method shouldn't be called twice on the same object,
> otherwise the first call won't get unhooked properly. If that's the case,
please
> add a DCHECK to guard against this.

Done.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.cc:290: if (original_function_ == new_function)
On 2016/08/03 04:52:45, robertshield wrote:
> If this is an error condition, please consider adding to the documentation in
> the header, that |new_function| should not be the same as the function
> referenced in the import table.  Should Hook() return an error if this
happens?

I've added a check in IATFindHookFuncCallback() (when trying to apply the hook),
and return an error.  This should really never happen.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
File chrome_elf/hook_util/hook_util.h (right):

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:19: // Systen Service hooking support
On 2016/08/03 04:52:45, robertshield wrote:
> s/Systen/System/

Done.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:29: class IATHook {
On 2016/08/03 04:52:45, robertshield wrote:
> It would be pretty cool (and not too much work I think) to have a unit test
for
> this class.
> 
> A test that:
> 1) Loads a stub dll that has the exports Foo() { g_call_count++; } and
> NumberOFTimesFooWasCalled() { return g_call_count; }
> 2) calls Foo(), EXPECTs NumberOFTimesFooWasCalled() == 1
> 3) Hooks Foo(), redirects to a no-op function, same as we do for the unhandled
> exception handler.
> 4) calls Foo(), EXPECTs NumberOFTimesFooWasCalled() == 1
> 5) Unhooks Foo()
> 6) calls Foo(), EXPECTs NumberOFTimesFooWasCalled() == 2
> 
> would do the trick. Some of the other elf tests already have small test dlls
> they build and load, could copy their approach to generate another one, this
> link has the places to look to duplicate:
>
https://codesearch.chromium.org/search/?q=blacklist_test_dll_2&sq=package:chr....
> 
> What do you think, worth it?

Done.
Was going to leave this until I finish removing deps from hook_util... but mine
as well add the tests now, so it's obviously with this CL.  DONE.  I'll be
making system-service hooking local in one of the next couple CLs, so will add
more tests then.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:40: // function_name          Name of the API
to be intercepted
On 2016/08/03 04:52:45, robertshield wrote:
> new_function should be added here for completeness

Done.

https://codereview.chromium.org/2183263003/diff/100001/chrome_elf/hook_util/h...
chrome_elf/hook_util/hook_util.h:43: DWORD Hook(HMODULE module,
On 2016/08/03 04:52:45, robertshield wrote:
> From the implementation, it looks like Hook should only be called once for a
> given instance of an IATHook, if that's the case, please document here.

Done.

[commit-bot: I haz the power, 2016-08-04 20:03:07]

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

[commit-bot: I haz the power, 2016-08-04 20:03:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[penny, 2016-08-04 21:49:39]

Patchset #7 (id:140001) has been deleted

[robertshield, 2016-08-10 20:14:48]

Code LGTM, small request for an extra couple of lines in the hook test if
possible.

https://codereview.chromium.org/2183263003/diff/160001/chrome_elf/hook_util/t...
File chrome_elf/hook_util/test/hook_util_test.cc (right):

https://codereview.chromium.org/2183263003/diff/160001/chrome_elf/hook_util/t...
chrome_elf/hook_util/test/hook_util_test.cc:32: TEST_F(HookTest, IATHook) {
Can you test that double Hook()ing fails as expected (possibly by using
EXPECT_THROW in debug mode?).

[penny, 2016-08-11 17:13:06]

Patchset #8 (id:180001) has been deleted

[penny, 2016-08-11 17:13:19]

Patchset #9 (id:220001) has been deleted

[penny, 2016-08-11 20:57:34]

Patchset #9 (id:240001) has been deleted

[penny, 2016-08-13 04:39:42]

Patchset #9 (id:260001) has been deleted

[penny, 2016-08-13 04:49:12]

Patchset #9 (id:280001) has been deleted

[penny, 2016-08-13 19:22:15]

Robert, I think I finally managed to get all the tests running clean.  :)

Feel free to do one final scan of the last changes.

Thanks again!

https://codereview.chromium.org/2183263003/diff/160001/chrome_elf/hook_util/t...
File chrome_elf/hook_util/test/hook_util_test.cc (right):

https://codereview.chromium.org/2183263003/diff/160001/chrome_elf/hook_util/t...
chrome_elf/hook_util/test/hook_util_test.cc:32: TEST_F(HookTest, IATHook) {
On 2016/08/10 20:14:48, robertshield wrote:
> Can you test that double Hook()ing fails as expected (possibly by using
> EXPECT_THROW in debug mode?).

So I'm not exactly sure what you meant here... but I've added two more tests:
1) Trying to hook twice from the same IATHook object fails.
2) Added a check at the bottom that tests if someone else hooks the same IAT
entry (over top of the first hook).  We should leave it alone on Unhook.

Let me know if you meant something that these two don't test!

[penny, 2016-08-13 19:22:27]

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

[commit-bot: I haz the power, 2016-08-13 19:22:32]

Dry run: 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-08-13 19:29:08]

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

[commit-bot: I haz the power, 2016-08-13 19:29:09]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[penny, 2016-08-15 16:05:42]

The CQ bit was checked by pennymac@chromium.org

[penny, 2016-08-15 16:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/2183263003/#ps300001
(title: "Final fixes?")

[commit-bot: I haz the power, 2016-08-15 16:05:56]

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-08-15 18:04:17]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.


BUG=631771
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.


BUG=631771
==========

[commit-bot: I haz the power, 2016-08-15 18:04:20]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2016-08-15 18:11:15]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.


BUG=631771
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}
==========

[commit-bot: I haz the power, 2016-08-15 18:11:16]

Patchset 9 (id:??) landed as
https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}

[Avi (use Gerrit), 2016-08-15 20:07:08]

A revert of this CL (patchset #9 id:300001) has been created in
https://codereview.chromium.org/2242323002/ by avi@chromium.org.

The reason for reverting is: Win7 Tests (dbg)(1) started failing.

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...

Excerpt from step 103, chrome_elf_unittests on Windows-7-SP1
chrome_elf_unittests on Windows-7-SP1 :

[ RUN      ] HookTest.IATHook
Assertion failed: false, file
c:\b\c\b\win\src\chrome_elf\hook_util\hook_util.cc, line 302
[43/49] HookTest.IATHook (TIMED OUT)
.

[penny, 2016-08-24 21:11:31]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}
==========

[penny, 2016-08-26 21:44:19]

Patchset #9 (id:300001) has been deleted

[penny, 2016-08-26 23:24:37]

The CQ bit was checked by pennymac@chromium.org

[penny, 2016-08-26 23:24:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/2183263003/#ps320001
(title: "Remove debug asserts that hang tests.")

[commit-bot: I haz the power, 2016-08-26 23:25:15]

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-08-27 00:33:42]

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

[commit-bot: I haz the power, 2016-08-27 00:33:43]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[penny, 2016-08-27 03:44:34]

Patchset #10 (id:340001) has been deleted

[penny, 2016-08-27 03:45:50]

The CQ bit was checked by pennymac@chromium.org

[penny, 2016-08-27 03:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/2183263003/#ps360001
(title: "Update with latest trunk.")

[commit-bot: I haz the power, 2016-08-27 03:46:09]

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-08-27 10:45:32]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}
==========

[commit-bot: I haz the power, 2016-08-27 10:45:34]

Committed patchset #10 (id:360001)

[commit-bot: I haz the power, 2016-08-27 10:47:32]

Description was changed from

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Cr-Commit-Position: refs/heads/master@{#411982}
==========

to

==========
[chrome_elf] Big cleanup and removing dependencies that recently crept in.  PART
1.

- Moving all crash related APIs into one place (and out of chrome_elf_main).
- Started to clean up external dependencies - starting with hook_utils.
- Removed dependency on base/win/iat_patch_function.

BUG=631771

Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58
Committed: https://crrev.com/5446d89cd2b5e01f28c0b479eef46e2d9fd9385a
Cr-Original-Commit-Position: refs/heads/master@{#411982}
Cr-Commit-Position: refs/heads/master@{#414898}
==========

[commit-bot: I haz the power, 2016-08-27 10:47:33]

Patchset 10 (id:??) landed as
https://crrev.com/5446d89cd2b5e01f28c0b479eef46e2d9fd9385a
Cr-Commit-Position: refs/heads/master@{#414898}