From 6aab15ddce2ae0e79f3ef1d07ba2fdca1da6ce0f Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Wed, 27 Sep 2017 16:31:08 -0400 Subject: Fix truncated webp images DO NOT MERGE Bug: b/65290323 Test: ag/2974889 Merged-In: Ib6f385766d6d46ed7fe56188cae5a71b100102bd Merged-In: I0cba5ab639f1e66b7c493a9f63735a0f5edbcfbf Change-Id: I0cba5ab639f1e66b7c493a9f63735a0f5edbcfbf Original message description: ======================================================================== If a webp file is truncated such that no rows can be decoded, WebPIDecGetRGB does not initialize its "last_y" parameter. We use rowsDecoded (passed as last_y) to determine which remaining rows to fill. Check the return value of WebPIDecGetRGB. If it fails (returns null), or rowsDecoded is <= 0 (matching Chromium's check), return kInvalidInput, since there is nothing to draw. Note that this is a change in behavior for Android. Previously we would decode an empty webp to just a transparent/black rectangle, whereas now we simply fail. I think this is a change for the better. Add a test which truncates a file to have 0 rows available and attempts to decode it. msan verifies that we no longer depend on the uninitialized value. Stop attempting to test decoding subsets from an incomplete webp (in CodecTest.cpp). Unless we have decoded the portion covered by the subset, this will fail. Remove test images inc0.webp (from both dm/ and colorspace/) and inc1.webp. These just decode to transparent rectangles. Replace them with inc2.webp and inc3.webp, which decode part of the image and then have to fill with transparent. Change-Id: I64d40be91c574b45963f9a43d8dd8f4929dd2939 Reviewed-on: https://skia-review.googlesource.com/50303 Commit-Queue: Leon Scroggins Reviewed-by: James Zern ======================================================================== For simplicity, this does not update VERSION or tasks.json, as does the original CL. It also does not include the test, which relied on more recent changes. Change-Id: I972d457c5c371cd082b4f025a484512fa2b80334 Exempt-From-Owner-Approval: I should be an owner. (cherry picked from commit 1d57f616c694330b4e52da912d8798904a319b86) Change-Id: Iba69fce605ecf0824879b92de09508a842ec0704 --- src/codec/SkWebpCodec.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp index 6cfb385294..8acd205d13 100644 --- a/src/codec/SkWebpCodec.cpp +++ b/src/codec/SkWebpCodec.cpp @@ -235,7 +235,10 @@ SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, while (true) { const size_t bytesRead = stream()->read(buffer, BUFFER_SIZE); if (0 == bytesRead) { - WebPIDecGetRGB(idec, rowsDecoded, NULL, NULL, NULL); + if (!WebPIDecGetRGB(idec, rowsDecoded, NULL, NULL, NULL) + || rowsDecoded && *rowsDecoded <= 0) { + return kInvalidInput; + } return kIncompleteInput; } -- cgit v1.2.3 From a7b921280152097fb619a20a708e6f560be1f645 Mon Sep 17 00:00:00 2001 From: Ben Wagner Date: Fri, 10 Mar 2017 13:08:15 -0500 Subject: Fix SkFILEStream. Cherry-picked from upstream Skia: 4d1955c43aaab045511b74a495dfbea4ef0057c5 Differences: - include instead of , to compile. - re-include "SkString.h", so we don't have to fix downstream clients that do not IWYU. - Stop building SkRTConf and its test. SkRTConf uses code that this patch deletes in SkOSFile. SkRTConf itself is deleted in upstream's 4e44efe50474d4eebcb30b762e784b3ef2126750. For simplicity in cherry-picking further back this CL does not delete the code, but simply disables it by not building it. (It is only used if SK_DEVELOPER is defined anyway, which is not the case on Android.) Bug: 65646012 Bug: 65426286 Test: I863385d797d7a6c54e37904b4f023ff694e01785 (Original) Change-Id: I8c66e4e3e857227aed3d0bc497982f4c0d96d917 Merged-In: I220ec2e2e83f4a002846e89dce855ed5926ca4a1 Merged-In: Ia36e16282eaf294709ce41f57a0f40fe887c1546 Merged-In: Ief1b80a012affeda2068b70405ab1a9f08b36867 Reviewed-on: https://skia-review.googlesource.com/9498 Commit-Queue: Ben Wagner Reviewed-by: Leon Scroggins Conflicts: include/core/SkStream.h src/core/SkStream.cpp src/pdf/SkPDFConvertType1FontStream.cpp src/ports/SkImageEncoder_WIC.cpp src/utils/SkShadowUtils.cpp Change-Id: Ib7aaf367f68f8764147887d858f65ee14fa7a1d9 Exempt-From-Owner-Approval: I should be an OWNER (cherry picked from commit 67f9bd2acfd17f64a33ae8ad14806a0c93b921d8) Change-Id: I7d1142040b3c62e764a726a82a1fd8850dab1458 --- Android.mk | 1 - dm/Android.mk | 1 - include/core/SkOSFile.h | 13 ++--- include/core/SkStream.h | 36 ++++++------- src/codec/SkIcoCodec.h | 1 + src/core/SkPath.cpp | 1 + src/core/SkStream.cpp | 116 +++++++++++++++++++----------------------- src/ports/SkOSFile_posix.cpp | 12 +++++ src/ports/SkOSFile_stdio.cpp | 50 ++---------------- src/ports/SkOSFile_win.cpp | 27 ++++++++++ tests/PathOpsExtendedTest.cpp | 2 +- tests/StreamTest.cpp | 2 +- tools/chrome_fuzz.cpp | 4 +- 13 files changed, 121 insertions(+), 145 deletions(-) diff --git a/Android.mk b/Android.mk index 77c258aa06..4dbd17bb63 100644 --- a/Android.mk +++ b/Android.mk @@ -643,7 +643,6 @@ LOCAL_SRC_FILES := \ src/utils/SkPatchGrid.cpp \ src/utils/SkPatchUtils.cpp \ src/utils/SkRGBAToYUV.cpp \ - src/utils/SkRTConf.cpp \ src/utils/SkTextBox.cpp \ src/utils/SkTextureCompressor.cpp \ src/utils/SkTextureCompressor_ASTC.cpp \ diff --git a/dm/Android.mk b/dm/Android.mk index e51da0fc6e..f362c99aca 100644 --- a/dm/Android.mk +++ b/dm/Android.mk @@ -213,7 +213,6 @@ LOCAL_SRC_FILES := \ ../tests/PremulAlphaRoundTripTest.cpp \ ../tests/QuickRejectTest.cpp \ ../tests/RRectInPathTest.cpp \ - ../tests/RTConfRegistryTest.cpp \ ../tests/RTreeTest.cpp \ ../tests/RandomTest.cpp \ ../tests/ReadPixelsTest.cpp \ diff --git a/include/core/SkOSFile.h b/include/core/SkOSFile.h index f977327e25..cb775e2bc1 100644 --- a/include/core/SkOSFile.h +++ b/include/core/SkOSFile.h @@ -31,20 +31,12 @@ FILE* sk_fopen(const char path[], SkFILE_Flags); void sk_fclose(FILE*); size_t sk_fgetsize(FILE*); -/** Return true if the file could seek back to the beginning -*/ -bool sk_frewind(FILE*); -size_t sk_fread(void* buffer, size_t byteCount, FILE*); size_t sk_fwrite(const void* buffer, size_t byteCount, FILE*); -char* sk_fgets(char* str, int size, FILE* f); - void sk_fflush(FILE*); void sk_fsync(FILE*); -bool sk_fseek(FILE*, size_t); -bool sk_fmove(FILE*, long); size_t sk_ftell(FILE*); /** Maps a file into memory. Returns the address and length on success, NULL otherwise. @@ -80,8 +72,9 @@ bool sk_exists(const char *path, SkFILE_Flags = (SkFILE_Flags)0); // Returns true if a directory exists at this path. bool sk_isdir(const char *path); -// Have we reached the end of the file? -int sk_feof(FILE *); +// Like pread, but may affect the file position marker. +// Returns the number of bytes read or SIZE_MAX if failed. +size_t sk_qread(FILE*, void* buffer, size_t count, size_t offset); // Create a new directory at this path; returns true if successful. diff --git a/include/core/SkStream.h b/include/core/SkStream.h index 4502416fd9..265fb641c7 100644 --- a/include/core/SkStream.h +++ b/include/core/SkStream.h @@ -11,8 +11,9 @@ #include "SkRefCnt.h" #include "SkScalar.h" -class SkData; +#include +class SkData; class SkStream; class SkStreamRewindable; class SkStreamSeekable; @@ -231,28 +232,20 @@ public: /** Initialize the stream by calling sk_fopen on the specified path. * This internal stream will be closed in the destructor. */ - explicit SkFILEStream(const char path[] = NULL); + explicit SkFILEStream(const char path[] = nullptr); - enum Ownership { - kCallerPasses_Ownership, - kCallerRetains_Ownership - }; /** Initialize the stream with an existing C file stream. - * While this stream exists, it assumes exclusive access to the C file stream. - * The C file stream will be closed in the destructor unless the caller specifies - * kCallerRetains_Ownership. + * The C file stream will be closed in the destructor. */ - explicit SkFILEStream(FILE* file, Ownership ownership = kCallerPasses_Ownership); + explicit SkFILEStream(FILE* file); virtual ~SkFILEStream(); /** Returns true if the current path could be opened. */ - bool isValid() const { return fFILE != NULL; } + bool isValid() const { return fFILE != nullptr; } - /** Close the current file, and open a new file with the specified path. - * If path is NULL, just close the current file. - */ - void setPath(const char path[]); + /** Close this SkFILEStream. */ + void close(); size_t read(void* buffer, size_t size) override; bool isAtEnd() const override; @@ -270,11 +263,14 @@ public: const void* getMemoryBase() override; private: - FILE* fFILE; - SkString fName; - Ownership fOwnership; - // fData is lazilly initialized when needed. - mutable SkAutoTUnref fData; + explicit SkFILEStream(std::shared_ptr, size_t size, size_t offset); + explicit SkFILEStream(std::shared_ptr, size_t size, size_t offset, size_t originalOffset); + + std::shared_ptr fFILE; + // My own council will I keep on sizes and offsets. + size_t fSize; + size_t fOffset; + size_t fOriginalOffset; typedef SkStreamAsset INHERITED; }; diff --git a/src/codec/SkIcoCodec.h b/src/codec/SkIcoCodec.h index 9a3f248af5..5a3740d2c0 100644 --- a/src/codec/SkIcoCodec.h +++ b/src/codec/SkIcoCodec.h @@ -8,6 +8,7 @@ #include "SkCodec.h" #include "SkImageInfo.h" #include "SkStream.h" +#include "SkTArray.h" #include "SkTypes.h" /* diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp index 320448a30e..9eb5cd184b 100644 --- a/src/core/SkPath.cpp +++ b/src/core/SkPath.cpp @@ -2082,6 +2082,7 @@ size_t SkPath::readFromMemory(const void* storage, size_t length) { /////////////////////////////////////////////////////////////////////////////// +#include "SkString.h" #include "SkStringUtils.h" #include "SkStream.h" diff --git a/src/core/SkStream.cpp b/src/core/SkStream.cpp index 9529308e86..4f00768747 100644 --- a/src/core/SkStream.cpp +++ b/src/core/SkStream.cpp @@ -180,105 +180,95 @@ bool SkWStream::writeStream(SkStream* stream, size_t length) { /////////////////////////////////////////////////////////////////////////////// -SkFILEStream::SkFILEStream(const char file[]) : fName(file), fOwnership(kCallerPasses_Ownership) { - fFILE = file ? sk_fopen(fName.c_str(), kRead_SkFILE_Flag) : nullptr; -} +SkFILEStream::SkFILEStream(std::shared_ptr file, size_t size, + size_t offset, size_t originalOffset) + : fFILE(std::move(file)) + , fSize(size) + , fOffset(SkTMin(offset, fSize)) + , fOriginalOffset(SkTMin(originalOffset, fSize)) +{ } -SkFILEStream::SkFILEStream(FILE* file, Ownership ownership) - : fFILE(file) - , fOwnership(ownership) { -} +SkFILEStream::SkFILEStream(std::shared_ptr file, size_t size, size_t offset) + : SkFILEStream(std::move(file), size, offset, offset) +{ } + +SkFILEStream::SkFILEStream(FILE* file) + : SkFILEStream(std::shared_ptr(file, sk_fclose), + file ? sk_fgetsize(file) : 0, + file ? sk_ftell(file) : 0) +{ } + + +SkFILEStream::SkFILEStream(const char path[]) + : SkFILEStream(path ? sk_fopen(path, kRead_SkFILE_Flag) : nullptr) +{ } SkFILEStream::~SkFILEStream() { - if (fFILE && fOwnership != kCallerRetains_Ownership) { - sk_fclose(fFILE); - } + this->close(); } -void SkFILEStream::setPath(const char path[]) { - fName.set(path); - if (fFILE) { - sk_fclose(fFILE); - fFILE = nullptr; - } - if (path) { - fFILE = sk_fopen(fName.c_str(), kRead_SkFILE_Flag); - } +void SkFILEStream::close() { + fFILE.reset(); + fSize = 0; + fOffset = 0; } size_t SkFILEStream::read(void* buffer, size_t size) { - if (fFILE) { - return sk_fread(buffer, size, fFILE); + if (size > fSize - fOffset) { + size = fSize - fOffset; + } + size_t bytesRead = size; + if (buffer) { + bytesRead = sk_qread(fFILE.get(), buffer, size, fOffset); + } + if (bytesRead == SIZE_MAX) { + return 0; } - return 0; + fOffset += bytesRead; + return bytesRead; } bool SkFILEStream::isAtEnd() const { - return sk_feof(fFILE); + if (fOffset == fSize) { + return true; + } + return fOffset >= sk_fgetsize(fFILE.get()); } bool SkFILEStream::rewind() { - if (fFILE) { - if (sk_frewind(fFILE)) { - return true; - } - // we hit an error - sk_fclose(fFILE); - fFILE = nullptr; - } - return false; + // TODO: fOriginalOffset instead of 0. + fOffset = 0; + return true; } SkStreamAsset* SkFILEStream::duplicate() const { - if (nullptr == fFILE) { - return new SkMemoryStream(); - } - - if (fData.get()) { - return new SkMemoryStream(fData); - } - - if (!fName.isEmpty()) { - SkAutoTDelete that(new SkFILEStream(fName.c_str())); - if (sk_fidentical(that->fFILE, this->fFILE)) { - return that.detach(); - } - } - - fData.reset(SkData::NewFromFILE(fFILE)); - if (nullptr == fData.get()) { - return nullptr; - } - return new SkMemoryStream(fData); + // TODO: fOriginalOffset instead of 0. + return new SkFILEStream(fFILE, fSize, 0, fOriginalOffset); } size_t SkFILEStream::getPosition() const { - return sk_ftell(fFILE); + return fOffset; } bool SkFILEStream::seek(size_t position) { - return sk_fseek(fFILE, position); + fOffset = position > fSize ? fSize : position; + return true; } bool SkFILEStream::move(long offset) { - return sk_fmove(fFILE, offset); + return this->seek(fOffset + offset); } SkStreamAsset* SkFILEStream::fork() const { - SkAutoTDelete that(this->duplicate()); - that->seek(this->getPosition()); - return that.detach(); + return new SkFILEStream(fFILE, fSize, fOffset, fOriginalOffset); } size_t SkFILEStream::getLength() const { - return sk_fgetsize(fFILE); + return fSize; } const void* SkFILEStream::getMemoryBase() { - if (nullptr == fData.get()) { - return nullptr; - } - return fData->data(); + return nullptr; } /////////////////////////////////////////////////////////////////////////////// diff --git a/src/ports/SkOSFile_posix.cpp b/src/ports/SkOSFile_posix.cpp index 396de68bbe..48b5b95ad3 100644 --- a/src/ports/SkOSFile_posix.cpp +++ b/src/ports/SkOSFile_posix.cpp @@ -95,6 +95,18 @@ void* sk_fmmap(FILE* f, size_t* size) { return sk_fdmmap(fd, size); } +size_t sk_qread(FILE* file, void* buffer, size_t count, size_t offset) { + int fd = sk_fileno(file); + if (fd < 0) { + return SIZE_MAX; + } + ssize_t bytesRead = pread(fd, buffer, count, offset); + if (bytesRead < 0) { + return SIZE_MAX; + } + return bytesRead; +} + //////////////////////////////////////////////////////////////////////////// struct SkOSFileIterData { diff --git a/src/ports/SkOSFile_stdio.cpp b/src/ports/SkOSFile_stdio.cpp index 915b87b67b..ee997e9db9 100644 --- a/src/ports/SkOSFile_stdio.cpp +++ b/src/ports/SkOSFile_stdio.cpp @@ -80,15 +80,6 @@ FILE* sk_fopen(const char path[], SkFILE_Flags flags) { return file; } -char* sk_fgets(char* str, int size, FILE* f) { - return fgets(str, size, (FILE *)f); -} - -int sk_feof(FILE *f) { - // no :: namespace qualifier because it breaks android - return feof((FILE *)f); -} - size_t sk_fgetsize(FILE* f) { SkASSERT(f); @@ -107,32 +98,6 @@ size_t sk_fgetsize(FILE* f) { return size; } -bool sk_frewind(FILE* f) { - SkASSERT(f); - ::rewind(f); - return true; -} - -size_t sk_fread(void* buffer, size_t byteCount, FILE* f) { - SkASSERT(f); - if (buffer == nullptr) { - size_t curr = ftell(f); - if ((long)curr == -1) { - SkDEBUGF(("sk_fread: ftell(%p) returned -1 feof:%d ferror:%d\n", f, feof(f), ferror(f))); - return 0; - } - int err = fseek(f, (long)byteCount, SEEK_CUR); - if (err != 0) { - SkDEBUGF(("sk_fread: fseek(%d) tell:%d failed with feof:%d ferror:%d returned:%d\n", - byteCount, curr, feof(f), ferror(f), err)); - return 0; - } - return byteCount; - } - else - return fread(buffer, 1, byteCount, f); -} - size_t sk_fwrite(const void* buffer, size_t byteCount, FILE* f) { SkASSERT(f); return fwrite(buffer, 1, byteCount, f); @@ -151,16 +116,6 @@ void sk_fsync(FILE* f) { #endif } -bool sk_fseek(FILE* f, size_t byteCount) { - int err = fseek(f, (long)byteCount, SEEK_SET); - return err == 0; -} - -bool sk_fmove(FILE* f, long byteCount) { - int err = fseek(f, byteCount, SEEK_CUR); - return err == 0; -} - size_t sk_ftell(FILE* f) { long curr = ftell(f); if (curr < 0) { @@ -170,8 +125,9 @@ size_t sk_ftell(FILE* f) { } void sk_fclose(FILE* f) { - SkASSERT(f); - fclose(f); + if (f) { + fclose(f); + } } bool sk_isdir(const char *path) { diff --git a/src/ports/SkOSFile_win.cpp b/src/ports/SkOSFile_win.cpp index 6bdf9ab163..414393a1d4 100644 --- a/src/ports/SkOSFile_win.cpp +++ b/src/ports/SkOSFile_win.cpp @@ -124,6 +124,33 @@ void* sk_fmmap(FILE* f, size_t* length) { return sk_fdmmap(fileno, length); } +size_t sk_qread(FILE* file, void* buffer, size_t count, size_t offset) { + int fileno = sk_fileno(file); + HANDLE fileHandle = (HANDLE)_get_osfhandle(fileno); + if (INVALID_HANDLE_VALUE == file) { + return SIZE_MAX; + } + + OVERLAPPED overlapped = {0}; + ULARGE_INTEGER winOffset; + winOffset.QuadPart = offset; + overlapped.Offset = winOffset.LowPart; + overlapped.OffsetHigh = winOffset.HighPart; + + if (!SkTFitsIn(count)) { + count = std::numeric_limits::max(); + } + + DWORD bytesRead; + if (ReadFile(fileHandle, buffer, static_cast(count), &bytesRead, &overlapped)) { + return bytesRead; + } + if (GetLastError() == ERROR_HANDLE_EOF) { + return 0; + } + return SIZE_MAX; +} + //////////////////////////////////////////////////////////////////////////// struct SkOSFileIterData { diff --git a/tests/PathOpsExtendedTest.cpp b/tests/PathOpsExtendedTest.cpp index c96cbcdae5..9e2dfbe2ae 100644 --- a/tests/PathOpsExtendedTest.cpp +++ b/tests/PathOpsExtendedTest.cpp @@ -595,7 +595,7 @@ void initializeTests(skiatest::Reporter* reporter, const char* test) { inData.setCount((int) inFile.getLength()); size_t inLen = inData.count(); inFile.read(inData.begin(), inLen); - inFile.setPath(nullptr); + inFile.close(); char* insert = strstr(inData.begin(), marker); if (insert) { insert += sizeof(marker) - 1; diff --git a/tests/StreamTest.cpp b/tests/StreamTest.cpp index a3df8d71bb..031cc1a623 100644 --- a/tests/StreamTest.cpp +++ b/tests/StreamTest.cpp @@ -67,7 +67,7 @@ static void test_filestreams(skiatest::Reporter* reporter, const char* tmpDir) { { FILE* file = ::fopen(path.c_str(), "rb"); - SkFILEStream stream(file, SkFILEStream::kCallerPasses_Ownership); + SkFILEStream stream(file); REPORTER_ASSERT(reporter, stream.isValid()); test_loop_stream(reporter, &stream, s, 26, 100); diff --git a/tools/chrome_fuzz.cpp b/tools/chrome_fuzz.cpp index f49e12693b..ff14b5b307 100644 --- a/tools/chrome_fuzz.cpp +++ b/tools/chrome_fuzz.cpp @@ -8,6 +8,8 @@ #include "SkOSFile.h" #include "SkString.h" +#include + static const int kBitmapSize = 24; static bool read_test_case(const char* filename, SkString* testdata) { @@ -22,7 +24,7 @@ static bool read_test_case(const char* filename, SkString* testdata) { return false; } testdata->resize(len); - (void) sk_fread(testdata->writable_str(), len, file); + (void) fread(testdata->writable_str(), len, file); return true; } -- cgit v1.2.3