spirv-reader: reject undef and null pointers
This is defensive. Without variable pointers capabilities, this is
definitely invalid, but not yet checked by the SPIRV-Tools validator.
Bug: tint:807
Change-Id: If9b0b19573b1ca14a1c55aa20c9d42784ec12568
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/56700
Auto-Submit: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index fe3d320..e5ca12f 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -4560,6 +4560,8 @@
}
switch (inst.opcode()) {
case SpvOpUndef:
+ return Fail()
+ << "undef pointer is not valid: " << inst.PrettyPrint();
case SpvOpVariable:
// Keep the default decision based on the result type.
break;
diff --git a/src/reader/spirv/function_memory_test.cc b/src/reader/spirv/function_memory_test.cc
index bb16d54..5249114 100644
--- a/src/reader/spirv/function_memory_test.cc
+++ b/src/reader/spirv/function_memory_test.cc
@@ -1280,12 +1280,11 @@
}
TEST_F(SpvParserMemoryTest, DISABLED_RemapStorageBuffer_ThroughFunctionCall) {
- // TODO(dneto): Blocked on OpFunctionCall support.
- // We might need this for passing pointers into atomic builtins.
+ // WGSL does not support pointer-to-storage-buffer as function parameter
}
TEST_F(SpvParserMemoryTest,
DISABLED_RemapStorageBuffer_ThroughFunctionParameter) {
- // TODO(dneto): Blocked on OpFunctionCall support.
+ // WGSL does not support pointer-to-storage-buffer as function parameter
}
std::string RuntimeArrayPreamble() {
@@ -1359,6 +1358,79 @@
)")) << body_str;
}
+std::string InvalidPointerPreamble() {
+ return R"(
+OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint Fragment %main "main"
+OpExecutionMode %main OriginUpperLeft
+
+%uint = OpTypeInt 32 0
+%ptr_ty = OpTypePointer Function %uint
+
+ %void = OpTypeVoid
+%voidfn = OpTypeFunction %void
+)";
+}
+
+TEST_F(SpvParserMemoryTest, InvalidPointer_Undef_ModuleScope_IsError) {
+ const std::string assembly = InvalidPointerPreamble() + R"(
+ %ptr = OpUndef %ptr_ty
+
+ %main = OpFunction %void None %voidfn
+ %entry = OpLabel
+ %1 = OpCopyObject %ptr_ty %ptr
+ %2 = OpAccessChain %ptr_ty %ptr
+ %3 = OpInBoundsAccessChain %ptr_ty %ptr
+; now show the invalid pointer propagates
+ %10 = OpCopyObject %ptr_ty %1
+ %20 = OpAccessChain %ptr_ty %2
+ %30 = OpInBoundsAccessChain %ptr_ty %3
+ OpReturn
+ OpFunctionEnd
+)";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_FALSE(p->BuildAndParseInternalModule()) << assembly;
+ EXPECT_EQ(p->error(), "undef pointer is not valid: %9 = OpUndef %6");
+}
+
+TEST_F(SpvParserMemoryTest, InvalidPointer_Undef_FunctionScope_IsError) {
+ const std::string assembly = InvalidPointerPreamble() + R"(
+
+ %main = OpFunction %void None %voidfn
+ %entry = OpLabel
+ %ptr = OpUndef %ptr_ty
+ OpReturn
+ OpFunctionEnd
+)";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_FALSE(p->BuildAndParseInternalModule()) << assembly;
+ EXPECT_EQ(p->error(), "undef pointer is not valid: %7 = OpUndef %3");
+}
+
+TEST_F(SpvParserMemoryTest, InvalidPointer_ConstantNull_IsError) {
+ // OpConstantNull on logical pointer requires variable-pointers, which
+ // is not (yet) supported by WGSL features.
+ const std::string assembly = InvalidPointerPreamble() + R"(
+ %ptr = OpConstantNull %ptr_ty
+
+ %main = OpFunction %void None %voidfn
+ %entry = OpLabel
+ %1 = OpCopyObject %ptr_ty %ptr
+ %2 = OpAccessChain %ptr_ty %ptr
+ %3 = OpInBoundsAccessChain %ptr_ty %ptr
+; now show the invalid pointer propagates
+ %10 = OpCopyObject %ptr_ty %1
+ %20 = OpAccessChain %ptr_ty %2
+ %30 = OpInBoundsAccessChain %ptr_ty %3
+ OpReturn
+ OpFunctionEnd
+)";
+ auto p = parser(test::Assemble(assembly));
+ EXPECT_FALSE(p->BuildAndParseInternalModule());
+ EXPECT_EQ(p->error(), "null pointer is not valid: %9 = OpConstantNull %6");
+}
+
} // namespace
} // namespace spirv
} // namespace reader
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index 25a67bf..32d3f9f 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/src/reader/spirv/parser_impl.cc
@@ -610,6 +610,9 @@
if (!RegisterTypes()) {
return false;
}
+ if (!RejectInvalidPointerRoots()) {
+ return false;
+ }
if (!EmitScalarSpecConstants()) {
return false;
}
@@ -1288,6 +1291,33 @@
return success_;
}
+bool ParserImpl::RejectInvalidPointerRoots() {
+ if (!success_) {
+ return false;
+ }
+ for (auto& inst : module_->types_values()) {
+ if (const auto* result_type = type_mgr_->GetType(inst.type_id())) {
+ if (result_type->AsPointer()) {
+ switch (inst.opcode()) {
+ case SpvOpVariable:
+ // This is the only valid case.
+ break;
+ case SpvOpUndef:
+ return Fail() << "undef pointer is not valid: "
+ << inst.PrettyPrint();
+ case SpvOpConstantNull:
+ return Fail() << "null pointer is not valid: "
+ << inst.PrettyPrint();
+ default:
+ return Fail() << "module-scope pointer is not valid: "
+ << inst.PrettyPrint();
+ }
+ }
+ }
+ }
+ return success();
+}
+
bool ParserImpl::EmitScalarSpecConstants() {
if (!success_) {
return false;
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h
index b93a993..99b7cf7 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -349,6 +349,11 @@
/// @returns true if parser is still successful.
bool RegisterTypes();
+ /// Fail if there are any module-scope pointer values other than those
+ /// declared by OpVariable.
+ /// @returns true if parser is still successful.
+ bool RejectInvalidPointerRoots();
+
/// Register sampler and texture usage for memory object declarations.
/// This must be called after we've registered line numbers for all
/// instructions. This is a no-op if the parser has already failed.
diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc
index 54c8468..0ca4db3 100644
--- a/src/reader/spirv/parser_impl_module_var_test.cc
+++ b/src/reader/spirv/parser_impl_module_var_test.cc
@@ -344,7 +344,7 @@
const std::string assembly = PerVertexPreamble() + R"(
%main = OpFunction %void None %voidfn
%entry = OpLabel
- %1000 = OpUndef %11
+ %1000 = OpCopyObject %11 %1
OpReturn
OpFunctionEnd
)";
@@ -352,7 +352,7 @@
EXPECT_FALSE(p->BuildAndParseInternalModule());
EXPECT_THAT(p->error(),
Eq("operations producing a pointer to a per-vertex structure are "
- "not supported: %1000 = OpUndef %11"))
+ "not supported: %1000 = OpCopyObject %11 %1"))
<< p->error();
}
