Bug 1435484 - Split the integer-overflow blacklist into two blacklists, one for signed integer overflow and one for unsigned integer overflow, and rename both configure flags to be clearer. r=decoder, r=froydnj
authorJeff Walden <jwalden@mit.edu>
Fri, 02 Feb 2018 21:25:31 -0800
changeset 455350 7144fcd531df304bea9bc2031fab6bc56c405095
parent 455349 5d9cab23db6756aeb64142fa577b593b0695c10d
child 455351 3bbc8c689dd3b8dafea3097df1a8c73a0057ffee
push id8799
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 16:46:23 +0000
treeherdermozilla-beta@15334014dc67 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdecoder, froydnj
bugs1435484
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1435484 - Split the integer-overflow blacklist into two blacklists, one for signed integer overflow and one for unsigned integer overflow, and rename both configure flags to be clearer. r=decoder, r=froydnj
build/autoconf/sanitize.m4
build/moz.configure/old.configure
build/sanitizers/ubsan_blacklist_int.txt
build/sanitizers/ubsan_signed_overflow_blacklist.txt
build/sanitizers/ubsan_unsigned_overflow_blacklist.txt
mfbt/Attributes.h
mfbt/MathAlgorithms.h
--- a/build/autoconf/sanitize.m4
+++ b/build/autoconf/sanitize.m4
@@ -75,53 +75,54 @@ if test -n "$MOZ_TSAN"; then
     MOZ_PATH_PROG(LLVM_SYMBOLIZER, llvm-symbolizer)
 fi
 AC_SUBST(MOZ_TSAN)
 
 dnl ========================================================
 dnl = Use UndefinedBehavior Sanitizer to find integer overflows
 dnl ========================================================
 
-MOZ_ARG_ENABLE_BOOL(ubsan-int-overflow,
-[  --enable-ubsan-int-overflow       Enable UndefinedBehavior Sanitizer (Signed Integer Overflow Parts, default=no)],
-   MOZ_UBSAN_INT_OVERFLOW=1,
-   MOZ_UBSAN_INT_OVERFLOW= )
-MOZ_ARG_ENABLE_BOOL(ubsan-uint-overflow,
-[  --enable-ubsan-uint-overflow       Enable UndefinedBehavior Sanitizer (Unsigned Integer Overflow Parts, default=no)],
-   MOZ_UBSAN_UINT_OVERFLOW=1,
-   MOZ_UBSAN_UINT_OVERFLOW= )
+MOZ_ARG_ENABLE_BOOL(signed-overflow-sanitizer,
+[  --enable-signed-overflow-sanitizer       Enable UndefinedBehavior Sanitizer (Signed Integer Overflow Parts, default=no)],
+   MOZ_SIGNED_OVERFLOW_SANITIZE=1,
+   MOZ_SIGNED_OVERFLOW_SANITIZE= )
+MOZ_ARG_ENABLE_BOOL(unsigned-overflow-sanitizer,
+[  --enable-unsigned-overflow-sanitizer       Enable UndefinedBehavior Sanitizer (Unsigned Integer Overflow Parts, default=no)],
+   MOZ_UNSIGNED_OVERFLOW_SANITIZE=1,
+   MOZ_UNSIGNED_OVERFLOW_SANITIZE= )
 
-if test -n "$MOZ_UBSAN_INT_OVERFLOW$MOZ_UBSAN_UINT_OVERFLOW"; then
+if test -n "$MOZ_SIGNED_OVERFLOW_SANITIZE$MOZ_UNSIGNED_OVERFLOW_SANITIZE"; then
     MOZ_LLVM_HACKS=1
     MOZ_UBSAN=1
-    # The blacklist really should be split into separate signed/unsigned
-    # blacklists, but we leave that task for another day.
-    CFLAGS="-fsanitize-blacklist=$_topsrcdir/build/sanitizers/ubsan_blacklist_int.txt $CFLAGS"
-    CXXFLAGS="-fsanitize-blacklist=$_topsrcdir/build/sanitizers/ubsan_blacklist_int.txt $CXXFLAGS"
-    if test -n "$MOZ_UBSAN_INT_OVERFLOW"; then
+    SANITIZER_BLACKLISTS=""
+    if test -n "$MOZ_SIGNED_OVERFLOW_SANITIZE"; then
+        SANITIZER_BLACKLISTS="-fsanitize-blacklist=$_topsrcdir/build/sanitizers/ubsan_signed_overflow_blacklist.txt $SANITIZER_BLACKLISTS"
         CFLAGS="-fsanitize=signed-integer-overflow $CFLAGS"
         CXXFLAGS="-fsanitize=signed-integer-overflow $CXXFLAGS"
         if test -z "$CLANG_CL"; then
             LDFLAGS="-fsanitize=signed-integer-overflow $LDFLAGS"
         fi
