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 447377 7144fcd531df304bea9bc2031fab6bc56c405095
parent 447376 5d9cab23db6756aeb64142fa577b593b0695c10d
child 447378 3bbc8c689dd3b8dafea3097df1a8c73a0057ffee
push id135
push userfmarier@mozilla.com
push dateSat, 10 Feb 2018 02:56:15 +0000
reviewersdecoder, froydnj
bugs1435484
milestone60.0a1
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