Fix fragmented image signature handling

third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp

 /*
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Library General Public
  *  License as published by the Free Software Foundation; either
  *  version 2 of the License, or (at your option) any later version.
  *
  *  This library is distributed in the hope that it will be useful,
  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
  *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  *  Library General Public License for more details.
  *
  *  You should have received a copy of the GNU Library General Public License
  *  along with this library; see the file COPYING.LIB.  If not, write to
  *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  *  Boston, MA 02110-1301, USA.
  *
  */
 
 #include "platform/image-decoders/ImageDecoder.h"
 
 #include "platform/PlatformInstrumentation.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/graphics/BitmapImageMetrics.h"
+#include "platform/image-decoders/FastSharedBufferReader.h"
 #include "platform/image-decoders/bmp/BMPImageDecoder.h"
 #include "platform/image-decoders/gif/GIFImageDecoder.h"
 #include "platform/image-decoders/ico/ICOImageDecoder.h"
 #include "platform/image-decoders/jpeg/JPEGImageDecoder.h"
 #include "platform/image-decoders/png/PNGImageDecoder.h"
 #include "platform/image-decoders/webp/WEBPImageDecoder.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 namespace blink {
@@ skipping 11 matching lines @@
 #endif // USE(QCMSLIB)
 
 
 inline bool matchesJPEGSignature(const char* contents)
 {
     return !memcmp(contents, "\xFF\xD8\xFF", 3);
 }
 
 inline bool matchesPNGSignature(const char* contents)
 {
     return !memcmp(contents, "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A", 8);

[Peter Kasting, 2016/08/16 22:10:18]

Random aside: I just realized that bytes 2-4 here are "PNG"... it'd be a little
more readable IMO if this code said that...

 }
 
 inline bool matchesGIFSignature(const char* contents)
 {
     return !memcmp(contents, "GIF87a", 6) || !memcmp(contents, "GIF89a", 6);
 }
 
 inline bool matchesWebPSignature(const char* contents)
 {
     return !memcmp(contents, "RIFF", 4) && !memcmp(contents + 8, "WEBPVP", 6);
@@ skipping 32 matching lines @@
     case SniffResult::BMP:
         return wrapUnique(new BMPImageDecoder(alphaOption, colorOptions, maxDecodedBytes));
     case SniffResult::InsufficientData:
     case SniffResult::Invalid:
         return nullptr;
     }
     NOTREACHED();
     return nullptr;
 }
 
