Disable the ASan SEGV handler for NaCl processes on Mac.

Disable the ASan SEGV handler for NaCl processes on Mac.

Implement __asan_default_options() for the Chrome OSX binary. This function
sets handle_segv=0 iff the binary is executed with --type=nacl-loader.

BUG=323510
TEST=browser_tests pass under ASan on OSX x64
R=cpu@chromium.org, mseaborn@chromium.org

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

[Alexander Potapenko, 2013-11-29 11:29:12]

Please take a look.

[Mark Seaborn, 2013-11-29 16:50:35]

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
File chrome/app/chrome_exe_main_mac.cc (right):

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:12: // NaCl doesn't play well with ASan's
signal handler, so we need to add
You mean ASan doesn't play well with NaCl's signal handler. :-)  Is there
anything NaCl could do to coexist with ASan's signal handler, other than set
handle_segv=0?

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:19: static const char kNaclFlag[] =
"--type=nacl-loader";
"Nacl" -> "NaCl"

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:22: char ***_NSGetArgv();
You can get this decl from crt_externs.h

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:27: __attribute__((used))
Why do you need this?  I thought this is only used for suppressing warnings on
static functions.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:30: char ***argvp = _NSGetArgv();
Nit: Use "*** " spacing (Chromium style)

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:35: for (int i = 0; argv[i]; ++i) {
Should you use _NSGetArgc() rather than assuming the array is NULL-terminated?

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:36: if (strncmp(argv[i], kNaclFlag,
kNaclFlagLen + 1) == 0) {
Why not "strcmp(argv[i], kNaClFlag)"?  You don't need to do a prefix match for
"--type=nacl-loader".

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:37: is_nacl = true;
You could just do "return kNaClDefaultOptions" here.

[cpu_(ooo_6.6-7.5), 2013-11-29 18:57:04]

rubberstamp lgtm once Mark is happy.

[Alexander Potapenko, 2013-12-02 15:50:19]

Addressed the comments. Please take another look.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
File chrome/app/chrome_exe_main_mac.cc (right):

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:12: // NaCl doesn't play well with ASan's
signal handler, so we need to add
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> You mean ASan doesn't play well with NaCl's signal handler. :-)  Is there
> anything NaCl could do to coexist with ASan's signal handler, other than set
> handle_segv=0?

I think we've discussed signal handler chaining some time ago, but nothing has
been done to support that.
Fixed the comment.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:19: static const char kNaclFlag[] =
"--type=nacl-loader";
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> "Nacl" -> "NaCl"

Done.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:22: char ***_NSGetArgv();
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> You can get this decl from crt_externs.h

Done. Let me know if you prefer not having #ifdef around this #include to make
the code shorter.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:27: __attribute__((used))
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> Why do you need this?  I thought this is only used for suppressing warnings on
> static functions.
Because this function is only referenced from the dynamic ASan runtime, and ld
may delete it as dead code.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:30: char ***argvp = _NSGetArgv();
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> Nit: Use "*** " spacing (Chromium style)

Done.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:35: for (int i = 0; argv[i]; ++i) {
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> Should you use _NSGetArgc() rather than assuming the array is NULL-terminated?

I think the system somewhat guarantees that, but _NSGetArgc() is indeed cleaner.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:36: if (strncmp(argv[i], kNaclFlag,
kNaclFlagLen + 1) == 0) {
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> Why not "strcmp(argv[i], kNaClFlag)"?  You don't need to do a prefix match for
> "--type=nacl-loader".

Done.

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:37: is_nacl = true;
On 2013/11/29 16:50:36, Mark Seaborn wrote:
> You could just do "return kNaClDefaultOptions" here.

True.

[Mark Seaborn, 2013-12-02 16:51:32]

LGTM

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
File chrome/app/chrome_exe_main_mac.cc (right):

https://codereview.chromium.org/96683003/diff/1/chrome/app/chrome_exe_main_ma...
chrome/app/chrome_exe_main_mac.cc:27: __attribute__((used))
On 2013/12/02 15:50:19, Alexander Potapenko wrote:
> On 2013/11/29 16:50:36, Mark Seaborn wrote:
> > Why do you need this?  I thought this is only used for suppressing warnings
on
> > static functions.
> Because this function is only referenced from the dynamic ASan runtime, and ld
> may delete it as dead code.

Ah, with Clang, __attribute__((used)) adds the function to @llvm.used, so it can
affect linking (unlike with GCC).  I didn't realise that before.

https://codereview.chromium.org/96683003/diff/20001/chrome/app/chrome_exe_mai...
File chrome/app/chrome_exe_main_mac.cc (right):

https://codereview.chromium.org/96683003/diff/20001/chrome/app/chrome_exe_mai...
chrome/app/chrome_exe_main_mac.cc:38: const int kNaClFlagLen =
strlen(kNaClFlag);
This isn't used

[Alexander Potapenko, 2013-12-03 09:40:38]

Fixed the comments.

Note on making ASan and NaCl signal handlers work together: this is actually a
low-priority issue since the ASan SEGV handler is only used to handle trivial
crashes. Having another handler report them doesn't affect ASan's capability to
report other types of memory corruptions.

https://codereview.chromium.org/96683003/diff/20001/chrome/app/chrome_exe_mai...
File chrome/app/chrome_exe_main_mac.cc (right):

https://codereview.chromium.org/96683003/diff/20001/chrome/app/chrome_exe_mai...
chrome/app/chrome_exe_main_mac.cc:38: const int kNaClFlagLen =
strlen(kNaClFlag);
On 2013/12/02 16:51:32, Mark Seaborn wrote:
> This isn't used

Done.

[Alexander Potapenko, 2013-12-03 09:41:06]

Committed patchset #3 manually as r238328 (presubmit successful).