Limit the maximum buffer size of SkRawBufferedStream

src/codec/SkRawCodec.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodec.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 181 matching lines @@
    virtual bool read(void* data, size_t offset, size_t length) = 0;
 
     /*
      * Creates an SkMemoryStream from the offset with size.
      * Note: for performance reason, this function is destructive to the SkRawStream. One should
      *       abandon current object after the function call.
      */
    virtual SkMemoryStream* transferBuffer(size_t offset, size_t size) = 0;
 };
 
+class SkSizeLimitedDynamicMemoryWStream : public SkDynamicMemoryWStream {
+public:
+    virtual ~SkSizeLimitedDynamicMemoryWStream() {}
+
+    bool write(const void* buffer, size_t size) override {
+        size_t newSize;
+        if (!safe_add_to_size_t(this->bytesWritten(), size, &newSize) ||

[scroggo, 2016/03/09 16:32:45]

If we limit to 10,240,000, do we still need to check safe_add_to_size_t? That
fits in a 32 bit integer, and I assume size_t is at least that size.

I guess we *could* revisit that number, at which point we will need
safe_add_to_size_t. If you remove the call, add a comment why it's not
necessary.

[yujieqin, 2016/03/09 17:40:30]

I still prefer to check here, because there is not limit on the argument "size",
which can surely lead to overflow as soon as the bytesWritten() is larger than
0.

+            newSize > kMaxStreamSize) {

[scroggo, 2016/03/09 16:32:45]

nit: this lines up with the line below. Would be better either if it's indented
an extra four spaces or if the brace is on the next line by itself. (Unless we
remove safe_add_to_size_t, in which case, this fits on one line.

[yujieqin, 2016/03/09 17:40:30]

Done.

+            return false;

[scroggo, 2016/03/09 16:32:45]

Should we print some kind of error message? It seems like this *could* be a
valid DNG, but we've set a cutoff.

[yujieqin, 2016/03/09 17:40:30]

added a SkCodecPrintf

+        }
+        return SkDynamicMemoryWStream::write(buffer, size);

[scroggo, 2016/03/09 16:32:45]

typically in Skia, we use a typedef INHERITED to refer to the base class. So
this should have a typedef in private, and this line should look like:

return this->INHERITED::write(buffer, size);

[yujieqin, 2016/03/09 17:40:30]

Done.

+    }
+
+private:
+    const size_t kMaxStreamSize = 100 * 1024 * 1024;  // 100MB
+};
+
+// Note: the maximum buffer size is 100MB (limited by SkSizeLimitedDynamicMemoryWStream).
 class SkRawBufferedStream : public SkRawStream {
 public:
     // Will take the ownership of the stream.
     explicit SkRawBufferedStream(SkStream* stream)
         : fStream(stream)
         , fWholeStreamRead(false)
     {
         // Only use SkRawBufferedStream when the stream is not an asset stream.
         SkASSERT(!is_asset_stream(*stream));
     }
@@ skipping 82 matching lines @@
         const size_t bytesRead = fStream->read(tempBuffer.get(), sizeToRead);
         if (bytesRead < sizeRequested) {
             return false;
         }
         return fStreamBuffer.write(tempBuffer.get(), bytesRead);
     }
 
     SkAutoTDelete<SkStream> fStream;
     bool fWholeStreamRead;
 
-    SkDynamicMemoryWStream fStreamBuffer;
+    // Use a size-limited stream to avoid holding too huge buffer.
+    SkSizeLimitedDynamicMemoryWStream fStreamBuffer;
 
     const size_t kReadToEnd = 0;
 };
 
 class SkRawAssetStream : public SkRawStream {
 public:
     // Will take the ownership of the stream.
     explicit SkRawAssetStream(SkStream* stream)
         : fStream(stream)
     {
@@ skipping 394 matching lines @@
     SkISize sizeFloor = this->onGetScaledDimensions(1.f / std::floor(fullShortEdge / shortEdge));
     SkISize sizeCeil = this->onGetScaledDimensions(1.f / std::ceil(fullShortEdge / shortEdge));
     return sizeFloor == dim || sizeCeil == dim;
 }
 
 SkRawCodec::~SkRawCodec() {}
 
 SkRawCodec::SkRawCodec(SkDngImage* dngImage)
     : INHERITED(dngImage->getImageInfo(), nullptr)
     , fDngImage(dngImage) {}