Bug 867348 - Part 2: Apply MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT to CheckedInt's constructor; r=jrmuizel,cpearce
authorEhsan Akhgari <ehsan@mozilla.com>
Thu, 18 Dec 2014 15:27:05 -0500
changeset 220683 c9b0bbb1d4f9565da1b91e47029bbc9a7c773e63
parent 220682 a07116d74024426c192a5b85df52f0ef7eae6e54
child 220684 001e0c8c26b4c87276ffce5ad8845a51f269dfb4
push id27994
push userryanvm@gmail.com
push dateSat, 20 Dec 2014 03:00:19 +0000
treeherdermozilla-central@490f124d7dea [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel, cpearce
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,cpearce 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)