+namespace {
+
+constexpr size_t kLongestSignatureLength = sizeof("RIFF????WEBPVP") - 1;
+static_assert(kLongestSignatureLength == 14, "kLongestSignatureLength mismatch");

[Peter Kasting, 2016/08/16 22:10:18]

Hmm.  This static_assert duplicates the old DCHECK, but it seems a little
unhelpful to me.  For example, it doesn't prevent us from introducing a memory
error bug by changing/adding a matchesXXXSignature function that simply reads
more than 14 bytes.  It mostly feels like a change detector.

In theory, determineImageType() doesn't really need to wait for the longest
signature to come in (and I suppose said signature could be longer than the
shortest valid file of some other type, although that seems unlikely).  I'm
imagining some sort of system where each filetype detector asks for as many
bytes as it needs and they're auto-tried in shortest-to-longest order, but
that's all probably way over-engineered for what we need.

Perhaps just replacing the assertion here with a simple comment that this must
be updated if we add a matchesXXXLength() function that requires more data is
good enough.

[f(malita), 2016/08/17 17:09:31]

Done.

Another thing we could do is add static asserts to all matchesFooSignature()
methods, to check against their actual strings.  I'll do that in a follow-up if
it sounds like a good idea.

[Peter Kasting, 2016/08/17 22:46:12]

You mean, like, asserting that the length of "BM" is really 2?  That also seems
unlikely to catch problems since we basically write these functions once and
don't modify the signatures later.  I'd skip it.

[f(malita), 2016/08/17 23:19:08]

More like refactoring all signature matchers to call a common helper instead of
direct memcmp(), and then assert that what we're trying to match <=
kLongestSignatureLength.  But yeah, that won't prevent anyone from adding a new
signature method which doesn't call the helper.

A better approach would be to use some sort of signature->factory registry +
generalized signature matcher.  That would force everyone onto a common code
path where we can check the invariant, but unfortunately we have the two-piece
"RIFF????WEBPVP" to complicate things.  Solvable, but we're in over-engineered
territory by now - so I'll take your advice and pass :)

+
+} // anonymous ns
+
 ImageDecoder::SniffResult ImageDecoder::determineImageType(const char* contents, size_t length)
 {
-    const size_t longestSignatureLength = sizeof("RIFF????WEBPVP") - 1;
-    DCHECK_EQ(14u, longestSignatureLength);
-
-    if (length < longestSignatureLength)
+    if (length < kLongestSignatureLength)
         return SniffResult::InsufficientData;
     if (matchesJPEGSignature(contents))
         return SniffResult::JPEG;
     if (matchesPNGSignature(contents))
         return SniffResult::PNG;
     if (matchesGIFSignature(contents))
         return SniffResult::GIF;
     if (matchesWebPSignature(contents))
         return SniffResult::WEBP;
     if (matchesICOSignature(contents) || matchesCURSignature(contents))
         return SniffResult::ICO;
     if (matchesBMPSignature(contents))
         return SniffResult::BMP;
     return SniffResult::Invalid;
 }
 
 ImageDecoder::SniffResult ImageDecoder::determineImageType(const SharedBuffer& data)
 {
-    const char* contents;
-    const size_t length = data.getSomeData<size_t>(contents);
-    return determineImageType(contents, length);
+    // TODO(fmalita): refactor the method signature to avoid casting.
+    RefPtr<SegmentReader> reader = SegmentReader::createFromSharedBuffer(const_cast<SharedBuffer*>(&data));

[f(malita), 2016/08/16 21:54:03]

I'm trying to minimize the footprint in order to facilitate merging.  Will clean
up in a separate CL.

[Peter Kasting, 2016/08/16 22:10:18]

It might be easier to understand to make this CL do the clean thing you want,
and then make the merge CLs be more hacky.  That way we can decide at merge time
if it's easy enough to just go ahead and land the cleaner version.

[f(malita), 2016/08/17 17:09:31]

This is what I have in mind (not ready for review just yet):
https://codereview.chromium.org/2257513002/

Essentially fold the signature sniffer into the factory, and get rid of the
public sniffer methods if possible.

It's significantly more involved, so I'd rather stick with a localized fix for
the regression and handle the cleanup separately.

[Peter Kasting, 2016/08/17 22:46:13]

Sounds like a good plan.

+
+    return determineImageType(*reader);
 }
 
 ImageDecoder::SniffResult ImageDecoder::determineImageType(const SegmentReader& data)
 {
-    const char* contents;
-    const size_t length = data.getSomeData(contents, 0);
-    return determineImageType(contents, length);
+    // TODO(fmalita): refactor the method signature to avoid casting.
+    const FastSharedBufferReader fastReader(const_cast<SegmentReader*>(&data));
+    char buffer[kLongestSignatureLength];
+    const size_t len = std::min(kLongestSignatureLength, data.size());
+
+    return determineImageType(fastReader.getConsecutiveData(0, len, buffer), len);
 }
 
 size_t ImageDecoder::frameCount()
 {
     const size_t oldSize = m_frameBufferCache.size();
     const size_t newSize = decodeFrameCount();
     if (oldSize != newSize) {
         m_frameBufferCache.resize(newSize);
         for (size_t i = oldSize; i < newSize; ++i) {
             m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha);
@@ skipping 251 matching lines @@
         return;
 
     qcms_data_type dataFormat = hasAlpha ? QCMS_DATA_RGBA_8 : QCMS_DATA_RGB_8;
 
     // FIXME: Don't force perceptual intent if the image profile contains an intent.
     m_sourceToOutputDeviceColorTransform.reset(qcms_transform_create(inputProfile.get(), dataFormat, gTargetColorProfile, QCMS_DATA_RGBA_8, QCMS_INTENT_PERCEPTUAL));
 #endif // USE(QCMSLIB)
 }
 
 } // namespace blink