Make chrome.exe built with ASan/Win work with sandbox enabled

Make chrome.exe built with ASan/Win work with sandbox enabled

Also add an ASan test to sbox_integration_tests on Windows

BUG=382867
Committed: https://crrev.com/af81370a77349709ce37cbaec93020e88400ef7e
Cr-Commit-Position: refs/heads/master@{#314780}

[Timur Iskhodzhanov, 2015-01-30 16:05:29]

timurrrr@chromium.org changed reviewers:
+ cpu@chromium.org, jschuh@chromium.org

[Timur Iskhodzhanov, 2015-01-30 16:05:30]

Hi Carlos, Justin,

Can you please review this patch?

Thanks,
Timur

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/handle_inher...
File sandbox/win/src/handle_inheritance_test.cc (left):

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/handle_inher...
sandbox/win/src/handle_inheritance_test.cc:28: attrs.lpSecurityDescriptor =
NULL;
FYI I think this is redundant

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
sandbox/win/src/target_services.cc:80: // TODO: find a better place to do this?
Please advise where to move this code?

[Will Harris, 2015-01-30 17:49:17]

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

[Will Harris, 2015-01-30 17:49:17]

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
sandbox/win/src/target_services.cc:80: // TODO: find a better place to do this?
On 2015/01/30 16:05:30, Timur Iskhodzhanov wrote:
> Please advise where to move this code?

Warmup is usually done specific for each sandboxed process type in <process
type>MainPlatformDelegate::EnableSandbox

e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

but you want this for tests, so maybe it should go in controller.cc
DispatchCall?

[cpu_(ooo_6.6-7.5), 2015-01-30 20:28:24]

https://codereview.chromium.org/868253011/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/868253011/diff/1/content/common/sandbox_win.c...
content/common/sandbox_win.cc:344: wchar_t main_module_path[MAX_PATH];
use base::FILE_EXE or base::FILE_MODULE

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
sandbox/win/src/target_services.cc:83: ::TerminateProcess(::GetCurrentProcess(),
0x1234);
correct, this happens ahead on each client.

[cpu_(ooo_6.6-7.5), 2015-01-30 20:28:57]

cpu@chromium.org changed reviewers:
+ rvargas@chromium.org

[Timur Iskhodzhanov, 2015-02-02 15:38:58]

Please take another look

https://codereview.chromium.org/868253011/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/868253011/diff/1/content/common/sandbox_win.c...
content/common/sandbox_win.cc:344: wchar_t main_module_path[MAX_PATH];
On 2015/01/30 20:28:24, cpu wrote:
> use base::FILE_EXE or base::FILE_MODULE

Done, thanks!
Also fixed a similar code in the ASan sandbox test.

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
sandbox/win/src/target_services.cc:80: // TODO: find a better place to do this?
I actually want it to run always (including chrome.exe on ClusterFuzz) rather
than just for tests.

Do you think it's reasonable to duplicate code between <process
type>MainPlatformDelegate::EnableSandbox?
I'd argue this code should be placed somewhere in the shared sandbox code that
runs always when AddressSanitizer is enabled.  I don't see why this should be
process-specific.

https://codereview.chromium.org/868253011/diff/1/sandbox/win/src/target_servi...
sandbox/win/src/target_services.cc:83: ::TerminateProcess(::GetCurrentProcess(),
0x1234);
On 2015/01/30 20:28:24, cpu wrote:
> correct, this happens ahead on each client.

Can you please clarify this comment?
Did you mean "yes, this is a good place, let's keep it here. please define a new
SBOX_FATAL exit code"?

[rvargas (doing something else), 2015-02-02 20:55:58]

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
sandbox/win/src/address_sanitizer_test.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
nit: no (c)

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
sandbox/win/src/address_sanitizer_test.cc:43: #if defined(ADDRESS_SANITIZER)
Can we just have the whole test inside the ifdef? This pattern looks like there
is a problem with the test and something is temporarily disabled.

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/t...
File sandbox/win/src/target_services.cc (right):

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/t...
sandbox/win/src/target_services.cc:80: // TODO: find a better place to do this?
I'd say this is a type of warming so it should be done from the Chrome layer (or
where the *.pdb policy is set), even if that means code duplication for other
targets.