-        AC_DEFINE(MOZ_UBSAN_INT_OVERFLOW)
+        AC_DEFINE(MOZ_SIGNED_OVERFLOW_SANITIZE)
     fi
-    if test -n "$MOZ_UBSAN_UINT_OVERFLOW"; then
+    if test -n "$MOZ_UNSIGNED_OVERFLOW_SANITIZE"; then
+        SANITIZER_BLACKLISTS="-fsanitize-blacklist=$_topsrcdir/build/sanitizers/ubsan_unsigned_overflow_blacklist.txt $SANITIZER_BLACKLISTS"
         CFLAGS="-fsanitize=unsigned-integer-overflow $CFLAGS"
         CXXFLAGS="-fsanitize=unsigned-integer-overflow $CXXFLAGS"
         if test -z "$CLANG_CL"; then
             LDFLAGS="-fsanitize=unsigned-integer-overflow $LDFLAGS"
         fi
-        AC_DEFINE(MOZ_UBSAN_UINT_OVERFLOW)
+        AC_DEFINE(MOZ_UNSIGNED_OVERFLOW_SANITIZE)
     fi
+    CFLAGS="$SANITIZER_BLACKLISTS $CFLAGS"
+    CXXFLAGS="$SANITIZER_BLACKLISTS $CXXFLAGS"
     AC_DEFINE(MOZ_UBSAN)
     MOZ_PATH_PROG(LLVM_SYMBOLIZER, llvm-symbolizer)
 fi
-AC_SUBST(MOZ_UBSAN_INT_OVERFLOW)
-AC_SUBST(MOZ_UBSAN_UINT_OVERFLOW)
+AC_SUBST(MOZ_SIGNED_OVERFLOW_SANITIZE)
+AC_SUBST(MOZ_UNSIGNED_OVERFLOW_SANITIZE)
 AC_SUBST(MOZ_UBSAN)
 
 # The LLVM symbolizer is used by all sanitizers
 AC_SUBST(LLVM_SYMBOLIZER)
 
 dnl ========================================================
 dnl = Enable hacks required for LLVM instrumentations
 dnl ========================================================
--- a/build/moz.configure/old.configure
+++ b/build/moz.configure/old.configure
@@ -217,19 +217,19 @@ def old_configure_options(*options):
     '--enable-strip',
     '--enable-synth-pico',
     '--enable-system-cairo',
     '--enable-system-extension-dirs',
     '--enable-system-pixman',
     '--enable-system-sqlite',
     '--enable-tasktracer',
     '--enable-thread-sanitizer',
-    '--enable-ubsan-int-overflow',
-    '--enable-ubsan-uint-overflow',
+    '--enable-signed-overflow-sanitizer',
     '--enable-universalchardet',
+    '--enable-unsigned-overflow-sanitizer',
     '--enable-updater',
     '--enable-valgrind',
     '--enable-verify-mar',
     '--enable-xul',
     '--enable-zipwriter',
     '--includedir',
     '--libdir',
     '--no-create',
rename from build/sanitizers/ubsan_blacklist_int.txt
rename to build/sanitizers/ubsan_signed_overflow_blacklist.txt
--- a/build/sanitizers/ubsan_blacklist_int.txt
+++ b/build/sanitizers/ubsan_signed_overflow_blacklist.txt
@@ -1,30 +1,52 @@
 # This file contains an extensive compile-time blacklist for silencing highly
