Redefine the ASan options for nacl_helper so that ASan does not handle SIGSEGV/SIGBUS

Redefine the ASan options for nacl_helper so that ASan does not handle SIGSEGV/SIGBUS
This should allow the platform qualification test to pass and make it possible to launch NaCl programs from an instrumented browser without additional env settings.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152314

[Alexander Potapenko, 2012-07-25 15:32:28]

Mark, Brad, what do you think?
This is better than https://chromiumcodereview.appspot.com/10095012/ since we do
not introduce a runtime dependency on ASan into NaCl.

[Brad Chen (chromium), 2012-07-26 20:04:58]

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:201: #if defined(ADDRESS_SANITIZER)
I don't understand how this will change the behavior of the ASAN version of
Chrome. Apart from making the PQ test pass, does it have any further impact in
terms of feature/function? Am I reading this code correctly that it will change
behavior of ASAN but not of NaCl?

[Alexander Potapenko, 2012-07-27 08:42:54]

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:201: #if defined(ADDRESS_SANITIZER)
On 2012/07/26 20:04:58, Brad Chen (chromium) wrote:
> I don't understand how this will change the behavior of the ASAN version of
> Chrome. Apart from making the PQ test pass, does it have any further impact in
> terms of feature/function? Am I reading this code correctly that it will
change
> behavior of ASAN but not of NaCl? 

This will prevent ASan from installing its signal handler, which shall allow
NaCl to install that of his own. This means that every crash that happens e.g.
because of an access violation (but NOT an OOB read or other bug caught by ASan)
will result in executing NaCl SEGV handler.
Now can you please tell whether this is fine or not to use your SEGV handler?
Will it report a crash message? Will it send a crash to the crash server? Is it
more secure than the handler installed by ASan (I'm assuming yes)?

[Mark Seaborn, 2012-07-27 17:12:02]

LGTM

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:204: const char *kAsanDefaultOptionsNaCl =
"handle_segv=0";
The Chromium style is
  const char kConstant[] = "blah";
like the constants earlier in the file.

Also, this can be static, right?

[Mark Seaborn, 2012-07-27 17:21:03]

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:201: #if defined(ADDRESS_SANITIZER)
On 2012/07/26 20:04:58, Brad Chen (chromium) wrote:
> I don't understand how this will change the behavior of the ASAN version of
> Chrome. Apart from making the PQ test pass, does it have any further impact in
> terms of feature/function?

It has two other effects:

 1) It will allow NaCl's fault handling to work under Asan (untrusted exception
handling, debug stub, and in the future, Breakpad).
 2) If Asan catches a memory error in NaCl on Linux, we won't get a helpful
message for the location of the error (AFAIK).

We could fix (2) as I commented in
https://chromiumcodereview.appspot.com/10095012/.  BTW, Alexander, I was not
really sure why you dropped that change.

[Alexander Potapenko, 2012-07-27 18:47:33]

> It has two other effects:
> 
>  1) It will allow NaCl's fault handling to work under Asan (untrusted
exception
> handling, debug stub, and in the future, Breakpad).
This is true.
>  2) If Asan catches a memory error in NaCl on Linux, we won't get a helpful
> message for the location of the error (AFAIK).
Are you speaking about the trusted or untrusted part?
If we instrument trusted code, we probably can catch the OOB accesses and
use-after-free errors as long as they do not cause a segfault (they usually
should not). We can't instrument untrusted code at all, so this is really true
for it.
> We could fix (2) as I commented in
> https://chromiumcodereview.appspot.com/10095012/.  BTW, Alexander, I was not
> really sure why you dropped that change.
Do you mean signal handler chaining? Yes, this is a perfect solution, but it's
kind of orthogonal to what I'm doing now (making a fully functional browser run
under ASan).
I've dropped that change because after some discussion in the ASan team we've
decided not to add a hook called by NaCl for disabling the ASan signal handler
to the interface, but instead make ASan ask the client programs for the
additional options.
Effectively we're still just disabling our signal handler for all NaCl
processes.

[Alexander Potapenko, 2012-07-27 18:51:23]

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux.cc
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/10830009/diff/1/chrome/nacl/nacl_helper_linux....
chrome/nacl/nacl_helper_linux.cc:204: const char *kAsanDefaultOptionsNaCl =
"handle_segv=0";
On 2012/07/27 17:12:02, Mark Seaborn wrote:
> The Chromium style is
>   const char kConstant[] = "blah";
> like the constants earlier in the file.
> 
> Also, this can be static, right?

Done.

[Alexander Potapenko, 2012-07-31 17:38:46]

Brad, the today's Clang roll will bring the support for
__asan_default_options(), so I'll be able to land this change if you don't
object.

[Brad Chen, 2012-07-31 19:42:49]

LGTM

Yes please go ahead when asan support is ready.

[Alexander Potapenko, 2012-08-20 08:45:57]

On 2012/07/31 19:42:49, Brad Chen wrote:
> LGTM
> 
> Yes please go ahead when asan support is ready.

FYI, I'm about to land this now.

[commit-bot: I haz the power, 2012-08-20 08:46:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glider@chromium.org/10830009/4002

[commit-bot: I haz the power, 2012-08-20 11:20:33]

Try job failure for 10830009-4002 (retry) on win_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Alexander Potapenko, 2012-08-20 11:22:23]

On 2012/08/20 11:20:33, I haz the power (commit-bot) wrote:
> Try job failure for 10830009-4002 (retry) on win_rel for step
> "interactive_ui_tests".
> It's a second try, previously, step "interactive_ui_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Committing the CL with --no_presubmit.
The win_rel failures are obviously unrelated.