Fix array timeout discovered by the fuzzer.
The fuzzer discovered that, when we attempt to verify that an array
doesn't contain any literal values that are out-of-range for its base
type, we pay a linear-time cost based on the size of the array. This
happens even when the array value isn't known at compile time; we still
iterate over its slot count and diligently discover that every single
constant-subexpression slot in the expression is "null".
We now have a helper function on Expression,
`allowsConstantSubexpressions`, which only returns true for expression
kinds that can contain constant subexpressions. We use this helper to
skip over this linear-per-subexpression check when the expression
cannot possibly contain a constant subexpression. In particular,
`AnyConstructor::compareConstant` and `Type::checkForOutOfRangeLiteral`
will now early-out for expressions that can't possibly contain a
constant subexpression.
Change-Id: Ia34e422afa67b478a8616acb0a0e9cd211b29698
Bug: oss-fuzz:37900
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/444136
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index e4fda42..6f84056 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -351,6 +351,7 @@
"/sksl/shared/Ossfuzz36852.sksl",
"/sksl/shared/Ossfuzz37466.sksl",
"/sksl/shared/Ossfuzz37677.sksl",
+ "/sksl/shared/Ossfuzz37900.sksl",
"/sksl/shared/OutParams.sksl",
"/sksl/shared/OutParamsNoInline.sksl",
"/sksl/shared/OutParamsTricky.sksl",
diff --git a/resources/sksl/shared/Ossfuzz37900.sksl b/resources/sksl/shared/Ossfuzz37900.sksl
new file mode 100644
index 0000000..ce7a352
--- /dev/null
+++ b/resources/sksl/shared/Ossfuzz37900.sksl
@@ -0,0 +1,3 @@
+void main() {
+ int[2147483646] a, b=a, c=a, d=a, e=a, f=a, g=a, h=a, i=a, j=a, k=a;
+}
diff --git a/src/sksl/ir/SkSLBoolLiteral.h b/src/sksl/ir/SkSLBoolLiteral.h
index be7fedd..7857457 100644
--- a/src/sksl/ir/SkSLBoolLiteral.h
+++ b/src/sksl/ir/SkSLBoolLiteral.h
@@ -69,6 +69,10 @@
return std::make_unique<BoolLiteral>(fOffset, this->value(), &this->type());
}
+ bool allowsConstantSubexpressions() const override {
+ return true;
+ }
+
const Expression* getConstantSubexpression(int n) const override {
SkASSERT(n == 0);
return this;
diff --git a/src/sksl/ir/SkSLConstructor.cpp b/src/sksl/ir/SkSLConstructor.cpp
index d188659..b24da92 100644
--- a/src/sksl/ir/SkSLConstructor.cpp
+++ b/src/sksl/ir/SkSLConstructor.cpp
@@ -185,9 +185,13 @@
}
Expression::ComparisonResult AnyConstructor::compareConstant(const Expression& other) const {
- ComparisonResult result = ComparisonResult::kEqual;
SkASSERT(this->type().slotCount() == other.type().slotCount());
+ if (!other.allowsConstantSubexpressions()) {
+ return ComparisonResult::kUnknown;
+ }
+
+ ComparisonResult result = ComparisonResult::kEqual;
int exprs = this->type().slotCount();
for (int n = 0; n < exprs; ++n) {
// Get the n'th subexpression from each side. If either one is null, return "unknown."
diff --git a/src/sksl/ir/SkSLConstructor.h b/src/sksl/ir/SkSLConstructor.h
index e6afade..f752565 100644
--- a/src/sksl/ir/SkSLConstructor.h
+++ b/src/sksl/ir/SkSLConstructor.h
@@ -68,6 +68,7 @@
return true;
}
+ bool allowsConstantSubexpressions() const override { return true; }
const Expression* getConstantSubexpression(int n) const override;
ComparisonResult compareConstant(const Expression& other) const override;
diff --git a/src/sksl/ir/SkSLConstructorDiagonalMatrix.h b/src/sksl/ir/SkSLConstructorDiagonalMatrix.h
index b9fd229..99372eb 100644
--- a/src/sksl/ir/SkSLConstructorDiagonalMatrix.h
+++ b/src/sksl/ir/SkSLConstructorDiagonalMatrix.h
@@ -40,6 +40,7 @@
argument()->clone());
}
+ bool allowsConstantSubexpressions() const override { return true; }
const Expression* getConstantSubexpression(int n) const override;
private:
diff --git a/src/sksl/ir/SkSLConstructorMatrixResize.h b/src/sksl/ir/SkSLConstructorMatrixResize.h
index 0036180..5bffe33 100644
--- a/src/sksl/ir/SkSLConstructorMatrixResize.h
+++ b/src/sksl/ir/SkSLConstructorMatrixResize.h
@@ -42,6 +42,7 @@
argument()->clone());
}
+ bool allowsConstantSubexpressions() const override { return true; }
const Expression* getConstantSubexpression(int n) const override;
private:
diff --git a/src/sksl/ir/SkSLConstructorSplat.h b/src/sksl/ir/SkSLConstructorSplat.h
index 4a27e00..0409bee 100644
--- a/src/sksl/ir/SkSLConstructorSplat.h
+++ b/src/sksl/ir/SkSLConstructorSplat.h
@@ -38,6 +38,10 @@
return std::make_unique<ConstructorSplat>(fOffset, this->type(), argument()->clone());
}
+ bool allowsConstantSubexpressions() const override {
+ return true;
+ }
+
const Expression* getConstantSubexpression(int n) const override {
SkASSERT(n >= 0 && n < this->type().columns());
return this->argument()->getConstantSubexpression(0);
diff --git a/src/sksl/ir/SkSLExpression.h b/src/sksl/ir/SkSLExpression.h
index 76073bb..4d524f7 100644
--- a/src/sksl/ir/SkSLExpression.h
+++ b/src/sksl/ir/SkSLExpression.h
@@ -159,14 +159,29 @@
}
/**
+ * Returns true if this expression type supports `getConstantSubexpression`. (This particular
+ * expression may or may not actually contain a constant value.) It's harmless to call
+ * `getConstantSubexpression` on expressions which don't allow constant subexpressions or don't
+ * contain any constant values, but if `allowsConstantSubexpressions` returns false, you can
+ * assume that `getConstantSubexpression` will return null for every slot of this expression.
+ * This allows for early-out opportunities in some cases. (Some expressions have tons of slots
+ * but never have a constant subexpression; e.g. a variable holding a very large array.)
+ */
+ virtual bool allowsConstantSubexpressions() const {
+ return false;
+ }
+
+ /**
* Returns the n'th compile-time constant expression within a literal or constructor.
* Use Type::slotCount to determine the number of subexpressions within an expression.
* Subexpressions which are not compile-time constants will return null.
* `vec4(1, vec2(2), 3)` contains four subexpressions: (1, 2, 2, 3)
* `mat2(f)` contains four subexpressions: (null, 0,
* 0, null)
+ * All classes which override this function must also implement `allowsConstantSubexpression`.
*/
virtual const Expression* getConstantSubexpression(int n) const {
+ SkASSERT(!this->allowsConstantSubexpressions());
return nullptr;
}
diff --git a/src/sksl/ir/SkSLFloatLiteral.h b/src/sksl/ir/SkSLFloatLiteral.h
index b8c27b2..5f6c409 100644
--- a/src/sksl/ir/SkSLFloatLiteral.h
+++ b/src/sksl/ir/SkSLFloatLiteral.h
@@ -75,6 +75,10 @@
return std::make_unique<FloatLiteral>(fOffset, this->value(), &this->type());
}
+ bool allowsConstantSubexpressions() const override {
+ return true;
+ }
+
const Expression* getConstantSubexpression(int n) const override {
SkASSERT(n == 0);
return this;
diff --git a/src/sksl/ir/SkSLIntLiteral.h b/src/sksl/ir/SkSLIntLiteral.h
index c18ea09..eb06de6 100644
--- a/src/sksl/ir/SkSLIntLiteral.h
+++ b/src/sksl/ir/SkSLIntLiteral.h
@@ -77,6 +77,10 @@
return std::make_unique<IntLiteral>(fOffset, this->value(), &this->type());
}
+ bool allowsConstantSubexpressions() const override {
+ return true;
+ }
+
const Expression* getConstantSubexpression(int n) const override {
SkASSERT(n == 0);
return this;
diff --git a/src/sksl/ir/SkSLType.cpp b/src/sksl/ir/SkSLType.cpp
index 40e8447..38db128 100644
--- a/src/sksl/ir/SkSLType.cpp
+++ b/src/sksl/ir/SkSLType.cpp
@@ -755,22 +755,23 @@
if (baseType.isInteger()) {
// Replace constant expressions with their corresponding values.
const Expression* valueExpr = ConstantFolder::GetConstantValueForVariable(expr);
-
- // Iterate over every constant subexpression in the value.
- int numSlots = valueExpr->type().slotCount();
- for (int slot = 0; slot < numSlots; ++slot) {
- const Expression* subexpr = valueExpr->getConstantSubexpression(slot);
- if (!subexpr || !subexpr->is<IntLiteral>()) {
- continue;
- }
- // Look for an IntLiteral value that is out of range for the corresponding type.
- SKSL_INT value = subexpr->as<IntLiteral>().value();
- if (value < baseType.minimumValue() || value > baseType.maximumValue()) {
- // We found a value that can't fit in the type. Flag it as an error.
- context.fErrors->error(expr.fOffset,
- String("integer is out of range for type '") +
- this->displayName().c_str() + "': " + to_string(value));
- foundError = true;
+ if (valueExpr->allowsConstantSubexpressions()) {
+ // Iterate over every constant subexpression in the value.
+ int numSlots = valueExpr->type().slotCount();
+ for (int slot = 0; slot < numSlots; ++slot) {
+ const Expression* subexpr = valueExpr->getConstantSubexpression(slot);
+ if (!subexpr || !subexpr->is<IntLiteral>()) {
+ continue;
+ }
+ // Look for an IntLiteral value that is out of range for the corresponding type.
+ SKSL_INT value = subexpr->as<IntLiteral>().value();
+ if (value < baseType.minimumValue() || value > baseType.maximumValue()) {
+ // We found a value that can't fit in the type. Flag it as an error.
+ context.fErrors->error(expr.fOffset,
+ String("integer is out of range for type '") +
+ this->displayName().c_str() + "': " + to_string(value));
+ foundError = true;
+ }
}
}
}
diff --git a/tests/sksl/shared/Ossfuzz37900.asm.frag b/tests/sksl/shared/Ossfuzz37900.asm.frag
new file mode 100644
index 0000000..dd45657
--- /dev/null
+++ b/tests/sksl/shared/Ossfuzz37900.asm.frag
@@ -0,0 +1,17 @@
+OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main" %sk_Clockwise
+OpExecutionMode %main OriginUpperLeft
+OpName %sk_Clockwise "sk_Clockwise"
+OpName %main "main"
+OpDecorate %sk_Clockwise BuiltIn FrontFacing
+%bool = OpTypeBool
+%_ptr_Input_bool = OpTypePointer Input %bool
+%sk_Clockwise = OpVariable %_ptr_Input_bool Input
+%void = OpTypeVoid
+%7 = OpTypeFunction %void
+%main = OpFunction %void None %7
+%8 = OpLabel
+OpReturn
+OpFunctionEnd
diff --git a/tests/sksl/shared/Ossfuzz37900.glsl b/tests/sksl/shared/Ossfuzz37900.glsl
new file mode 100644
index 0000000..2933520
--- /dev/null
+++ b/tests/sksl/shared/Ossfuzz37900.glsl
@@ -0,0 +1,3 @@
+
+void main() {
+}
diff --git a/tests/sksl/shared/Ossfuzz37900.metal b/tests/sksl/shared/Ossfuzz37900.metal
new file mode 100644
index 0000000..2402b80
--- /dev/null
+++ b/tests/sksl/shared/Ossfuzz37900.metal
@@ -0,0 +1,13 @@
+#include <metal_stdlib>
+#include <simd/simd.h>
+using namespace metal;
+struct Inputs {
+};
+struct Outputs {
+ float4 sk_FragColor [[color(0)]];
+};
+fragment Outputs fragmentMain(Inputs _in [[stage_in]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) {
+ Outputs _out;
+ (void)_out;
+ return _out;
+}