From 6b3d6e92109fff606cbb3ede28574d913f40a4eb Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Mon, 1 Jun 2020 16:33:28 -0400 Subject: [PATCH] Rewrite SkVertices serialization to use SkReadBuffer/SkWriteBuffer These classes are much safer (there's no way to safely deserialize a string with SkReader32 without knowledge of how it works internally). Prior to this CL, SkVertices was the only complex type that had manual serialization using the lower level types - now it works like everything else. Additionally: the versioning can now be tied to picture versions going forward (like everything else). Bug: oss-fuzz:22909 Bug: oss-fuzz:22918 Bug: skia:9984 Bug: skia:10304 Change-Id: I3cf537eb765b5c8ce98b554c0f200e5d67c33d14 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/293349 Reviewed-by: Mike Klein Commit-Queue: Brian Osman --- include/core/SkVertices.h | 13 +- src/core/SkPictureData.cpp | 9 +- src/core/SkPicturePriv.h | 4 +- src/core/SkVertices.cpp | 235 ++++++++++++++++--------------------- src/core/SkVerticesPriv.h | 8 +- tests/VerticesTest.cpp | 14 ++- 6 files changed, 126 insertions(+), 157 deletions(-) diff --git a/include/core/SkVertices.h b/include/core/SkVertices.h index 20a0fa811e..0f666f83c8 100644 --- a/include/core/SkVertices.h +++ b/include/core/SkVertices.h @@ -158,6 +158,7 @@ public: std::unique_ptr fIntermediateFanIndices; friend class SkVertices; + friend class SkVerticesPriv; }; uint32_t uniqueID() const { return fUniqueID; } @@ -166,18 +167,6 @@ public: // returns approximate byte size of the vertices object size_t approximateSize() const; - /** - * Recreate a vertices from a buffer previously created by calling encode(). - * Returns null if the data is corrupt or the length is incorrect for the contents. - */ - static sk_sp Decode(const void* buffer, size_t length); - - /** - * Pack the vertices object into a byte buffer. This can be used to recreate the vertices - * by calling Decode() with the buffer. - */ - sk_sp encode() const; - // Provides access to functions that aren't part of the public API. SkVerticesPriv priv(); const SkVerticesPriv priv() const; diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp index bb6d67c379..c4ae9a002e 100644 --- a/src/core/SkPictureData.cpp +++ b/src/core/SkPictureData.cpp @@ -15,6 +15,7 @@ #include "src/core/SkPictureRecord.h" #include "src/core/SkReadBuffer.h" #include "src/core/SkTextBlobPriv.h" +#include "src/core/SkVerticesPriv.h" #include "src/core/SkWriteBuffer.h" #include @@ -174,7 +175,7 @@ void SkPictureData::flattenToBuffer(SkWriteBuffer& buffer, bool textBlobsOnly) c if (!fVertices.empty()) { write_tag_size(buffer, SK_PICT_VERTICES_BUFFER_TAG, fVertices.count()); for (const auto& vert : fVertices) { - buffer.writeDataAsByteArray(vert->encode().get()); + vert->priv().encode(buffer); } } @@ -374,10 +375,6 @@ bool SkPictureData::parseStreamTag(SkStream* stream, static sk_sp create_image_from_buffer(SkReadBuffer& buffer) { return buffer.readImage(); } -static sk_sp create_vertices_from_buffer(SkReadBuffer& buffer) { - auto data = buffer.readByteArrayAsData(); - return data ? SkVertices::Decode(data->data(), data->size()) : nullptr; -} static sk_sp create_drawable_from_buffer(SkReadBuffer& buffer) { return sk_sp((SkDrawable*)buffer.readFlattenable(SkFlattenable::kSkDrawable_Type)); @@ -440,7 +437,7 @@ void SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t new_array_from_buffer(buffer, size, fTextBlobs, SkTextBlobPriv::MakeFromBuffer); break; case SK_PICT_VERTICES_BUFFER_TAG: - new_array_from_buffer(buffer, size, fVertices, create_vertices_from_buffer); + new_array_from_buffer(buffer, size, fVertices, SkVerticesPriv::Decode); break; case SK_PICT_IMAGE_BUFFER_TAG: new_array_from_buffer(buffer, size, fImages, create_image_from_buffer); diff --git a/src/core/SkPicturePriv.h b/src/core/SkPicturePriv.h index a14cb3225f..16f0e42e1c 100644 --- a/src/core/SkPicturePriv.h +++ b/src/core/SkPicturePriv.h @@ -75,6 +75,7 @@ public: // V72: SkColorFilter_Matrix domain (rgba vs. hsla) // V73: Use SkColor4f in per-edge AA quad API // V74: MorphologyImageFilter internal radius is SkScaler + // V75: SkVertices switched from unsafe use of SkReader32 to SkReadBuffer (like everything else) enum Version { kTileModeInBlurImageFilter_Version = 56, @@ -96,10 +97,11 @@ public: kMatrixColorFilterDomain_Version = 72, kEdgeAAQuadColor4f_Version = 73, kMorphologyTakesScalar_Version = 74, + kVerticesUseReadBuffer_Version = 75, // Only SKPs within the min/current picture version range (inclusive) can be read. kMin_Version = kTileModeInBlurImageFilter_Version, - kCurrent_Version = kMorphologyTakesScalar_Version + kCurrent_Version = kVerticesUseReadBuffer_Version }; static_assert(kMin_Version <= 62, "Remove kFontAxes_bad from SkFontDescriptor.cpp"); diff --git a/src/core/SkVertices.cpp b/src/core/SkVertices.cpp index 532bb4f17d..0f1a834027 100644 --- a/src/core/SkVertices.cpp +++ b/src/core/SkVertices.cpp @@ -11,11 +11,11 @@ #include "include/private/SkTo.h" #include "src/core/SkCanvasPriv.h" #include "src/core/SkOpts.h" -#include "src/core/SkReader32.h" +#include "src/core/SkReadBuffer.h" #include "src/core/SkSafeMath.h" #include "src/core/SkSafeRange.h" #include "src/core/SkVerticesPriv.h" -#include "src/core/SkWriter32.h" +#include "src/core/SkWriteBuffer.h" #include #include @@ -370,173 +370,138 @@ bool SkVerticesPriv::hasUsage(SkVertices::Attribute::Usage u) const { /////////////////////////////////////////////////////////////////////////////////////////////////// -enum EncodedVerticesVersions { - kNamedMarkers_Version = 4, // Marker IDs changed to strings - - kCurrent_Version = kNamedMarkers_Version -}; - -struct EncodedAttribute { - SkVertices::Attribute::Type fType; - SkVertices::Attribute::Usage fUsage; - bool fHasMarkerName; -}; - -struct Header_v4 { - uint32_t fPacked; - int32_t fVertexCount; - int32_t fIndexCount; - int32_t fAttributeCount; - // [EncodedAttributes] + [MarkerNames] + [pos] + [customData] + [texs] + [colors] + [indices] -}; - -#define kCurrentHeaderSize sizeof(Header_v4) - // storage = packed | vertex_count | index_count | attr_count // | pos[] | custom[] | texs[] | colors[] | indices[] #define kMode_Mask 0x0FF #define kHasTexs_Mask 0x100 #define kHasColors_Mask 0x200 -// new as of 3/2020 -#define kVersion_Shift 24 -#define kVersion_Mask (0xFF << kVersion_Shift) -sk_sp SkVertices::encode() const { - // packed has room for addtional flags in the future (e.g. versioning) - uint32_t packed = static_cast(fMode); +void SkVerticesPriv::encode(SkWriteBuffer& buffer) const { + // packed has room for additional flags in the future + uint32_t packed = static_cast(fVertices->fMode); SkASSERT((packed & ~kMode_Mask) == 0); // our mode fits in the mask bits - if (fTexs) { + if (fVertices->fTexs) { packed |= kHasTexs_Mask; } - if (fColors) { + if (fVertices->fColors) { packed |= kHasColors_Mask; } - packed |= kCurrent_Version << kVersion_Shift; - size_t attrSize = SkAlign4(sizeof(EncodedAttribute) * fAttributeCount); - for (int i = 0; i < fAttributeCount; ++i) { - if (fAttributes[i].fMarkerName) { - attrSize += SkWriter32::WriteStringSize(fAttributes[i].fMarkerName); - } - } - - Sizes sizes = this->getSizes(); + SkVertices::Sizes sizes = fVertices->getSizes(); SkASSERT(!sizes.fBuilderTriFanISize); - // need to force alignment to 4 for SkWriter32 -- will pad w/ 0s as needed - const size_t size = SkAlign4(kCurrentHeaderSize + attrSize + sizes.fArrays); - - sk_sp data = SkData::MakeUninitialized(size); - SkWriter32 writer(data->writable_data(), data->size()); // Header - writer.write32(packed); - writer.write32(fVertexCount); - writer.write32(fIndexCount); - writer.write32(fAttributeCount); + buffer.writeUInt(packed); + buffer.writeInt(fVertices->fVertexCount); + buffer.writeInt(fVertices->fIndexCount); + buffer.writeInt(fVertices->fAttributeCount); - // Encoded attributes (may not be 4 byte aligned) - EncodedAttribute* encodedAttrs = - (EncodedAttribute*)writer.reservePad(fAttributeCount * sizeof(EncodedAttribute)); - for (int i = 0; i < fAttributeCount; ++i) { - encodedAttrs[i] = {fAttributes[i].fType, fAttributes[i].fUsage, - SkToBool(fAttributes[i].fMarkerName)}; - } - - // Marker names - for (int i = 0; i < fAttributeCount; ++i) { - if (fAttributes[i].fMarkerName) { - writer.writeString(fAttributes[i].fMarkerName); - } + // Attribute metadata + for (int i = 0; i < fVertices->fAttributeCount; ++i) { + buffer.writeInt(static_cast(fVertices->fAttributes[i].fType)); + buffer.writeInt(static_cast(fVertices->fAttributes[i].fUsage)); + buffer.writeString(fVertices->fAttributes[i].fMarkerName); } // Data arrays - writer.write(fPositions, sizes.fVSize); - writer.write(fCustomData, sizes.fDSize); - writer.write(fTexs, sizes.fTSize); - writer.write(fColors, sizes.fCSize); + buffer.writeByteArray(fVertices->fPositions, sizes.fVSize); + buffer.writeByteArray(fVertices->fCustomData, sizes.fDSize); + buffer.writeByteArray(fVertices->fTexs, sizes.fTSize); + buffer.writeByteArray(fVertices->fColors, sizes.fCSize); // if index-count is odd, we won't be 4-bytes aligned, so we call the pad version - writer.writePad(fIndices, sizes.fISize); - - return data; + buffer.writeByteArray(fVertices->fIndices, sizes.fISize); } -sk_sp SkVertices::Decode(const void* data, size_t length) { - if (length < sizeof(Header_v4)) { +sk_sp SkVerticesPriv::Decode(SkReadBuffer& buffer) { + if (buffer.isVersionLT(SkPicturePriv::kVerticesUseReadBuffer_Version)) { + // Old versions used an embedded blob that was serialized with SkWriter32/SkReader32. + // We don't support loading those, but skip over the vertices to keep the buffer valid. + auto data = buffer.readByteArrayAsData(); + (void)data; return nullptr; } - SkReader32 reader(data, length); - SkSafeRange safe; + auto decode = [](SkReadBuffer& buffer) -> sk_sp { + SkSafeRange safe; - const uint32_t packed = reader.readInt(); - const unsigned version = safe.checkLE((packed & kVersion_Mask) >> kVersion_Shift, - kCurrent_Version); - const int vertexCount = safe.checkGE(reader.readInt(), 0); - const int indexCount = safe.checkGE(reader.readInt(), 0); - const int attrCount = safe.checkGE(reader.readInt(), 0); - const VertexMode mode = safe.checkLE(packed & kMode_Mask, - SkVertices::kLast_VertexMode); - const bool hasTexs = SkToBool(packed & kHasTexs_Mask); - const bool hasColors = SkToBool(packed & kHasColors_Mask); + const uint32_t packed = buffer.readUInt(); + const int vertexCount = safe.checkGE(buffer.readInt(), 0); + const int indexCount = safe.checkGE(buffer.readInt(), 0); + const int attrCount = safe.checkGE(buffer.readInt(), 0); + const SkVertices::VertexMode mode = safe.checkLE( + packed & kMode_Mask, SkVertices::kLast_VertexMode); + const bool hasTexs = SkToBool(packed & kHasTexs_Mask); + const bool hasColors = SkToBool(packed & kHasColors_Mask); - if (!safe // Invalid header fields - || attrCount > kMaxCustomAttributes // Too many custom attributes? - || version < kNamedMarkers_Version // Old (unsupported) version - || (attrCount > 0 && (hasTexs || hasColors))) { // Overspecified (incompatible features) - return nullptr; - } + if (!safe // Invalid header fields + || attrCount > SkVertices::kMaxCustomAttributes // Too many custom attributes? + || (attrCount > 0 && (hasTexs || hasColors))) { // Overspecified (incompatible features) + return nullptr; + } - if (!reader.isAvailable(attrCount * sizeof(EncodedAttribute))) { - return nullptr; - } - - Attribute attrs[kMaxCustomAttributes]; - const EncodedAttribute* encodedAttrs = - (const EncodedAttribute*)reader.skip(attrCount * sizeof(EncodedAttribute)); - for (int i = 0; i < attrCount; ++i) { - attrs[i] = Attribute(encodedAttrs[i].fType, encodedAttrs[i].fUsage, - encodedAttrs[i].fHasMarkerName ? reader.readString() : nullptr); - } - - const Desc desc{ - mode, vertexCount, indexCount, hasTexs, hasColors, attrCount ? attrs : nullptr, attrCount - }; - Sizes sizes(desc); - if (!sizes.isValid()) { - return nullptr; - } - // logically we can be only 2-byte aligned, but our buffer is always 4-byte aligned - if (reader.available() != SkAlign4(sizes.fArrays)) { - return nullptr; - } - - Builder builder(desc); - if (!builder.isValid()) { - return nullptr; - } - SkSafeMath safe_math; - - reader.read(builder.positions(), sizes.fVSize); - reader.read(builder.customData(), sizes.fDSize); - reader.read(builder.texCoords(), sizes.fTSize); - reader.read(builder.colors(), sizes.fCSize); - size_t isize = (mode == kTriangleFan_VertexMode) ? sizes.fBuilderTriFanISize : sizes.fISize; - reader.read(builder.indices(), isize); - if (indexCount > 0) { - // validate that the indices are in range - const uint16_t* indices = builder.indices(); - for (int i = 0; i < indexCount; ++i) { - if (indices[i] >= (unsigned)vertexCount) { + SkVertices::Attribute attrs[SkVertices::kMaxCustomAttributes]; + SkString attrNames[SkVertices::kMaxCustomAttributes]; + for (int i = 0; i < attrCount; ++i) { + auto type = buffer.checkRange(SkVertices::Attribute::Type::kFloat, + SkVertices::Attribute::Type::kByte4_unorm); + auto usage = buffer.checkRange(SkVertices::Attribute::Usage::kRaw, + SkVertices::Attribute::Usage::kPosition); + buffer.readString(&attrNames[i]); + const char* markerName = attrNames[i].isEmpty() ? nullptr : attrNames[i].c_str(); + if (markerName && !SkCanvasPriv::ValidateMarker(markerName)) { return nullptr; } + attrs[i] = SkVertices::Attribute(type, usage, markerName); } - } - if (!safe_math.ok()) { - return nullptr; + // Ensure that all of the attribute metadata was valid before proceeding + if (!buffer.isValid()) { + return nullptr; + } + + const SkVertices::Desc desc{mode, vertexCount, indexCount, hasTexs, hasColors, + attrCount ? attrs : nullptr, attrCount}; + SkVertices::Sizes sizes(desc); + if (!sizes.isValid()) { + return nullptr; + } + + SkVertices::Builder builder(desc); + if (!builder.isValid()) { + return nullptr; + } + + buffer.readByteArray(builder.positions(), sizes.fVSize); + buffer.readByteArray(builder.customData(), sizes.fDSize); + buffer.readByteArray(builder.texCoords(), sizes.fTSize); + buffer.readByteArray(builder.colors(), sizes.fCSize); + size_t isize = (mode == SkVertices::kTriangleFan_VertexMode) ? sizes.fBuilderTriFanISize + : sizes.fISize; + buffer.readByteArray(builder.indices(), isize); + + if (!buffer.isValid()) { + return nullptr; + } + + if (indexCount > 0) { + // validate that the indices are in range + const uint16_t* indices = builder.indices(); + for (int i = 0; i < indexCount; ++i) { + if (indices[i] >= (unsigned)vertexCount) { + return nullptr; + } + } + } + + return builder.detach(); + }; + + if (auto verts = decode(buffer)) { + return verts; } - return builder.detach(); + buffer.validate(false); + return nullptr; } void SkVertices::operator delete(void* p) { diff --git a/src/core/SkVerticesPriv.h b/src/core/SkVerticesPriv.h index 9a998e98bb..17591d32db 100644 --- a/src/core/SkVerticesPriv.h +++ b/src/core/SkVerticesPriv.h @@ -10,7 +10,10 @@ #include "include/core/SkVertices.h" -struct SkVertices_DeprecatedBone { float values[6]; }; +class SkReadBuffer; +class SkWriteBuffer; + +struct SkVertices_DeprecatedBone { float values[6]; }; /** Class that adds methods to SkVertices that are only intended for use internal to Skia. This class is purely a privileged window into SkVertices. It should never have additional @@ -41,6 +44,9 @@ public: // Never called due to RVO in priv(), but must exist for MSVC 2017. SkVerticesPriv(const SkVerticesPriv&) = default; + void encode(SkWriteBuffer&) const; + static sk_sp Decode(SkReadBuffer&); + private: explicit SkVerticesPriv(SkVertices* vertices) : fVertices(vertices) {} SkVerticesPriv& operator=(const SkVerticesPriv&) = delete; diff --git a/tests/VerticesTest.cpp b/tests/VerticesTest.cpp index 5517e64a9a..5f2cbbc3b5 100644 --- a/tests/VerticesTest.cpp +++ b/tests/VerticesTest.cpp @@ -8,7 +8,10 @@ #include "include/core/SkCanvas.h" #include "include/core/SkSurface.h" #include "include/core/SkVertices.h" +#include "src/core/SkAutoMalloc.h" +#include "src/core/SkReadBuffer.h" #include "src/core/SkVerticesPriv.h" +#include "src/core/SkWriteBuffer.h" #include "tests/Test.h" #include "tools/ToolUtils.h" @@ -73,9 +76,16 @@ static bool equal(const SkVertices* vert0, const SkVertices* vert1) { } static void self_test(sk_sp v0, skiatest::Reporter* reporter) { - sk_sp data = v0->encode(); - sk_sp v1 = SkVertices::Decode(data->data(), data->size()); + SkBinaryWriteBuffer writer; + v0->priv().encode(writer); + SkAutoMalloc buf(writer.bytesWritten()); + writer.writeToMemory(buf.get()); + SkReadBuffer reader(buf.get(), writer.bytesWritten()); + + sk_sp v1 = SkVerticesPriv::Decode(reader); + + REPORTER_ASSERT(reporter, v1 != nullptr); REPORTER_ASSERT(reporter, v0->uniqueID() != 0); REPORTER_ASSERT(reporter, v1->uniqueID() != 0); REPORTER_ASSERT(reporter, v0->uniqueID() != v1->uniqueID());