Fix fragmented image signature handling

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
index 874b7d06e12a3f288ac7c7d56ac845b6f1a6bcfa..dd29894273062c8d3ba9fe332ebb3bb24ed2c541 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
@@ -23,6 +23,7 @@
 #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"
@@ -107,12 +108,16 @@ std::unique_ptr<ImageDecoder> ImageDecoder::create(SniffResult sniffResult, Alph
     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;
@@ -131,16 +136,20 @@ ImageDecoder::SniffResult ImageDecoder::determineImageType(const char* contents,
 
 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()