Make SkStream readers report failure.
This also fixes an issue noticed while making this change where
SkFontDescriptor improperly round trips negative axis values.
Change-Id: Iacc5929a185659dcacc18c802c4908e4f34c6899
Reviewed-on: https://skia-review.googlesource.com/128341
Commit-Queue: Ben Wagner <bungeman@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>
diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index 1a5ecb3..f0e9e0e 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -174,6 +174,8 @@
static const uint32_t MIN_PICTURE_VERSION = 56; // august 2017
static const uint32_t CURRENT_PICTURE_VERSION = 62;
+ static_assert(MIN_PICTURE_VERSION <= 62, "Remove kFontAxes_bad from SkFontDescriptor.cpp");
+
static bool IsValidPictInfo(const SkPictInfo& info);
static sk_sp<SkPicture> Forwardport(const SkPictInfo&,
const SkPictureData*,
diff --git a/include/core/SkStream.h b/include/core/SkStream.h
index 75fef12..2d9f9db 100644
--- a/include/core/SkStream.h
+++ b/include/core/SkStream.h
@@ -83,17 +83,22 @@
*/
virtual bool isAtEnd() const = 0;
- int8_t readS8();
- int16_t readS16();
- int32_t readS32();
+ bool SK_WARN_UNUSED_RESULT readS8(int8_t*);
+ bool SK_WARN_UNUSED_RESULT readS16(int16_t*);
+ bool SK_WARN_UNUSED_RESULT readS32(int32_t*);
- uint8_t readU8() { return (uint8_t)this->readS8(); }
- uint16_t readU16() { return (uint16_t)this->readS16(); }
- uint32_t readU32() { return (uint32_t)this->readS32(); }
+ bool SK_WARN_UNUSED_RESULT readU8(uint8_t* i) { return this->readS8((int8_t*)i); }
+ bool SK_WARN_UNUSED_RESULT readU16(uint16_t* i) { return this->readS16((int16_t*)i); }
+ bool SK_WARN_UNUSED_RESULT readU32(uint32_t* i) { return this->readS32((int32_t*)i); }
- bool readBool() { return this->readU8() != 0; }
- SkScalar readScalar();
- size_t readPackedUInt();
+ bool SK_WARN_UNUSED_RESULT readBool(bool* b) {
+ uint8_t i;
+ if (!this->readU8(&i)) { return false; }
+ *b = (i != 0);
+ return true;
+ }
+ bool SK_WARN_UNUSED_RESULT readScalar(SkScalar*);
+ bool SK_WARN_UNUSED_RESULT readPackedUInt(size_t*);
//SkStreamRewindable
/** Rewinds to the beginning of the stream. Returns true if the stream is known
@@ -247,16 +252,16 @@
bool newline() { return this->write("\n", strlen("\n")); }
- bool writeDecAsText(int32_t);
- bool writeBigDecAsText(int64_t, int minDigits = 0);
- bool writeHexAsText(uint32_t, int minDigits = 0);
- bool writeScalarAsText(SkScalar);
+ bool writeDecAsText(int32_t);
+ bool writeBigDecAsText(int64_t, int minDigits = 0);
+ bool writeHexAsText(uint32_t, int minDigits = 0);
+ bool writeScalarAsText(SkScalar);
- bool writeBool(bool v) { return this->write8(v); }
- bool writeScalar(SkScalar);
- bool writePackedUInt(size_t);
+ bool writeBool(bool v) { return this->write8(v); }
+ bool writeScalar(SkScalar);
+ bool writePackedUInt(size_t);
- bool writeStream(SkStream* input, size_t length);
+ bool writeStream(SkStream* input, size_t length);
/**
* This returns the number of bytes in the stream required to store
diff --git a/src/core/SkFontDescriptor.cpp b/src/core/SkFontDescriptor.cpp
index 14b0318..ccc19eb 100644
--- a/src/core/SkFontDescriptor.cpp
+++ b/src/core/SkFontDescriptor.cpp
@@ -11,6 +11,8 @@
#include "SkData.h"
enum {
+ kInvalid = 0x00,
+
// these must match the sfnt 'name' enums
kFontFamilyName = 0x01,
kFullName = 0x04,
@@ -18,40 +20,45 @@
// These count backwards from 0xFF, so as not to collide with the SFNT
// defines for names in its 'name' table.
- kFontAxes = 0xFC,
+ kFontAxes = 0xFB,
+ kFontAxes_bad = 0xFC, // Broken negative axes, remove when MIN_PICTURE_VERSION > 62.
kFontIndex = 0xFD,
kSentinel = 0xFF,
};
SkFontDescriptor::SkFontDescriptor() { }
-static void read_string(SkStream* stream, SkString* string) {
- const uint32_t length = SkToU32(stream->readPackedUInt());
+static bool SK_WARN_UNUSED_RESULT read_string(SkStream* stream, SkString* string) {
+ size_t length;
+ if (!stream->readPackedUInt(&length)) { return false; }
if (length > 0) {
string->resize(length);
- stream->read(string->writable_str(), length);
+ if (stream->read(string->writable_str(), length) != length) { return false; }
}
+ return true;
}
-static void write_string(SkWStream* stream, const SkString& string, uint32_t id) {
- if (!string.isEmpty()) {
- stream->writePackedUInt(id);
- stream->writePackedUInt(string.size());
- stream->write(string.c_str(), string.size());
- }
+static bool write_string(SkWStream* stream, const SkString& string, uint32_t id) {
+ if (string.isEmpty()) { return true; }
+ return stream->writePackedUInt(id) &&
+ stream->writePackedUInt(string.size()) &&
+ stream->write(string.c_str(), string.size());
}
-static size_t read_uint(SkStream* stream) {
- return stream->readPackedUInt();
+static bool write_uint(SkWStream* stream, size_t n, uint32_t id) {
+ return stream->writePackedUInt(id) &&
+ stream->writePackedUInt(n);
}
-static void write_uint(SkWStream* stream, size_t n, uint32_t id) {
- stream->writePackedUInt(id);
- stream->writePackedUInt(n);
+static size_t SK_WARN_UNUSED_RESULT read_id(SkStream* stream) {
+ size_t i;
+ if (!stream->readPackedUInt(&i)) { return kInvalid; }
+ return i;
}
bool SkFontDescriptor::Deserialize(SkStream* stream, SkFontDescriptor* result) {
- size_t styleBits = stream->readPackedUInt();
+ size_t styleBits;
+ if (!stream->readPackedUInt(&styleBits)) { return false; }
result->fStyle = SkFontStyle((styleBits >> 16) & 0xFFFF,
(styleBits >> 8 ) & 0xFF,
static_cast<SkFontStyle::Slant>(styleBits & 0xFF));
@@ -59,26 +66,35 @@
SkAutoSTMalloc<4, SkFixed> axis;
size_t axisCount = 0;
size_t index = 0;
- for (size_t id; (id = stream->readPackedUInt()) != kSentinel;) {
+ for (size_t id; (id = read_id(stream)) != kSentinel;) {
switch (id) {
case kFontFamilyName:
- read_string(stream, &result->fFamilyName);
+ if (!read_string(stream, &result->fFamilyName)) { return false; }
break;
case kFullName:
- read_string(stream, &result->fFullName);
+ if (!read_string(stream, &result->fFullName)) { return false; }
break;
case kPostscriptName:
- read_string(stream, &result->fPostscriptName);
+ if (!read_string(stream, &result->fPostscriptName)) { return false; }
break;
case kFontAxes:
- axisCount = read_uint(stream);
+ if (!stream->readPackedUInt(&axisCount)) { return false; }
axis.reset(axisCount);
for (size_t i = 0; i < axisCount; ++i) {
- axis[i] = read_uint(stream);
+ if (!stream->readS32(&axis[i])) { return false; }
+ }
+ break;
+ case kFontAxes_bad:
+ if (!stream->readPackedUInt(&axisCount)) { return false; }
+ axis.reset(axisCount);
+ for (size_t i = 0; i < axisCount; ++i) {
+ size_t packedAxis;
+ if (!stream->readPackedUInt(&packedAxis)) { return false; }
+ axis[i] = packedAxis;
}
break;
case kFontIndex:
- index = read_uint(stream);
+ if (!stream->readPackedUInt(&index)) { return false; }
break;
default:
SkDEBUGFAIL("Unknown id used by a font descriptor");
@@ -86,16 +102,16 @@
}
}
- size_t length = stream->readPackedUInt();
+ size_t length;
+ if (!stream->readPackedUInt(&length)) { return false; }
if (length > 0) {
sk_sp<SkData> data(SkData::MakeUninitialized(length));
- if (stream->read(data->writable_data(), length) == length) {
- result->fFontData = skstd::make_unique<SkFontData>(
- SkMemoryStream::Make(std::move(data)), index, axis, axisCount);
- } else {
+ if (stream->read(data->writable_data(), length) != length) {
SkDEBUGFAIL("Could not read font data");
return false;
}
+ result->fFontData = skstd::make_unique<SkFontData>(
+ SkMemoryStream::Make(std::move(data)), index, axis, axisCount);
}
return true;
}
@@ -114,7 +130,7 @@
if (fFontData->getAxisCount()) {
write_uint(stream, fFontData->getAxisCount(), kFontAxes);
for (int i = 0; i < fFontData->getAxisCount(); ++i) {
- stream->writePackedUInt(fFontData->getAxis()[i]);
+ stream->write32(fFontData->getAxis()[i]);
}
}
}
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index 85b22ac..5993e49 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -80,24 +80,25 @@
SkPictInfo info;
SkASSERT(sizeof(kMagic) == sizeof(info.fMagic));
- if (!stream->read(&info.fMagic, sizeof(kMagic))) {
+ if (stream->read(&info.fMagic, sizeof(kMagic)) != sizeof(kMagic)) {
return false;
}
- info.setVersion( stream->readU32());
- info.fCullRect.fLeft = stream->readScalar();
- info.fCullRect.fTop = stream->readScalar();
- info.fCullRect.fRight = stream->readScalar();
- info.fCullRect.fBottom = stream->readScalar();
+ uint32_t version;
+ if (!stream->readU32(&version)) { return false; }
+ info.setVersion(version);
+ if (!stream->readScalar(&info.fCullRect.fLeft )) { return false; }
+ if (!stream->readScalar(&info.fCullRect.fTop )) { return false; }
+ if (!stream->readScalar(&info.fCullRect.fRight )) { return false; }
+ if (!stream->readScalar(&info.fCullRect.fBottom)) { return false; }
if (info.getVersion() < SkReadBuffer::kRemoveHeaderFlags_Version) {
- (void)stream->readU32(); // used to be flags
+ if (!stream->readU32(nullptr)) { return false; }
}
- if (IsValidPictInfo(info)) {
- if (pInfo) { *pInfo = info; }
- return true;
- }
- return false;
+ if (!IsValidPictInfo(info)) { return false; }
+
+ if (pInfo) { *pInfo = info; }
+ return true;
}
bool SkPicture_StreamIsSKP(SkStream* stream, SkPictInfo* pInfo) {
return SkPicture::StreamIsSKP(stream, pInfo);
@@ -171,15 +172,17 @@
procs = *procsPtr;
}
- switch (stream->readU8()) {
+ uint8_t trailingStreamByteAfterPictInfo;
+ if (!stream->readU8(&trailingStreamByteAfterPictInfo)) { return nullptr; }
+ switch (trailingStreamByteAfterPictInfo) {
case kPictureData_TrailingStreamByteAfterPictInfo: {
std::unique_ptr<SkPictureData> data(
SkPictureData::CreateFromStream(stream, info, procs, typefaces));
return Forwardport(info, data.get(), nullptr);
}
case kCustom_TrailingStreamByteAfterPictInfo: {
- int32_t ssize = stream->readS32();
- if (ssize >= 0 || !procs.fPictureProc) {
+ int32_t ssize;
+ if (!stream->readS32(&ssize) || ssize >= 0 || !procs.fPictureProc) {
return nullptr;
}
size_t size = sk_negate_to_size_t(ssize);
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index efd4183..6be36dd 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -276,11 +276,12 @@
break;
case SK_PICT_FACTORY_TAG: {
SkASSERT(!haveBuffer);
- size = stream->readU32();
+ if (!stream->readU32(&size)) { return false; }
fFactoryPlayback = skstd::make_unique<SkFactoryPlayback>(size);
for (size_t i = 0; i < size; i++) {
SkString str;
- const size_t len = stream->readPackedUInt();
+ size_t len;
+ if (!stream->readPackedUInt(&len)) { return false; }
str.resize(len);
if (stream->read(str.writable_str(), len) != len) {
return false;
@@ -480,12 +481,14 @@
const SkDeserialProcs& procs,
SkTypefacePlayback* topLevelTFPlayback) {
for (;;) {
- uint32_t tag = stream->readU32();
+ uint32_t tag;
+ if (!stream->readU32(&tag)) { return false; }
if (SK_PICT_EOF_TAG == tag) {
break;
}
- uint32_t size = stream->readU32();
+ uint32_t size;
+ if (!stream->readU32(&size)) { return false; }
if (!this->parseStreamTag(stream, tag, size, procs, topLevelTFPlayback)) {
return false; // we're invalid
}
diff --git a/src/core/SkStream.cpp b/src/core/SkStream.cpp
index dfdc257..57fd7f5 100644
--- a/src/core/SkStream.cpp
+++ b/src/core/SkStream.cpp
@@ -20,50 +20,43 @@
///////////////////////////////////////////////////////////////////////////////
-int8_t SkStream::readS8() {
- int8_t value;
- SkDEBUGCODE(size_t len =) this->read(&value, 1);
- SkASSERT(1 == len);
- return value;
+bool SkStream::readS8(int8_t* i) {
+ return this->read(i, sizeof(*i)) == sizeof(*i);
}
-int16_t SkStream::readS16() {
- int16_t value;
- SkDEBUGCODE(size_t len =) this->read(&value, 2);
- SkASSERT(2 == len);
- return value;
+bool SkStream::readS16(int16_t* i) {
+ return this->read(i, sizeof(*i)) == sizeof(*i);
}
-int32_t SkStream::readS32() {
- int32_t value;
- SkDEBUGCODE(size_t len =) this->read(&value, 4);
- SkASSERT(4 == len);
- return value;
+bool SkStream::readS32(int32_t* i) {
+ return this->read(i, sizeof(*i)) == sizeof(*i);
}
-SkScalar SkStream::readScalar() {
- SkScalar value;
- SkDEBUGCODE(size_t len =) this->read(&value, sizeof(SkScalar));
- SkASSERT(sizeof(SkScalar) == len);
- return value;
+bool SkStream::readScalar(SkScalar* i) {
+ return this->read(i, sizeof(*i)) == sizeof(*i);
}
#define SK_MAX_BYTE_FOR_U8 0xFD
#define SK_BYTE_SENTINEL_FOR_U16 0xFE
#define SK_BYTE_SENTINEL_FOR_U32 0xFF
-size_t SkStream::readPackedUInt() {
+bool SkStream::readPackedUInt(size_t* i) {
uint8_t byte;
if (!this->read(&byte, 1)) {
- return 0;
+ return false;
}
if (SK_BYTE_SENTINEL_FOR_U16 == byte) {
- return this->readU16();
+ uint16_t i16;
+ if (!this->readU16(&i16)) { return false; }
+ *i = i16;
} else if (SK_BYTE_SENTINEL_FOR_U32 == byte) {
- return this->readU32();
+ uint32_t i32;
+ if (!this->readU32(&i32)) { return false; }
+ *i = i32;
} else {
- return byte;
+ *i = byte;
}
+ return true;
}
//////////////////////////////////////////////////////////////////////////////////////
diff --git a/src/utils/SkMultiPictureDocument.cpp b/src/utils/SkMultiPictureDocument.cpp
index b09db06..493e6d4 100644
--- a/src/utils/SkMultiPictureDocument.cpp
+++ b/src/utils/SkMultiPictureDocument.cpp
@@ -110,16 +110,16 @@
stream = nullptr;
return 0;
}
- uint32_t versionNumber = stream->readU32();
- if (versionNumber != kVersion) {
+ uint32_t versionNumber;
+ if (!stream->readU32(&versionNumber) || versionNumber != kVersion) {
return 0;
}
- uint32_t pageCount = stream->readU32();
- if (pageCount > INT_MAX) {
+ uint32_t pageCount;
+ if (!stream->readU32(&pageCount) || pageCount > INT_MAX) {
return 0;
}
// leave stream position right here.
- return (int)pageCount;
+ return SkTo<int>(pageCount);
}
bool SkMultiPictureDocumentReadPageSizes(SkStreamSeekable* stream,
diff --git a/tests/StreamTest.cpp b/tests/StreamTest.cpp
index 49810ae..7cf8d92 100644
--- a/tests/StreamTest.cpp
+++ b/tests/StreamTest.cpp
@@ -159,9 +159,12 @@
std::unique_ptr<SkStreamAsset> rstream(wstream.detachAsStream());
for (i = 0; i < SK_ARRAY_COUNT(sizes); ++i) {
- size_t n = rstream->readPackedUInt();
+ size_t n;
+ if (!rstream->readPackedUInt(&n)) {
+ ERRORF(reporter, "[%d] sizes:%x could not be read\n", i, sizes[i]);
+ }
if (sizes[i] != n) {
- ERRORF(reporter, "sizes:%x != n:%x\n", i, sizes[i], n);
+ ERRORF(reporter, "[%d] sizes:%x != n:%x\n", i, sizes[i], n);
}
}
}
diff --git a/tests/TypefaceTest.cpp b/tests/TypefaceTest.cpp
index 34fa357..fcef366 100644
--- a/tests/TypefaceTest.cpp
+++ b/tests/TypefaceTest.cpp
@@ -8,6 +8,7 @@
#include "SkAdvancedTypefaceMetrics.h"
#include "SkData.h"
#include "SkFixed.h"
+#include "SkFontDescriptor.h"
#include "SkFontMgr.h"
#include "SkMakeUnique.h"
#include "SkOTTable_OS_2.h"
@@ -109,6 +110,30 @@
REPORTER_ASSERT(reporter, typeface2);
}
+DEF_TEST(FontDescriptorNegativeVariationSerialize, reporter) {
+ SkFontDescriptor desc;
+ SkFixed axis = -SK_Fixed1;
+ auto font = skstd::make_unique<SkMemoryStream>("a", 1, false);
+ desc.setFontData(skstd::make_unique<SkFontData>(std::move(font), 0, &axis, 1));
+
+ SkDynamicMemoryWStream stream;
+ desc.serialize(&stream);
+ SkFontDescriptor descD;
+ SkFontDescriptor::Deserialize(stream.detachAsStream().get(), &descD);
+ std::unique_ptr<SkFontData> fontData = descD.detachFontData();
+ if (!fontData) {
+ REPORT_FAILURE(reporter, "fontData", SkString());
+ return;
+ }
+
+ if (fontData->getAxisCount() != 1) {
+ REPORT_FAILURE(reporter, "fontData->getAxisCount() != 1", SkString());
+ return;
+ }
+
+ REPORTER_ASSERT(reporter, fontData->getAxis()[0] == -SK_Fixed1);
+};
+
DEF_TEST(TypefaceAxes, reporter) {
std::unique_ptr<SkStreamAsset> distortable(GetResourceAsStream("fonts/Distortable.ttf"));
if (!distortable) {
diff --git a/tools/skpinfo.cpp b/tools/skpinfo.cpp
index 865492d..93834bb 100644
--- a/tools/skpinfo.cpp
+++ b/tools/skpinfo.cpp
@@ -65,7 +65,9 @@
info.fCullRect.fRight, info.fCullRect.fBottom);
}
- if (!stream.readBool()) {
+ bool hasData;
+ if (!stream.readBool(&hasData)) { return kTruncatedFile; }
+ if (!hasData) {
// If we read true there's a picture playback object flattened
// in the file; if false, there isn't a playback, so we're done
// reading the file.
@@ -73,12 +75,14 @@
}
for (;;) {
- uint32_t tag = stream.readU32();
+ uint32_t tag;
+ if (!stream.readU32(&tag)) { return kTruncatedFile; }
if (SK_PICT_EOF_TAG == tag) {
break;
}
- uint32_t chunkSize = stream.readU32();
+ uint32_t chunkSize;
+ if (!stream.readU32(&chunkSize)) { return kTruncatedFile; }
size_t curPos = stream.getPosition();
// "move" doesn't error out when seeking beyond the end of file