Bug 1315554 - Part 2. The BMP decoder should be responsible for adjusting its size when embedded inside an ICO. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Sat, 22 Jul 2017 07:50:31 -0400
changeset 419112 6fef952ac780d1b42ae97e21e9d6204a6bb49e78
parent 419111 cd85620ea21d93be0450ef31c6f195c6f2946436
child 419113 92d1377f6bb80267ab189483e1859c3b807e9c46
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)
reviewerstnikkel
bugs1315554
milestone56.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 1315554 - Part 2. The BMP decoder should be responsible for adjusting its size when embedded inside an ICO. r=tnikkel
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,16 +540,24 @@ 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,70 +110,16 @@ 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);
@@ -400,23 +346,16 @@ 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,26 +82,16 @@ 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,17 +85,22 @@ 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);
-  EXPECT_EQ(aTestCase.mSize.height, metadataSize.height);
+  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);
+  }
 
   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));