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 31 matching lines @@
 }
 
 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;
 
     // Return a pointer into |m_data| if the request doesn't span segments.

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

We can simplify the function by replacing everything from here on with:

    char* out_ptr = m_segment;
    for (char* copy_dest = buffer; ; copy_dest += m_segmentLength) {
        m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
        ASSERT(m_segmentLength);
        if (out_ptr == buffer)
            memcpy(copy_dest, m_segment, m_segmentLength);
        if (length <= m_segmentLength)
            return out_ptr;
        if (out_ptr != buffer) {
            out_ptr = buffer;
            memcpy(copy_dest, m_segment, m_segmentLength);
        }
        m_dataPosition += m_segmentLength;
        length -= m_segmentLength;
    }

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

Quite frankly I don't see how is this simpler. To me separating the cases when
pointer to m_segment is returned and when the data is copied to output buffer is
much easier to reason about.

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

Fair enough.  "Simpler" in this case means that it's a half-dozen lines shorter
and we're not duplicating the getSomeData() call, but it does make the control
flow a bit thornier.  I'm fine leaving it.

     m_dataPosition = dataPosition;
     m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
     ASSERT(m_segmentLength);
     if (length <= m_segmentLength)
         return m_segment;
 
-    for (char* tempBuffer = buffer; length;) {
+    char* dest = buffer;

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

Nit: Convert the while to a for so you can move this declaration inside it
("declare variables in the narrowest possible scope").

+    while (true) {
         size_t copy = std::min(length, m_segmentLength);
-        memcpy(tempBuffer, m_segment, copy);
+        memcpy(dest, m_segment, copy);
         m_dataPosition += copy;

[kbalazs, 2015/03/25 19:38:10]

I also introduced an error. m_dataPosition should not be changed if we are not
continue reading. I will make some refactor too make this code easier to read.

         length -= copy;
-        tempBuffer += copy;
+        if (!length) {
+            // Done.
+            return buffer;
+        }
 
+        // Continue reading the next segment.
+        dest += copy;
         m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
         ASSERT(m_segmentLength);
     }
-    return buffer;
+
+    ASSERT_NOT_REACHED();

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

I think these two lines will trigger MSVC warning C4702, and you should remove
them.

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

Acknowledged.

+    return 0;
 }
 
 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;
 }
 
 } // namespace blink