Bug 867348 - Part 2: Apply MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT to CheckedInt's constructor; r=jrmuizel
☠☠ backed out by 29c50b824923 ☠ ☠
authorEhsan Akhgari <ehsan@mozilla.com>
Thu, 18 Dec 2014 15:27:05 -0500
changeset 248008 acb4dd16755cc2c771268d05a40fa6190569a553
parent 248007 40768f72399053af7aca879d2fd7d5d93eeef8f7
child 248009 5a7684665cabf277e473225f555cba48d4dc76c5
push id698
push userjlund@mozilla.com
push dateMon, 23 Mar 2015 22:08:11 +0000
treeherdermozilla-release@b0c0ae7b02a3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs867348
milestone37.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 867348 - Part 2: Apply MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT to CheckedInt's constructor; r=jrmuizel Note that the analysis currently just looks at the AST subtree of the function call site and is therefore unable to correctly deal with cases such as the last two hunks of the change to OggCodecState.cpp. Fixing the analysis to deal with that would be very difficult, so we currently adjust the code so that it compiles. The first hunk in that file though is a real bug that this analysis found.
dom/media/ogg/OggCodecState.cpp
mfbt/Attributes.h
mfbt/CheckedInt.h
mfbt/tests/TestCheckedInt.cpp
--- a/dom/media/ogg/OggCodecState.cpp
+++ b/dom/media/ogg/OggCodecState.cpp
@@ -931,17 +931,17 @@ int64_t OpusState::Time(int64_t aGranule
 }
 
 int64_t OpusState::Time(int aPreSkip, int64_t aGranulepos)
 {
   if (aGranulepos < 0)
     return -1;
 
   // Ogg Opus always runs at a granule rate of 48 kHz.
-  CheckedInt64 t = CheckedInt64(aGranulepos - aPreSkip) * USECS_PER_S;
+  CheckedInt64 t = (CheckedInt64(aGranulepos) - aPreSkip) * USECS_PER_S;
   return t.isValid() ? t.value() / 48000 : -1;
 }
 
 bool OpusState::IsHeader(ogg_packet* aPacket)
 {
   return aPacket->bytes >= 16 &&
          (!memcmp(aPacket->packet, "OpusHead", 8) ||
           !memcmp(aPacket->packet, "OpusTags", 8));
@@ -1192,25 +1192,27 @@ bool SkeletonState::DecodeIndex(ogg_pack
   int64_t timeDenom = LittleEndian::readInt64(aPacket->packet + INDEX_TIME_DENOM_OFFSET);
   if (timeDenom == 0) {
     LOG(PR_LOG_DEBUG, ("Ogg Skeleton Index packet for stream %u has 0 "
                        "timestamp denominator.", serialno));
     return (mActive = false);
   }
 
   // Extract the start time.
-  CheckedInt64 t = CheckedInt64(LittleEndian::readInt64(p + INDEX_FIRST_NUMER_OFFSET)) * USECS_PER_S;
+  int64_t timeRawInt = LittleEndian::readInt64(p + INDEX_FIRST_NUMER_OFFSET);
+  CheckedInt64 t = CheckedInt64(timeRawInt) * USECS_PER_S;
   if (!t.isValid()) {
     return (mActive = false);
   } else {
     startTime = t.value() / timeDenom;
   }
 
   // Extract the end time.
-  t = LittleEndian::readInt64(p + INDEX_LAST_NUMER_OFFSET) * USECS_PER_S;
+  timeRawInt = LittleEndian::readInt64(p + INDEX_LAST_NUMER_OFFSET);
+  t = CheckedInt64(timeRawInt) * USECS_PER_S;
   if (!t.isValid()) {
     return (mActive = false);
   } else {
     endTime = t.value() / timeDenom;
   }
 
   // Check the numKeyPoints value read, ensure we're not going to run out of
   // memory while trying to decode the index packet.
--- a/mfbt/Attributes.h
+++ b/mfbt/Attributes.h
@@ -497,37 +497,41 @@
  *   need not be provided in such cases.
  * MOZ_HEAP_ALLOCATOR: Applies to any function. This indicates that the return
  *   value is allocated on the heap, and will as a result check such allocations
  *   during MOZ_STACK_CLASS and MOZ_NONHEAP_CLASS annotation checking.
  * MOZ_IMPLICIT: Applies to constructors. Implicit conversion constructors
  *   are disallowed by default unless they are marked as MOZ_IMPLICIT. This
  *   attribute must be used for constructors which intend to provide implicit
  *   conversions.
+ * MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT: Applies to functions. Makes it a compile
+ *   time error to path arithmetic expressions on variables to the function.
  */
 #ifdef MOZ_CLANG_PLUGIN
 #  define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
 #  define MOZ_STACK_CLASS __attribute__((annotate("moz_stack_class")))
 #  define MOZ_NONHEAP_CLASS __attribute__((annotate("moz_nonheap_class")))
 #  define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
+#  define MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT __attribute__((annotate("moz_no_arith_expr_in_arg")))
 /*
  * It turns out that clang doesn't like void func() __attribute__ {} without a
  * warning, so use pragmas to disable the warning. This code won't work on GCC
  * anyways, so the warning is safe to ignore.
  */
 #  define MOZ_HEAP_ALLOCATOR \
     _Pragma("clang diagnostic push") \
     _Pragma("clang diagnostic ignored \"-Wgcc-compat\"") \
     __attribute__((annotate("moz_heap_allocator"))) \
     _Pragma("clang diagnostic pop")
 #else
 #  define MOZ_MUST_OVERRIDE /* nothing */
 #  define MOZ_STACK_CLASS /* nothing */
 #  define MOZ_NONHEAP_CLASS /* nothing */
 #  define MOZ_IMPLICIT /* nothing */
+#  define MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT /* nothing */
 #  define MOZ_HEAP_ALLOCATOR /* nothing */
 #endif /* MOZ_CLANG_PLUGIN */
 
 /*
  * MOZ_THIS_IN_INITIALIZER_LIST is used to avoid a warning when we know that
  * it's safe to use 'this' in an initializer list.
  */
 #ifdef _MSC_VER
--- a/mfbt/CheckedInt.h
+++ b/mfbt/CheckedInt.h
@@ -6,16 +6,17 @@
 
 /* Provides checked integers, detecting integer overflow and divide-by-0. */
 
 #ifndef mozilla_CheckedInt_h
 #define mozilla_CheckedInt_h
 
 #include <stdint.h>
 #include "mozilla/Assertions.h"
+#include "mozilla/Attributes.h"
 #include "mozilla/IntegerTypeTraits.h"
 
 namespace mozilla {
 
 template<typename T> class CheckedInt;
 
 namespace detail {
 
@@ -520,17 +521,17 @@ public:
    *
    * This constructor is not explicit. Instead, the type of its argument is a
    * separate template parameter, ensuring that no conversion is performed
    * before this constructor is actually called. As explained in the above
    * documentation for class CheckedInt, this constructor checks that its
    * argument is valid.
    */
   template<typename U>
-  CheckedInt(U aValue)
+  CheckedInt(U aValue) MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT
     : mValue(T(aValue)),
       mIsValid(detail::IsInRange<T>(aValue))
   {
     static_assert(detail::IsSupported<T>::value &&
                   detail::IsSupported<U>::value,
                   "This type is not supported by CheckedInt");
   }
 
--- a/mfbt/tests/TestCheckedInt.cpp
+++ b/mfbt/tests/TestCheckedInt.cpp
@@ -516,17 +516,17 @@ void test()
     VERIFY_IS_VALID_IF(CheckedInt<T>(MaxValue<U>::value), \
       (sizeof(T) > sizeof(U) || ((sizeof(T) == sizeof(U)) && (isUSigned || !isTSigned)))); \
     VERIFY_IS_VALID_IF(CheckedInt<T>(MinValue<U>::value), \
       isUSigned == false ? 1 \
                          : bool(isTSigned) == false ? 0 \
                                                     : sizeof(T) >= sizeof(U)); \
   }
   #define VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(U) \
-    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,U,+0) \
+    VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,U,+zero) \
     VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE2(U,CheckedInt<U>,.toChecked<T>())
 
   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(int8_t)
   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(uint8_t)
   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(int16_t)
   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(uint16_t)
   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(int32_t)
   VERIFY_CONSTRUCTION_FROM_INTEGER_TYPE(uint32_t)