[Timur Iskhodzhanov, 2015-02-02 21:28:39]

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
sandbox/win/src/address_sanitizer_test.cc:43: #if defined(ADDRESS_SANITIZER)
On 2015/02/02 20:55:58, rvargas (Out OO) wrote:
> Can we just have the whole test inside the ifdef? This pattern looks like
there
> is a problem with the test and something is temporarily disabled.

The idea behind using MAYBE_ here was that I want to keep this test compileable
in all Windows builds to prevent typos from passing CQ even though ASan/Win is
not tree blocker [it will not be a blocker for a while].

What do you think?

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/t...
File sandbox/win/src/target_services.cc (right):

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/t...
sandbox/win/src/target_services.cc:80: // TODO: find a better place to do this?
On 2015/02/02 20:55:58, rvargas (Out OO) wrote:
> I'd say this is a type of warming so it should be done from the Chrome layer
(or
> where the *.pdb policy is set), even if that means code duplication for other
> targets.

Carlos, do you agree?

Btw, the *.pdb policy is set in the parent process, whilst LoadLibrary should be
called from a [almost sandboxed] child process

[cpu_(ooo_6.6-7.5), 2015-02-02 22:57:23]

yes the *.pdb needs to be set on client of the sandbox, the sandbox should not
know about it.

[rvargas (doing something else), 2015-02-03 00:42:29]

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://chromiumcodereview.appspot.com/868253011/diff/20001/sandbox/win/src/a...
sandbox/win/src/address_sanitizer_test.cc:43: #if defined(ADDRESS_SANITIZER)
On 2015/02/02 21:28:38, Timur Iskhodzhanov wrote:
> On 2015/02/02 20:55:58, rvargas (Out OO) wrote:
> > Can we just have the whole test inside the ifdef? This pattern looks like
> there
> > is a problem with the test and something is temporarily disabled.
> 
> The idea behind using MAYBE_ here was that I want to keep this test
compileable
> in all Windows builds to prevent typos from passing CQ even though ASan/Win is
> not tree blocker [it will not be a blocker for a while].
> 
> What do you think?

That's a good point. Maybe we need another name for tests that are not really
disabled. I think we should try to have zero disabled tests, and if some tests
are in that category all the time it is pretty hard to notice that there is code
not being tested so it can regress without warnings.

So between breaking a bot because there was no compile try-bot and regressing
code because tests are disabled I'd vote for the former.

[Timur Iskhodzhanov, 2015-02-03 12:26:49]

Another option is to have the test return immediately if ADDRESS_SANITIZER is
not defined, yet still compile its code.
  + 0 disabled tests
  + ASan test code is still compiled
  - the test will appear to work even in the non-ASan build, possibly with
LOG(WARNING)

[Timur Iskhodzhanov, 2015-02-03 13:25:22]

OK, I've moved the LoadLibrary calls to clients.
All the changes to sandbox/ are now test-related.

Please take another look!

https://codereview.chromium.org/868253011/diff/20001/sandbox/win/src/address_...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/20001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/02/02 20:55:58, rvargas wrote:
> nit: no (c)

