Backed out changeset e1eec63b920f (bug 1315554)
authorSebastian Hengst <archaeopteryx@coole-files.de>
Sat, 22 Jul 2017 11:04:22 +0200
changeset 419104 d0bb537fcefe580aaebb059f6879e7bd8b2bfe0e
parent 419103 082df1a7a6414401b5899b6b31c901c3da5c5fc5
child 419105 8b52b9a7c5f80abc74ab0716b51b9471d28ad6a2
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1315554
milestone56.0a1
backs oute1eec63b920fe003676d38aca1303dc158646ebb
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
Backed out changeset e1eec63b920f (bug 1315554)
image/decoders/nsBMPDecoder.cpp
image/decoders/nsICODecoder.cpp
image/decoders/nsICODecoder.h
image/test/gtest/TestMetadata.cpp
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -540,24 +540,16 @@ nsBMPDecoder::ReadInfoHeaderRest(const c
     mH.mNumColors   = aLength >= 32 ? LittleEndian::readUint32(aData + 28) : 0;
     // We ignore the important_colors (aData + 36) field.
 
     // For WinBMPv4, WinBMPv5 and (possibly) OS2-BMPv2 there are additional
     // fields in the info header which we ignore, with the possible exception
     // of the color bitfields (see below).
   }
 
-  // The height for BMPs embedded inside an ICO includes spaces for the AND
-  // mask even if it is not present, thus we need to adjust for that here.
-  if (mIsWithinICO) {
-    // XXX(seth): Should we really be writing the absolute value from
-    // the BIH below? Seems like this could be problematic for inverted BMPs.
-    mH.mHeight = abs(mH.mHeight) / 2;
-  }
-
   // Run with MOZ_LOG=BMPDecoder:5 set to see this output.
   MOZ_LOG(sBMPLog, LogLevel::Debug,
           ("BMP: bihsize=%u, %d x %d, bpp=%u, compression=%u, colors=%u\n",
           mH.mBIHSize, mH.mWidth, mH.mHeight, uint32_t(mH.mBpp),
           mH.mCompression, mH.mNumColors));
 
   // BMPs with negative width are invalid. Also, reject extremely wide images
   // to keep the math sane. And reject INT_MIN as a height because you can't
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -110,16 +110,70 @@ nsICODecoder::GetFinalStateFromContained
   nsresult rv = HasError() || mContainedDecoder->HasError()
               ? NS_ERROR_FAILURE
               : NS_OK;
 
   MOZ_ASSERT(NS_FAILED(rv) || !mCurrentFrame || mCurrentFrame->IsFinished());
   return rv;
 }
 
