Fix potential bug in FastSharedBufferReader::getConsecutiveData

Source/platform/image-decoders/FastSharedBufferReaderTest.cpp

Index: Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
diff --git a/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp b/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
index 2a989f9b0e7e1e8cdf40a78a3119e92f32a20fb0..7979441ce230a54406e233c6e6adfb82694818b2 100644
--- a/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
+++ b/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
@@ -38,6 +38,9 @@ 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)
@@ -49,7 +52,7 @@ void prepareReferenceData(char* buffer, size_t size)
 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));
@@ -69,7 +72,7 @@ TEST(FastSharedBufferReaderTest, nonSequentialReads)
 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));
@@ -89,7 +92,7 @@ TEST(FastSharedBufferReaderTest, readBackwards)
 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));
@@ -100,4 +103,20 @@ TEST(FastSharedBufferReaderTest, byteByByte)
     }
 }
 
+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