-# frequent signed and unsigned integer overflows in our codebase, found by the
-# use of -fsanitize=integer. All of the overflows that caused an entry in this
-# list are highly frequent in our test suites (> 500 times per run) and therefore
-# unlikely to be bugs. Nevertheless, the slow down this test mode significantly
-# if left active. Without this list, the -fsanitize=integer test mode is unusable
-# both because of performance and the large number of results to check.
+# frequent signed integer overflows in our codebase, found by the use of
+# -fsanitize=signed-integer-overflow.  C/C++ say signed integer overflow is
+# undefined behavior, so instances of this need to be fixed.  But not all code
+# has been properly written to not overflow, and overflow-checking can have
+# significant compile time and runtime costs, so we will sometimes  disable
+# signed overflow checking.
+#
+# The rules in this file are applied at compile time; changes to this list
+# usually require a full rebuild to apply. If you can modify the source in
+# question to exempt individual functions using MOZ_NO_SANITIZE_SINT_OVERFLOW,
+# do that instead.
+#
+# The extensive number of entries below is for two reasons.
 #
-# Some of the entries on this list are more aggressive to get the build into a
-# state that allows any testing to happen at all. This is not an optimal solution
-# and it would be good if we could refine the tool and shorten this list over
-# the time. Source code annotations can also help with this.
+# First, compiler instrumentation for signed integer overflows has a cost, at
+# compile time and at runtime.  In performance-critical code proven to have no
+# signed overflow, it makes sense to turn off overflow detection to avoid both
+# costs.  (Indeed, -fsanitize=signed-integer-overflow is unusably slow without
+# this.)
+#
+# Second, many entries here are overly aggressive to get the build into a state
+# that allows any testing to happen at all.  Some of the entries here are for
+# issues that are highly frequent in our test suites -- over 500 times per run.
+# Aggressive entries now let us start using this mode, without having to first
+# fix wide swaths of existing code.
 #
-# 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.
+# Entries should be removed 1) as issues are fixed; and 2) as blacklist entries
+# can be moved out of this centralized file, into source-level blacklist
+# attributes on individual functions.
 
-# Ignore common overflows in the C++ std headers
+# All entries in this file are to suppress signed-integer-overflow problems.
+# Blacklists for other reasons should go in separate blacklist files.
+[signed-integer-overflow]
+
+# Overflows in the C++ std headers aren't necessarily bugs, because code inside
+# a language implementation can depend on compiler-specific behavior where C/C++
+# leave the behavior undefined.
 src:*bits/basic_string.h
 
-# Assume everything running through CheckedInt.h is ok. The CheckedInt class
-# casts signed integers to unsigned first and then does a post-overflow
-# check causing lots of unsigned integer overflow messages.
+# Assume everything running through CheckedInt.h is ok.  Signed overflows here
+# should generally have been guarded by safe overflow checks, so it's likely
+# safe to exempt it from overflow checking.  (This should eventually be verified
+# and functions individually tagged safe so this entry can be removed.)
 src:*/CheckedInt.h
 
 # Exclude bignum
 src:*/mfbt/double-conversion/source/bignum.cc
 
 # Exclude anything within gtests
 src:*/gtest/*
 
copy from build/sanitizers/ubsan_blacklist_int.txt
copy to build/sanitizers/ubsan_unsigned_overflow_blacklist.txt
--- a/build/sanitizers/ubsan_blacklist_int.txt
+++ b/build/sanitizers/ubsan_unsigned_overflow_blacklist.txt
@@ -1,25 +1,45 @@
 # This file contains an extensive compile-time blacklist for silencing highly
-# frequent signed and unsigned integer overflows in our codebase, found by the
-# use of -fsanitize=integer. All of the overflows that caused an entry in this
-# list are highly frequent in our test suites (> 500 times per run) and therefore
-# unlikely to be bugs. Nevertheless, the slow down this test mode significantly
-# if left active. Without this list, the -fsanitize=integer test mode is unusable
-# both because of performance and the large number of results to check.
+# frequent *un*signed integer overflows in our codebase, found by the use of
+# -fsanitize=unsigned-integer-overflow.  Such overflows are not necessarily
+# bugs -- unsigned integer overflow has well-defined semantics in C/C++.  But
+# overflow may still be *unexpected* and incorrectly handled, so we try to
+# annotate those places where unsigned overflow is correct and desired.
+#
+# The rules in this file are applied at compile time; changes to this list
+# usually require a full rebuild to apply. If you can modify the source in
+# question to exempt individual functions using MOZ_NO_SANITIZE_UINT_OVERFLOW,
+# do that instead.
+#
+# The extensive number of entries below is for two reasons.
 #
-# Some of the entries on this list are more aggressive to get the build into a
-# state that allows any testing to happen at all. This is not an optimal solution
-# and it would be good if we could refine the tool and shorten this list over
-# the time. Source code annotations can also help with this.
+# First, compiler instrumentation for unsigned integer overflows has a cost, at
+# compile time and at runtime.  In places where code expects and depends upon
+# overflow behavior -- and especially in performance-critical code -- it makes
+# sense to turn off overflow detection to avoid both costs.  (Indeed,
+# -fsanitize=signed-integer-overflow is unusably slow without this.)
+#
+# Second, many entries here are overly aggressive to get the build into a state
+# that allows any testing to happen at all.  Some of the entries here are for
+# issues that are highly frequent in our test suites -- over 500 times per run.
+# Aggressive entries now let us start using this mode, without having to first
+# fix wide swaths of existing code.
 #
-# 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.
+# Entries should be removed 1) as issues are fixed; and 2) as blacklist entries
+# can be moved out of this centralized file, into source-level blacklist
+# attributes on individual functions.
 
-# Ignore common overflows in the C++ std headers
+# All entries in this file are to suppress unsigned-integer-overflow problems.
+# Blacklists for other reasons should go in separate blacklist files.
+[unsigned-integer-overflow]
+
+# Overflows in the C++ std headers aren't necessarily bugs, because code inside
+# a language implementation can depend on compiler-specific behavior where C/C++
+# leave the behavior undefined.
 src:*bits/basic_string.h
 
 # Assume everything running through CheckedInt.h is ok. The CheckedInt class
 # casts signed integers to unsigned first and then does a post-overflow
 # check causing lots of unsigned integer overflow messages.
 src:*/CheckedInt.h
 
 # Exclude bignum
