Closed Bug 1474488 Opened Last year Closed Last year

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: pdknsk, Assigned: pdknsk)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

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
: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.
Status: UNCONFIRMED → RESOLVED
Closed: Last year
Resolution: --- → DUPLICATE
Duplicate of bug: 1469910
Component: Untriaged → General
Product: Firefox → Firefox Build System
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).
Status: RESOLVED → UNCONFIRMED
Flags: needinfo?(choller)
Resolution: DUPLICATE → ---
Attached patch ubsan (obsolete) — Splinter Review
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.
Flags: needinfo?(choller) → needinfo?(twsmith)
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.
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.
Flags: needinfo?(twsmith)
Attached patch ubsan-2 (obsolete) — Splinter Review
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.
Assignee: nobody → pdknsk
Attachment #9004141 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached file patch (obsolete) —
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
Attachment #9005925 - Attachment is obsolete: true
Attachment #9005925 - Attachment is obsolete: false
Attached patch enable-ubsan-in-asan.patch (obsolete) — Splinter Review
Attachment #9005941 - Attachment is obsolete: true
Only enable in select builds. Thanks decoder.
Attachment #9005942 - Attachment is obsolete: true
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).
Attachment #9006256 - Flags: review?(nfroyd)
Attachment #9005925 - Flags: review?(nfroyd)
Attachment #9006256 - Flags: review?(nfroyd) → review+
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.
Attachment #9005925 - Flags: review?(nfroyd) → feedback+
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.
(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.
Attached patch ubsan-3 (obsolete) — Splinter Review
Updated with requested changes. And I removed the speculative part. If someone does unify the txt files, they can still make that change.
Args, an unrelated change in dom/ got into the patch.
Attached patch ubsan-4Splinter Review
Attachment #9006382 - Attachment is obsolete: true
(I'm not sure how to interpret feedback+ exactly.)
Attachment #9005925 - Attachment is obsolete: true
Attachment #9006384 - Flags: review?(nfroyd)
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.
Attachment #9006384 - Flags: review?(nfroyd) → review+
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.
Attached patch ubsan-5Splinter Review
With the requested change, and -rdynamic addition as per the other patch.
Keywords: checkin-needed
pdknsk  only two patches have review+, should i import only those?
Flags: needinfo?(pdknsk)
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.
Flags: needinfo?(pdknsk)
Pushed by csabou@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(pdknsk)
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.
Depends on: 1490828
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+.
Flags: needinfo?(twsmith)
Flags: needinfo?(pdknsk)
(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.
Flags: needinfo?(twsmith)
Flags: needinfo?(pdknsk)
Pushed by btara@mozilla.com:
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
Keywords: checkin-needed
Depends on: 1491510
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.
Are the bots running the same tests that run locally with mach gtest?
https://hg.mozilla.org/mozilla-central/rev/815719383085
https://hg.mozilla.org/mozilla-central/rev/75f0a4384b0f
Status: ASSIGNED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1491809
Depends on: 1494207
Duplicate of this bug: 1469910
You need to log in before you can comment on or make changes to this bug.