Handle overflowing id in merge return
If the ids overflow when creating an integer constant in the ir_builder, there will be a nullptr dereference. This is happening from inside merge return.
We need to propagate the error up, and make sure it is handled appropriately.
diff --git a/source/opt/ir_builder.h b/source/opt/ir_builder.h
index fe5feff..4433cf0 100644
--- a/source/opt/ir_builder.h
+++ b/source/opt/ir_builder.h
@@ -359,8 +359,9 @@
return AddInstruction(std::move(select));
}
- // Adds a signed int32 constant to the binary.
- // The |value| parameter is the constant value to be added.
+ // Returns a pointer to the definition of a signed 32-bit integer constant
+ // with the given value. Returns |nullptr| if the constant does not exist and
+ // cannot be created.
Instruction* GetSintConstant(int32_t value) {
return GetIntConstant<int32_t>(value, true);
}
@@ -381,21 +382,24 @@
GetContext()->TakeNextId(), ops));
return AddInstruction(std::move(construct));
}
- // Adds an unsigned int32 constant to the binary.
- // The |value| parameter is the constant value to be added.
+
+ // Returns a pointer to the definition of an unsigned 32-bit integer constant
+ // with the given value. Returns |nullptr| if the constant does not exist and
+ // cannot be created.
Instruction* GetUintConstant(uint32_t value) {
return GetIntConstant<uint32_t>(value, false);
}
uint32_t GetUintConstantId(uint32_t value) {
Instruction* uint_inst = GetUintConstant(value);
- return uint_inst->result_id();
+ return (uint_inst != nullptr ? uint_inst->result_id() : 0);
}
// Adds either a signed or unsigned 32 bit integer constant to the binary
- // depedning on the |sign|. If |sign| is true then the value is added as a
+ // depending on the |sign|. If |sign| is true then the value is added as a
// signed constant otherwise as an unsigned constant. If |sign| is false the
- // value must not be a negative number.
+ // value must not be a negative number. Returns false if the constant does
+ // not exists and could be be created.
template <typename T>
Instruction* GetIntConstant(T value, bool sign) {
// Assert that we are not trying to store a negative number in an unsigned
@@ -411,6 +415,10 @@
uint32_t type_id =
GetContext()->get_type_mgr()->GetTypeInstruction(&int_type);
+ if (type_id == 0) {
+ return nullptr;
+ }
+
// Get the memory managed type so that it is safe to be stored by
// GetConstant.
analysis::Type* rebuilt_type =
diff --git a/source/opt/merge_return_pass.cpp b/source/opt/merge_return_pass.cpp
index f160104..a962a7c 100644
--- a/source/opt/merge_return_pass.cpp
+++ b/source/opt/merge_return_pass.cpp
@@ -111,7 +111,9 @@
}
RecordImmediateDominators(function);
- AddSingleCaseSwitchAroundFunction();
+ if (!AddSingleCaseSwitchAroundFunction()) {
+ return false;
+ }
std::list<BasicBlock*> order;
cfg()->ComputeStructuredOrder(function, &*function->begin(), &order);
@@ -770,7 +772,7 @@
list->insert(pos, new_element);
}
-void MergeReturnPass::AddSingleCaseSwitchAroundFunction() {
+bool MergeReturnPass::AddSingleCaseSwitchAroundFunction() {
CreateReturnBlock();
CreateReturn(final_return_block_);
@@ -778,7 +780,10 @@
cfg()->RegisterBlock(final_return_block_);
}
- CreateSingleCaseSwitch(final_return_block_);
+ if (!CreateSingleCaseSwitch(final_return_block_)) {
+ return false;
+ }
+ return true;
}
BasicBlock* MergeReturnPass::CreateContinueTarget(uint32_t header_label_id) {
@@ -813,7 +818,7 @@
return new_block;
}
-void MergeReturnPass::CreateSingleCaseSwitch(BasicBlock* merge_target) {
+bool MergeReturnPass::CreateSingleCaseSwitch(BasicBlock* merge_target) {
// Insert the switch before any code is run. We have to split the entry
// block to make sure the OpVariable instructions remain in the entry block.
BasicBlock* start_block = &*function_->begin();
@@ -830,13 +835,17 @@
context(), start_block,
IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping);
- builder.AddSwitch(builder.GetUintConstantId(0u), old_block->id(), {},
- merge_target->id());
+ uint32_t const_zero_id = builder.GetUintConstantId(0u);
+ if (const_zero_id == 0) {
+ return false;
+ }
+ builder.AddSwitch(const_zero_id, old_block->id(), {}, merge_target->id());
if (context()->AreAnalysesValid(IRContext::kAnalysisCFG)) {
cfg()->RegisterBlock(old_block);
cfg()->AddEdges(start_block);
}
+ return true;
}
bool MergeReturnPass::HasNontrivialUnreachableBlocks(Function* function) {
diff --git a/source/opt/merge_return_pass.h b/source/opt/merge_return_pass.h
index 06a3e7b..4096ce7 100644
--- a/source/opt/merge_return_pass.h
+++ b/source/opt/merge_return_pass.h
@@ -277,7 +277,7 @@
// current function where the switch and case value are both zero and the
// default is the merge block. Returns after the switch is executed. Sets
// |final_return_block_|.
- void AddSingleCaseSwitchAroundFunction();
+ bool AddSingleCaseSwitchAroundFunction();
// Creates a new basic block that branches to |header_label_id|. Returns the
// new basic block. The block will be the second last basic block in the
@@ -286,7 +286,7 @@
// Creates a one case switch around the executable code of the function with
// |merge_target| as the merge node.
- void CreateSingleCaseSwitch(BasicBlock* merge_target);
+ bool CreateSingleCaseSwitch(BasicBlock* merge_target);
// Returns true if |function| has an unreachable block that is not a continue
// target that simply branches back to the header, or a merge block containing
diff --git a/test/opt/pass_merge_return_test.cpp b/test/opt/pass_merge_return_test.cpp
index fd97efa..21960d1 100644
--- a/test/opt/pass_merge_return_test.cpp
+++ b/test/opt/pass_merge_return_test.cpp
@@ -2567,6 +2567,39 @@
SinglePassRunAndMatch<MergeReturnPass>(before, true);
}
+TEST_F(MergeReturnPassTest, OverflowTest1) {
+ const std::string text =
+ R"(
+; CHECK: OpReturn
+; CHECK-NOT: OpReturn
+; CHECK: OpFunctionEnd
+ OpCapability ClipDistance
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Fragment %2 "main"
+ OpExecutionMode %2 OriginUpperLeft
+ %void = OpTypeVoid
+ %6 = OpTypeFunction %void
+ %2 = OpFunction %void None %6
+ %4194303 = OpLabel
+ OpBranch %18
+ %18 = OpLabel
+ OpLoopMerge %19 %20 None
+ OpBranch %21
+ %21 = OpLabel
+ OpReturn
+ %20 = OpLabel
+ OpBranch %18
+ %19 = OpLabel
+ OpUnreachable
+ OpFunctionEnd
+)";
+
+ SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+ auto result =
+ SinglePassRunToBinary<MergeReturnPass>(text, /* skip_nop = */ true);
+ EXPECT_EQ(Pass::Status::Failure, std::get<1>(result));
+}
+
} // namespace
} // namespace opt
} // namespace spvtools
