SkIcoCodec: Read the entire stream into data

Bug: 932080
Bug: b/142252770

An ICO file has a directory of images that are stored later in the file.
The directory contains the offset and size of the images. SkIcoCodec
uses these to create embedded SkPng/SkBmpCodecs. The old implementation
allocated a block of memory for each image and copied the stream into
those blocks so that the embedded SkCodecs could independently read
their encoded data.

Although SkIcoCodec checks for null, this still allows large (albeit
temporary - since we'll discard them if the stream does not contain
enough data to fill them) allocations and the potential for over-
commit.

Instead, read the entire stream into a contiguous buffer. If the stream
is already actually a buffer, just use that directly. In this case, the
new code will do less work. Otherwise, the memory we allocate is
limited by the size of the stream.

Note that this is a behavior change for a stream that contains two
consecutive ICOs, where the client expects to be able to read the second
one later. This was an issue for PNGs on Android (b/34073812), but I
suspect no one is relying on this behavior for ICO. Update Codec_end
test to remove the ICO test.

Alternatives to consider:
- only buffer the individual encoded images. This will allow us to
continue passing the Codec_end test.
- lazily read the embedded streams. Currently we read their start to
verify they are valid images (at least in the header) and read their
actual sizes and bit-depths, which could differ from that listed in
the directory. We use those to make a guess at the "best" image to use.
An image with mismatched sizes may now decode differently.

Change-Id: I30e5f6c8c2e5a0fa135348f61efe151a7f5d4756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/277058
Auto-Submit: Leon Scroggins <scroggo@google.com>
Commit-Queue: Derek Sollenberger <djsollen@google.com>
Reviewed-by: Derek Sollenberger <djsollen@google.com>

src/codec/SkCodecPriv.h

