Bug 176131 – Compile WebKit with UBSan

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

UbSan is enabled in XCode 9.  Add the ability to build WebKit with UbSan.

[Radar WebKit Bug Importer]

<rdar://problem/34174018>

[David Kilzer (:ddkilzer)]

UBSan is the clang Undefined Behavior Sanitizer.

[David Kilzer (:ddkilzer)]

FYI, here is a list of UBSan checkers:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

[David Kilzer (:ddkilzer)]

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)

[David Kilzer (:ddkilzer)]

Building through the WebKit project.  Next step is to fix linker errors for WebKit.framework.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[Don Olmstead]

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.

[Darin Adler]

Tell us more about the class exports and how UBSan uncovers the need for them?

[Darin Adler]

Is this related to our use of compiler switches that disable type info?

[Darin Adler]

-fno-rtti maybe

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(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.)

[David Kilzer (:ddkilzer)]

(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.)

[David Kilzer (:ddkilzer)]

Created attachment 424475 [details]
Patch v2

[David Kilzer (:ddkilzer)]

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

[Alexey Proskuryakov]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 424502 [details]
Patch for landing

[EWS]

Committed r275150: <https://commits.webkit.org/r275150>

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