--- a/mfbt/Attributes.h
+++ b/mfbt/Attributes.h
@@ -227,36 +227,88 @@
 #    define MOZ_TSAN_BLACKLIST MOZ_NEVER_INLINE __attribute__((no_sanitize_thread))
 #  else
 #    define MOZ_TSAN_BLACKLIST /* nothing */
 #  endif
 #else
 #  define MOZ_TSAN_BLACKLIST /* nothing */
 #endif
 
-/*
- * The MOZ_NO_SANITIZE_* family of macros is an annotation based on a more recently
- * introduced Clang feature that allows disabling various sanitizer features for
- * the particular function, including those from UndefinedBehaviorSanitizer.
- */
-
 #if defined(__has_attribute)
 #  if __has_attribute(no_sanitize)
 #    define MOZ_HAVE_NO_SANITIZE_ATTR
 #  endif
 #endif
 
+/*
+ * MOZ_NO_SANITIZE_UNSIGNED_OVERFLOW disables *un*signed integer overflow
+ * checking on the function it annotates, in builds configured to perform it.
+ * (Currently this is only Clang using -fsanitize=unsigned-integer-overflow, or
+ * via --enable-unsigned-overflow-sanitizer in Mozilla's build system.)  It has
+ * no effect in other builds.
+ *
+ * Place this attribute at the very beginning of a function declaration.
+ *
+ * Unsigned integer overflow isn't *necessarily* a bug.  It's well-defined in
+ * C/C++, and code may reasonably depend upon it.  For example,
+ *
+ *   MOZ_NO_SANITIZE_UNSIGNED_OVERFLOW inline bool
+ *   IsDecimal(char aChar)
+ *   {
+ *     // For chars less than '0', unsigned integer underflow occurs, to a value
+ *     // much greater than 10, so the overall test is false.
+ *     // For chars greater than '0', no overflow occurs, and only '0' to '9'
+ *     // pass the overall test.
+ *     return static_cast<unsigned int>(aChar) - '0' < 10;
+ *   }
+ *
+ * But even well-defined unsigned overflow often causes bugs when it occurs, so
+ * it should be restricted to functions annotated with this attribute.
+ *
+ * The compiler instrumentation to detect unsigned integer overflow has costs
+ * both at compile time and at runtime.  Functions that are repeatedly inlined
+ * at compile time will also implicitly inline the necessary instrumentation,
+ * increasing compile time.  Similarly, frequently-executed functions that
+ * require large amounts of instrumentation will also notice significant runtime
+ * slowdown to execute that instrumentation.  Use this attribute to eliminate
+ * those costs -- but only after carefully verifying that no overflow can occur.
+ */
 #if defined(MOZ_HAVE_NO_SANITIZE_ATTR)
