Add some function and URLs to induce ASan crashes.

Add some function to induce ASan crashes.

This will allows to induce ASan crashes in the browser and in the renderer processes, this is really useful when trying to debug an ASan crash (to make sure that the build is really instrumented).

This code add the following crash urls to chrome:

chrome://crash/browser-heap-overflow
chrome://crash/browser-heap-underflow
chrome://crash/browser-use-after-free
chrome://crash/browser-corrupt-heap-block
chrome://crash/browser-corrupt-heap

Those URLs induce a crash in the browser process, while those:

chrome://crash/heap-overflow
chrome://crash/heap-underflow
chrome://crash/use-after-free
chrome://crash/corrupt-heap-block
chrome://crash/corrupt-heap

induce a crash in the renderer process.

We need this because as these process use a different DLL (chrome.dll vs
chrome_child.dll) one of them could be ASan-instrumented while the other one
isn't... So the current code in renderer/ is useless for the browser-only
instrumented builds...


BUG=

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

[Sébastien Marchand, 2014-05-28 22:56:04]

Hey Reid,

any idea why the code in base/memory/asan_invalid_access.cc doesn't trigger an
ASan error on Linux ? I've tried it with the following commands:

GYP_DEFINES='asan=1' gclient runhooks
ninja -C out/Release base_unittests --gtest_filter=AsanInvalidAccess.*

And I get the following result:

[ RUN      ] AsanInvalidAccess.AsanHeapOverflow

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:825:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test couldn't detect the number of threads.
../../base/memory/asan_invalid_access_unittest.cc:37: Failure
Death test: AsanHeapOverflow()
    Result: failed to die.
 Error msg:
[  DEATH   ]
[  FAILED  ] AsanInvalidAccess.AsanHeapOverflow (3 ms)
[ RUN      ] AsanInvalidAccess.AsanHeapUnderflow

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:825:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test couldn't detect the number of threads.
../../base/memory/asan_invalid_access_unittest.cc:48: Failure
Death test: AsanHeapUnderflow()
    Result: failed to die.
 Error msg:
