Bug 1024803 - Added comments and asserts to nsBMPDecoder to make it clearer that an overflow is not possible. r=milan
authorWalter Litwinczyk <wlitwinczyk@mozilla.com>
Thu, 03 Jul 2014 09:56:11 -0700
changeset 192164 c630687dad45fe1d13623cb58ca7ea7308f4cf8d
parent 192163 9f1c312a85eed91076741d9b4921ca4ada6d3f99
child 192165 cd5ec2d1435d2640274c8618b91dba882d84a6cc
push id45769
push userkwierso@gmail.com
push dateThu, 03 Jul 2014 17:58:19 +0000
treeherdermozilla-inbound@c630687dad45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmilan
bugs1024803
milestone33.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 1024803 - Added comments and asserts to nsBMPDecoder to make it clearer that an overflow is not possible. r=milan
image/decoders/nsBMPDecoder.cpp
image/decoders/nsBMPDecoder.h
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -192,44 +192,133 @@ void
 nsBMPDecoder::WriteInternal(const char* aBuffer, uint32_t aCount, DecodeStrategy)
 {
     NS_ABORT_IF_FALSE(!HasError(), "Shouldn't call WriteInternal after error!");
 
     // aCount=0 means EOF, mCurLine=0 means we're past end of image
     if (!aCount || !mCurLine)
         return;
 
+    // This code assumes that mRawBuf == WIN_V3_INTERNAL_BIH_LENGTH
+    // and that sizeof(mRawBuf) >= BFH_INTERNAL_LENGTH
+    MOZ_ASSERT(sizeof(mRawBuf) == WIN_V3_INTERNAL_BIH_LENGTH);
+    MOZ_ASSERT(sizeof(mRawBuf) >= BFH_INTERNAL_LENGTH);
+    MOZ_ASSERT(OS2_INTERNAL_BIH_LENGTH < WIN_V3_INTERNAL_BIH_LENGTH);
+    // This code also assumes it's working with a byte array
+    MOZ_ASSERT(sizeof(mRawBuf[0]) == 1);
+
     if (mPos < BFH_INTERNAL_LENGTH) { /* In BITMAPFILEHEADER */
+        // BFH_INTERNAL_LENGTH < sizeof(mRawBuf)
+        // mPos < BFH_INTERNAL_LENGTH
+        // BFH_INTERNAL_LENGTH - mPos < sizeof(mRawBuf)
+        // so toCopy <= BFH_INTERNAL_LENGTH
+        // so toCopy < sizeof(mRawBuf)
+        // so toCopy > 0 && toCopy <= BFH_INTERNAL_LENGTH
         uint32_t toCopy = BFH_INTERNAL_LENGTH - mPos;
         if (toCopy > aCount)
             toCopy = aCount;
+
+        // mRawBuf is a byte array of size WIN_V3_INTERNAL_BIH_LENGTH (verified above)
+        // mPos is < BFH_INTERNAL_LENGTH
+        // BFH_INTERNAL_LENGTH < WIN_V3_INTERNAL_BIH_LENGTH
+        // so mPos < sizeof(mRawBuf)
+        //
+        // Therefore this assert should hold
+        MOZ_ASSERT(mPos < sizeof(mRawBuf));
+
+        // toCopy <= BFH_INTERNAL_LENGTH
+        // mPos >= 0 && mPos < BFH_INTERNAL_LENGTH
+        // sizeof(mRawBuf) >= BFH_INTERNAL_LENGTH (verified above)
+        //
+        // Therefore this assert should hold
+        MOZ_ASSERT(mPos + toCopy <= sizeof(mRawBuf));
+
         memcpy(mRawBuf + mPos, aBuffer, toCopy);
         mPos += toCopy;
         aCount -= toCopy;
         aBuffer += toCopy;
     }
     if (mPos == BFH_INTERNAL_LENGTH) {
         ProcessFileHeader();
         if (mBFH.signature[0] != 'B' || mBFH.signature[1] != 'M') {
             PostDataError();
             return;
         }
         if (mBFH.bihsize == OS2_BIH_LENGTH)
             mLOH = OS2_HEADER_LENGTH;
     }
     if (mPos >= BFH_INTERNAL_LENGTH && mPos < mLOH) { /* In BITMAPINFOHEADER */
+        // mLOH == WIN_V3_HEADER_LENGTH || mLOH == OS2_HEADER_LENGTH
+        // OS2_HEADER_LENGTH < WIN_V3_HEADER_LENGTH
+        // BFH_INTERNAL_LENGTH < OS2_HEADER_LENGTH
+        // BFH_INTERNAL_LENGTH < WIN_V3_HEADER_LENGTH
+        //
+        // So toCopy is in the range
+        //      1 to (WIN_V3_HEADER_LENGTH - BFH_INTERNAL_LENGTH)
+        // or   1 to (OS2_HEADER_LENGTH - BFH_INTERNAL_LENGTH)
+        //
+        // But WIN_V3_HEADER_LENGTH = BFH_INTERNAL_LENGTH + WIN_V3_INTERNAL_BIH_LENGTH
+        // and OS2_HEADER_LENGTH = BFH_INTERNAL_LENGTH + OS2_INTERNAL_BIH_LENGTH
+        //
+        // So toCopy is in the range
+        //
+        //      1 to WIN_V3_INTERNAL_BIH_LENGTH
+        // or   1 to OS2_INTERNAL_BIH_LENGTH
+        // and  OS2_INTERNAL_BIH_LENGTH < WIN_V3_INTERNAL_BIH_LENGTH
+        //
+        // sizeof(mRawBuf) = WIN_V3_INTERNAL_BIH_LENGTH
+        // so toCopy <= sizeof(mRawBuf)
         uint32_t toCopy = mLOH - mPos;
         if (toCopy > aCount)
             toCopy = aCount;
-        memcpy(mRawBuf + (mPos - BFH_INTERNAL_LENGTH), aBuffer, toCopy);
+
+        // mPos is in the range
+        //      BFH_INTERNAL_LENGTH to (WIN_V3_HEADER_LENGTH - 1)
+        //
+        // offset is then in the range (see toCopy comments for more details)
+        //      0 to (WIN_V3_INTERNAL_BIH_LENGTH - 1)
+        //
+        // sizeof(mRawBuf) is WIN_V3_INTERNAL_BIH_LENGTH so this
+        // offset stays within bounds and this assert should hold
+        const uint32_t offset = mPos - BFH_INTERNAL_LENGTH;
+        MOZ_ASSERT(offset < sizeof(mRawBuf));
+
+        // Two cases:
+        //      mPos = BFH_INTERNAL_LENGTH
+        //      mLOH = WIN_V3_HEADER_LENGTH
+        //
+        // offset = 0
+        // toCopy = WIN_V3_INTERNAL_BIH_LENGTH
+        //
+        //      This will be in the bounds of sizeof(mRawBuf)
+        //
+        // Second Case:
+        //      mPos = WIN_V3_HEADER_LENGTH - 1
+        //      mLOH = WIN_V3_HEADER_LENGTH
+        //
+        // offset = WIN_V3_INTERNAL_BIH_LENGTH - 1
+        // toCopy = 1
+        //
+        //      This will be in the bounds of sizeof(mRawBuf)
+        //
+        // As sizeof(mRawBuf) == WIN_V3_INTERNAL_BIH_LENGTH (verified above)
+        // and WIN_V3_HEADER_LENGTH is the largest range of values. If mLOH
+        // was equal to OS2_HEADER_LENGTH then the ranges are smaller.
+        MOZ_ASSERT(offset + toCopy <= sizeof(mRawBuf));
+
+        memcpy(mRawBuf + offset, aBuffer, toCopy);
         mPos += toCopy;
         aCount -= toCopy;
         aBuffer += toCopy;
     }
 
+    // At this point mPos should be >= mLOH unless aBuffer did not have enough
+    // data. In the latter case aCount should be 0.
+    MOZ_ASSERT(mPos >= mLOH || aCount == 0);
+
     // HasSize is called to ensure that if at this point mPos == mLOH but
     // we have no data left to process, the next time WriteInternal is called
     // we won't enter this condition again.
     if (mPos == mLOH && !HasSize()) {
         ProcessInfoHeader();
         PR_LOG(GetBMPLog(), PR_LOG_DEBUG, ("BMP is %lix%lix%lu. compression=%lu\n",
                mBIH.width, mBIH.height, mBIH.bpp, mBIH.compression));
         // Verify we support this bit depth
@@ -371,22 +460,72 @@ nsBMPDecoder::WriteInternal(const char* 
                     break;
             }
             mPos++; aBuffer++; aCount--;
             at = (at + 1) % bytesPerColor;
         }
       }
     }
     else if (aCount && mBIH.compression == BI_BITFIELDS && mPos < (WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH)) {
-        // If compression is used, this is a windows bitmap, hence we can
-        // use WIN_HEADER_LENGTH instead of mLOH
+        // If compression is used, this is a windows bitmap (compression can't be used with OS/2 bitmaps),
+        // hence we can use WIN_V3_HEADER_LENGTH instead of mLOH.
+        // (verified below)
+
+        // If aCount != 0 then mPos should be >= mLOH due to the if statements
+        // at the beginning of the function
+        MOZ_ASSERT(mPos >= mLOH);
+        MOZ_ASSERT(mLOH == WIN_V3_HEADER_LENGTH);
+
+        // mLOH == WIN_V3_HEADER_LENGTH (verified above)
+        // mPos >= mLOH (verified above)
+        // mPos < WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH
+        //
+        // So toCopy is in the range
+        //      0 to (BITFIELD_LENGTH - 1)
         uint32_t toCopy = (WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH) - mPos;
         if (toCopy > aCount)
             toCopy = aCount;
-        memcpy(mRawBuf + (mPos - WIN_V3_HEADER_LENGTH), aBuffer, toCopy);
+
+        // mPos >= WIN_V3_HEADER_LENGTH
+        // mPos < WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH
+        //
+        // offset is in the range
+        //      0 to (BITFIELD_LENGTH - 1)
+        //
+        // BITFIELD_LENGTH < WIN_V3_INTERNAL_BIH_LENGTH
+        // and sizeof(mRawBuf) == WIN_V3_INTERNAL_BIH_LENGTH (verified at top of function)
+        //
+        // Therefore this assert should hold
+        const uint32_t offset = mPos - WIN_V3_HEADER_LENGTH;
+        MOZ_ASSERT(offset < sizeof(mRawBuf));
+
+        // Two cases:
+        //      mPos = WIN_V3_HEADER_LENGTH
+        //
+        // offset = 0
+        // toCopy = BITFIELD_LENGTH
+        //
+        //      This will be in the bounds of sizeof(mRawBuf)
+        //
+        // Second case:
+        //
+        //      mPos = WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH - 1
+        //
+        // offset = BITFIELD_LENGTH - 1
+        // toCopy = 1
+        //
+        //      This will be in the bounds of sizeof(mRawBuf)
+        //
+        // As BITFIELD_LENGTH < WIN_V3_INTERNAL_BIH_LENGTH and
+        // sizeof(mRawBuf) == WIN_V3_INTERNAL_BIH_LENGTH
+        //
+        // Therefore this assert should hold
+        MOZ_ASSERT(offset + toCopy <= sizeof(mRawBuf));
+
+        memcpy(mRawBuf + offset, aBuffer, toCopy);
         mPos += toCopy;
         aBuffer += toCopy;
         aCount -= toCopy;
     }
     if (mPos == WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH && 
         mBIH.compression == BI_BITFIELDS) {
         mBitFields.red = LittleEndian::readUint32(reinterpret_cast<uint32_t*>(mRawBuf));
         mBitFields.green = LittleEndian::readUint32(reinterpret_cast<uint32_t*>(mRawBuf + 4));
--- a/image/decoders/nsBMPDecoder.h
+++ b/image/decoders/nsBMPDecoder.h
@@ -49,21 +49,21 @@ public:
     virtual void FinishInternal();
 
 private:
 
     /** Calculates the red-, green- and blueshift in mBitFields using
      * the bitmasks from mBitFields */
     NS_METHOD CalcBitShift();
 
-    uint32_t mPos;
+    uint32_t mPos; ///< Number of bytes read from aBuffer in WriteInternal()
 
     BMPFILEHEADER mBFH;
     BITMAPV5HEADER mBIH;
-    char mRawBuf[WIN_V3_INTERNAL_BIH_LENGTH];
+    char mRawBuf[WIN_V3_INTERNAL_BIH_LENGTH]; ///< If this is changed, WriteInternal() MUST be updated
 
     uint32_t mLOH; ///< Length of the header
 
     uint32_t mNumColors; ///< The number of used colors, i.e. the number of entries in mColors
     colorTable *mColors;
 
     bitFields mBitFields;