Bug 1388590 - StreamingLexer::Clone should bail if SourceBufferIterator::Advance returns not ready. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 15 Aug 2017 17:44:03 -0400
changeset 375017 4c956cbe4e5fc00f5650b902b3d551cab2728828
parent 375016 479d2103841e154147b5509cb5ade7f2ecbacc44
child 375018 2241628188e42f3d716132e2388aa60f054bdf31
push id32344
push usercbook@mozilla.com
push dateWed, 16 Aug 2017 09:23:42 +0000
treeherdermozilla-central@6ebc251bd288 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1388590
milestone57.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 1388590 - StreamingLexer::Clone should bail if SourceBufferIterator::Advance returns not ready. r=tnikkel 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());