[  DEATH   ]
[  FAILED  ] AsanInvalidAccess.AsanHeapUnderflow (2 ms)
[ RUN      ] AsanInvalidAccess.AsanHeapUseAfterFree

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:825:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test couldn't detect the number of threads.
[       OK ] AsanInvalidAccess.AsanHeapUseAfterFree (16 ms)
[----------] 3 tests from AsanInvalidAccess (21 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (21 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] AsanInvalidAccess.AsanHeapOverflow
[  FAILED  ] AsanInvalidAccess.AsanHeapUnderflow



The UAF bug pass, but the over/underflow ones fail... Don't worry too much about
the rest of the code, I know that there's already some test functions in
tools_sanity_unittest.cc but I'd like to expose some of the crash functions (the
ones that I've defined in base/memory/asan_invalid_access.*) to use them to
induce an ASan crash in chrome by accessing a specific URL in an instrumented
build. (its already possible to induce an ASan crash in the renderer, but I'd
like to also be able to do this for the browser)

[Reid Kleckner, 2014-05-29 00:34:41]

+timur

My theory is that ASan disables its checks after fork.  What happens if you use
--gtest_death_test_style=threadsafe?

[Timur Iskhodzhanov, 2014-05-29 10:23:21]

glider@ is likely to know

[Alexander Potapenko, 2014-05-29 12:14:22]

On 2014/05/29 00:34:41, Reid Kleckner wrote:
> +timur
> 
> My theory is that ASan disables its checks after fork.  What happens if you
use
> --gtest_death_test_style=threadsafe?

ASan doesn't disable anything after fork. When the process is forked the
instrumentation is still there, so it makes sense to retain the shadow memory
layout (which is the easiest thing to do)

[Alexander Potapenko, 2014-05-29 12:16:49]

On 2014/05/29 12:14:22, Alexander Potapenko wrote:
> On 2014/05/29 00:34:41, Reid Kleckner wrote:
> > +timur
> > 
> > My theory is that ASan disables its checks after fork.  What happens if you
> use
> > --gtest_death_test_style=threadsafe?
> 
> ASan doesn't disable anything after fork. When the process is forked the
> instrumentation is still there, so it makes sense to retain the shadow memory
> layout (which is the easiest thing to do)

Sébastien, have you tried looking at the assembly?
Clang may be simply optimizing your dummy arrays out.

[Alexander Potapenko, 2014-05-29 12:24:58]

On 2014/05/29 12:16:49, Alexander Potapenko wrote:
> On 2014/05/29 12:14:22, Alexander Potapenko wrote:
> > On 2014/05/29 00:34:41, Reid Kleckner wrote:
> > > +timur
> > > 
> > > My theory is that ASan disables its checks after fork.  What happens if
you
> > use
> > > --gtest_death_test_style=threadsafe?
> > 
> > ASan doesn't disable anything after fork. When the process is forked the
> > instrumentation is still there, so it makes sense to retain the shadow
memory
> > layout (which is the easiest thing to do)
> 
> Sébastien, have you tried looking at the assembly?
> Clang may be simply optimizing your dummy arrays out.

By the way, there's base/tools_sanity_unittest.cc, which you may want to modify
instead of adding your own tests.

[Sébastien Marchand, 2014-05-29 19:30:19]

So it turns out that the base::Alias(...) wasn't enough on Linux to prevent the
variable from being optimized out, I've switched to using the volatile attribute
in the variable declaration and it works fine.

Yeah I'll move those tests to tools_sanity_unittest.cc thanks.

[Sébastien Marchand, 2014-06-04 20:44:39]

Can one of you take care of reviewing the changes in tools_sanity_unittest ? And
maybe the new asan_invalid_access.* files...

[Timur Iskhodzhanov, 2014-06-05 15:32:08]

See the comments for the base/ part.

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:19: // when we free it or when another crash
happen (if |induce_crash| is set to
happens?

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:46: //     order to trigger an Address
Sanitizer (ASAN) error report.
ditto

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.h (right):

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:6: // order to trigger an Address Sanitizer
(ASAN) error report.
nit: AddressSanitizer (ASan)

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:30: #if defined(SYZYASAN) &&
defined(COMPILER_MSVC)
Why do you need COMPILER_MSVC here?

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:207: HARMFUL_ACCESS(AsanHeapOverflow(),
"AsanRuntime::OnError")
can you put the OnError stuff into the HARMFUL_ACCESS macro instead?

[Timur Iskhodzhanov, 2014-06-05 15:32:58]

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:46: //     order to trigger an Address
Sanitizer (ASAN) error report.
On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> ditto

that was about
AddressSanitizer (ASan)

[Sébastien Marchand, 2014-06-05 19:44:34]

Thanks for the review Timur, PTanL. (I'm looking for other reviewers for the
other files).

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:19: // when we free it or when another crash
happen (if |induce_crash| is set to
On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> happens?

Done. Sorry for the grammatical nits and thanks for pointing them out !

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:46: //     order to trigger an Address
Sanitizer (ASAN) error report.
On 2014/06/05 15:32:59, Timur Iskhodzhanov wrote:
> On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> > ditto
> 
> that was about
> AddressSanitizer (ASan)

Done.

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:46: //     order to trigger an Address
Sanitizer (ASAN) error report.
On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> ditto

Done.

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.h (right):

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:6: // order to trigger an Address Sanitizer
(ASAN) error report.
On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> nit: AddressSanitizer (ASan)

Done.

https://codereview.chromium.org/306753003/diff/150001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:30: #if defined(SYZYASAN) &&
defined(COMPILER_MSVC)
On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> Why do you need COMPILER_MSVC here?

It's not needed anymore... I was using __try/__except and RaiseException in the
implementation of those function initially, which isn't supported when we build
with clang on Windows (for nacl).  I've switched to using try/catch and
CHECK(false).

Note that there's still no reason to compile this with clang on Windows... but
there's nothing preventing it.

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:207: HARMFUL_ACCESS(AsanHeapOverflow(),
"AsanRuntime::OnError")
On 2014/06/05 15:32:09, Timur Iskhodzhanov wrote:
> can you put the OnError stuff into the HARMFUL_ACCESS macro instead?

Nop, mostly because of ToolsSanityTest.AsanCorruptHeap as in this case this
frame won't be in the stack trace...

[Sébastien Marchand, 2014-06-05 19:54:45]

+nasko@ for content/browser/frame_host/debug_urls.cc
+jamesr@ for content/renderer/render_frame_impl.cc
+thakis@ for owner approval for the files in base/

PTAL.

[Sébastien Marchand, 2014-06-05 19:55:13]

+thakis@ for real this time.

[jamesr, 2014-06-05 20:01:35]

lgtm

[Timur Iskhodzhanov, 2014-06-06 11:36:56]

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:207: HARMFUL_ACCESS(AsanHeapOverflow(),
"AsanRuntime::OnError")
:(
Maybe we can handle only that test differently (especially since it's
SyzyASan-only) rather than all of the tests?

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.cc (right):

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:48: //     order to trigger an Address
Sanitizer (ASan) error report.
ditto

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.h (right):

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:6: // order to trigger an Address Sanitizer
(ASan) error report.
and again, AddressSanitizer

[nasko, 2014-06-06 17:08:46]

lgtm

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.h (right):

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
nit: no "(c)"

[Sébastien Marchand, 2014-06-09 14:47:39]

Thanks.

Timur, PTAnL.

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
File base/tools_sanity_unittest.cc (right):

https://codereview.chromium.org/306753003/diff/150001/base/tools_sanity_unitt...
base/tools_sanity_unittest.cc:207: HARMFUL_ACCESS(AsanHeapOverflow(),
"AsanRuntime::OnError")
On 2014/06/06 11:36:56, Timur Iskhodzhanov wrote:
> :(
> Maybe we can handle only that test differently (especially since it's
> SyzyASan-only) rather than all of the tests?

Done.

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.cc (right):

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
base/debug/asan_invalid_access.cc:48: //     order to trigger an Address
Sanitizer (ASan) error report.
On 2014/06/06 11:36:56, Timur Iskhodzhanov wrote:
> ditto

Done.

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
File base/debug/asan_invalid_access.h (right):

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
On 2014/06/06 17:08:46, nasko wrote:
> nit: no "(c)"

Done.

https://codereview.chromium.org/306753003/diff/230001/base/debug/asan_invalid...
base/debug/asan_invalid_access.h:6: // order to trigger an Address Sanitizer
(ASan) error report.
On 2014/06/06 11:36:56, Timur Iskhodzhanov wrote:
> and again, AddressSanitizer

Oh, sorry I missed this one.

[Timur Iskhodzhanov, 2014-06-09 16:08:22]

base/ part LGTM, but I'm not an OWNER.

[Sébastien Marchand, 2014-06-09 20:05:30]

+brettw@ for base/ lgtm.

[Nico, 2014-06-12 18:13:18]

Does this need to be in base? It looks like there's only one client, could the
code be just there? It doesn't seem like code that will be called from many
places (?)

[Sébastien Marchand, 2014-06-12 18:20:47]

Well this code is called from content/{browser|renderer} and from
base/tools_sanity_unittest.cc . If you prefer I can move these new files (the
ones in base/debug/) to content/ and create a new
content/.../asan_invalid_access_unittests unittests, but this'll mean that there
will be some redundancy between base/tools_sanity_unittest.cc and this new test
file. (I could probably also move tools_sanity_unittest.cc somewhere else but I
don't own this file so I don't know if it's in base for any particular reason...

I put it in base because base/debug seemed like an appropriate place (there's
already a bunch of sanitizer/leak/profiler... files there).

[Nico, 2014-06-12 18:36:56]

After looking at the rest of the CL, I'm pretty confused by it. It looks like
there was code for something like this in renderer/ already?

From what I can tell, this uses the urls whateverscheme://crash/foobar to do
things instead of chrome:// urls?

https://codereview.chromium.org/306753003/diff/250001/content/browser/frame_h...
File content/browser/frame_host/debug_urls.cc (right):

https://codereview.chromium.org/306753003/diff/250001/content/browser/frame_h...
content/browser/frame_host/debug_urls.cc:25: static const char
kAsanCrashDomain[] = "crash";
(const already has implicit internal linkage, and this is also in an unnamed
namespace already, so this has full n+2 redundancy for internal linkage)

https://codereview.chromium.org/306753003/diff/250001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/306753003/diff/250001/content/renderer/render...
content/renderer/render_frame_impl.cc:238: NOINLINE static void
CorruptMemoryBlock() {
I suppose this isn't used any longer?

[Sébastien Marchand, 2014-06-12 19:54:13]

This code add the following crash urls to chrome:

chrome://crash/browser-heap-overflow
chrome://crash/browser-heap-underflow
chrome://crash/browser-use-after-free
chrome://crash/browser-corrupt-heap-block
chrome://crash/browser-corrupt-heap

Those URLs induce a crash in the browser process, while those:

chrome://crash/heap-overflow
chrome://crash/heap-underflow
chrome://crash/use-after-free
chrome://crash/corrupt-heap-block
chrome://crash/corrupt-heap

induce a crash in the renderer process.

We need this because as these process use a different DLL (chrome.dll vs
chrome_child.dll) one of them could be ASan-instrumented while the other one
isn't... So the current code in renderer/ is useless for the browser-only
instrumented builds...

https://codereview.chromium.org/306753003/diff/250001/content/browser/frame_h...
File content/browser/frame_host/debug_urls.cc (right):

https://codereview.chromium.org/306753003/diff/250001/content/browser/frame_h...
content/browser/frame_host/debug_urls.cc:25: static const char
kAsanCrashDomain[] = "crash";
On 2014/06/12 18:36:55, Nico (away) wrote:
> (const already has implicit internal linkage, and this is also in an unnamed
> namespace already, so this has full n+2 redundancy for internal linkage)

Done.

https://codereview.chromium.org/306753003/diff/250001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/306753003/diff/250001/content/renderer/render...
content/renderer/render_frame_impl.cc:238: NOINLINE static void
CorruptMemoryBlock() {
On 2014/06/12 18:36:56, Nico (away) wrote:
> I suppose this isn't used any longer?

Good catch, thanks.

[Nico, 2014-06-12 21:10:44]

The new CL description is much easier for me to understand, thanks!

Sorry, I can't find the check for the chrome:// scheme :-( Can you point me to
it?

[Sébastien Marchand, 2014-06-12 22:05:41]

Here's the one in the renderer:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

As for the URLs in the browser I've been able to confirm that it only crash with
the chrome:// scheme, but I can't find where's the actual check... I've added a
check anyway.

[Nico, 2014-06-13 18:35:48]

lgtm

[Sébastien Marchand, 2014-06-13 18:36:38]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-06-13 18:38:08]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/306753003/310001

[commit-bot: I haz the power, 2014-06-13 22:48:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-13 22:51:06]

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

[commit-bot: I haz the power, 2014-06-13 22:51:08]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Sébastien Marchand, 2014-06-14 03:51:11]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-06-14 03:53:33]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/306753003/310001

[commit-bot: I haz the power, 2014-06-14 08:29:47]

Change committed as 277201