Bug 1249578 (Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it. r=njn
☠☠ backed out by 2245bcadd89f ☠ ☠
authorSeth Fowler <mark.seth.fowler@gmail.com>
Tue, 21 Jun 2016 17:56:24 -0700
changeset 302258 a2a645bf7ccf4ffd4313fa55019889feb7957082
parent 302257 7d9629d35a329fae97e48065dd557fd530361068
child 302259 caddb604d934f633e88f0bcf8e1835f5f3249157
push id78669
push usermfowler@mozilla.com
push dateWed, 22 Jun 2016 01:06:44 +0000
treeherdermozilla-inbound@caddb604d934 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn
bugs1249578
milestone50.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 1249578 (Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it. r=njn
image/decoders/nsICODecoder.cpp
image/decoders/nsICODecoder.h
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -97,68 +97,67 @@ nsICODecoder::GetFinalStateFromContained
   mDecodeAborted = mContainedDecoder->WasAborted();
   mProgress |= mContainedDecoder->TakeProgress();
   mInvalidRect.UnionRect(mInvalidRect, mContainedDecoder->TakeInvalidRect());
   mCurrentFrame = mContainedDecoder->GetCurrentFrameRef();
 
   MOZ_ASSERT(HasError() || !mCurrentFrame || mCurrentFrame->IsFinished());
 }
 
-// A BMP inside of an ICO has *2 height because of the AND mask
-// that follows the actual bitmap.  The BMP shouldn't know about
-// this difference though.
 bool
-nsICODecoder::FixBitmapHeight(int8_t* bih)
+nsICODecoder::CheckAndFixBitmapSize(int8_t* aBIH)
 {
-  // Get the height from the BMP file information header.
-  int32_t height = LittleEndian::readInt32(bih + 8);
+  // 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 != 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 bitmap height is by definition * 2 what it should be to account for
-  // the 'AND mask'. It is * 2 even if the `AND mask` is not present.
+  // 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;
   }
 
-  // We should always trust the height from the bitmap itself instead of
-  // the ICO height.  So fix the ICO height.
-  if (height == 256) {
-    mDirEntry.mHeight = 0;
-  } else {
-    mDirEntry.mHeight = (int8_t)height;
-  }
-
-  // Fix the BMP height in the BIH so that the BMP decoder can work properly
-  LittleEndian::writeInt32(bih + 8, height);
-  return true;
-}
-
-// We should always trust the contained resource for the width
-// information over our own information.
-bool
-nsICODecoder::FixBitmapWidth(int8_t* bih)
-{
-  // Get the width from the BMP file information header.
-  int32_t width = LittleEndian::readInt32(bih + 4);
-
-  if (width > 256) {
+  // Verify that the BMP height matches the height we got from the ICO directory
+  // entry. If not, again, decoding fails.
+  if (height != GetRealHeight()) {
     return false;
   }
 
-  // We should always trust the width from the bitmap itself instead of
-  // the ICO width.
-  if (width == 256) {
-    mDirEntry.mWidth = 0;
-  } else {
-    mDirEntry.mWidth = (int8_t)width;
-  }
+  // 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)) {
@@ -376,25 +375,20 @@ nsICODecoder::ReadBIH(const char* aData)
   mContainedDecoder->SetMetadataDecode(IsMetadataDecode());
   mContainedDecoder->SetDecoderFlags(GetDecoderFlags());
   mContainedDecoder->SetSurfaceFlags(GetSurfaceFlags());
   if (mDownscaler) {
     mContainedDecoder->SetTargetSize(mDownscaler->TargetSize());
   }
   mContainedDecoder->Init();
 
-  // Fix the ICO height from the BIH. It needs to be halved so our BMP decoder
-  // will understand, because the BMP decoder doesn't expect the alpha mask that
-  // follows the BMP data in an ICO.
-  if (!FixBitmapHeight(reinterpret_cast<int8_t*>(mBIHraw))) {
-    return Transition::TerminateFailure();
-  }
-
-  // Fix the ICO width from the BIH.
-  if (!FixBitmapWidth(reinterpret_cast<int8_t*>(mBIHraw))) {
+  // 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();
   }
 
--- a/image/decoders/nsICODecoder.h
+++ b/image/decoders/nsICODecoder.h
@@ -80,24 +80,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.
   void GetFinalStateFromContainedDecoder();
 
-  // Fixes the ICO height to match that of the BIH.
-  // and also fixes the BIH height to be /2 of what it was.
-  // See definition for explanation.
-  // Returns false if invalid information is contained within.
-  bool FixBitmapHeight(int8_t* bih);
-  // Fixes the ICO width to match that of the BIH.
-  // Returns false if invalid information is contained within.
-  bool FixBitmapWidth(int8_t* bih);
+  /**
+   * 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);