Discard data written to a null BufferWriter
In Ganesh this was written such that a failed buffer mapping would be
detected before it got wrapped in a writer. In the cases where this
wasn't as convenient, a CPU buffer was wrapped in a writer so the
writes went somewhere before the tasks would eventually report the
failure.
In Graphite, we could always employ this same CPU buffer wrapping
approach but maybe it's simpler to just drop the written data if the
mapping failed. Branch prediction should hopefully mean that this
doesn't impact perf. If that's the case, then a follow up CL will just
have BufferManager track if a buffer failed to map so it can trigger
a failed Recording. If there are regressions then we can explore
having BufferManager allocate CPU arrays.
The alternative to checking on each write within the BufferWriter
is to have every call site within Graphite check if it's
writer is valid. This is exactly the same number of if checks and
this seems cleaner to have it be internal.
This should avoid the crashes in b/334461625, although we'll need the
follow-up CL to address correctness from what would now be an a
DrawPass with bind commands pointing to null Buffer resources
or non-null Buffer resources that failed to be mapped and are then
filled with random data.
Bug: b/334461625
Change-Id: Ibbd1c241ac8bd1131c4d58ce7f9d7ea6524636b5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/841836
Reviewed-by: James Godfrey-Kittle <jamesgk@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/gpu/BufferWriter.h b/src/gpu/BufferWriter.h
index 032420e..543f577 100644
--- a/src/gpu/BufferWriter.h
+++ b/src/gpu/BufferWriter.h
@@ -92,19 +92,22 @@
// Templated so that it can create subclasses directly.
template<typename W>
W makeOffset(size_t offsetInBytes) const {
- this->validate(offsetInBytes);
- void* p = SkTAddOffset<void>(fPtr, offsetInBytes);
- Mark end{SkDEBUGCODE(fEnd)};
- SkDEBUGCODE(fEnd = Mark(p);)
- return W{p, end};
+ if (this->validate(offsetInBytes)) {
+ void* p = SkTAddOffset<void>(fPtr, offsetInBytes);
+ Mark end{SkDEBUGCODE(fEnd)};
+ SkDEBUGCODE(fEnd = Mark(p);)
+ return W{p, end};
+ } else {
+ return W{nullptr, 0};
+ }
}
- void validate(size_t bytesToWrite) const {
+ bool validate(size_t bytesToWrite) const {
// If the buffer writer had an end marked, make sure we're not crossing it.
// Ideally, all creators of BufferWriters mark the end, but a lot of legacy code is not set
// up to easily do this.
- SkASSERT(fPtr || bytesToWrite == 0);
- SkASSERT(!fEnd || Mark(fPtr, bytesToWrite) <= fEnd);
+ SkASSERT(!fPtr || !fEnd || Mark(fPtr, bytesToWrite) <= fEnd);
+ return fPtr != nullptr;
}
protected:
@@ -276,9 +279,10 @@
template <typename T>
inline VertexWriter& operator<<(VertexWriter& w, const T& val) {
static_assert(std::is_trivially_copyable<T>::value, "");
- w.validate(sizeof(T));
- memcpy(w.fPtr, &val, sizeof(T));
- w = w.makeOffset(sizeof(T));
+ if (w.validate(sizeof(T))) {
+ memcpy(w.fPtr, &val, sizeof(T));
+ w = w.makeOffset(sizeof(T));
+ }
return w;
}
@@ -300,9 +304,10 @@
template <typename T>
inline VertexWriter& operator<<(VertexWriter& w, const VertexWriter::ArrayDesc<T>& array) {
static_assert(std::is_trivially_copyable<T>::value, "");
- w.validate(array.fCount * sizeof(T));
- memcpy(w.fPtr, array.fArray, array.fCount * sizeof(T));
- w = w.makeOffset(sizeof(T) * array.fCount);
+ if (w.validate(array.fCount * sizeof(T))) {
+ memcpy(w.fPtr, array.fArray, array.fCount * sizeof(T));
+ w = w.makeOffset(sizeof(T) * array.fCount);
+ }
return w;
}
@@ -316,9 +321,10 @@
template <>
[[maybe_unused]] inline VertexWriter& operator<<(VertexWriter& w, const skvx::float4& vector) {
- w.validate(sizeof(vector));
- vector.store(w.fPtr);
- w = w.makeOffset(sizeof(vector));
+ if (w.validate(sizeof(vector))) {
+ vector.store(w.fPtr);
+ w = w.makeOffset(sizeof(vector));
+ }
return w;
}
@@ -398,18 +404,20 @@
void writeArray(const uint16_t* array, int count) {
size_t arraySize = count * sizeof(uint16_t);
- this->validate(arraySize);
- memcpy(fPtr, array, arraySize);
- fPtr = SkTAddOffset<void>(fPtr, arraySize);
+ if (this->validate(arraySize)) {
+ memcpy(fPtr, array, arraySize);
+ fPtr = SkTAddOffset<void>(fPtr, arraySize);
+ }
}
friend IndexWriter& operator<<(IndexWriter& w, uint16_t val);
};
inline IndexWriter& operator<<(IndexWriter& w, uint16_t val) {
- w.validate(sizeof(uint16_t));
- memcpy(w.fPtr, &val, sizeof(uint16_t));
- w = w.makeOffset(1);
+ if (w.validate(sizeof(uint16_t))) {
+ memcpy(w.fPtr, &val, sizeof(uint16_t));
+ w = w.makeOffset(1);
+ }
return w;
}
@@ -436,13 +444,15 @@
}
void write(const void* src, size_t bytes) {
- this->validate(bytes);
- memcpy(fPtr, src, bytes);
- fPtr = SkTAddOffset<void>(fPtr, bytes);
+ if (this->validate(bytes)) {
+ memcpy(fPtr, src, bytes);
+ fPtr = SkTAddOffset<void>(fPtr, bytes);
+ }
}
void skipBytes(size_t bytes) {
- this->validate(bytes);
- fPtr = SkTAddOffset<void>(fPtr, bytes);
+ if (this->validate(bytes)) {
+ fPtr = SkTAddOffset<void>(fPtr, bytes);
+ }
}
};
@@ -466,18 +476,20 @@
// `srcRowBytes` wide, and the written block is `dstRowBytes` wide and `rowCount` bytes tall.
void write(size_t offset, const void* src, size_t srcRowBytes, size_t dstRowBytes,
size_t trimRowBytes, int rowCount) {
- this->validate(dstRowBytes * rowCount);
- void* dst = SkTAddOffset<void>(fPtr, offset);
- SkRectMemcpy(dst, dstRowBytes, src, srcRowBytes, trimRowBytes, rowCount);
+ if (this->validate(dstRowBytes * rowCount)) {
+ void* dst = SkTAddOffset<void>(fPtr, offset);
+ SkRectMemcpy(dst, dstRowBytes, src, srcRowBytes, trimRowBytes, rowCount);
+ }
}
void convertAndWrite(size_t offset,
const SkImageInfo& srcInfo, const void* src, size_t srcRowBytes,
const SkImageInfo& dstInfo, size_t dstRowBytes) {
SkASSERT(srcInfo.width() == dstInfo.width() && srcInfo.height() == dstInfo.height());
- this->validate(dstRowBytes * dstInfo.height());
- void* dst = SkTAddOffset<void>(fPtr, offset);
- SkAssertResult(SkConvertPixels(dstInfo, dst, dstRowBytes, srcInfo, src, srcRowBytes));
+ if (this->validate(dstRowBytes * dstInfo.height())) {
+ void* dst = SkTAddOffset<void>(fPtr, offset);
+ SkAssertResult(SkConvertPixels(dstInfo, dst, dstRowBytes, srcInfo, src, srcRowBytes));
+ }
}
// Writes a block of image data to the upload buffer. It converts src data of RGB_888x
