From 396c016b13b9b13b2ae947704696a2cc99f05b6c Mon Sep 17 00:00:00 2001
From: Seth Fowler <mark.seth.fowler@gmail.com>
Date: Thu, 14 Jul 2016 12:30:58 -0700
Subject: [PATCH] Bug 1285867 (Part 4) - Decide whether we're done decoding by
 checking if we've reached a terminal state. r=edwin

---
 image/Decoder.cpp                               |  16 +++++++++++-----
 image/decoders/nsBMPDecoder.cpp                 |   9 +++++----
 image/test/gtest/Common.cpp                     |   9 +++++++++
 image/test/gtest/Common.h                       |   1 +
 image/test/gtest/TestDecoders.cpp               |  10 ++++++++++
 image/test/gtest/invalid-truncated-metadata.bmp | Bin 0 -> 54 bytes
 image/test/gtest/moz.build                      |   1 +
 7 files changed, 37 insertions(+), 9 deletions(-)
 create mode 100644 image/test/gtest/invalid-truncated-metadata.bmp

diff --git a/image/Decoder.cpp b/image/Decoder.cpp
index 4d317703b023..4f6299aa9ad8 100644
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -128,6 +128,12 @@ Decoder::Decode(NotNull<IResumable*> aOnResume)
   // terminal state) or there are no more chunks available.
   Maybe<TerminalState> terminalState;
   do {
+    if (GetDecodeDone()) {
+      MOZ_ASSERT_UNREACHABLE("Finished decode without reaching terminal state?");
+      terminalState = Some(TerminalState::SUCCESS);
+      break;
+    }
+
     switch (mIterator->AdvanceOrScheduleResume(aOnResume.get())) {
       case SourceBufferIterator::WAITING:
         // We can't continue because the rest of the data hasn't arrived from
@@ -166,17 +172,17 @@ Decoder::Decode(NotNull<IResumable*> aOnResume)
         MOZ_ASSERT_UNREACHABLE("Unknown SourceBufferIterator state");
         terminalState = Some(TerminalState::FAILURE);
     }
-  } while (!GetDecodeDone() && !terminalState);
+  } while (!terminalState);
+
+  MOZ_ASSERT(terminalState);
 
   // If decoding failed, record that fact.
   if (terminalState == Some(TerminalState::FAILURE)) {
     PostDataError();
   }
 
-  // If we're done decoding, perform final cleanup.
-  if (terminalState) {
-    CompleteDecode();
-  }
+  // We're done decoding; perform final cleanup.
+  CompleteDecode();
 
   return HasError() ? NS_ERROR_FAILURE : NS_OK;
 }
diff --git a/image/decoders/nsBMPDecoder.cpp b/image/decoders/nsBMPDecoder.cpp
index 6faaffeb2b9c..dd2054d7a480 100644
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -581,10 +581,8 @@ nsBMPDecoder::ReadInfoHeaderRest(const char* aData, size_t aLength)
     return Transition::TerminateFailure();
   }
 
-  // Post our size to the superclass.
-  uint32_t absHeight = AbsoluteHeight();
-  PostSize(mH.mWidth, absHeight);
-  mCurrentRow = absHeight;
+  // Initialize our current row to the top of the image.
+  mCurrentRow = AbsoluteHeight();
 
   // Round it up to the nearest byte count, then pad to 4-byte boundary.
   // Compute this even for a metadate decode because GetCompressedImageSize()
@@ -652,6 +650,9 @@ nsBMPDecoder::ReadBitfields(const char* aData, size_t aLength)
     PostHasTransparency();
   }
 
+  // Post our size to the superclass.
+  PostSize(mH.mWidth, AbsoluteHeight());
+
   // We've now read all the headers. If we're doing a metadata decode, we're
   // done.
   if (IsMetadataDecode()) {
diff --git a/image/test/gtest/Common.cpp b/image/test/gtest/Common.cpp
index c959d2c4696c..6f9e1571140b 100644
--- a/image/test/gtest/Common.cpp
+++ b/image/test/gtest/Common.cpp
@@ -533,6 +533,15 @@ ImageTestCase CorruptTestCase()
                        TEST_CASE_HAS_ERROR);
 }
 
+ImageTestCase CorruptBMPWithTruncatedHeader()
+{
+  // This BMP has a header which is truncated right between the BIH and the
+  // bitfields, which is a particularly error-prone place w.r.t. the BMP decoder
+  // state machine.
+  return ImageTestCase("invalid-truncated-metadata.bmp", "image/bmp",
+                       IntSize(100, 100), TEST_CASE_HAS_ERROR);
+}
+
 ImageTestCase CorruptICOWithBadBMPWidthTestCase()
 {
   // This ICO contains a BMP icon which has a width that doesn't match the size
diff --git a/image/test/gtest/Common.h b/image/test/gtest/Common.h
index 366603fd388f..13497ea66f62 100644
--- a/image/test/gtest/Common.h
+++ b/image/test/gtest/Common.h
@@ -340,6 +340,7 @@ ImageTestCase GreenFirstFrameAnimatedGIFTestCase();
 ImageTestCase GreenFirstFrameAnimatedPNGTestCase();
 
 ImageTestCase CorruptTestCase();
+ImageTestCase CorruptBMPWithTruncatedHeader();
 ImageTestCase CorruptICOWithBadBMPWidthTestCase();
 ImageTestCase CorruptICOWithBadBMPHeightTestCase();
 
diff --git a/image/test/gtest/TestDecoders.cpp b/image/test/gtest/TestDecoders.cpp
index 2ea09bd5b650..8f5e4b9e3677 100644
--- a/image/test/gtest/TestDecoders.cpp
+++ b/image/test/gtest/TestDecoders.cpp
@@ -340,6 +340,16 @@ TEST_F(ImageDecoders, CorruptMultiChunk)
   CheckDecoderMultiChunk(CorruptTestCase());
 }
 
+TEST_F(ImageDecoders, CorruptBMPWithTruncatedHeaderSingleChunk)
+{
+  CheckDecoderSingleChunk(CorruptBMPWithTruncatedHeader());
+}
+
+TEST_F(ImageDecoders, CorruptBMPWithTruncatedHeaderMultiChunk)
+{
+  CheckDecoderMultiChunk(CorruptBMPWithTruncatedHeader());
+}
+
 TEST_F(ImageDecoders, CorruptICOWithBadBMPWidthSingleChunk)
 {
   CheckDecoderSingleChunk(CorruptICOWithBadBMPWidthTestCase());
diff --git a/image/test/gtest/invalid-truncated-metadata.bmp b/image/test/gtest/invalid-truncated-metadata.bmp
new file mode 100644
index 0000000000000000000000000000000000000000..228c5c99923bbc940c6178ed08637c86cf38bdc4
GIT binary patch
literal 54
pcmZ?rb&6nO00Ac;)&OD$Mh1otK$?+3fPooEGXUjZ02v?*0|1zS1q%QG

literal 0
HcmV?d00001

diff --git a/image/test/gtest/moz.build b/image/test/gtest/moz.build
index 0d6f4ecf2c8d..4d1fa22bd0be 100644
--- a/image/test/gtest/moz.build
+++ b/image/test/gtest/moz.build
@@ -50,6 +50,7 @@ TEST_HARNESS_FILES.gtest += [
     'green.icon',
     'green.jpg',
     'green.png',
+    'invalid-truncated-metadata.bmp',
     'no-frame-delay.gif',
     'rle4.bmp',
     'rle8.bmp',