diff options
-rw-r--r-- | dex2oat/dex2oat_test.cc | 9 | ||||
-rw-r--r-- | libdexfile/dex/dex_instruction.cc | 7 | ||||
-rw-r--r-- | libdexfile/dex/dex_instruction_iterator.h | 77 | ||||
-rw-r--r-- | runtime/verifier/method_verifier.cc | 84 |
4 files changed, 73 insertions, 104 deletions
diff --git a/dex2oat/dex2oat_test.cc b/dex2oat/dex2oat_test.cc index f81345b345..7013bbce0c 100644 --- a/dex2oat/dex2oat_test.cc +++ b/dex2oat/dex2oat_test.cc @@ -1816,13 +1816,6 @@ TEST_F(Dex2oatTest, AppImageResolveStrings) { const_cast<Instruction&>(last_instruction.Inst()) .SetOpcode(Instruction::CONST_STRING_JUMBO); mutated_successfully = true; - // Test that the safe iterator doesn't go past the end. - SafeDexInstructionIterator it2(instructions.begin(), instructions.end()); - while (!it2.IsErrorState()) { - ++it2; - } - EXPECT_TRUE(it2 == last_instruction); - EXPECT_TRUE(it2 < instructions.end()); methods.push_back(method.GetIndex()); mutated_successfully = true; } else if (method_name == "startUpMethod") { @@ -1890,8 +1883,6 @@ TEST_F(Dex2oatTest, AppImageResolveStrings) { // Classes initializers EXPECT_TRUE(seen.find("Startup init") != seen.end()); EXPECT_TRUE(seen.find("Other class init") == seen.end()); - // Expect the sets match. - EXPECT_GE(seen.size(), seen.size()); // Verify what strings are marked as boot image. std::set<std::string> boot_image_strings; diff --git a/libdexfile/dex/dex_instruction.cc b/libdexfile/dex/dex_instruction.cc index 67147bba11..c4db658ae1 100644 --- a/libdexfile/dex/dex_instruction.cc +++ b/libdexfile/dex/dex_instruction.cc @@ -74,7 +74,12 @@ size_t Instruction::SizeInCodeUnitsComplexOpcode() const { uint16_t element_size = insns[1]; uint32_t length = insns[2] | (((uint32_t)insns[3]) << 16); // The plus 1 is to round up for odd size and width. - return (4 + (element_size * length + 1) / 2); + uint32_t result = (4 + (element_size * length + 1) / 2); + // This function is used only after the `MethodVerifier` checked that the 32-bit calculation + // does not overflow. Let's `DCHECK()` the result against a 64-bit calculation. + DCHECK_EQ(result, + 4 + (static_cast<uint64_t>(element_size) * static_cast<uint64_t>(length) + 1) / 2); + return result; } default: if ((*insns & 0xFF) == 0) { diff --git a/libdexfile/dex/dex_instruction_iterator.h b/libdexfile/dex/dex_instruction_iterator.h index da494e1f08..9f4e39030c 100644 --- a/libdexfile/dex/dex_instruction_iterator.h +++ b/libdexfile/dex/dex_instruction_iterator.h @@ -156,83 +156,6 @@ class DexInstructionIterator : public DexInstructionIteratorBase { } }; -// A safe version of DexInstructionIterator that is guaranteed to not go past the end of the code -// item. -class SafeDexInstructionIterator : public DexInstructionIteratorBase { - public: - explicit SafeDexInstructionIterator(const DexInstructionIteratorBase& start, - const DexInstructionIteratorBase& end) - : DexInstructionIteratorBase(&start.Inst(), start.DexPc()) - , num_code_units_(end.DexPc()) { - DCHECK_EQ(start.Instructions(), end.Instructions()) - << "start and end must be in the same code item."; - } - - // Value after modification, does not read past the end of the allowed region. May increment past - // the end of the code item though. - SafeDexInstructionIterator& operator++() { - AssertValid(); - const size_t size_code_units = Inst().CodeUnitsRequiredForSizeComputation(); - const size_t available = NumCodeUnits() - DexPc(); - if (UNLIKELY(size_code_units > available)) { - error_state_ = true; - return *this; - } - const size_t instruction_code_units = Inst().SizeInCodeUnits(); - if (UNLIKELY(instruction_code_units > available)) { - error_state_ = true; - return *this; - } - data_.dex_pc_ += instruction_code_units; - return *this; - } - - // Value before modification. - SafeDexInstructionIterator operator++(int) { - SafeDexInstructionIterator temp = *this; - ++*this; - return temp; - } - - const value_type& operator*() const { - AssertValid(); - return data_; - } - - const Instruction* operator->() const { - AssertValid(); - return &data_.Inst(); - } - - // Return the current instruction of the iterator. - ALWAYS_INLINE const Instruction& Inst() const { - return data_.Inst(); - } - - const uint16_t* Instructions() const { - return data_.Instructions(); - } - - // Returns true if the iterator is in an error state. This occurs when an instruction couldn't - // have its size computed without reading past the end iterator. - bool IsErrorState() const { - return error_state_; - } - - private: - ALWAYS_INLINE void AssertValid() const { - DCHECK(!IsErrorState()); - DCHECK_LT(DexPc(), NumCodeUnits()); - } - - ALWAYS_INLINE uint32_t NumCodeUnits() const { - return num_code_units_; - } - - const uint32_t num_code_units_ = 0; - bool error_state_ = false; -}; - } // namespace art #endif // ART_LIBDEXFILE_DEX_DEX_INSTRUCTION_ITERATOR_H_ diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc index 68c0268720..42f694c62f 100644 --- a/runtime/verifier/method_verifier.cc +++ b/runtime/verifier/method_verifier.cc @@ -1619,29 +1619,79 @@ template <bool kVerifierDebug> bool MethodVerifier<kVerifierDebug>::ComputeWidthsAndCountOps() { // We can't assume the instruction is well formed, handle the case where calculating the size // goes past the end of the code item. - SafeDexInstructionIterator it(code_item_accessor_.begin(), code_item_accessor_.end()); - if (it == code_item_accessor_.end()) { + const uint32_t insns_size = code_item_accessor_.InsnsSizeInCodeUnits(); + if (insns_size == 0u) { Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "code item has no opcode"; return false; } - for ( ; !it.IsErrorState() && it < code_item_accessor_.end(); ++it) { - // In case the instruction goes past the end of the code item, make sure to not process it. - SafeDexInstructionIterator next = it; - ++next; - if (next.IsErrorState()) { - break; + const Instruction* inst = &code_item_accessor_.InstructionAt(0u); + uint32_t dex_pc = 0u; + while (dex_pc != insns_size) { + const uint32_t remaining_code_units = insns_size - dex_pc; + const uint16_t inst_data = inst->Fetch16(0); + const Instruction::Code opcode = inst->Opcode(inst_data); + uint32_t instruction_size = 0u; + bool ok; + if (opcode == Instruction::NOP) { + auto check_switch = [&](uint32_t base_size, uint32_t entry_size) ALWAYS_INLINE { + if (UNLIKELY(base_size > remaining_code_units)) { + return false; + } + // This 32-bit calculation cannot overflow because `num_entries` starts as 16-bit. + uint32_t num_entries = inst->Fetch16(1); + instruction_size = base_size + num_entries * entry_size; + if (UNLIKELY(instruction_size > remaining_code_units)) { + return false; + } + return true; + }; + switch (inst_data) { + case Instruction::kPackedSwitchSignature: + ok = check_switch(4u, 2u); + break; + case Instruction::kSparseSwitchSignature: + ok = check_switch(2u, 4u); + break; + case Instruction::kArrayDataSignature: + if (UNLIKELY(remaining_code_units < 4u)) { + ok = false; + } else { + uint16_t element_size = inst->Fetch16(1); + uint32_t length = inst->Fetch16(2) | (((uint32_t)inst->Fetch16(3)) << 16); + // Use 64-bit calculation to avoid arithmetic overflow. + uint64_t bytes = static_cast<uint64_t>(element_size) * static_cast<uint64_t>(length); + uint64_t code_units = UINT64_C(4) + (bytes + /* round up */ UINT64_C(1)) / UINT64_C(2); + if (UNLIKELY(code_units > remaining_code_units)) { + ok = false; + } else { + instruction_size = dchecked_integral_cast<uint32_t>(code_units); + ok = true; + } + } + break; + default: + instruction_size = 1u; + ok = true; + break; + } + } else { + instruction_size = Instruction::SizeInCodeUnits(Instruction::FormatOf(opcode)); + DCHECK_EQ(instruction_size, inst->SizeInCodeUnits()); + ok = LIKELY(instruction_size <= remaining_code_units); } - GetModifiableInstructionFlags(it.DexPc()).SetIsOpcode(); - } - - if (it != code_item_accessor_.end()) { - const size_t insns_size = code_item_accessor_.InsnsSizeInCodeUnits(); - Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "code did not end where expected (" - << it.DexPc() << " vs. " << insns_size << ")"; - return false; + if (!ok) { + Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "code did not end where expected (" + << dex_pc << " vs. " << insns_size << ")"; + return false; + } + GetModifiableInstructionFlags(dex_pc).SetIsOpcode(); + DCHECK_NE(instruction_size, 0u); + DCHECK_EQ(instruction_size, inst->SizeInCodeUnits()); + DCHECK_LE(instruction_size, remaining_code_units); + dex_pc += instruction_size; + inst = inst->RelativeAt(instruction_size); } DCHECK(GetInstructionFlags(0).IsOpcode()); - return true; } |