Use an alternate mechanism for CreateFile calls in Chrome

Use an alternate mechanism for CreateFile calls in Chrome

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

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

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

[Cait (Slow), 2014-01-15 20:03:24]

robertshield: PTAL at chrome_elf/ changes
rvargas: PTAL at sandbox/nt_internals.h changes (just the addition of a few more
windows-specific constants).

Thanks!

[rvargas (doing something else), 2014-01-15 23:00:39]

sandbox lgtm

https://codereview.chromium.org/138593004/diff/1/sandbox/win/src/nt_internals.h
File sandbox/win/src/nt_internals.h (right):

https://codereview.chromium.org/138593004/diff/1/sandbox/win/src/nt_internals...
sandbox/win/src/nt_internals.h:129: // Create/open result values.
Add to the comment that these are the disposition values returned on the io
status information.

[robertshield, 2014-01-16 16:10:26]

lgtm

https://codereview.chromium.org/138593004/diff/1/chrome_elf/create_file/chrom...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/138593004/diff/1/chrome_elf/create_file/chrom...
chrome_elf/create_file/chrome_create_file.cc:57: HMODULE shell32 =
::LoadLibrary(L"shell32.dll");
Add a comment that these handles are intentionally leaked.

[commit-bot: I haz the power, 2014-01-16 21:18:38]

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

[commit-bot: I haz the power, 2014-01-16 22:36:21]

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

[commit-bot: I haz the power, 2014-01-17 00:01:42]

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

[commit-bot: I haz the power, 2014-01-17 07:20:57]

Change committed as 245464

[Cait (Slow), 2014-01-17 23:05:41]

cpu: PTAL at base/win/iat_patch_function.cc -- basically, for testing purposes,
we need to patch the import address table of kernel32 on certain platforms
(vista and xp), and it was causing crashes because the Patcher was trying to
execute code that had just been made non-executable (Kernel32::VirtualProtect). 

I've added a check to see if we are patching kernel32 (which should hopefully
only be done in tests) and keep the page being patched executable in that case.
WDYT?

Thanks!

https://codereview.chromium.org/138593004/diff/1/chrome_elf/create_file/chrom...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/138593004/diff/1/chrome_elf/create_file/chrom...
chrome_elf/create_file/chrome_create_file.cc:57: HMODULE shell32 =
::LoadLibrary(L"shell32.dll");
On 2014/01/16 16:10:26, robertshield wrote:
> Add a comment that these handles are intentionally leaked.

Done.

https://codereview.chromium.org/138593004/diff/1/sandbox/win/src/nt_internals.h
File sandbox/win/src/nt_internals.h (right):

https://codereview.chromium.org/138593004/diff/1/sandbox/win/src/nt_internals...
sandbox/win/src/nt_internals.h:129: // Create/open result values.
On 2014/01/15 23:00:39, rvargas wrote:
> Add to the comment that these are the disposition values returned on the io
> status information.

Done.

[cpu_(ooo_6.6-7.5), 2014-01-18 00:17:18]

I'll defer to rvargas.

[rvargas (doing something else), 2014-01-18 00:27:34]

yeah, that crash is an old known issue of that code.

We should not have dll-specific code here. You should query the  protection of
the address to patch and preserve the old bits.

[Cait (Slow), 2014-01-20 17:04:02]

rvargas: PTAL -- I removed all dll-specific code and instead am just checking
the original protection on the address.

[rvargas (doing something else), 2014-01-21 18:48:50]

https://codereview.chromium.org/138593004/diff/460001/base/win/iat_patch_func...
File base/win/iat_patch_function.cc (right):

https://codereview.chromium.org/138593004/diff/460001/base/win/iat_patch_func...
base/win/iat_patch_function.cc:63: ::VirtualQuery(old_code, &memory_info,
sizeof(memory_info));
Validate the return value.

https://codereview.chromium.org/138593004/diff/460001/base/win/iat_patch_func...
base/win/iat_patch_function.cc:66: PAGE_EXECUTE_READWRITE |
PAGE_EXECUTE_WRITECOPY) &
nit: either indent under PAGE_EXECUTE or drop the first two flags to a new line.

[Cait (Slow), 2014-01-21 19:53:40]

rvargas: comments addressed. PTAL.

https://codereview.chromium.org/138593004/diff/460001/base/win/iat_patch_func...
File base/win/iat_patch_function.cc (right):

https://codereview.chromium.org/138593004/diff/460001/base/win/iat_patch_func...
base/win/iat_patch_function.cc:63: ::VirtualQuery(old_code, &memory_info,
sizeof(memory_info));
On 2014/01/21 18:48:50, rvargas wrote:
> Validate the return value.

Done.

https://codereview.chromium.org/138593004/diff/460001/base/win/iat_patch_func...
base/win/iat_patch_function.cc:66: PAGE_EXECUTE_READWRITE |
PAGE_EXECUTE_WRITECOPY) &
On 2014/01/21 18:48:50, rvargas wrote:
> nit: either indent under PAGE_EXECUTE or drop the first two flags to a new
line.

Done.

[rvargas (doing something else), 2014-01-21 20:03:25]

lgtm

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
File base/win/iat_patch_function.cc (right):

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
base/win/iat_patch_function.cc:71: memory_info.Protect;
tiny nit: one less space here.

[rvargas (doing something else), 2014-01-21 21:19:49]

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
File base/win/iat_patch_function.cc (right):

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
base/win/iat_patch_function.cc:65: NOTREACHED();
On second thoughts, we should probably just remove this notreached (and the one
below). I don't think they are really useful.