Thanks for catching this!
Self-for-the-record: I've missed
http://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/868253011/diff/20001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:43: #if defined(ADDRESS_SANITIZER)
Why do you think disabling tests leads to regressing code that much?
I assume if a test is [temporarily] disabled for a good reason.  If the reason
is not good, it doesn't matter if you disable or #ifdef, one should do neither. 
If the temporary reason for disabling is good, the test should be assigned a
crbug and the owner of the crbug should be responsible for fixing anything
caused by this exclusion.   Compiling even disabled tests [in contrast with
#ifdef'ing] might also be helpful in case you need to bisect to find a new bug
introduced while the test was disabled...

This is your codebase, not mine.  I'm #ifdef'ing the test as you said for now,
but willing to get back to MAYBE_ or ASAN_ONLY_ or whatever if you agree with my
arguments above.

https://codereview.chromium.org/868253011/diff/40001/components/nacl/loader/n...
File components/nacl/loader/nacl_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/868253011/diff/40001/components/nacl/loader/n...
components/nacl/loader/nacl_main_platform_delegate_win.cc:25:
rand_s(&dummy_rand);
note: rand_s and GetUserDefaultL*ID calls seem to be used in most places before
a LowerToken() call, same as the LoadLibrary call I add.  I wonder if we should
create a shared helper function?

https://codereview.chromium.org/868253011/diff/40001/content/ppapi_plugin/ppa...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/868253011/diff/40001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:416: rand_s(&dummy_rand);
e.g. here and inside WarmupWindowsLocales

https://codereview.chromium.org/868253011/diff/40001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/868253011/diff/40001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:114:
rand_s(&dummy_rand);
ditto

[rvargas (doing something else), 2015-02-03 19:47:11]

https://codereview.chromium.org/868253011/diff/20001/sandbox/win/src/address_...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/20001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:43: #if defined(ADDRESS_SANITIZER)
On 2015/02/03 13:25:22, Timur Iskhodzhanov wrote:
> Why do you think disabling tests leads to regressing code that much?
> I assume if a test is [temporarily] disabled for a good reason.  If the reason
> is not good, it doesn't matter if you disable or #ifdef, one should do
neither. 
> If the temporary reason for disabling is good, the test should be assigned a
> crbug and the owner of the crbug should be responsible for fixing anything
> caused by this exclusion.   Compiling even disabled tests [in contrast with
> #ifdef'ing] might also be helpful in case you need to bisect to find a new bug
> introduced while the test was disabled...
> 
> This is your codebase, not mine.  I'm #ifdef'ing the test as you said for now,
> but willing to get back to MAYBE_ or ASAN_ONLY_ or whatever if you agree with
my
> arguments above.

What I've seen happening with disabled tests is that in practice they are not
prioritized accordingly with the effect (as in, the feature would have not been
checked in without the test). And that basically means that they just stay
disabled for months/years and filing a bug doesn't help (because we have
thousands of bugs open anyway).

I actually like your suggestion of leaving the test enabled and doing nothing
for a normal build (aka, minimizing the code that is not compiled by default).

[Timur Iskhodzhanov, 2015-02-04 06:47:20]

It's there anything else left to do?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Timur Iskhodzhanov, 2015-02-04 06:47:49]

*is (sorry, android)

ср, 4 февр. 2015, 9:47, Timur Iskhodzhanov <timurrrr@chromium.org>:

> It's there anything else left to do?
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Timur Iskhodzhanov, 2015-02-04 20:29:57]

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:50: #if defined(ADDRESS_SANITIZER)
How about this?
Unfortunately, I don't think there's a handy 0/1 global variable/constant
available to avoid #ifdef here.

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:76: pdb_path.value().c_str()));
Please note I actually run the ASan-inspecific part of the test to make sure it
doesn't break either.

[rvargas (doing something else), 2015-02-04 20:34:27]

Sandbox PS4 LGTM

[Timur Iskhodzhanov, 2015-02-04 20:43:36]

Carlos,
Can you review the content+components part?

[cpu_(ooo_6.6-7.5), 2015-02-04 20:59:55]

lgtm w/ nit

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:39: int *blah = new int[42];
state what errors we should see from asan, what would be the wording of them. I
see two.

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:58:
ASSERT_NE(::GetTempFileName(temp_directory, L"test", 0, temp_file_name), 0u);
we have scoped temp directory  temp file helpers in base I think.

