Bug 1201796 (Part 1) - Treat ICOs with wrong widths and heights as corrupt. r=tn

image/decoders/nsICODecoder.cpp

@@ -312,6 +312,11 @@ nsICODecoder::ReadDirEntry(const char* aData)
   }
 
   if (mCurrIcon == mNumIcons) {
+    PostSize(GetRealWidth(mDirEntry), GetRealHeight(mDirEntry));
+    if (IsMetadataDecode()) {
+      return Transition::Terminate(ICOState::SUCCESS);
+    }
+
     size_t offsetToResource = mDirEntry.mImageOffset - FirstResourceOffset();
     return Transition::ToUnbuffered(ICOState::FOUND_RESOURCE,
                                     ICOState::SKIP_TO_RESOURCE,
@@ -382,15 +387,6 @@ nsICODecoder::ReadPNG(const char* aData, uint32_t aLen)
     return Transition::Terminate(ICOState::FAILURE);
   }
 
-  if (!HasSize() && mContainedDecoder->HasSize()) {
-    nsIntSize size = mContainedDecoder->GetSize();
-    PostSize(size.width, size.height);
-
-    if (IsMetadataDecode()) {
-      return Transition::Terminate(ICOState::SUCCESS);
-    }
-  }
-
   // Raymond Chen says that 32bpp only are valid PNG ICOs
   // http://blogs.msdn.com/b/oldnewthing/archive/2010/10/22/10079192.aspx
   if (!IsMetadataDecode() &&
@@ -444,14 +440,6 @@ nsICODecoder::ReadBIH(const char* aData)
     return Transition::Terminate(ICOState::FAILURE);
   }
 
-  nsIntSize size = mContainedDecoder->GetSize();
-  PostSize(size.width, size.height);
-
-  // We have the size. If we're doing a metadata decode, we're done.
-  if (IsMetadataDecode()) {
-    return Transition::Terminate(ICOState::SUCCESS);
-  }
-
   // Sometimes the ICO BPP header field is not filled out so we should trust the
   // contained resource over our own information.
   // XXX(seth): Is this ever different than the value we obtained from
@@ -574,6 +562,20 @@ nsICODecoder::ReadMaskRow(const char* aData)
   return Transition::To(ICOState::READ_MASK_ROW, mMaskRowSize);
 }
 
+LexerTransition<ICOState>
+nsICODecoder::FinishResource()
+{
+  // Make sure the actual size of the resource matches the size in the directory
+  // entry. If not, we consider the image corrupt.
+  IntSize expectedSize(GetRealWidth(mDirEntry), GetRealHeight(mDirEntry));
+  if (mContainedDecoder->HasSize() &&
+      mContainedDecoder->GetSize() != expectedSize) {
+    return Transition::Terminate(ICOState::FAILURE);
+  }
+
+  return Transition::Terminate(ICOState::SUCCESS);
+}
+
 void
 nsICODecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
 {
@@ -608,7 +610,7 @@ nsICODecoder::WriteInternal(const char* aBuffer, uint32_t aCount)
         case ICOState::SKIP_MASK:
           return Transition::ContinueUnbuffered(ICOState::SKIP_MASK);
         case ICOState::FINISHED_RESOURCE:
-          return Transition::Terminate(ICOState::SUCCESS);
+          return FinishResource();
         default:
           MOZ_ASSERT_UNREACHABLE("Unknown ICOState");
           return Transition::Terminate(ICOState::FAILURE);

image/decoders/nsICODecoder.h

@@ -115,6 +115,7 @@ private:
   LexerTransition<ICOState> ReadBMP(const char* aData, uint32_t aLen);
   LexerTransition<ICOState> PrepareForMask();
   LexerTransition<ICOState> ReadMaskRow(const char* aData);
+  LexerTransition<ICOState> FinishResource();
 
   StreamingLexer<ICOState, 32> mLexer; // The lexer.
   nsRefPtr<Decoder> mContainedDecoder; // Either a BMP or PNG decoder.

image/test/crashtests/crashtests.list

@@ -52,3 +52,8 @@ skip-if(AddressSanitizer) skip-if(B2G) load 944353.jpg
 load invalid-disposal-method-1.gif
 load invalid-disposal-method-2.gif
 load invalid-disposal-method-3.gif
+
+# Ensure we handle ICO directory entries which specify the wrong size for the
+# contained resource.
+load invalid_ico_height.ico
+load invalid_ico_width.ico

image/test/reftest/ico/ico-bmp-corrupted/reftest.list

@@ -8,8 +8,3 @@
 == wrapper.html?invalid-compression-RLE8.ico about:blank
 # Invalid compression value - detected when decoding the image data.
 == wrapper.html?invalid-compression.ico about:blank
-
-# Invalid ICO width and heigth should be ignored if the
-# contained BMP is correct.
-== invalid_ico_height.ico 16x16.png
-== invalid_ico_width.ico 16x16.png