Fix potential bug in FastSharedBufferReader::getConsecutiveData

Source/platform/image-decoders/FastSharedBufferReader.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 17 matching lines @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "config.h"
 #include "platform/image-decoders/FastSharedBufferReader.h"
 
 namespace blink {
 
 FastSharedBufferReader::FastSharedBufferReader(PassRefPtr<SharedBuffer> data)
     : m_data(data)
-    , m_segment(0)
-    , m_segmentLength(0)
-    , m_dataPosition(0)
 {
 }
 
+FastSharedBufferReader::Cache::Cache()
+    : segment(0)
+    , segmentLength(0)
+    , dataPosition(0)
+{
+}
+
 const char* FastSharedBufferReader::getConsecutiveData(size_t dataPosition, size_t length, char* buffer)
 {
     RELEASE_ASSERT(dataPosition + length <= m_data->size());
 
     // Use the cached segment if it can serve the request.
-    if (dataPosition >= m_dataPosition && dataPosition + length <= m_dataPosition + m_segmentLength)
-        return m_segment + dataPosition - m_dataPosition;
+    size_t cacheEndPosition = m_cache.dataPosition + m_cache.segmentLength;
+    if (dataPosition >= m_cache.dataPosition && dataPosition + length <= cacheEndPosition) {
+        size_t skip = dataPosition - m_cache.dataPosition;
+        return m_cache.segment + skip;
+    }

[Peter Kasting, 2015/03/25 19:53:11]

Nit: The old code was more readable than this more verbose version.

 
     // Return a pointer into |m_data| if the request doesn't span segments.
-    m_dataPosition = dataPosition;
-    m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
-    ASSERT(m_segmentLength);
-    if (length <= m_segmentLength)
-        return m_segment;
+    getSomeDataIntoCache(dataPosition);
+    if (length <= m_cache.segmentLength)
+        return m_cache.segment;
 
-    for (char* tempBuffer = buffer; length;) {
-        size_t copy = std::min(length, m_segmentLength);
-        memcpy(tempBuffer, m_segment, copy);
-        m_dataPosition += copy;
+    for (const char* dest = buffer; ; ) {
+        size_t copy = std::min(length, m_cache.segmentLength);
+        memcpy(dest, m_cache.segment, copy);
         length -= copy;
-        tempBuffer += copy;
+        if (!length) {
+            // Done.

[Peter Kasting, 2015/03/25 19:53:11]

Nit: This comment adds nothing, remove it.

+            return buffer;
+        }
 
-        m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
-        ASSERT(m_segmentLength);
+        // Continue reading the next segment.
+        dest += copy;

[Peter Kasting, 2015/03/25 19:53:11]

Nit: Can place this into the for loop declaration.

[kbalazs, 2015/03/25 21:05:19]

copy is local to the loop

[Peter Kasting, 2015/03/25 21:10:47]

True.  Ignore that.

+        dataPosition += copy;

[Peter Kasting, 2015/03/25 19:53:11]

Nit: Can omit this line and just pass (m_dataPosition + copy) to the function
call below.

+        getSomeDataIntoCache(dataPosition);
     }
-    return buffer;
 }
 
 size_t FastSharedBufferReader::getSomeData(const char*& someData, size_t dataPosition)
 {
-    m_segmentLength = m_data->getSomeData(m_segment, dataPosition);
-    someData = m_segment;
-    m_dataPosition = dataPosition;
-    ASSERT(m_segmentLength);
-    return m_segmentLength;
+    getSomeDataIntoCache(dataPosition);
+    someData = m_cache.segment;
+    return m_cache.segmentLength;
+}
+
+void FastSharedBufferReader::getSomeDataIntoCache(unsigned dataPosition)
+{
+    m_cache.dataPosition = dataPosition;
+    m_cache.segmentLength = m_data->getSomeData(m_cache.segment, dataPosition);
+    ASSERT(m_cache.segmentLength);
 }
 
 } // namespace blink