Bug 216746

Summary: Enable AddressSanitizer in C++ std library templates
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Tools / TestsAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, dino, kkinnunen, peng.liu6, ryanhaddad, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=230100
Bug Depends on: 216318    
Bug Blocks:    
Attachments:
Description Flags
Patch v1 none

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].