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: chromium:1105720
Bug: chromium:1105723
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 <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
(cherry picked from commit 6b3d6e92109fff606cbb3ede28574d913f40a4eb)
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/303659
diff --git a/include/core/SkVertices.h b/include/core/SkVertices.h
index 20a0fa8..0f666f8 100644
--- a/include/core/SkVertices.h
+++ b/include/core/SkVertices.h
@@ -158,6 +158,7 @@
std::unique_ptr<uint8_t[]> fIntermediateFanIndices;
friend class SkVertices;
+ friend class SkVerticesPriv;
};
uint32_t uniqueID() const { return fUniqueID; }
@@ -166,18 +167,6 @@
// 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<SkVertices> 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<SkData> 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 bb6d67c..c4ae9a0 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 <new>
@@ -174,7 +175,7 @@
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 @@
static sk_sp<SkImage> create_image_from_buffer(SkReadBuffer& buffer) {
return buffer.readImage();
}
-static sk_sp<SkVertices> create_vertices_from_buffer(SkReadBuffer& buffer) {
- auto data = buffer.readByteArrayAsData();
- return data ? SkVertices::Decode(data->data(), data->size()) : nullptr;
-}
static sk_sp<SkDrawable> create_drawable_from_buffer(SkReadBuffer& buffer) {
return sk_sp<SkDrawable>((SkDrawable*)buffer.readFlattenable(SkFlattenable::kSkDrawable_Type));
@@ -440,7 +437,7 @@
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 a14cb32..16f0e42 100644
--- a/src/core/SkPicturePriv.h
+++ b/src/core/SkPicturePriv.h
@@ -75,6 +75,7 @@
// 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 @@
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 532bb4f..0f1a834 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 <atomic>
#include <new>
@@ -370,173 +370,138 @@
///////////////////////////////////////////////////////////////////////////////////////////////////
-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<SkData> SkVertices::encode() const {
- // packed has room for addtional flags in the future (e.g. versioning)
- uint32_t packed = static_cast<uint32_t>(fMode);
+void SkVerticesPriv::encode(SkWriteBuffer& buffer) const {
+ // packed has room for additional flags in the future
+ uint32_t packed = static_cast<uint32_t>(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<SkData> 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<int>(fVertices->fAttributes[i].fType));
+ buffer.writeInt(static_cast<int>(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> SkVertices::Decode(const void* data, size_t length) {
- if (length < sizeof(Header_v4)) {
+sk_sp<SkVertices> 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<SkVertices> {
+ SkSafeRange safe;
- const uint32_t packed = reader.readInt();
- const unsigned version = safe.checkLE<unsigned>((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<VertexMode>(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<SkVertices::VertexMode>(
+ 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 9a998e9..17591d3 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 @@
// Never called due to RVO in priv(), but must exist for MSVC 2017.
SkVerticesPriv(const SkVerticesPriv&) = default;
+ void encode(SkWriteBuffer&) const;
+ static sk_sp<SkVertices> 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 5517e64..5f2cbbc 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 void self_test(sk_sp<SkVertices> v0, skiatest::Reporter* reporter) {
- sk_sp<SkData> data = v0->encode();
- sk_sp<SkVertices> 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<SkVertices> 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());