[Cait (Slow), 2014-01-21 21:47:19]

cpu@: base/win/ changes have been approved by rvagas@, but I need your OWNERS
approval to commit.

Thanks!

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
File base/win/iat_patch_function.cc (right):

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
base/win/iat_patch_function.cc:65: NOTREACHED();
On 2014/01/21 21:19:49, rvargas wrote:
> On second thoughts, we should probably just remove this notreached (and the
one
> below). I don't think they are really useful.

Done.

https://codereview.chromium.org/138593004/diff/110014/base/win/iat_patch_func...
base/win/iat_patch_function.cc:71: memory_info.Protect;
On 2014/01/21 20:03:26, rvargas wrote:
> tiny nit: one less space here.

Done.

[cpu_(ooo_6.6-7.5), 2014-01-22 00:14:44]

lgtm

[commit-bot: I haz the power, 2014-01-22 02:57:12]

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

[commit-bot: I haz the power, 2014-01-22 13:49:05]

Change committed as 246313

[kochi, 2014-01-23 09:00:32]

On 2014/01/22 13:49:05, I haz the power (commit-bot) wrote:
> Change committed as 246313

Sorry to bother you again, but I reverted r246313 at r246541.

This caused on my dev environment (corp Windows8) failure immediately after
launching the process.

Here are some sawbuck output from the failing process.

ERROR	25180	41644	16:33:13-830	d:\src\chrome\src\chrome\browser\process_singleton_win.cc	437	Lock
file can not be created! Error code: 1008
ERROR	25180	41644	16:33:13-832	d:\src\chrome\src\chrome\browser\chrome_browser_main.cc	1179	Failed
to create a ProcessSingleton for your profile directory. This means that running
multiple instances would start multiple browser processes rather than opening a
new window in the existing process. Aborting now to avoid profile corruption.
WARNING	25180	38620	16:33:13-851	d:\src\chrome\src\base\files\important_file_writer.cc	48	temp
file failure: 1F4D3898 : could not open temporary file:
���݂��Ȃ��g�[�N����Q�Ƃ��悤�Ƃ��܂����B
WARNING	25180	38620	16:33:13-866	d:\src\chrome\src\base\files\important_file_writer.cc	48	temp
file failure: 1F4D3C08 : could not open temporary file:
���݂��Ȃ��g�[�N����Q�Ƃ��悤�Ƃ��܂����B

As a workaround, specifing --user-data-dir to a new directory worked for me.

Could you check this?

[Cait (Slow), 2014-01-27 18:39:49]

robertshield: PTAL at chrome_create_file* (I've made the changes we discussed).
Also reworked things such that chrome_redirects is only built in component
builds.

Thanks!

[robertshield, 2014-01-29 17:59:32]

lgtm

https://codereview.chromium.org/138593004/diff/1060001/base/win/iat_patch_fun...
File base/win/iat_patch_function.cc (right):

https://codereview.chromium.org/138593004/diff/1060001/base/win/iat_patch_fun...
base/win/iat_patch_function.cc:63: if (!::VirtualQuery(old_code, &memory_info,
sizeof(memory_info))) {
Nit, convention in this file seems to be not to prefix API calls with the scope
operator. I don't much like this convention, but I'd suggest remaining
consistent or updating the file.

https://codereview.chromium.org/138593004/diff/1060001/chrome_elf/create_file...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/138593004/diff/1060001/chrome_elf/create_file...
chrome_elf/create_file/chrome_create_file.cc:273: bool is_settings_file = 
wcscmp(file_name, kPreferencesFilename) == 0 ||
nit "  " -> " "

https://codereview.chromium.org/138593004/diff/1060001/chrome_elf/create_file...
chrome_elf/create_file/chrome_create_file.cc:276: // Check if we are trying to
access a Preferences in the UserData dir. If so,
s/a Preferences/the Preferences file/

https://codereview.chromium.org/138593004/diff/1060001/chrome_elf/create_file...
File chrome_elf/create_file/chrome_create_file.h (right):

https://codereview.chromium.org/138593004/diff/1060001/chrome_elf/create_file...
chrome_elf/create_file/chrome_create_file.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
nit: 2014

[commit-bot: I haz the power, 2014-01-30 23:54:54]

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

[commit-bot: I haz the power, 2014-01-31 01:03:24]

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

[commit-bot: I haz the power, 2014-01-31 01:03:27]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 02:56:28]

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

[commit-bot: I haz the power, 2014-01-31 05:12:35]

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

[commit-bot: I haz the power, 2014-01-31 05:12:42]

CQ bit was unchecked on CL. Ignoring.

[Cait (Slow), 2014-01-31 17:04:22]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-01-31 17:12:45]

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

[commit-bot: I haz the power, 2014-01-31 18:31:07]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 18:31:10]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 18:31:14]

CQ bit was unchecked on CL. Ignoring.

[Cait (Slow), 2014-01-31 18:32:01]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-01-31 18:33:11]

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

[commit-bot: I haz the power, 2014-01-31 20:33:57]

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

[commit-bot: I haz the power, 2014-01-31 20:33:58]

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

[commit-bot: I haz the power, 2014-01-31 20:34:05]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 20:34:05]

CQ bit was unchecked on CL. Ignoring.

[Cait (Slow), 2014-02-01 19:08:55]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-01 19:09:03]

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

[commit-bot: I haz the power, 2014-02-02 05:52:43]

Change committed as 248380

[commit-bot: I haz the power, 2014-02-02 05:52:52]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-02 05:52:52]

CQ bit was unchecked on CL. Ignoring.