Cache ntdll exports in ELF

Cache ntdll proc addresses in ELF

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238539

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239437

[Cait (Slow), 2013-11-25 20:15:37]

Hey Robert,

Here are the bits of the original Chrome_elf patch that deal with the caching if
ntdll exports (I figured I'd split them off into their own CL in case you need
it for the blacklist).

[robertshield, 2013-11-27 20:53:32]

On 2013/11/25 20:15:37, Cait Phillips wrote:
> Hey Robert,
> 
> Here are the bits of the original Chrome_elf patch that deal with the caching
if
> ntdll exports (I figured I'd split them off into their own CL in case you need
> it for the blacklist).

Is it possible that the chrome_elf_types.h file is missing?

[robertshield, 2013-11-28 13:57:46]

Looks great, a couple of nits and one question

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
File chrome_elf/chrome_elf_main.cc (right):

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:17: if (reason == DLL_PROCESS_ATTACH)
nit: indent

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:40: IMAGE_DATA_DIRECTORY*  exports_data_dir =
nit: extra space

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:43: IMAGE_EXPORT_DIRECTORY* exports  =
nit: extra space

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:46: WORD* ordinals = (WORD*)(base_addr +
exports->AddressOfNameOrdinals);
use C++ style casts (reinterpret_cast) here and below.

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:55: FARPROC func_addr =  (FARPROC) (func  +
base_addr);
nit: extra spaces after = and func, also use a C++ style cast

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_main.h
File chrome_elf/chrome_elf_main.h (right):

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.h:17: CHROME_ELF_API FunctionLookupTable
g_ntdll_lookup;
why does this need to be exported? It can still be used within the chrome_elf
module without being so.

[Cait (Slow), 2013-12-03 03:06:14]

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
File chrome_elf/chrome_elf_main.cc (right):

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:17: if (reason == DLL_PROCESS_ATTACH)
On 2013/11/28 13:57:46, robertshield wrote:
> nit: indent

Done.

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:40: IMAGE_DATA_DIRECTORY*  exports_data_dir =
On 2013/11/28 13:57:46, robertshield wrote:
> nit: extra space

Done.

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:43: IMAGE_EXPORT_DIRECTORY* exports  =
On 2013/11/28 13:57:46, robertshield wrote:
> nit: extra space

Done.

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:46: WORD* ordinals = (WORD*)(base_addr +
exports->AddressOfNameOrdinals);
On 2013/11/28 13:57:46, robertshield wrote:
> use C++ style casts (reinterpret_cast) here and below.

Done.

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:55: FARPROC func_addr =  (FARPROC) (func  +
base_addr);
On 2013/11/28 13:57:46, robertshield wrote:
> nit: extra spaces after = and func, also use a C++ style cast

Done.

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_main.h
File chrome_elf/chrome_elf_main.h (right):

https://codereview.chromium.org/85403005/diff/20001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.h:17: CHROME_ELF_API FunctionLookupTable
g_ntdll_lookup;
On 2013/11/28 13:57:46, robertshield wrote:
> why does this need to be exported? It can still be used within the chrome_elf
> module without being so.

Done.

[robertshield, 2013-12-03 03:26:25]

lgtm

https://codereview.chromium.org/85403005/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/85403005/diff/60001/build/common.gypi#newcode...
build/common.gypi:1456: ],
Since this is the only change outside of chrome_elf, you may want to split it
off into a separate CL and TBR it, or drop it altogether.

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
File chrome_elf/chrome_elf_main.cc (right):

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:9: FunctionLookupTable g_ntdll_lookup =
FunctionLookupTable();
I don't think you need the extra assignment here, just: 

FunctionLookupTable g_ntdll_lookup;

should be fine.

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:26: // The module handle is actually the base
address where the header is.
s/base address where the header is/address of the header/

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:38: PBYTE base_addr =
reinterpret_cast<PBYTE>(ntdll_handle);
nit: use BYTE* rather than PBYTE.

[Cait (Slow), 2013-12-03 19:59:40]

