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 327336 a2a645bf7ccf4ffd4313fa55019889feb7957082
parent 327335 7d9629d35a329fae97e48065dd557fd530361068
child 327337 caddb604d934f633e88f0bcf8e1835f5f3249157
push id9858
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 14:37:10 +0000
treeherdermozilla-aurora@203106ef6cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn
bugs1249578
milestone50.0a1
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);