@@ -141,7 +141,7 @@ static inline size_t compute_row_bytes(int width, uint32_t bitsPerPixel) {
  * Get a byte from a buffer
  * This method is unsafe, the caller is responsible for performing a check
  */
-static inline uint8_t get_byte(uint8_t* buffer, uint32_t i) {
+static inline uint8_t get_byte(const uint8_t* buffer, uint32_t i) {
     return buffer[i];
 }
 
@@ -149,7 +149,7 @@ static inline uint8_t get_byte(uint8_t* buffer, uint32_t i) {
  * Get a short from a buffer
  * This method is unsafe, the caller is responsible for performing a check
  */
-static inline uint16_t get_short(uint8_t* buffer, uint32_t i) {
+static inline uint16_t get_short(const uint8_t* buffer, uint32_t i) {
     uint16_t result;
     memcpy(&result, &(buffer[i]), 2);
 #ifdef SK_CPU_BENDIAN
@@ -163,7 +163,7 @@ static inline uint16_t get_short(uint8_t* buffer, uint32_t i) {
  * Get an int from a buffer
  * This method is unsafe, the caller is responsible for performing a check
  */
-static inline uint32_t get_int(uint8_t* buffer, uint32_t i) {
+static inline uint32_t get_int(const uint8_t* buffer, uint32_t i) {
     uint32_t result;
     memcpy(&result, &(buffer[i]), 4);
 #ifdef SK_CPU_BENDIAN

src/codec/SkIcoCodec.cpp

@@ -13,6 +13,7 @@
 #include "src/codec/SkCodecPriv.h"
 #include "src/codec/SkIcoCodec.h"
 #include "src/codec/SkPngCodec.h"
+#include "src/core/SkStreamPriv.h"
 #include "src/core/SkTSort.h"
 
 /*
@@ -28,20 +29,35 @@ bool SkIcoCodec::IsIco(const void* buffer, size_t bytesRead) {
 
 std::unique_ptr<SkCodec> SkIcoCodec::MakeFromStream(std::unique_ptr<SkStream> stream,
                                                     Result* result) {
+    // It is helpful to have the entire stream in a contiguous buffer. In some cases,
+    // this is already the case anyway, so this method is faster. In others, this is
+    // safer than the old method, which required allocating a block of memory whose
+    // byte size is stored in the stream as a uint32_t, and may result in a large or
+    // failed allocation.
+    sk_sp<SkData> data = nullptr;
+    if (stream->getMemoryBase()) {
+        // It is safe to make without copy because we'll hold onto the stream.
+        data = SkData::MakeWithoutCopy(stream->getMemoryBase(), stream->getLength());
+    } else {
+        data = SkCopyStreamToData(stream.get());
+
+        // If we are forced to copy the stream to a data, we can go ahead and delete the stream.
+        stream.reset(nullptr);
+    }
+
     // Header size constants
     constexpr uint32_t kIcoDirectoryBytes = 6;
     constexpr uint32_t kIcoDirEntryBytes = 16;
 
     // Read the directory header
-    std::unique_ptr<uint8_t[]> dirBuffer(new uint8_t[kIcoDirectoryBytes]);
+    if (data->size() < kIcoDirectoryBytes) {
-    if (stream->read(dirBuffer.get(), kIcoDirectoryBytes) != kIcoDirectoryBytes) {
         SkCodecPrintf("Error: unable to read ico directory header.\n");
         *result = kIncompleteInput;
         return nullptr;
     }
 
     // Process the directory header
-    const uint16_t numImages = get_short(dirBuffer.get(), 4);
+    const uint16_t numImages = get_short(data->bytes(), 4);
     if (0 == numImages) {
         SkCodecPrintf("Error: No images embedded in ico.\n");
         *result = kInvalidInput;
@@ -66,8 +82,8 @@ std::unique_ptr<SkCodec> SkIcoCodec::MakeFromStream(std::unique_ptr<SkStream> st
 
     // Iterate over directory entries
     for (uint32_t i = 0; i < numImages; i++) {
-        uint8_t entryBuffer[kIcoDirEntryBytes];
+        const uint8_t* entryBuffer = data->bytes() + kIcoDirectoryBytes + i * kIcoDirEntryBytes;
-        if (stream->read(entryBuffer, kIcoDirEntryBytes) != kIcoDirEntryBytes) {
+        if (data->size() < kIcoDirectoryBytes + (i+1) * kIcoDirEntryBytes) {
             SkCodecPrintf("Error: Dir entries truncated in ico.\n");
             *result = kIncompleteInput;
             return nullptr;
@@ -123,45 +139,36 @@ std::unique_ptr<SkCodec> SkIcoCodec::MakeFromStream(std::unique_ptr<SkStream> st
 
         // If we cannot skip, assume we have reached the end of the stream and
         // stop trying to make codecs
-        if (stream->skip(offset - bytesRead) != offset - bytesRead) {
+        if (offset >= data->size()) {
             SkCodecPrintf("Warning: could not skip to ico offset.\n");
             break;
         }
         bytesRead = offset;
 
-        // Create a new stream for the embedded codec
+        if (offset + size > data->size()) {
-        SkAutoFree buffer(sk_malloc_canfail(size));
-        if (!buffer) {
-            SkCodecPrintf("Warning: OOM trying to create embedded stream.\n");
-            break;
-        }
-
-        if (stream->read(buffer.get(), size) != size) {
             SkCodecPrintf("Warning: could not create embedded stream.\n");
             *result = kIncompleteInput;
             break;
         }
 
-        sk_sp<SkData> data(SkData::MakeFromMalloc(buffer.release(), size));
+        sk_sp<SkData> embeddedData(SkData::MakeSubset(data.get(), offset, size));
-        auto embeddedStream = SkMemoryStream::Make(data);
+        auto embeddedStream = SkMemoryStream::Make(embeddedData);
         bytesRead += size;
 
         // Check if the embedded codec is bmp or png and create the codec
         std::unique_ptr<SkCodec> codec;
         Result dummyResult;
-        if (SkPngCodec::IsPng((const char*) data->bytes(), data->size())) {
+        if (SkPngCodec::IsPng(embeddedData->bytes(), embeddedData->size())) {
             codec = SkPngCodec::MakeFromStream(std::move(embeddedStream), &dummyResult);
         } else {
             codec = SkBmpCodec::MakeFromIco(std::move(embeddedStream), &dummyResult);
         }
 
-        // Save a valid codec
         if (nullptr != codec) {
             codecs->push_back().reset(codec.release());
         }
     }
 
-    // Recognize if there are no valid codecs
     if (0 == codecs->count()) {
         SkCodecPrintf("Error: could not find any valid embedded ico codecs.\n");
         return nullptr;
@@ -183,15 +190,15 @@ std::unique_ptr<SkCodec> SkIcoCodec::MakeFromStream(std::unique_ptr<SkStream> st
     auto maxInfo = codecs->operator[](maxIndex)->getEncodedInfo().copy();
 
     *result = kSuccess;
-    // The original stream is no longer needed, because the embedded codecs own their
+    return std::unique_ptr<SkCodec>(new SkIcoCodec(std::move(maxInfo), std::move(stream),
-    // own streams.
+                                    codecs.release()));
-    return std::unique_ptr<SkCodec>(new SkIcoCodec(std::move(maxInfo), codecs.release()));
 }
 
-SkIcoCodec::SkIcoCodec(SkEncodedInfo&& info, SkTArray<std::unique_ptr<SkCodec>, true>* codecs)
+SkIcoCodec::SkIcoCodec(SkEncodedInfo&& info, std::unique_ptr<SkStream> stream,
+                       SkTArray<std::unique_ptr<SkCodec>, true>* codecs)
     // The source skcms_PixelFormat will not be used. The embedded
     // codec's will be used instead.
-    : INHERITED(std::move(info), skcms_PixelFormat(), nullptr)
+    : INHERITED(std::move(info), skcms_PixelFormat(), std::move(stream))
     , fEmbeddedCodecs(codecs)
     , fCurrCodec(nullptr)
 {}

src/codec/SkIcoCodec.h

@@ -86,7 +86,8 @@ private:
      * Constructor called by NewFromStream
      * @param embeddedCodecs codecs for the embedded images, takes ownership
      */
-    SkIcoCodec(SkEncodedInfo&& info, SkTArray<std::unique_ptr<SkCodec>, true>* embeddedCodecs);
+    SkIcoCodec(SkEncodedInfo&& info, std::unique_ptr<SkStream>,
+               SkTArray<std::unique_ptr<SkCodec>, true>* embeddedCodecs);
 
     std::unique_ptr<SkTArray<std::unique_ptr<SkCodec>, true>> fEmbeddedCodecs;
 

tests/CodecExactReadTest.cpp

@@ -53,8 +53,6 @@ DEF_TEST(Codec_end, r) {
     for (const char* path : { "images/plane.png",
                               "images/yellow_rose.png",
                               "images/plane_interlaced.png",
-                              "images/google_chrome.ico",
-                              "images/color_wheel.ico",
                               "images/mandrill.wbmp",
                               "images/randPixels.bmp",
                               }) {