+bool
+nsICODecoder::CheckAndFixBitmapSize(int8_t* aBIH)
+{
+  // Get the width from the BMP file information header. This is
+  // (unintuitively) a signed integer; see the documentation at:
+  //
+  //   https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx
+  //
+  // However, we reject negative widths since they aren't meaningful.
+  const int32_t width = LittleEndian::readInt32(aBIH + 4);
+  if (width <= 0 || width > 256) {
+    return false;
+  }
+
+  // Verify that the BMP width matches the width we got from the ICO directory
+  // entry. If not, decoding fails, because if we were to allow it to continue
+  // the intrinsic size of the image wouldn't match the size of the decoded
+  // surface.
+  if (width != int32_t(GetRealWidth())) {
+    return false;
+  }
+
+  // Get the height from the BMP file information header. This is also signed,
+  // but in this case negative values are meaningful; see below.
+  int32_t height = LittleEndian::readInt32(aBIH + 8);
+  if (height == 0) {
+    return false;
+  }
+
+  // BMPs can be stored inverted by having a negative height.
+  // XXX(seth): Should we really be writing the absolute value into the BIH
+  // below? Seems like this could be problematic for inverted BMPs.
+  height = abs(height);
+
+  // The height field is double the actual height of the image to account for
+  // the AND mask. This is true even if the AND mask is not present.
+  height /= 2;
+  if (height > 256) {
+    return false;
+  }
+
+  // Verify that the BMP height matches the height we got from the ICO directory
+  // entry. If not, again, decoding fails.
+  if (height != int32_t(GetRealHeight())) {
+    return false;
+  }
+
+  // Fix the BMP height in the BIH so that the BMP decoder, which does not know
+  // about the AND mask that may follow the actual bitmap, can work properly.
+  LittleEndian::writeInt32(aBIH + 8, GetRealHeight());
+
+  return true;
+}
+
 LexerTransition<ICOState>
 nsICODecoder::ReadHeader(const char* aData)
 {
   // If the third byte is 1, this is an icon. If 2, a cursor.
   if ((aData[2] != 1) && (aData[2] != 2)) {
     return Transition::TerminateFailure();
   }
   mIsCursor = (aData[2] == 2);
@@ -346,16 +400,23 @@ nsICODecoder::ReadBIH(const char* aData)
     DecoderFactory::CreateDecoderForICOResource(DecoderType::BMP,
                                                 WrapNotNull(mContainedSourceBuffer),
                                                 WrapNotNull(this),
                                                 Some(GetRealSize()),
                                                 Some(dataOffset));
   RefPtr<nsBMPDecoder> bmpDecoder =
     static_cast<nsBMPDecoder*>(mContainedDecoder.get());
 
+  // Verify that the BIH width and height values match the ICO directory entry,
+  // and fix the BIH height value to compensate for the fact that the underlying
+  // BMP decoder doesn't know about AND masks.
+  if (!CheckAndFixBitmapSize(reinterpret_cast<int8_t*>(mBIHraw))) {
+    return Transition::TerminateFailure();
+  }
+
   // Write out the BMP's bitmap info header.
   if (!WriteToContainedDecoder(mBIHraw, sizeof(mBIHraw))) {
     return Transition::TerminateFailure();
   }
 
   // Check to make sure we have valid color settings.
   uint16_t numColors = GetNumColors();
   if (numColors == uint16_t(-1)) {
--- a/image/decoders/nsICODecoder.h
+++ b/image/decoders/nsICODecoder.h
@@ -82,16 +82,26 @@ private:
 
   // Writes to the contained decoder and sets the appropriate errors
   // Returns true if there are no errors.
   bool WriteToContainedDecoder(const char* aBuffer, uint32_t aCount);
 
   // Gets decoder state from the contained decoder so it's visible externally.
   nsresult GetFinalStateFromContainedDecoder();
 
+  /**
+   * Verifies that the width and height values in @aBIH are valid and match the
+   * values we read from the ICO directory entry. If everything looks OK, the
+   * height value in @aBIH is updated to compensate for the AND mask, which the
+   * underlying BMP decoder doesn't know about.
+   *
+   * @return true if the width and height values in @aBIH are valid and correct.
+   */
+  bool CheckAndFixBitmapSize(int8_t* aBIH);
+
   // Obtains the number of colors from the BPP, mBPP must be filled in
   uint16_t GetNumColors();
 
   LexerTransition<ICOState> ReadHeader(const char* aData);
   LexerTransition<ICOState> ReadDirEntry(const char* aData);
   LexerTransition<ICOState> SniffResource(const char* aData);
   LexerTransition<ICOState> ReadPNG(const char* aData, uint32_t aLen);
   LexerTransition<ICOState> ReadBIH(const char* aData);
--- a/image/test/gtest/TestMetadata.cpp
+++ b/image/test/gtest/TestMetadata.cpp
@@ -85,22 +85,17 @@ CheckMetadata(const ImageTestCase& aTest
 
   EXPECT_TRUE(decoder->GetDecodeDone() && !decoder->HasError());
 
   // Check that we got the expected metadata.
   EXPECT_TRUE(metadataProgress & FLAG_SIZE_AVAILABLE);
 
   IntSize metadataSize = decoder->Size();
   EXPECT_EQ(aTestCase.mSize.width, metadataSize.width);
-  if (aBMPWithinICO == BMPWithinICO::YES) {
-    // Half the data is considered to be part of the AND mask if embedded
-    EXPECT_EQ(aTestCase.mSize.height / 2, metadataSize.height);
-  } else {
-    EXPECT_EQ(aTestCase.mSize.height, metadataSize.height);
-  }
+  EXPECT_EQ(aTestCase.mSize.height, metadataSize.height);
 
   bool expectTransparency = aBMPWithinICO == BMPWithinICO::YES
                           ? true
                           : bool(aTestCase.mFlags & TEST_CASE_IS_TRANSPARENT);
   EXPECT_EQ(expectTransparency, bool(metadataProgress & FLAG_HAS_TRANSPARENCY));
 
   EXPECT_EQ(bool(aTestCase.mFlags & TEST_CASE_IS_ANIMATED),
             bool(metadataProgress & FLAG_IS_ANIMATED));