Revert "Revert "Fix SkPathRef deserialization malloc crash""
This reverts commit a4ce4b1f6bef22e7ca5c7a952197fc2bc70923fc.
Fix SkPathRef deserialization malloc crash
If the path says it has more points/verbs/etc than the buffer could
be holding, then resetToSize could try to allocate something huge
and crash.
Bug: skia:
Change-Id: I23b8870e9f74386aca89fb8f9a60d3b452044094
Reviewed-on: https://skia-review.googlesource.com/26805
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
diff --git a/include/core/SkPath.h b/include/core/SkPath.h
index b36fa65..d0841fd 100644
--- a/include/core/SkPath.h
+++ b/include/core/SkPath.h
@@ -12,6 +12,7 @@
#include "../private/SkPathRef.h"
class SkAutoPathBoundsUpdate;
+class SkData;
class SkRRect;
class SkWStream;
@@ -1090,6 +1091,12 @@
* If buffer is NULL, it still returns the number of bytes.
*/
size_t writeToMemory(void* buffer) const;
+
+ /**
+ * Write the path to a buffer. Can recreate the path by calling readFromMemory().
+ */
+ sk_sp<SkData> serialize() const;
+
/**
* Initializes the path from the buffer
*
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 2be3251..e304b4d 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -8,6 +8,7 @@
#include <cmath>
#include "SkBuffer.h"
#include "SkCubicClipper.h"
+#include "SkData.h"
#include "SkGeometry.h"
#include "SkMath.h"
#include "SkPathPriv.h"
@@ -2061,6 +2062,13 @@
return buffer.pos();
}
+sk_sp<SkData> SkPath::serialize() const {
+ size_t size = this->writeToMemory(nullptr);
+ sk_sp<SkData> data = SkData::MakeUninitialized(size);
+ this->writeToMemory(data->writable_data());
+ return data;
+}
+
size_t SkPath::readFromMemory(const void* storage, size_t length) {
SkRBuffer buffer(storage, length);
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index 3338225..abe1f90 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -243,29 +243,42 @@
unsigned rrectOrOvalStartIdx = (packed >> kRRectOrOvalStartIdx_SerializationShift) & 0x7;
int32_t verbCount, pointCount, conicCount;
- ptrdiff_t maxPtrDiff = std::numeric_limits<ptrdiff_t>::max();
if (!buffer->readU32(&(ref->fGenerationID)) ||
- !buffer->readS32(&verbCount) ||
- verbCount < 0 ||
- static_cast<uint32_t>(verbCount) > maxPtrDiff/sizeof(uint8_t) ||
- !buffer->readS32(&pointCount) ||
- pointCount < 0 ||
- static_cast<uint32_t>(pointCount) > maxPtrDiff/sizeof(SkPoint) ||
- sizeof(uint8_t) * verbCount + sizeof(SkPoint) * pointCount >
- static_cast<size_t>(maxPtrDiff) ||
- !buffer->readS32(&conicCount) ||
- conicCount < 0) {
+ !buffer->readS32(&verbCount) || (verbCount < 0) ||
+ !buffer->readS32(&pointCount) || (pointCount < 0) ||
+ !buffer->readS32(&conicCount) || (conicCount < 0))
+ {
return nullptr;
}
+ uint64_t pointSize64 = sk_64_mul(pointCount, sizeof(SkPoint));
+ uint64_t conicSize64 = sk_64_mul(conicCount, sizeof(SkScalar));
+ if (!SkTFitsIn<size_t>(pointSize64) || !SkTFitsIn<size_t>(conicSize64)) {
+ return nullptr;
+ }
+
+ size_t verbSize = verbCount * sizeof(uint8_t);
+ size_t pointSize = SkToSizeT(pointSize64);
+ size_t conicSize = SkToSizeT(conicSize64);
+
+ {
+ uint64_t requiredBufferSize = sizeof(SkRect);
+ requiredBufferSize += verbSize;
+ requiredBufferSize += pointSize;
+ requiredBufferSize += conicSize;
+ if (buffer->available() < requiredBufferSize) {
+ return nullptr;
+ }
+ }
+
ref->resetToSize(verbCount, pointCount, conicCount);
- SkASSERT(verbCount == ref->countVerbs());
+ SkASSERT(verbCount == ref->countVerbs());
SkASSERT(pointCount == ref->countPoints());
SkASSERT(conicCount == ref->fConicWeights.count());
- if (!buffer->read(ref->verbsMemWritable(), verbCount * sizeof(uint8_t)) ||
- !buffer->read(ref->fPoints, pointCount * sizeof(SkPoint)) ||
- !buffer->read(ref->fConicWeights.begin(), conicCount * sizeof(SkScalar)) ||
+ if (!buffer->read(ref->verbsMemWritable(), verbSize) ||
+ !buffer->read(ref->fPoints, pointSize) ||
+ !buffer->read(ref->fConicWeights.begin(), conicSize) ||
!buffer->read(&ref->fBounds, sizeof(SkRect))) {
return nullptr;
}
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 117da5b..cdaa1c5 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -4775,3 +4775,58 @@
orr.setRectRadii(ro, rdo);
SkMakeNullCanvas()->drawDRRect(orr, irr, SkPaint());
}
+
+DEF_TEST(PathRefSerialization, reporter) {
+ SkPath path;
+ const size_t numMoves = 5;
+ const size_t numConics = 7;
+ const size_t numPoints = numMoves + 2 * numConics;
+ const size_t numVerbs = numMoves + numConics;
+ for (size_t i = 0; i < numMoves; ++i) path.moveTo(1, 2);
+ for (size_t i = 0; i < numConics; ++i) path.conicTo(1, 2, 3, 4, 5);
+ REPORTER_ASSERT(reporter, path.countPoints() == numPoints);
+ REPORTER_ASSERT(reporter, path.countVerbs() == numVerbs);
+
+ // Verify that path serializes/deserializes properly.
+ sk_sp<SkData> data = path.serialize();
+ size_t bytesWritten = data->size();
+
+ {
+ SkPath readBack;
+ REPORTER_ASSERT(reporter, readBack != path);
+ size_t bytesRead = readBack.readFromMemory(data->data(), bytesWritten);
+ REPORTER_ASSERT(reporter, bytesRead == bytesWritten);
+ REPORTER_ASSERT(reporter, readBack == path);
+ }
+
+ // uint32_t[] offset into serialized path.
+ const size_t verbCountOffset = 4;
+ const size_t pointCountOffset = 5;
+ const size_t conicCountOffset = 6;
+
+ // Verify that this test is changing the right values.
+ const int* writtenValues = static_cast<const int*>(data->data());
+ REPORTER_ASSERT(reporter, writtenValues[verbCountOffset] == numVerbs);
+ REPORTER_ASSERT(reporter, writtenValues[pointCountOffset] == numPoints);
+ REPORTER_ASSERT(reporter, writtenValues[conicCountOffset] == numConics);
+
+ // Too many verbs, points, or conics fails to deserialize silently.
+ const int tooManyObjects = INT_MAX;
+ size_t offsets[] = {verbCountOffset, pointCountOffset, conicCountOffset};
+ for (size_t i = 0; i < 3; ++i) {
+ SkAutoMalloc storage_copy(bytesWritten);
+ memcpy(storage_copy.get(), data->data(), bytesWritten);
+ static_cast<int*>(storage_copy.get())[offsets[i]] = tooManyObjects;
+ SkPath readBack;
+ size_t bytesRead = readBack.readFromMemory(storage_copy.get(), bytesWritten);
+ REPORTER_ASSERT(reporter, !bytesRead);
+ }
+
+ // One less byte (rounded down to alignment) than was written will also
+ // fail to be deserialized.
+ {
+ SkPath readBack;
+ size_t bytesRead = readBack.readFromMemory(data->data(), bytesWritten - 4);
+ REPORTER_ASSERT(reporter, !bytesRead);
+ }
+}