[Timur Iskhodzhanov, 2015-02-05 08:08:22]

New patchsets have been uploaded after l-g-t-m from
rvargas@chromium.org,cpu@chromium.org

[Timur Iskhodzhanov, 2015-02-05 08:15:52]

Thanks for the review!

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:39: int *blah = new int[42];
Done.
The memory leak was not important, but I've plugged it anyway.

https://codereview.chromium.org/868253011/diff/60001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:58:
ASSERT_NE(::GetTempFileName(temp_directory, L"test", 0, temp_file_name), 0u);
Done, thanks for the suggestion!

https://codereview.chromium.org/868253011/diff/80001/sandbox/win/src/address_...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/80001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:20: public:
self-oops: the formatting was wrong here.

https://codereview.chromium.org/868253011/diff/80001/sandbox/win/src/address_...
sandbox/win/src/address_sanitizer_test.cc:63:
ASSERT_TRUE(CreateTemporaryFileInDir(temp_directory.path(), &temp_file_name));
FTR, ScopedTempDir cleans up its contents when it gets destroyed, so no need to
destroy the file.

https://codereview.chromium.org/868253011/diff/80001/sandbox/win/src/handle_i...
File sandbox/win/src/handle_inheritance_test.cc (right):

https://codereview.chromium.org/868253011/diff/80001/sandbox/win/src/handle_i...
sandbox/win/src/handle_inheritance_test.cc:23: base::ScopedTempDir
temp_directory;
I went ahead and updated this test (which was a prototype for my new test) to
use the scoped helpers as well

https://codereview.chromium.org/868253011/diff/80001/sandbox/win/src/handle_i...
sandbox/win/src/handle_inheritance_test.cc:35:
EXPECT_TRUE(tmp_handle.IsValid());
I'm pretty sure all these EXPECT_'s should actually be ASSERTs per
https://code.google.com/p/googletest/wiki/V1_7_Primer#Assertions

Looks like the reason why these were EXPECT_'s in the first place is that you
wanted to always delete the file at the end?
Anyways, I went ahead and just fixed that.

https://codereview.chromium.org/868253011/diff/100001/sandbox/win/src/handle_...
File sandbox/win/src/handle_inheritance_test.cc (right):

https://codereview.chromium.org/868253011/diff/100001/sandbox/win/src/handle_...
sandbox/win/src/handle_inheritance_test.cc:35:
ASSERT_TRUE(tmp_handle.IsValid());
Forgot to fix this one in PS5, fixed in PS6.

[Timur Iskhodzhanov, 2015-02-05 08:16:08]

The CQ bit was checked by timurrrr@chromium.org

[commit-bot: I haz the power, 2015-02-05 08:17:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/868253011/100001

[commit-bot: I haz the power, 2015-02-05 09:19:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-05 09:21:57]

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

[Nico, 2015-02-09 15:17:16]

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

[Nico, 2015-02-09 15:17:17]

https://codereview.chromium.org/868253011/diff/100001/sandbox/win/src/address...
File sandbox/win/src/address_sanitizer_test.cc (right):

https://codereview.chromium.org/868253011/diff/100001/sandbox/win/src/address...
sandbox/win/src/address_sanitizer_test.cc:97: ASSERT_TRUE(strstr(data.c_str(),
strrchr(__FILE__, '\\')))
This test isn't passing on the bots:
http://build.chromium.org/p/chromium.fyi/builders/CrWinAsan%20tester
http://build.chromium.org/p/chromium.fyi/builders/CrWinAsan%28dll%29%20tester...

[Timur Iskhodzhanov, 2015-02-09 20:45:29]

That's https://code.google.com/p/chromium/issues/detail?id=418090

[Timur Iskhodzhanov, 2015-02-09 20:46:07]

i.e. not a bug in the test, but rather a misconfiguration of the builder+tester
setup