Bug 176131 - Compile WebKit with UBSan
Summary: Compile WebKit with UBSan
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 224009 224343
  Show dependency treegraph
 
Reported: 2017-08-30 15:30 PDT by Jonathan Bedard
Modified: 2021-04-08 15:26 PDT (History)
32 users (show)

See Also:


Attachments
Summary of UBSan issues running layout tests on macOS Release build (157.01 KB, text/plain)
2021-03-21 12:47 PDT, David Kilzer (:ddkilzer)
no flags Details
WIP Patch v1 (33.80 KB, patch)
2021-03-21 12:57 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (15.38 KB, patch)
2021-03-27 16:28 PDT, David Kilzer (:ddkilzer)
ap: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (13.57 KB, patch)
2021-03-28 10:34 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 Jonathan Bedard 2017-08-30 15:30:58 PDT
UbSan is enabled in XCode 9.  Add the ability to build WebKit with UbSan.
Comment 1 Radar WebKit Bug Importer 2017-08-30 15:31:46 PDT
<rdar://problem/34174018>
Comment 2 David Kilzer (:ddkilzer) 2021-03-11 08:14:53 PST
UBSan is the clang Undefined Behavior Sanitizer.
Comment 3 David Kilzer (:ddkilzer) 2021-03-11 08:58:36 PST
FYI, here is a list of UBSan checkers:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Comment 4 David Kilzer (:ddkilzer) 2021-03-14 12:01:47 PDT
These are the kinds of build errors I'm working through after enabling UBSan:

Undefined symbols for architecture x86_64:
  "typeinfo for WebCore::TimerBase", referenced from:
      typeinfo for WebCore::Timer in WebFakeXRDevice.o
  "typeinfo for webrtc::SessionDescriptionInterface", referenced from:
      typeinfo for WebCore::MockLibWebRTCSessionDescription in MockLibWebRTCPeerConnection.o
  "typeinfo for webrtc::RtpTransceiverInterface", referenced from:
      typeinfo for WebCore::MockRtpTransceiver in MockLibWebRTCPeerConnection.o
  "typeinfo for webrtc::RtpReceiverInterface", referenced from:
      typeinfo for WebCore::MockRtpReceiver in MockLibWebRTCPeerConnection.o
  "typeinfo for webrtc::MediaStream", referenced from:
      typeinfo for rtc::RefCountedObject<webrtc::MediaStream> in MockLibWebRTCPeerConnection.o
  "typeinfo for webrtc::RtpSenderInterface", referenced from:
      typeinfo for WebCore::MockRtpSender in MockLibWebRTCPeerConnection.o
  "typeinfo for WebCore::CDMPrivate", referenced from:
      typeinfo for WebCore::MockCDM in MockCDMFactory.o
  "typeinfo for webrtc::AudioTrackInterface", referenced from:
      typeinfo for WebCore::MockLibWebRTCAudioTrack in MockLibWebRTCPeerConnection.o
  "typeinfo for WebCore::ContextDestructionObserver", referenced from:
      typeinfo for WebCore::Internals in Internals-227951BA24B9AE61.o
      typeinfo for WebCore::AbortSignal in Internals-227951BA24B9AE61.o
  "typeinfo for webrtc::VideoTrackInterface", referenced from:
      typeinfo for WebCore::MockLibWebRTCVideoTrack in MockLibWebRTCPeerConnection.o
  "typeinfo for WebCore::EventTarget", referenced from:
      typeinfo for WebCore::EventTargetWithInlineData in Internals-227951BA24B9AE61.o
  "typeinfo for webrtc::IceCandidateInterface", referenced from:
      typeinfo for WebCore::MockLibWebRTCIceCandidate in MockLibWebRTCPeerConnection.o
  "typeinfo for WebCore::ApplePaySetupFeature", referenced from:
      typeinfo for WebCore::MockApplePaySetupFeature in MockApplePaySetupFeature.o
  "typeinfo for webrtc::DataChannelInterface", referenced from:
      typeinfo for WebCore::MockLibWebRTCDataChannel in MockLibWebRTCPeerConnection.o
  "typeinfo for webrtc::PeerConnectionInterface", referenced from:
      typeinfo for WebCore::MockLibWebRTCPeerConnection in MockLibWebRTCPeerConnection.o
  "typeinfo for WebCore::ActiveDOMCallback", referenced from:
      typeinfo for WebCore::XRSimulateUserActivationFunction in JSXRSimulateUserActivationFunction.o
  "typeinfo for WebCore::InspectorFrontendClientLocal", referenced from:
      typeinfo for WebCore::InspectorStubFrontend in Internals-227951BA24B9AE61.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Comment 5 David Kilzer (:ddkilzer) 2021-03-14 20:05:44 PDT
Building through the WebKit project.  Next step is to fix linker errors for WebKit.framework.
Comment 6 David Kilzer (:ddkilzer) 2021-03-21 12:47:10 PDT
Created attachment 423833 [details]
Summary of UBSan issues running layout tests on macOS Release build