-#  define MOZ_NO_SANITIZE_UINT_OVERFLOW __attribute__((no_sanitize("unsigned-integer-overflow")))
-#  define MOZ_NO_SANITIZE_INT_OVERFLOW __attribute__((no_sanitize("signed-integer-overflow")))
+#  define MOZ_NO_SANITIZE_UNSIGNED_OVERFLOW __attribute__((no_sanitize("unsigned-integer-overflow")))
 #else
-#  define MOZ_NO_SANITIZE_UINT_OVERFLOW /* nothing */
-#  define MOZ_NO_SANITIZE_INT_OVERFLOW /* nothing */
+#  define MOZ_NO_SANITIZE_UNSIGNED_OVERFLOW /* nothing */
 #endif
 
+/*
+ * MOZ_NO_SANITIZE_SIGNED_OVERFLOW disables *signed* integer overflow checking
+ * on the function it annotates, in builds configured to perform it.  (Currently
+ * this is only Clang using -fsanitize=signed-integer-overflow, or via
+ * --enable-signed-overflow-sanitizer in Mozilla's build system.  GCC support
+ * will probably be added in the future.)  It has no effect in other builds.
+ *
+ * Place this attribute at the very beginning of a function declaration.
+ *
+ * Signed integer overflow is undefined behavior in C/C++: *anything* can happen
+ * when it occurs.  *Maybe* wraparound behavior will occur, but maybe also the
+ * compiler will assume no overflow happens and will adversely optimize the rest
+ * of your code.  Code that contains signed integer overflow needs to be fixed.
+ *
+ * The compiler instrumentation to detect signed integer overflow has costs both
+ * at compile time and at runtime.  Functions that are repeatedly inlined at
+ * compile time will also implicitly inline the necessary instrumentation,
+ * increasing compile time.  Similarly, frequently-executed functions that
+ * require large amounts of instrumentation will also notice significant runtime
+ * slowdown to execute that instrumentation.  Use this attribute to eliminate
+ * those costs -- but only after carefully verifying that no overflow can occur.
+ */
+#if defined(MOZ_HAVE_NO_SANITIZE_ATTR)
+#  define MOZ_NO_SANITIZE_SIGNED_OVERFLOW __attribute__((no_sanitize("signed-integer-overflow")))
+#else
+#  define MOZ_NO_SANITIZE_SIGNED_OVERFLOW /* nothing */
+#endif
 
 #undef MOZ_HAVE_NO_SANITIZE_ATTR
 
 
 /**
  * MOZ_ALLOCATOR tells the compiler that the function it marks returns either a
  * "fresh", "pointer-free" block of memory, or nullptr. "Fresh" means that the
  * block is not pointed to by any other reachable pointer in the program.
--- a/mfbt/MathAlgorithms.h
+++ b/mfbt/MathAlgorithms.h
@@ -556,16 +556,20 @@ struct WrapToSignedHelper
     (UnsignedType(1) << (CHAR_BIT * sizeof(SignedType) - 1)) - 1;
   static constexpr SignedType MinValue = -MaxValue - 1;
 
   static constexpr UnsignedType MinValueUnsigned =
     static_cast<UnsignedType>(MinValue);
   static constexpr UnsignedType MaxValueUnsigned =
     static_cast<UnsignedType>(MaxValue);
 
+  // Overflow-correctness was proven in bug 1432646 and is explained in the
+  // comment below.  This function is very hot, both at compile time and
+  // runtime, so disable all overflow checking in it.
+  MOZ_NO_SANITIZE_UNSIGNED_OVERFLOW MOZ_NO_SANITIZE_SIGNED_OVERFLOW
   static constexpr SignedType compute(UnsignedType aValue)
   {
     // This algorithm was originally provided here:
     // https://stackoverflow.com/questions/13150449/efficient-unsigned-to-signed-cast-avoiding-implementation-defined-behavior
     //
     // If the value is in the non-negative signed range, just cast.
     //
     // If the value will be negative, compute its delta from the first number