Backed out changeset e1eec63b920f (bug 1315554)

image/decoders/nsBMPDecoder.cpp

@@ -545,14 +545,6 @@ nsBMPDecoder::ReadInfoHeaderRest(const char* aData, size_t aLength)
     // 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",

image/decoders/nsICODecoder.cpp

@@ -115,6 +115,60 @@ nsICODecoder::GetFinalStateFromContainedDecoder()
   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)
 {
@@ -351,6 +405,13 @@ nsICODecoder::ReadBIH(const char* aData)
   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();

image/decoders/nsICODecoder.h

@@ -87,6 +87,16 @@ private:
   // 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();
 

image/test/gtest/TestMetadata.cpp

@@ -90,12 +90,7 @@ CheckMetadata(const ImageTestCase& aTestCase,
 
   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