Support -fsanitize=integer in the build system and add blacklist

RESOLVED FIXED in Firefox 56

Status

()

Core
Build Config
--
major
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

({sec-audit, sec-want})

Trunk
mozilla56
Unspecified
Linux
sec-audit, sec-want
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [adv-main56-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
The -fsanitize=integer analysis from UBSan can be helpful to detect signed and unsigned integer overflows in the codebase. Unfortunately, those occur very frequently, making it impossible to test anything with it without the use of a huge blacklist.

The upcoming patch includes a blacklist that is broad enough to silence everything that would drain performance too much. But even with this blacklist, neither tests nor fuzzing is "clean". We can however in the future combine this with static analysis to limit ourselves to interesting places to look at, or improve the dynamic analysis to omit typical benign overflows.

It also adds another attribute that can be used on functions. It is not used right now because I figured it is initially easier to add things to the compile-time blacklist as a start.

Finally, it includes a runtime suppression list and patches various parts in the test harnesses to support that. It is currently empty and it should not be used on frequent overflows because it is expensive. However, it has the advantage that it can be used to differentiate between signed and unsigned overflows while the compile-time blacklist cannot do that. So it can be used to e.g. silence unsigned integer overflows on a file or function while still reporting signed issues.

This is part of Q2 goals work so I would like to commit it here, even though we probably won't make heavy use of it immediately.
Comment hidden (mozreview-request)
Comment on attachment 8878043 [details]
Bug 1373256 - Changes to support -fsanitize=integer in the codebase.

https://reviewboard.mozilla.org/r/149460/#review154536

Lots of little things that I think we should fix before adding this.  The giant blacklist is also a little concerning; I would ask questions about more things, but that'd be a lot of comments on it.

::: commit-message-035c2:1
(Diff revision 1)
> +Bug 1373256 - Changes to support -fsanitize=integer in the codebase. r?froydnj

Your initial comment, slightly modified (e.g. not talking about Q2 goals), would be a great commit message.

::: build/autoconf/sanitize.m4:83
(Diff revision 1)
> +dnl ========================================================
> +dnl = Use UndefinedBehavior Sanitizer to find integer overflows
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(ubsan-intovf,
> +[  --enable-ubsan-intovf       Enable UndefinedBehavior Sanitizer (Integer Overflow Parts, default=no)],
> +   MOZ_UBSAN_INTOVF=1,
> +   MOZ_UBSAN_INTOVF= )

It would be ideal if we had `--enable-undefined-sanitizer=...`, where you could pass multiple things corresponding to the various things listed in http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html  But I think that's a little much to require at the moment.

::: build/sanitizers/ubsan_blacklist.txt:1
(Diff revision 1)
> +# The rules in this file are only applied at compile time. If you can modify the
> +# source in question, consider function attributes to disable instrumentation.

ubsan_suppressions.txt should have *some* commentary in it describing what it's there for, and why you should prefer adding things to this file if possible.

::: build/sanitizers/ubsan_blacklist.txt:10
(Diff revision 1)
> +# Assume everything running through CheckedInt.h is ok
> +src:*/CheckedInt.h

There shouldn't be *any* integer overflows in CheckedInt.h; the whole point of using that class is to safely detect when a naive operation would have overflowed and therefore been subject to UB.

So if bugs are being reported here, either there are serious implementation problems with the class, or there are bugs in the sanitizer.

Or is ubsan being very aggressive and saying that even if you have integer overflow in unsigned arithmetic, that's a problem?  That'd be...pretty annoying.

::: build/sanitizers/ubsan_blacklist.txt:86
(Diff revision 1)
> +# Hot path in xpcom
> +src:*/nsCharTraits.h

Really?  How so?

::: build/sanitizers/ubsan_blacklist.txt:100
(Diff revision 1)
> +# Apparently this overflows a lot, because it contains some allocators
> +# that keep overflowing, not sure why. Disabling by function didn't seem
> +# to work here for operator new.
> +src:*/xpcom/ds/nsArrayEnumerator.cpp

Again: really?  How so?

::: mfbt/Attributes.h:232
(Diff revision 1)
> +#if defined(__has_attribute)
> +#  define MOZ_HAVE_NO_SANITIZE_ATTR __has_attribute(no_sanitize)
> +#else
> +#  define MOZ_HAVE_NO_SANITIZE_ATTR 0
> +#endif
> +
> +#if MOZ_HAVE_NO_SANITIZE_ATTR

Please structure this similarly to the other checks:

```c++
#if defined(__has_attribute)
#  if defined __has_attribute(no_sanitize)
#    define MOZ_HAVE_NO_SANITIZE_ATTR
#  endif
#endif

#if defined(MOZ_HAVE_NO_SANITIZE_ATTR)
...
#endif

::: mfbt/Attributes.h:242
(Diff revision 1)
> +#  define NO_SANITIZE_UINTOVFL /* nothing */
> +#  define NO_SANITIZE_INTOVFL /* nothing */

These should be `MOZ_NO_SANITIZE_...`.

Also, I think we should just be verbose here and elsewhere: `MOZ_NO_SANITIZE_UINT_OVERFLOW`; `OVFL` is not particularly mnemonic.
Attachment #8878043 - Flags: review?(nfroyd)
Comment hidden (mozreview-request)
Comment on attachment 8878043 [details]
Bug 1373256 - Changes to support -fsanitize=integer in the codebase.

https://reviewboard.mozilla.org/r/149460/#review157272

Thanks for the changes.
Attachment #8878043 - Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request)

Comment 6

6 months ago
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07e0c4e4ae85
Changes to support -fsanitize=integer in the codebase. r=froydnj
sorry had to back this out for breaking android like https://treeherder.mozilla.org/logviewer.html#?job_id=110296519&repo=autoland
Flags: needinfo?(choller)

Comment 8

6 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f29ce5d69da
Backed out changeset 07e0c4e4ae85 for breaking android tests
(Assignee)

Comment 9

6 months ago
(In reply to Carsten Book [:Tomcat] from comment #7)
> sorry had to back this out for breaking android like
> https://treeherder.mozilla.org/logviewer.html#?job_id=110296519&repo=autoland

Sorry for that. Apparently I missed adding ubsanPath to the arguments environment() for remote tests. I'll modify my patch to just include ubsanPath in the two places where I forgot it (with the variable not being used, just like lsanPath), so the interfaces are compatible. This doesn't cause any functional changes though.
Flags: needinfo?(choller)
Comment hidden (mozreview-request)

Comment 11

6 months ago
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0882faffdd7
Changes to support -fsanitize=integer in the codebase. r=froydnj

Comment 12

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0882faffdd7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [adv-main56-]
You need to log in before you can comment on or make changes to this bug.