From 0817ce7c5bce135a980b2457658f4429738b222b Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Fri, 25 Feb 2022 15:06:07 -0500 Subject: [PATCH] Add SK_LENIENT_SKSL_DESERIALIZATION, enabled in Debugger When deserializing an SkSL shader, it's possible to encounter code that doesn't compile (particularly from the last year, when syntax tweaks were being made). By default, we still treat this as an error and fail to load the SKP. In the SKP debugger, though, we add a lenient mode that permits these failures. If any of the children were SkShaders, we instead return that. If all else fails, we just return nullptr, which will cause the triggering draw to use a solid paint-color, but at least allow the SKP to be loaded and viewed. For color-filters and blenders, we allow malformed SkSL, but always return nullptr. The bug that prompted this change involved Android's overscroll stretch shader, where this approach works particularly well (it turns the stretch into a simple pass-through of the underlying content). Change-Id: I756c694739d31b11efa1b82c126f34440a7de66a Reviewed-on: https://skia-review.googlesource.com/c/skia/+/515543 Reviewed-by: John Stiles Commit-Queue: Brian Osman --- include/effects/SkRuntimeEffect.h | 3 ++ src/core/SkReadBuffer.cpp | 15 ++++-- src/core/SkReadBuffer.h | 1 + src/core/SkRuntimeEffect.cpp | 90 +++++++++++++++++++++++++++---- 4 files changed, 95 insertions(+), 14 deletions(-) diff --git a/include/effects/SkRuntimeEffect.h b/include/effects/SkRuntimeEffect.h index 2591c362aa..a77a737074 100644 --- a/include/effects/SkRuntimeEffect.h +++ b/include/effects/SkRuntimeEffect.h @@ -184,6 +184,9 @@ public: ChildPtr(sk_sp cf) : fChild(std::move(cf)) {} ChildPtr(sk_sp b) : fChild(std::move(b)) {} + // Asserts that the flattenable is either null, or one of the legal derived types + ChildPtr(sk_sp f); + std::optional type() const; SkShader* shader() const; diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp index 88f7a925d7..3d6bfe0377 100644 --- a/src/core/SkReadBuffer.cpp +++ b/src/core/SkReadBuffer.cpp @@ -393,7 +393,7 @@ sk_sp SkReadBuffer::readTypeface() { } } -SkFlattenable* SkReadBuffer::readFlattenable(SkFlattenable::Type ft) { +SkFlattenable* SkReadBuffer::readRawFlattenable() { SkFlattenable::Factory factory = nullptr; if (fFactoryCount > 0) { @@ -446,10 +446,6 @@ SkFlattenable* SkReadBuffer::readFlattenable(SkFlattenable::Type ft) { this->validate(false); return nullptr; } - if (obj && obj->getFlattenableType() != ft) { - this->validate(false); - return nullptr; - } } else { // we must skip the remaining data this->skip(sizeRecorded); @@ -460,6 +456,15 @@ SkFlattenable* SkReadBuffer::readFlattenable(SkFlattenable::Type ft) { return obj.release(); } +SkFlattenable* SkReadBuffer::readFlattenable(SkFlattenable::Type ft) { + SkFlattenable* obj = this->readRawFlattenable(); + if (obj && obj->getFlattenableType() != ft) { + this->validate(false); + return nullptr; + } + return obj; +} + /////////////////////////////////////////////////////////////////////////////////////////////////// int32_t SkReadBuffer::checkInt(int32_t min, int32_t max) { diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h index 08338ca018..700fad49e4 100644 --- a/src/core/SkReadBuffer.h +++ b/src/core/SkReadBuffer.h @@ -112,6 +112,7 @@ public: return SkPaintPriv::Unflatten(*this); } + SkFlattenable* readRawFlattenable(); SkFlattenable* readFlattenable(SkFlattenable::Type); template sk_sp readFlattenable() { return sk_sp((T*)this->readFlattenable(T::GetFlattenableType())); diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp index 3e5543cad1..886bb734d1 100644 --- a/src/core/SkRuntimeEffect.cpp +++ b/src/core/SkRuntimeEffect.cpp @@ -49,10 +49,32 @@ #include +#if defined(SK_BUILD_FOR_DEBUGGER) + #define SK_LENIENT_SKSL_DESERIALIZATION 1 +#else + #define SK_LENIENT_SKSL_DESERIALIZATION 0 +#endif + using ChildType = SkRuntimeEffect::ChildType; #ifdef SK_ENABLE_SKSL +static bool flattenable_is_valid_as_child(const SkFlattenable* f) { + if (!f) { return true; } + switch (f->getFlattenableType()) { + case SkFlattenable::kSkShader_Type: + case SkFlattenable::kSkColorFilter_Type: + case SkFlattenable::kSkBlender_Type: + return true; + default: + return false; + } +} + +SkRuntimeEffect::ChildPtr::ChildPtr(sk_sp f) : fChild(std::move(f)) { + SkASSERT(flattenable_is_valid_as_child(fChild.get())); +} + static sk_sp make_skvm_debug_trace(SkRuntimeEffect* effect, const SkIPoint& coord) { auto debugTrace = sk_make_sp(); @@ -114,27 +136,41 @@ static bool verify_child_effects(const std::vector& refl return true; } +/** + * If `effect` is specified, then the number and type of child objects are validated against the + * children() of `effect`. If it's nullptr, this is skipped, allowing deserialization of children, + * even when the effect could not be constructed (ie, due to malformed SkSL). + */ static bool read_child_effects(SkReadBuffer& buffer, const SkRuntimeEffect* effect, SkTArray* children) { size_t childCount = buffer.read32(); - if (!buffer.validate(childCount == effect->children().size())) { + if (effect && !buffer.validate(childCount == effect->children().size())) { return false; } children->reset(); children->reserve_back(childCount); - for (const auto& child : effect->children()) { - if (child.type == ChildType::kShader) { - children->emplace_back(buffer.readShader()); - } else if (child.type == ChildType::kColorFilter) { - children->emplace_back(buffer.readColorFilter()); - } else if (child.type == ChildType::kBlender) { - children->emplace_back(buffer.readBlender()); - } else { + for (size_t i = 0; i < childCount; i++) { + sk_sp obj(buffer.readRawFlattenable()); + if (!flattenable_is_valid_as_child(obj.get())) { + buffer.validate(false); return false; } + children->push_back(std::move(obj)); + } + + // If we are validating against an effect, make sure any (non-null) children are the right type + if (effect) { + auto childInfo = effect->children(); + SkASSERT(childInfo.size() == children->size()); + for (size_t i = 0; i < childCount; i++) { + std::optional ct = (*children)[i].type(); + if (ct.has_value() && (*ct) != childInfo[i].type) { + buffer.validate(false); + } + } } return buffer.isValid(); @@ -1084,15 +1120,24 @@ sk_sp SkRuntimeColorFilter::CreateProc(SkReadBuffer& buffer) { sk_sp uniforms = buffer.readByteArrayAsData(); auto effect = SkMakeCachedRuntimeEffect(SkRuntimeEffect::MakeForColorFilter, std::move(sksl)); +#if !SK_LENIENT_SKSL_DESERIALIZATION if (!buffer.validate(effect != nullptr)) { return nullptr; } +#endif SkSTArray<4, SkRuntimeEffect::ChildPtr> children; if (!read_child_effects(buffer, effect.get(), &children)) { return nullptr; } +#if SK_LENIENT_SKSL_DESERIALIZATION + if (!effect) { + SkDebugf("Serialized SkSL failed to compile. Ignoring/dropping SkSL color filter.\n"); + return nullptr; + } +#endif + return effect->makeColorFilter(std::move(uniforms), SkMakeSpan(children)); } @@ -1219,15 +1264,33 @@ sk_sp SkRTShader::CreateProc(SkReadBuffer& buffer) { } auto effect = SkMakeCachedRuntimeEffect(SkRuntimeEffect::MakeForShader, std::move(sksl)); +#if !SK_LENIENT_SKSL_DESERIALIZATION if (!buffer.validate(effect != nullptr)) { return nullptr; } +#endif SkSTArray<4, SkRuntimeEffect::ChildPtr> children; if (!read_child_effects(buffer, effect.get(), &children)) { return nullptr; } +#if SK_LENIENT_SKSL_DESERIALIZATION + if (!effect) { + // If any children were SkShaders, return the first one. This is a reasonable fallback. + for (int i = 0; i < children.count(); i++) { + if (children[i].shader()) { + SkDebugf("Serialized SkSL failed to compile. Replacing shader with child %d.\n", i); + return sk_ref_sp(children[i].shader()); + } + } + + // We don't know what to do, so just return nullptr (but *don't* poison the buffer). + SkDebugf("Serialized SkSL failed to compile. Ignoring/dropping SkSL shader.\n"); + return nullptr; + } +#endif + return effect->makeShader(std::move(uniforms), SkMakeSpan(children), localMPtr); } @@ -1304,15 +1367,24 @@ sk_sp SkRuntimeBlender::CreateProc(SkReadBuffer& buffer) { sk_sp uniforms = buffer.readByteArrayAsData(); auto effect = SkMakeCachedRuntimeEffect(SkRuntimeEffect::MakeForBlender, std::move(sksl)); +#if !SK_LENIENT_SKSL_DESERIALIZATION if (!buffer.validate(effect != nullptr)) { return nullptr; } +#endif SkSTArray<4, SkRuntimeEffect::ChildPtr> children; if (!read_child_effects(buffer, effect.get(), &children)) { return nullptr; } +#if SK_LENIENT_SKSL_DESERIALIZATION + if (!effect) { + SkDebugf("Serialized SkSL failed to compile. Ignoring/dropping SkSL blender.\n"); + return nullptr; + } +#endif + return effect->makeBlender(std::move(uniforms), SkMakeSpan(children)); }