Bug 188460

Created attachment 346878 [details]
Patch

Adds ENABLE_UNDEFINED_BEHAVIOR_SANITIZER flag

Comment on attachment 346878 [details]
Patch

Seems fine to me.

Please wait a day before committing in case Konstantin has some comment.

Comment on attachment 346878 [details]
Patch

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 346878 [details]
> Patch
> 
> Seems fine to me.
> 
> Please wait a day before committing in case Konstantin has some comment.

K I'll let Konstantin cq+ it if he approves

I uploaded the result of UBSan.

https://gist.github.com/fujii/219fd997e13a1d06cfbedb819ef286b1

> ./Tools/Scripts/build-webkit --gtk --release --cmakeargs=-DENABLE_UNDEFINED_BEHAVIOR_SANITIZER=1
> ./Tools/Scripts/run-webkit-tests --gtk --release --no-new-test-results fast/url

Some code is intentionally using undefined behavior.
It seems that you should define ASAN_ENABLED macro.

Comment on attachment 346878 [details]
Patch

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

> Source/cmake/WebKitCompilerFlags.cmake:185
> +        set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=undefined")

1. Ideally, when CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS are modified, one should also modify CMAKE_MODULE_LINKER_FLAGS
2. Why not just use ECMEnableSanitizers? [1] At least it makes sense to borrow there ideas, e.g. there must be a reason why they use -fno-optimize-sibling-calls with ubsan as well.

[1] https://github.com/KDE/extra-cmake-modules/blob/master/modules/ECMEnableSanitizers.cmake
https://api.kde.org/ecm/module/ECMEnableSanitizers.html

(In reply to Konstantin Tokarev from comment #5)
> Comment on attachment 346878 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346878&action=review
> 
> > Source/cmake/WebKitCompilerFlags.cmake:185
> > +        set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=undefined")
> 
> 1. Ideally, when CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS are
> modified, one should also modify CMAKE_MODULE_LINKER_FLAGS

Don, you should of course fix ENABLE_ADDRESS_SANITIZER as well, then.

> 2. Why not just use ECMEnableSanitizers? [1] At least it makes sense to
> borrow there ideas, e.g. there must be a reason why they use
> -fno-optimize-sibling-calls with ubsan as well.

No opinion from me on this, except the -DECM prefix on the CMake option is not great.

(In reply to Fujii Hironori from comment #4)
> Some code is intentionally using undefined behavior.

Scary.

> It seems that you should define ASAN_ENABLED macro.

Perhaps, but perhaps undefined behavior should always be avoided.

(In reply to Michael Catanzaro from comment #6)
> ...
> 
> Perhaps, but perhaps undefined behavior should always be avoided.

Completely avoiding all behavior UBSan flags will likely be controversial. As an example, (if I'm remember correctly, been a while since I worked with this) signed integer overflow is undefined behavior according to the C++ standards, but in practice, most compilers DO define this behavior so there are a few spots where we do (or perhaps did) rely on this being defined behavior.

If we rely on signed overflow anywhere, we must use -fwrapv with GCC and Clang, otherwise this behavior is undefined with these compilers

Konstantin beat me to this by a minute, but I'll post this anyway:

(In reply to Jonathan Bedard from comment #7)
> Completely avoiding all behavior UBSan flags will likely be controversial.
> As an example, (if I'm remember correctly, been a while since I worked with
> this) signed integer overflow is undefined behavior according to the C++
> standards

Yes.

> but in practice, most compilers DO define this behavior so there
> are a few spots where we do (or perhaps did) rely on this being defined
> behavior.

For GCC and Clang this is true only if building with -fwrapv, which we do not use in WebKit:

       -fwrapv
           This option instructs the compiler to assume that signed arithmetic
           overflow of addition, subtraction and multiplication wraps around
           using twos-complement representation.  This flag enables some
           optimizations and disables others.  The options -ftrapv and -fwrapv
           override each other, so using -ftrapv -fwrapv on the command-line
           results in -fwrapv being effective.  Note that only active options
           override, so using -ftrapv -fwrapv -fno-wrapv on the command-line
           results in -ftrapv being effective.

If any of our code assumes twos-complement overflow, I think that would be a bug.

Created attachment 347015 [details]
ares6 results

Results from running ares6 with UbSan and callstacks enabled for reference.

I propose adding silence_unsigned_overflow. In JSC, we know the semantics of unsigned overflow, and intentionally use it. While signed overflow is an UB, unsigned is not :)

AFAIU UBSan doesn't trap on unsigned overflow unless you pass -fsanitize=unsigned-integer-overflow explicitly

Comment on attachment 346878 [details]
Patch

Attachment 346878 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8896636

New failing tests:
imported/blink/transitions/unprefixed-transform.html
legacy-animation-engine/imported/blink/transitions/unprefixed-transform.html

Created attachment 347417 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 346878 [details]
Patch

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

>>> Source/cmake/WebKitCompilerFlags.cmake:185
>>> +        set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=undefined")
>> 
>> 1. Ideally, when CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS are modified, one should also modify CMAKE_MODULE_LINKER_FLAGS
>> 2. Why not just use ECMEnableSanitizers? [1] At least it makes sense to borrow there ideas, e.g. there must be a reason why they use -fno-optimize-sibling-calls with ubsan as well.
>> 
>> [1] https://github.com/KDE/extra-cmake-modules/blob/master/modules/ECMEnableSanitizers.cmake
>> https://api.kde.org/ecm/module/ECMEnableSanitizers.html
> 
> Don, you should of course fix ENABLE_ADDRESS_SANITIZER as well, then.

Still needs CMAKE_MODULE_LINKER_FLAGS (bug #188699)

Comment on attachment 346878 [details]
Patch

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

> Source/cmake/WebKitCompilerFlags.cmake:181
> +        WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-fno-omit-frame-pointer)

Put this inside an if (NOT ENABLE_ADDRESS_SANITIZER) so that we don't add it twice if both sanitizers are enabled. (ubsan is the only sanitizer that doesn't conflict with the other sanitizers. If we add more sanitizers in the future, we need to make sure to fail the build if conflicting sanitizers are enabled.)