Fix potential bug in FastSharedBufferReader::getConsecutiveData

Source/platform/image-decoders/FastSharedBufferReaderTest.cpp

 /*
  * Copyright (C) 2015 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 20 matching lines @@
 #include "config.h"
 
 #include "platform/image-decoders/FastSharedBufferReader.h"
 
 #include <gtest/gtest.h>
 
 namespace blink {
 
 namespace {
 
+const unsigned segmentSize = 0x1000;

[Peter Kasting, 2015/03/20 01:10:42]

I think you should make the existing constant in SharedBuffer.cpp public and
then use it here.

[kbalazs, 2015/03/20 20:20:42]

Acknowledged.

+const unsigned defaultTestSize = 4 * segmentSize;
+
 void prepareReferenceData(char* buffer, size_t size)
 {
     for (size_t i = 0; i < size; ++i)
         buffer[i] = i;
 }
 
 } // namespace
 
 TEST(FastSharedBufferReaderTest, nonSequentialReads)
 {
     // This is 4 times SharedBuffer's segment size.
-    char referenceData[16384];
+    char referenceData[defaultTestSize];
     prepareReferenceData(referenceData, sizeof(referenceData));
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
     FastSharedBufferReader reader(data);
 
     // Read size is prime such there will be a segment-spanning
     // read eventually.
     char tempBuffer[17];
     for (size_t dataPosition = 0; dataPosition + sizeof(tempBuffer) < sizeof(referenceData); dataPosition += sizeof(tempBuffer)) {
         const char* block = reader.getConsecutiveData(
             dataPosition, sizeof(tempBuffer), tempBuffer);
         ASSERT_FALSE(memcmp(block, referenceData + dataPosition, sizeof(tempBuffer)));
     }
 }
 
 TEST(FastSharedBufferReaderTest, readBackwards)
 {
     // This is 4 times SharedBuffer's segment size.
-    char referenceData[16384];
+    char referenceData[defaultTestSize];
     prepareReferenceData(referenceData, sizeof(referenceData));
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
     FastSharedBufferReader reader(data);
 
     // Read size is prime such there will be a segment-spanning
     // read eventually.
     char tempBuffer[17];
     for (size_t dataOffset = sizeof(tempBuffer); dataOffset < sizeof(referenceData); dataOffset += sizeof(tempBuffer)) {
         const char* block = reader.getConsecutiveData(
             sizeof(referenceData) - dataOffset, sizeof(tempBuffer), tempBuffer);
         ASSERT_FALSE(memcmp(block, referenceData + sizeof(referenceData) - dataOffset, sizeof(tempBuffer)));
     }
 }
 
 TEST(FastSharedBufferReaderTest, byteByByte)
 {
     // This is 4 times SharedBuffer's segment size.
-    char referenceData[16384];
+    char referenceData[defaultTestSize];
     prepareReferenceData(referenceData, sizeof(referenceData));
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
     FastSharedBufferReader reader(data);
     for (size_t i = 0; i < sizeof(referenceData); ++i) {
         ASSERT_EQ(referenceData[i], reader.getOneByte(i));
     }
 }
 
+TEST(FastSharedBufferReaderTest, readOverlappingLastSegmentBoundary)

[Peter Kasting, 2015/03/20 01:10:42]

This test should have comments describing the case it's testing, specifically,
that we need to do a read from (before the last segment) to (the very end of the
last segment).

[kbalazs, 2015/03/20 20:20:42]

Actually the read doesn't have to go for the very end, just overlap the one
before last and the last segments. I will add comment.

[Peter Kasting, 2015/03/20 20:29:33]

Are you sure?  I read the original code and AFAICT it wouldn't have a problem
unless the read in question went to the end of the last buffer:

* We'd first getSomeData() from the second-to-last segment.
* Then the loop would start.  We'd copy that data to the buffer, and request
data from the start of the last segment.  This would return the entire last
segment.
* The loop would iterate.  We'd copy min(length, m_segmentLength) bytes.  More
critically, we'd also increment m_dataPosition by that minimum value.  So if
length < m_segmentLength at this point, m_dataPosition would now be somewhere in
the middle of the last segment.
* We'd getSomeData() again.  Since m_dataPosition wouldn't be off-the-end, the
call would succeed.

This is why the existing test didn't show the issue, despite the fact that it
does read across the boundary of the last two segments.

+{
+    char referenceData[2 * segmentSize];
+    prepareReferenceData(referenceData, sizeof(referenceData));
+    RefPtr<SharedBuffer> data = SharedBuffer::create();
+    // Append it one segment at a time.

[Peter Kasting, 2015/03/20 01:10:42]

Why is this necessary?  If seems like SharedBuffer::append() should do the same
thing whether called once with a larger block or twice with a smaller.

[kbalazs, 2015/03/20 20:20:42]

Correct.

+    data->append(referenceData, segmentSize);
+    data->append(referenceData + segmentSize, segmentSize);
+
+    char buffer[2 * segmentSize];
+    FastSharedBufferReader reader(data);
+    reader.getConsecutiveData(0, 2 * segmentSize, buffer);
+
+    ASSERT_FALSE(memcmp(buffer, referenceData, 2 * segmentSize));
+}
+
 } // namespace blink