Got WebKit building.  Ran macOS layout tests with Release+UBSan build:

$ ./Tools/Scripts/run-webkit-tests --release --child-processes=2 --no-sample-on-timeout --time-out-ms=60000

Then I created a summary of issues found:

$ SOURCE_PATH=/abs/source/repo/path/; BUILD_PATH=/abs/build/path/; for F in `find ${BUILD_PATH}Release/layout-test-results/ -name \*-stderr.txt`; do grep -h 'runtime error: ' $F; done | sed -e "s#${SOURCE_PATH}##" -e "s#${BUILD_PATH}##" -e 's/0x[0-9a-f][0-9a-f]*/0xnnnnnnnnn/ig' -e 's/load of value [0-9][0-9]*,/load of value nnn,/g' | sort | uniq -c | sort -rn | tee ubsan-issues.txt
Comment 7 David Kilzer (:ddkilzer) 2021-03-21 12:57:05 PDT
Created attachment 423834 [details]
WIP Patch v1

This is a WIP patch that gets macOS Release+UBSan builds working.

Looking for for feedback on the approach:
- Is adding `class EXPORT_MACRO ClassName` okay?
  - Do I need a CLASS_EXPORT_MACRO for each project that only exports the class if compiling with UBSan enabled?
- For libwebrtic, I updated the *.exp files.  (I know the entrees aren't alphabetical.  Will fix.)
  - Do we need to exclude type info exports for libwebrtc by pre-processing the export files or merging from a separate file if UBSan is enabled?
- Are the default UBSan checkers okay, or do we need to turn some off (too chatty) or on (not enabled by default)?
  - See also attachment #423833 [details] ubsan-issues.txt.
Comment 8 Don Olmstead 2021-03-22 09:04:59 PDT
Comment on attachment 423834 [details]
WIP Patch v1

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

Great to see someone working on this on the Apple side!

> Source/WebCore/page/VisitedLinkStore.h:40
> -class VisitedLinkStore : public RefCounted<VisitedLinkStore> {
> +class WEBCORE_EXPORT VisitedLinkStore : public RefCounted<VisitedLinkStore> {
>  public:
>      WEBCORE_EXPORT VisitedLinkStore();
>      WEBCORE_EXPORT virtual ~VisitedLinkStore();

I am almost positive that clang using __declspec to do exports will not like the EXPORT macro being on the class and then on methods. So this will break clang-cl and our playstation build.
Comment 9 Darin Adler 2021-03-23 10:22:50 PDT
Tell us more about the class exports and how UBSan uncovers the need for them?
Comment 10 Darin Adler 2021-03-23 10:23:23 PDT
Is this related to our use of compiler switches that disable type info?
Comment 11 Darin Adler 2021-03-23 10:23:36 PDT
-fno-rtti maybe
Comment 12 David Kilzer (:ddkilzer) 2021-03-23 19:24:59 PDT
(In reply to Darin Adler from comment #9)
> Tell us more about the class exports and how UBSan uncovers the need for
> them?

(In reply to Darin Adler from comment #10)
> Is this related to our use of compiler switches that disable type info?

(In reply to Darin Adler from comment #11)
> -fno-rtti maybe

UBSan enables a vptr checker (-fsanitize=vptr) by default.  This seems worthwhile to enable when compiling with UBSan, so in Tools/sanitizer/ubsan.xcconfig, I re-enabled RTTI:

GCC_ENABLE_CPP_RTTI=YES;

After doing this, I hit linker errors as described in Comment #4:

>   "typeinfo for WebCore::TimerBase", referenced from:
>       typeinfo for WebCore::Timer in WebFakeXRDevice.o

To fix this, I made changes like this:

-class TimerBase {
+class WEBCORE_EXPORT TimerBase {

That fixed the linker errors, but I'm not sure if it's the best solution.

Options are:
1. Use -fsanitize=no-vptr to disable this check.
2. Determine if the workaround above is okay, or a different fix is possible.
Comment 13 David Kilzer (:ddkilzer) 2021-03-23 19:27:45 PDT
(In reply to Don Olmstead from comment #8)
> Comment on attachment 423834 [details]
> WIP Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423834&action=review
> 
> Great to see someone working on this on the Apple side!
> 
> > Source/WebCore/page/VisitedLinkStore.h:40
> > -class VisitedLinkStore : public RefCounted<VisitedLinkStore> {
> > +class WEBCORE_EXPORT VisitedLinkStore : public RefCounted<VisitedLinkStore> {
> >  public:
> >      WEBCORE_EXPORT VisitedLinkStore();
> >      WEBCORE_EXPORT virtual ~VisitedLinkStore();
> 
> I am almost positive that clang using __declspec to do exports will not like
> the EXPORT macro being on the class and then on methods. So this will break
> clang-cl and our playstation build.

Thanks Don!  Can you check this by just making one of these changes in your source tree and then compiling your port?  I have no way to test this at all.  (Unless the WinCairo/Win ports are hitting this, too, in which case I'll take a look at those bots after figuring out if we even want to enable vptr checks.)
Comment 14 David Kilzer (:ddkilzer) 2021-03-27 16:01:19 PDT
(In reply to David Kilzer (:ddkilzer) from comment #12)
> After doing this, I hit linker errors as described in Comment #4:
> 
> >   "typeinfo for WebCore::TimerBase", referenced from:
> >       typeinfo for WebCore::Timer in WebFakeXRDevice.o
> 
> To fix this, I made changes like this:
> 
> -class TimerBase {
> +class WEBCORE_EXPORT TimerBase {
> 
> That fixed the linker errors, but I'm not sure if it's the best solution.
> 
> Options are:
> 1. Use -fsanitize=no-vptr to disable this check.
> 2. Determine if the workaround above is okay, or a different fix is possible.

I guess I'll just disable the vptr checker to get this landed, then others can look into this later.  (I have no idea how to fix the above linker error other than exporting the entire class.)
Comment 15 David Kilzer (:ddkilzer) 2021-03-27 16:28:51 PDT
Created attachment 424475 [details]
Patch v2
Comment 16 David Kilzer (:ddkilzer) 2021-03-27 16:36:06 PDT
(In reply to David Kilzer (:ddkilzer) from comment #14)
> (In reply to David Kilzer (:ddkilzer) from comment #12)
> > After doing this, I hit linker errors as described in Comment #4:
> > 
> > >   "typeinfo for WebCore::TimerBase", referenced from:
> > >       typeinfo for WebCore::Timer in WebFakeXRDevice.o
> > 
> > To fix this, I made changes like this:
> > 
> > -class TimerBase {
> > +class WEBCORE_EXPORT TimerBase {
> > 
> > That fixed the linker errors, but I'm not sure if it's the best solution.
> > 
> > Options are:
> > 1. Use -fsanitize=no-vptr to disable this check.
> > 2. Determine if the workaround above is okay, or a different fix is possible.
> 
> I guess I'll just disable the vptr checker to get this landed, then others
> can look into this later.  (I have no idea how to fix the above linker error
> other than exporting the entire class.)

FWIW, running layout tests didn't find any issues with the vptr checker to my knowledge, so we're not missing any issues from that set of tests.
Comment 17 Alexey Proskuryakov 2021-03-27 19:00:04 PDT
Comment on attachment 424475 [details]
Patch v2

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

> Source/bmalloc/bmalloc/Environment.cpp:113
> +            if (!strncmp(sanitizerName, ubsanName, sizeof(ubsanName) - 1))
> +                return true;

I know that this function is named isSanitizerEnabled, but do we actually need or want to disable bmalloc for UBSan?

> Tools/sanitizer/ubsan.xcconfig:7
> +WK_ENABLE_SANITIZER = $(ENABLE_UNDEFINED_BEHAVIOR_SANITIZER);

Does this allow for combining ASan and UBSan?

> Tools/sanitizer/ubsan.xcconfig:11
> +WK_SANITIZER_OTHER_CFLAGS_YES = $(inherited) -fno-delete-null-pointer-checks -fno-optimize-sibling-calls -fno-sanitize=vptr;

It would be useful to have explanations of why these are needed: -fno-delete-null-pointer-checks -fno-optimize-sibling-calls.
Comment 18 David Kilzer (:ddkilzer) 2021-03-28 10:34:30 PDT
Comment on attachment 424475 [details]
Patch v2

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

>> Source/bmalloc/bmalloc/Environment.cpp:113
>> +                return true;
> 
> I know that this function is named isSanitizerEnabled, but do we actually need or want to disable bmalloc for UBSan?

Check with others and this isn't needed, so I removed it!

>> Tools/sanitizer/ubsan.xcconfig:7
>> +WK_ENABLE_SANITIZER = $(ENABLE_UNDEFINED_BEHAVIOR_SANITIZER);
> 
> Does this allow for combining ASan and UBSan?

Yes.  The last xcconfig file will "win" when setting the value, and when they're both enabled, it doesn't matter which one is set.

>> Tools/sanitizer/ubsan.xcconfig:11
>> +WK_SANITIZER_OTHER_CFLAGS_YES = $(inherited) -fno-delete-null-pointer-checks -fno-optimize-sibling-calls -fno-sanitize=vptr;
> 
> It would be useful to have explanations of why these are needed: -fno-delete-null-pointer-checks -fno-optimize-sibling-calls.

Added.
Comment 19 David Kilzer (:ddkilzer) 2021-03-28 10:34:45 PDT
Created attachment 424502 [details]
Patch for landing
Comment 20 EWS 2021-03-28 11:32:02 PDT
Committed r275150: <https://commits.webkit.org/r275150>

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