Bug 1388590 - StreamingLexer::Clone should bail if SourceBufferIterator::Advance returns not ready. r=tnikkel, a=lizzard
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 15 Aug 2017 17:44:03 -0400
changeset 421473 45080fa9b68c260a3f493e3415993a82c4eff826
parent 421472 dac913e90cba86c934d5302814c823a3e7c34caf
child 421474 a84600cca0fe71ad285cb8b87894c00cebe2ed48
push id7691
push userryanvm@gmail.com
push dateTue, 29 Aug 2017 23:43:59 +0000
treeherdermozilla-beta@1fd4f4337aae [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel, lizzard
bugs1388590
milestone56.0
Bug 1388590 - StreamingLexer::Clone should bail if SourceBufferIterator::Advance returns not ready. r=tnikkel, a=lizzard StreamingLexer::Clone should always succeed because we are merely creating a new SourceBufferIterator which is at the same position as the given iterator. However it is possible if there is no more data after, the current position, it could return COMPLETE instead of READY. This should not happen during the first Advance loop however. We handle the failure gracefully now, and if someone files a report with the invalid ICO file causing this problem, then we can investigate further.
image/StreamingLexer.h
image/decoders/nsICODecoder.cpp
--- a/image/StreamingLexer.h
+++ b/image/StreamingLexer.h
@@ -396,18 +396,18 @@ public:
   /**
    * From the given SourceBufferIterator, aIterator, create a new iterator at
    * the same position, with the given read limit, aReadLimit. The read limit
    * applies after adjusting for the position. If the given iterator has been
    * advanced, but required buffering inside StreamingLexer, the position
    * of the cloned iterator will be at the beginning of buffered data; this
    * should match the perspective of the caller.
    */
-  SourceBufferIterator Clone(SourceBufferIterator& aIterator,
-                             size_t aReadLimit) const
+  Maybe<SourceBufferIterator> Clone(SourceBufferIterator& aIterator,
+                                    size_t aReadLimit) const
   {
     // In order to advance to the current position of the iterator from the
     // perspective of the caller, we need to take into account if we are
     // buffering data.
     size_t pos = aIterator.Position();
     if (!mBuffer.empty()) {
       pos += aIterator.Length();
       MOZ_ASSERT(pos > mBuffer.length());
@@ -420,28 +420,36 @@ public:
     }
 
     SourceBufferIterator other = aIterator.Owner()->Iterator(readLimit);
 
     // Since the current iterator has already advanced to this point, we
     // know that the state can only be READY or COMPLETE. That does not mean
     // everything is stored in a single chunk, and may require multiple Advance
     // calls to get where we want to be.
-    DebugOnly<SourceBufferIterator::State> state;
+    SourceBufferIterator::State state;
     do {
       state = other.Advance(pos);
-      MOZ_ASSERT(state != SourceBufferIterator::WAITING);
+      if (state != SourceBufferIterator::READY) {
+        MOZ_ASSERT_UNREACHABLE("Cannot advance to existing position");
+        return Nothing();
+      }
       MOZ_ASSERT(pos >= other.Length());
       pos -= other.Length();
     } while (pos > 0);
 
     // Force the data pointer to be where we expect it to be.
     state = other.Advance(0);
-    MOZ_ASSERT(state != SourceBufferIterator::WAITING);
-    return other;
+    if (state != SourceBufferIterator::READY) {
+      // The current position could be the end of the buffer, in which case
+      // there is no point cloning with no more data to read.
+      MOZ_ASSERT(state == SourceBufferIterator::COMPLETE);
+      return Nothing();
+    }
+    return Some(Move(other));
   }
 
   template <typename Func>
   LexerResult Lex(SourceBufferIterator& aIterator,
                   IResumable* aOnResume,
                   Func aFunc)
   {
     if (mTransition.NextStateIsTerminal()) {
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -189,33 +189,41 @@ LexerTransition<ICOState>
 nsICODecoder::IterateUnsizedDirEntry()
 {
   MOZ_ASSERT(!mUnsizedDirEntries.IsEmpty());
 
   if (!mDirEntry) {
     // The first time we are here, there is no entry selected. We must prepare a
     // new iterator for the contained decoder to advance as it wills. Cloning at
     // this point ensures it will begin at the end of the dir entries.
-    mReturnIterator.emplace(mLexer.Clone(*mIterator, SIZE_MAX));
+    mReturnIterator = Move(mLexer.Clone(*mIterator, SIZE_MAX));
+    if (mReturnIterator.isNothing()) {
+      // If we cannot read further than this point, then there is no resource
+      // data to read.
+      return Transition::TerminateFailure();
+    }
   } else {
     // We have already selected an entry which means a metadata decoder has
     // finished. Verify the size is valid and if so, add to the discovered
     // resources.
     if (mDirEntry->mSize.width > 0 && mDirEntry->mSize.height > 0) {
       mDirEntries.AppendElement(*mDirEntry);
     }
 
     // Remove the entry from the unsized list either way.
     mDirEntry = nullptr;
     mUnsizedDirEntries.RemoveElementAt(0);
 
     // Our iterator is at an unknown point, so reset it to the point that we
     // saved.
-    mIterator.reset();
-    mIterator.emplace(mLexer.Clone(*mReturnIterator, SIZE_MAX));
+    mIterator = Move(mLexer.Clone(*mReturnIterator, SIZE_MAX));
+    if (mIterator.isNothing()) {
+      MOZ_ASSERT_UNREACHABLE("Cannot re-clone return iterator");
+      return Transition::TerminateFailure();
+    }
   }
 
   // There are no more unsized entries, so we can finally decide which entry to
   // select for decoding.
   if (mUnsizedDirEntries.IsEmpty()) {
     mReturnIterator.reset();
     return Transition::To(ICOState::FINISHED_DIR_ENTRY, 0);
   }
@@ -339,26 +347,29 @@ nsICODecoder::SniffResource(const char* 
                        PNGSIGNATURESIZE);
   if (isPNG) {
     if (mDirEntry->mBytesInRes <= BITMAPINFOSIZE) {
       return Transition::TerminateFailure();
     }
 
     // Prepare a new iterator for the contained decoder to advance as it wills.
     // Cloning at the point ensures it will begin at the resource offset.
-    SourceBufferIterator containedIterator
+    Maybe<SourceBufferIterator> containedIterator
       = mLexer.Clone(*mIterator, mDirEntry->mBytesInRes);
+    if (containedIterator.isNothing()) {
+      return Transition::TerminateFailure();
+    }
 
     // Create a PNG decoder which will do the rest of the work for us.
     bool metadataDecode = mReturnIterator.isSome();
     Maybe<IntSize> expectedSize = metadataDecode ? Nothing()
                                                  : Some(mDirEntry->mSize);
     mContainedDecoder =
       DecoderFactory::CreateDecoderForICOResource(DecoderType::PNG,
-                                                  Move(containedIterator),
+                                                  Move(containedIterator.ref()),
                                                   WrapNotNull(this),
                                                   metadataDecode,
                                                   expectedSize);
 
     // Read in the rest of the PNG unbuffered.
     size_t toRead = mDirEntry->mBytesInRes - BITMAPINFOSIZE;
     return Transition::ToUnbuffered(ICOState::FINISHED_RESOURCE,
                                     ICOState::READ_RESOURCE,
@@ -406,27 +417,30 @@ nsICODecoder::ReadBIH(const char* aData)
   // The ICO format when containing a BMP does not include the 14 byte
   // bitmap file header. So we create the BMP decoder via the constructor that
   // tells it to skip this, and pass in the required data (dataOffset) that
   // would have been present in the header.
   uint32_t dataOffset = bmp::FILE_HEADER_LENGTH + BITMAPINFOSIZE + 4 * numColors;
 
   // Prepare a new iterator for the contained decoder to advance as it wills.
   // Cloning at the point ensures it will begin at the resource offset.
-  SourceBufferIterator containedIterator
+  Maybe<SourceBufferIterator> containedIterator
     = mLexer.Clone(*mIterator, mDirEntry->mBytesInRes);
+  if (containedIterator.isNothing()) {
+    return Transition::TerminateFailure();
+  }
 
   // Create a BMP decoder which will do most of the work for us; the exception
   // is the AND mask, which isn't present in standalone BMPs.
   bool metadataDecode = mReturnIterator.isSome();
   Maybe<IntSize> expectedSize = metadataDecode ? Nothing()
                                                : Some(mDirEntry->mSize);
   mContainedDecoder =
     DecoderFactory::CreateDecoderForICOResource(DecoderType::BMP,
-                                                Move(containedIterator),
+                                                Move(containedIterator.ref()),
                                                 WrapNotNull(this),
                                                 metadataDecode,
                                                 expectedSize,
                                                 Some(dataOffset));
 
   RefPtr<nsBMPDecoder> bmpDecoder =
     static_cast<nsBMPDecoder*>(mContainedDecoder.get());