Bug 216746 - Enable AddressSanitizer in C++ std library templates
Summary: Enable AddressSanitizer in C++ std library templates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on: 216318
Blocks:
  Show dependency treegraph
 
Reported: 2020-09-20 10:47 PDT by David Kilzer (:ddkilzer)
Modified: 2020-09-21 13:02 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (2.44 KB, patch)
2020-09-20 11:10 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-09-20 10:47:50 PDT
Enable AddressSanitizer in C++ std library templates.

This amounts to undefining the _LIBCPP_HAS_NO_ASAN macro when building.

Although most of WebKit doesn't used C++ std library types like std::vector or std::hash, ANGLE and libwebrtc do use these types, so it's beneficial to enable AddressSanitizer in these cases when compiling with Asan enabled for the rest of WebKit.
Comment 1 David Kilzer (:ddkilzer) 2020-09-20 11:10:18 PDT
Created attachment 409239 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2020-09-20 11:11:25 PDT
(In reply to David Kilzer (:ddkilzer) from comment #0)
> Although most of WebKit doesn't used C++ std library types like std::vector
> or std::hash, ANGLE and libwebrtc do use these types, so it's beneficial to
> enable AddressSanitizer in these cases when compiling with Asan enabled for
> the rest of WebKit.

Actually, I think this is mostly affects std::vector and related classes, not std::hash.
Comment 3 David Kilzer (:ddkilzer) 2020-09-20 11:19:59 PDT
I CCed folks that work on ANGLE and libwebrtc since there is a small chance that enabling this might find some latent issues in those libraries when running layout tests.
Comment 4 Alexey Proskuryakov 2020-09-20 13:03:27 PDT
Comment on attachment 409239 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=409239&action=review

> Tools/sanitizer/asan.xcconfig:9
> +WK_SANITIZER_OTHER_CPLUSPLUSFLAGS_YES = $(inherited) -U_LIBCPP_HAS_NO_ASAN;

Nice. Did you run tests locally to confirm that nothing terrible was going to happen? EWS doesn't have ASan.

> Tools/sanitizer/sanitizer.xcconfig:7
>  WK_SANITIZER_OTHER_CFLAGS_YES = -fno-omit-frame-pointer -g;
> +WK_SANITIZER_OTHER_CPLUSPLUSFLAGS_YES = ;

Does "-fno-omit-frame-pointer -g" get added for C++ in some other way, or do we not need it?
Comment 5 David Kilzer (:ddkilzer) 2020-09-20 20:45:48 PDT
Comment on attachment 409239 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=409239&action=review

>> Tools/sanitizer/asan.xcconfig:9
>> +WK_SANITIZER_OTHER_CPLUSPLUSFLAGS_YES = $(inherited) -U_LIBCPP_HAS_NO_ASAN;
> 
> Nice. Did you run tests locally to confirm that nothing terrible was going to happen? EWS doesn't have ASan.

Yes.  No new ASan-related crashes in any std C++ objects.

>> Tools/sanitizer/sanitizer.xcconfig:7
>> +WK_SANITIZER_OTHER_CPLUSPLUSFLAGS_YES = ;
> 
> Does "-fno-omit-frame-pointer -g" get added for C++ in some other way, or do we not need it?

As noted in the ChangeLog, OTHER_CFLAGS applies to C++ sources, so it doesn't need to be listed twice.  I verified this by reviewing build log output.
Comment 6 Alexey Proskuryakov 2020-09-21 12:01:20 PDT
> As noted in the ChangeLog, OTHER_CFLAGS applies to C++ sources, so it doesn't need to be listed twice.

My understanding was that OTHER_CFLAGS was the default value for OTHER_CPLUSPLUSFLAGS, but it had no effect when OTHER_CPLUSPLUSFLAGS was defined.

> I verified this by reviewing build log output.

Surprising.
Comment 7 Radar WebKit Bug Importer 2020-09-21 12:25:34 PDT
<rdar://problem/69319755>
Comment 8 EWS 2020-09-21 13:02:29 PDT
Committed r267358: <https://trac.webkit.org/changeset/267358>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409239 [details].