1474488 - add --enable-undefined-sanitizer (mainly for fuzzing interface)

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[u473386]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

Akin to --enable-address-sanitizer. I noticed --enable-signed-overflow-sanitizer and --enable-unsigned-overflow-sanitizer is already there. The new flag could mostly match the UBSAN flags set by oss-fuzz (minus vptr, which can be problematic.)

https://github.com/google/oss-fuzz/blob/4e87bd622dfdde966e6ab4f40721b2dd02005ae0/infra/base-images/base-builder/Dockerfile#L25

And perhaps optionally, have --enable-undefined-sanitizer take a value, to pick flags manually, like this.

--enable-undefined-sanitizer=bool,array-bounds,float-divide-by-zero

(The above probably wouldn't work right now because of the following bug.)

https://bugzilla.mozilla.org/show_bug.cgi?id=1474486

Comment 1

[Christian Holler (:decoder)]

:tsmith is in charge of this and we have a subset of flags that are green on try to enable.

Being able to pass the flags like you suggested might be a good idea. Maybe you can work with Tyson to make this happen in bug 1469910.

Comment 2

[u473386]

I've de-duplicated the other bug because it has a bit different scope. It's about enabling UBSAN on the bots.

Please take a look at the patch. Works with ASAN and without.

ac_add_options --disable-debug
ac_add_options --disable-elf-hack
ac_add_options --disable-jemalloc
ac_add_options --disable-crashreporter
ac_add_options --enable-fuzzing
ac_add_options --enable-optimize=-O1
ac_add_options --enable-debug-symbols=-gline-tables-only
# ac_add_options --enable-address-sanitizer # optional
ac_add_options --enable-undefined-sanitizer=bool,bounds,signed-integer-overflow,vla-bound

Without custom checks, it enables everything (not recommended).

Comment 3

[u473386]

Attachment: ubsan

Comment 4

[Christian Holler (:decoder)]

In general, I like the idea of having --enable-undefined-sanitizer to be an alias for adding the flags + adding no-recover because we want no-recover both in the fuzzing and the CI use case.

And while bug 1469910 is about adding these flags for the bots, I am not sure if Tyson wanted to add such a flag to the build system himself or something similar with a preconfigured set (which might collide with your patch). Hence I'll put a needinfo for this on :tsmith.

Also, please don't add -fno-sanitize=vptr. The linker error comes from the fact that we build without rtti (runtime type information), but Tyson already successfully compiled with vptr once. It might just be that you need to force the build system to build with rtti in that case. Maybe Tyson can elaborate on his build configuration here too.

Comment 5

[u473386]

Ah, I was aware of rtti and vptr but didn't make the connection when the error was typeinfo unresolved symbols. If that can be enabled through C(XX)FLAGS those lines can just be removed, as it's probably the responsibility of whoever enables vptr checks to provide that then. I'll wait for the needinfo before I proceed.

Comment 6

[Tyson Smith [:tsmith]]

First of all thanks for all the help! I'm am really looking forward to solid UBSan support and I know others are too.

This approach with some minor changes would likely be a good way forward. As you suggested adding "--enable-undefined-sanitizer" and allowing it to take a list of checks is a good idea. In addition defaulting to a subset of "approved" checks should be done when only "--enable-undefined-sanitizer" is specified. When check are pass they should overwrite the defaults. The list of approved checks should initially be "bool,bounds,vla-bound". This list can grow over time as we get the build and tests passing.

To test the build we can initially piggy back on the ASan build with the default set of checks. Once we get a green try build (should pass with very little effort if is does not already) we can make push it to Tier 1 to avoid regressions. This adds very little over head to the overall build time (~5min for what I've seen) and test times seem less influenced.

In the future we can also add a standalone UBSan build to taskcluster as we increase the number of default checks we have.

Comment 7

[u473386]

Attachment: ubsan-2

Please take a look at the updated patch. In addition to the requested changes, the MOZ_UBSAN_BLACKLIST machinery is changed, as it turns out neither undefined_sanitizer nor no_sanitize_undefined are available.

Comment 8

[Tyson Smith [:tsmith]]

Attachment: patch

Looks good, thanks! I've pushed to try[1] with one minor addition (Enable on ASan build).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e47137e03cbc0b2acbba4f75bc1d0a2efc58c685

Comment 9

[Tyson Smith [:tsmith]]

Attachment: enable-ubsan-in-asan.patch

Comment 10

[Tyson Smith [:tsmith]]

Attachment: enable-ubsan-in-asan.patch

Only enable in select builds. Thanks decoder.

Comment 11

[Christian Holler (:decoder)]

Comment on attachment 9006256 [details] [diff] [review]
enable-ubsan-in-asan.patch

This looks good to me, as it only enables UBSan on the Tier1 ASan build and the build we use for fuzzing. :froydnj should probably approve it along with reviewing the first patch (for adding --enable-undefined-sanitizer).

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 9005925 [details] [diff] [review]
ubsan-2

Review of attachment 9005925 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/sanitize.m4
@@ +80,5 @@
> +    MOZ_UBSAN=1
> +    UBSAN_TXT="$_topsrcdir/build/sanitizers/ubsan_blacklist.txt"
> +    if ! test -f "$UBSAN_TXT"; then
> +        cat $_topsrcdir/build/sanitizers/ubsan_*_blacklist.txt > $UBSAN_TXT
> +    fi

Modifying the srcdir during builds is not OK.  This needs to be changed to create a temporary file in the object directory; I assume that passing multiple .txt files on the command line isn't an option?

::: build/moz.configure/toolchain.configure
@@ +1432,5 @@
> +def ubsan(options):
> +    checks = 'bool,bounds,vla-bound'
> +
> +    if len(options):
> +       checks = ','.join(options)

We should perform some kind of checking on these options.  I don't know if the option checking machinery is robust enough to check against a set of permitted options; if it's not, we can do the checking here.

I suggest writing this whole function as:

# Possibly split this over multiple lines.
defaults = ['bool', 'bounds', 'vla-bound']

checks = options if len(options) else defaults

return ','.join(checks)

::: mozglue/build/UbsanOptions.cpp
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/Attributes.h"
> +
> +#ifndef _MSC_VER // Not supported by clang-cl yet

Is UBSan itself not supported by clang-cl, or __ubsan_default_options not supported?  If the former, shouldn't we complain if you try to enable UBSan if clang-cl is being used?  If the latter, this comment should be made clearer.

Comment 13

[u473386]

Thanks for the review.

> Modifying the srcdir during builds is not OK.  This needs to be changed to
> create a temporary file in the object directory; I assume that passing
> multiple .txt files on the command line isn't an option?

I'll explain what the idea was here, maybe this can or should be done even differently.

There are already two files which have a blacklist for the respective checks.

ubsan_signed_overflow_blacklist.txt
ubsan_unsigned_overflow_blacklist.txt

The format is as below.

[check]
...

So they can be concatenated, and unused checks are ignored. Any my idea was that someone may what to unify them in a single file at some point. Until then, the build script does. Or they are kept separate, with new files added for additional checks, hence the wildcard.

> We should perform some kind of checking on these options.  I don't know if
> the option checking machinery is robust enough to check against a set of
> permitted options; if it's not, we can do the checking here.

There are about 20 checks, with new checks added regularly.

https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks

If you add an unsupported check, the compiler tells you.

> Is UBSan itself not supported by clang-cl, or __ubsan_default_options not
> supported?  If the former, shouldn't we complain if you try to enable UBSan
> if clang-cl is being used?  If the latter, this comment should be made
> clearer.

To be honest, I copied that from AsanOptions.cpp. I'm not familiar with clang-cl.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to pdknsk from comment #13)
> Thanks for the review.
> 
> > Modifying the srcdir during builds is not OK.  This needs to be changed to
> > create a temporary file in the object directory; I assume that passing
> > multiple .txt files on the command line isn't an option?
> 
> I'll explain what the idea was here, maybe this can or should be done even
> differently.
> 
> There are already two files which have a blacklist for the respective checks.
> 
> ubsan_signed_overflow_blacklist.txt
> ubsan_unsigned_overflow_blacklist.txt
> 
> The format is as below.
> 
> [check]
> ...
> 
> So they can be concatenated, and unused checks are ignored. Any my idea was
> that someone may what to unify them in a single file at some point. Until
> then, the build script does. Or they are kept separate, with new files added
> for additional checks, hence the wildcard.

That's fine, it looks like individual files can constrain their blacklisting to individual checks.  But the concatenated file needs to be created someplace else.

> > We should perform some kind of checking on these options.  I don't know if
> > the option checking machinery is robust enough to check against a set of
> > permitted options; if it's not, we can do the checking here.
> 
> There are about 20 checks, with new checks added regularly.
> 
> https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks
> 
> If you add an unsupported check, the compiler tells you.

Sure.  Part of the point of configure is to try and ensure you don't get into situations where you get compile or link errors.

I suppose that given that we use a wide variety of versions, which in turn support very different numbers of checks, it might not be feasible to check everything currently.  So this can be deferred to a future improvement.

Comment 15

[u473386]

Attachment: ubsan-3

Updated with requested changes. And I removed the speculative part. If someone does unify the txt files, they can still make that change.

Comment 16

[u473386]

Args, an unrelated change in dom/ got into the patch.

Comment 17

[u473386]

Attachment: ubsan-4

Comment 18

[u473386]

(I'm not sure how to interpret feedback+ exactly.)

Comment 19

[Tyson Smith [:tsmith]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5406fced0b0012683e6405170043ef0ca8a0b2

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 9006384 [details] [diff] [review]
ubsan-4

Review of attachment 9006384 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: mfbt/Attributes.h
@@ +247,5 @@
>  #endif
>  
>  /*
> + * MOZ_UBSAN_BLACKLIST is a macro to tell UndefinedSanitizer (a compile-time
> + * instrumentation shipped with Clang) to not instrument the annotated function.

I think we'd want a more finely-targeted attribute than all of ubsan (see the attributes below this one for examples), and given that nothing in this patch uses this attribute, please remove it and the #define above.

Comment 21

[Nathan Froyd [:froydnj]]

https://phabricator.services.mozilla.com/D5108 and related patches will probably break attachment 9006384 [details] [diff] [review], so just be aware of that patch before you land these.

Comment 22

[u473386]

Attachment: ubsan-5

With the requested change, and -rdynamic addition as per the other patch.

Comment 23

[Andreea Pavel [:apavel]]

pdknsk  only two patches have review+, should i import only those?

Comment 24

[u473386]

Only ubsan-5 and the other patch, which I think should be a separate commit? I don't know how this is handled with multiple authors. I haven't obsoleted ubsan-4 because it got the conditional review, and otherwise the follow-up patch ubsan-5 might seem review-less. It's a bit unclear to me what to do then.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b9fa5f7592
add --enable-undefined-sanitizer (mainly for fuzzing interface). r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffce46424c69
add --enable-undefined-sanitizer with custom checks. r=froydnj

Comment 26

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (Bug 1474488) for causing failures in build/src/obj-firefox/dist/include/mozilla/Assertions.h CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ffce46424c69ce49b0a8509dab4fccdcb71e3b4c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=198589044

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198589044&repo=mozilla-inbound&lineNumber=2052

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=43a95f0f47256ac54ce6bb2044216de7a9406574&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 27

[u473386]

The problem appears to be that the (unexpected) UBSAN report causes a SIGILL somehow which ASAN catches? I don't know why or how.

 INFO - checking window state
 INFO - TEST-START | browser/components/sessionstore/test/browser_frame_history.js
 INFO - GECKO(1075) | /builds/worker/workspace/build/src/docshell/shistory/nsSHEntry.cpp:1011:15: runtime error: load of value 228, which is not a valid value for type 'bool'
 INFO - GECKO(1075) | SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/workspace/build/src/docshell/shistory/nsSHEntry.cpp:1011:15 in
 INFO - GECKO(1075) | AddressSanitizer:DEADLYSIGNAL
 INFO - GECKO(1075) | =================================================================
ERROR - GECKO(1075) | ==1263==ERROR: AddressSanitizer: ILL on unknown address 0x7f13479f6c1b

Comment 28

[Tyson Smith [:tsmith]]

It looks like it is hitting a real error: 
>[task 2018-09-11T07:55:16.583Z] 07:55:16     INFO - GECKO(1075) | /builds/worker/workspace/build/src/docshell/shistory/nsSHEntry.cpp:1011:15: runtime error: load of value 228, which is not a valid value for type 'bool'
> [task 2018-09-11T07:55:16.584Z] 07:55:16     INFO - GECKO(1075) | SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/workspace/build/src/docshell/shistory/nsSHEntry.cpp:1011:15 in

https://treeherder.mozilla.org/logviewer.html#?job_id=198589044&repo=mozilla-inbound&lineNumber=2048

The SIGILLs seem to be coming from other processes not shutting down correctly.

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

Can someone please confirm?
Just enable-ubsan-in-asan.patch and ubsan-5 are to be landed.
Also ubsan-5 inherits ubsan-4's r+.

Comment 30

[Tyson Smith [:tsmith]]

(In reply to Bogdan Tara[:bogdan_tara] from comment #29)
> Can someone please confirm?
> Just enable-ubsan-in-asan.patch and ubsan-5 are to be landed.
> Also ubsan-5 inherits ubsan-4's r+.

Yes that is correct.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/815719383085
"add --enable-undefined-sanitizer (mainly for fuzzing interface)" r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/75f0a4384b0f
add --enable-undefined-sanitizer with custom checks r=froydnj

Comment 32

[u473386]

If there are many more of those expected, it might be easier to set UBSAN_OPTIONS=halt_on_error=0 temporarily for the tests, and then search the logs for UndefinedBehaviorSanitizer and fix them at once.

Comment 33

[u473386]

Are the bots running the same tests that run locally with mach gtest?

Comment 34

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/815719383085
https://hg.mozilla.org/mozilla-central/rev/75f0a4384b0f