Robert -- please have another (quick) look, I moved the caching code into a
separate lib as discussed.

https://codereview.chromium.org/85403005/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/85403005/diff/60001/build/common.gypi#newcode...
build/common.gypi:1456: ],
On 2013/12/03 03:26:26, robertshield wrote:
> Since this is the only change outside of chrome_elf, you may want to split it
> off into a separate CL and TBR it, or drop it altogether.

I think the was left over from when I had more changes in here..I'll drop it.

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
File chrome_elf/chrome_elf_main.cc (right):

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:9: FunctionLookupTable g_ntdll_lookup =
FunctionLookupTable();
On 2013/12/03 03:26:26, robertshield wrote:
> I don't think you need the extra assignment here, just: 
> 
> FunctionLookupTable g_ntdll_lookup;
> 
> should be fine.

Done.

https://codereview.chromium.org/85403005/diff/60001/chrome_elf/chrome_elf_mai...
chrome_elf/chrome_elf_main.cc:38: PBYTE base_addr =
reinterpret_cast<PBYTE>(ntdll_handle);
On 2013/12/03 03:26:26, robertshield wrote:
> nit: use BYTE* rather than PBYTE.

Done.

[robertshield, 2013-12-03 20:38:20]

lgtm

https://codereview.chromium.org/85403005/diff/110001/chrome_elf/ntdll_cache.h
File chrome_elf/ntdll_cache.h (right):

https://codereview.chromium.org/85403005/diff/110001/chrome_elf/ntdll_cache.h...
chrome_elf/ntdll_cache.h:13: void InitCache();
nit: please add a comment explaining what InitCache does.

[commit-bot: I haz the power, 2013-12-03 20:47:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/85403005/130001

[commit-bot: I haz the power, 2013-12-03 21:13:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-03 21:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/85403005/130001

[commit-bot: I haz the power, 2013-12-03 23:40:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/85403005/130001

[commit-bot: I haz the power, 2013-12-04 03:12:59]

Change committed as 238539

[robliao, 2013-12-04 23:00:28]

This appears to have broken Chromium x64 (side effect being telemetry_unittests
breaking).

We'll be reverting this one soon.

[robliao, 2013-12-04 23:01:32]

A revert of this CL has been created in
https://codereview.chromium.org/105573003/ by robliao@chromium.org.

The reason for reverting is: Breaks Chromium x64.

[Cait (Slow), 2013-12-06 21:06:41]

robertshield: PTAL. This should fix the x64 crash -- I also added tests to
ensure that I'm actually caching what I think I'm caching. Also: *facepalm*.

[robertshield, 2013-12-06 22:08:50]

lgtm

https://codereview.chromium.org/85403005/diff/190001/chrome_elf/ntdll_cache.cc
File chrome_elf/ntdll_cache.cc (right):

https://codereview.chromium.org/85403005/diff/190001/chrome_elf/ntdll_cache.c...
chrome_elf/ntdll_cache.cc:22: ntdll_handle + dos_header->e_lfanew /
sizeof(uint32_t));
nice :)

https://codereview.chromium.org/85403005/diff/190001/chrome_elf/ntdll_cache_u...
File chrome_elf/ntdll_cache_unittest.cc (right):

https://codereview.chromium.org/85403005/diff/190001/chrome_elf/ntdll_cache_u...
chrome_elf/ntdll_cache_unittest.cc:22: };
nit: feels like there should be a extra blank lines around the "Tests" line.
Also not sure if the line-length comments add much to a file this size, though I
do like this kind of file markup in general.

https://codereview.chromium.org/85403005/diff/190001/chrome_elf/ntdll_cache_u...
chrome_elf/ntdll_cache_unittest.cc:34: } // namespace
nit: two spaces before //

[commit-bot: I haz the power, 2013-12-06 22:40:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/85403005/210001

[commit-bot: I haz the power, 2013-12-07 03:16:35]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-09 03:28:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/85403005/210001

[commit-bot: I haz the power, 2013-12-09 06:15:31]

Change committed as 239437