|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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 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.
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.
Pushed by email@example.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
Backout by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1f29ce5d69da Backed out changeset 07e0c4e4ae85 for breaking android tests
(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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/e0882faffdd7 Changes to support -fsanitize=integer in the codebase. r=froydnj