Fix DexInstructionIterator overrun bug
Handle cases where the dex instructions can go past the end of the
code item in dexlayout.
Since dexlayout runs before method verification, we need to be
careful to not go past the end of the code item.
Added test.
Bug: 67104794
Test: test-art-host
Change-Id: Idf7d51344659b2c75207bdf444e39f271feb8d3a
diff --git a/dexlayout/dex_ir.cc b/dexlayout/dex_ir.cc
index 8c4ee6e..23c3a5c 100644
--- a/dexlayout/dex_ir.cc
+++ b/dexlayout/dex_ir.cc
@@ -167,10 +167,17 @@
std::vector<MethodId*>* method_ids,
std::vector<FieldId*>* field_ids) {
bool has_id = false;
- for (const Instruction& instruction : code->Instructions()) {
- CHECK_GT(instruction.SizeInCodeUnits(), 0u);
+ IterationRange<DexInstructionIterator> instructions = code->Instructions();
+ SafeDexInstructionIterator it(instructions.begin(), instructions.end());
+ for (; !it.IsErrorState() && it < instructions.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() || next > instructions.end()) {
+ break;
+ }
has_id |= GetIdFromInstruction(collections,
- &instruction,
+ it.Inst(),
type_ids,
string_ids,
method_ids,
diff --git a/dexlayout/dexlayout_test.cc b/dexlayout/dexlayout_test.cc
index f34e7ec..cdd0219 100644
--- a/dexlayout/dexlayout_test.cc
+++ b/dexlayout/dexlayout_test.cc
@@ -317,6 +317,30 @@
return true;
}
+ template <typename Mutator>
+ bool MutateDexFile(File* output_dex, const std::string& input_jar, const Mutator& mutator) {
+ std::vector<std::unique_ptr<const DexFile>> dex_files;
+ std::string error_msg;
+ CHECK(DexFileLoader::Open(input_jar.c_str(),
+ input_jar.c_str(),
+ /*verify*/ true,
+ /*verify_checksum*/ true,
+ &error_msg,
+ &dex_files)) << error_msg;
+ EXPECT_EQ(dex_files.size(), 1u) << "Only one input dex is supported";
+ for (const std::unique_ptr<const DexFile>& dex : dex_files) {
+ CHECK(dex->EnableWrite()) << "Failed to enable write";
+ mutator(const_cast<DexFile*>(dex.get()));
+ if (!output_dex->WriteFully(dex->Begin(), dex->Size())) {
+ return false;
+ }
+ }
+ if (output_dex->Flush() != 0) {
+ PLOG(FATAL) << "Could not flush the output file.";
+ }
+ return true;
+ }
+
// Create a profile with some subset of methods and classes.
void CreateProfile(const std::string& input_dex,
const std::string& out_profile,
@@ -518,8 +542,10 @@
const char* dex_filename,
ScratchFile* profile_file,
std::vector<std::string>& dexlayout_exec_argv) {
- WriteBase64ToFile(dex_filename, dex_file->GetFile());
- EXPECT_EQ(dex_file->GetFile()->Flush(), 0);
+ if (dex_filename != nullptr) {
+ WriteBase64ToFile(dex_filename, dex_file->GetFile());
+ EXPECT_EQ(dex_file->GetFile()->Flush(), 0);
+ }
if (profile_file != nullptr) {
CreateProfile(dex_file->GetFilename(), profile_file->GetFilename(), dex_file->GetFilename());
}
@@ -673,4 +699,51 @@
dexlayout_exec_argv));
}
+// Test that instructions that go past the end of the code items don't cause crashes.
+TEST_F(DexLayoutTest, CodeItemOverrun) {
+ ScratchFile temp_dex;
+ MutateDexFile(temp_dex.GetFile(), GetTestDexFileName("ManyMethods"), [] (DexFile* dex) {
+ bool mutated_successfully = false;
+ // Change the dex instructions to make an opcode that spans past the end of the code item.
+ for (size_t i = 0; i < dex->NumClassDefs(); ++i) {
+ const DexFile::ClassDef& def = dex->GetClassDef(i);
+ const uint8_t* data = dex->GetClassData(def);
+ if (data == nullptr) {
+ continue;
+ }
+ ClassDataItemIterator it(*dex, data);
+ it.SkipAllFields();
+ while (it.HasNextDirectMethod() || it.HasNextVirtualMethod()) {
+ DexFile::CodeItem* item = const_cast<DexFile::CodeItem*>(it.GetMethodCodeItem());
+ if (item != nullptr) {
+ IterationRange<DexInstructionIterator> instructions = item->Instructions();
+ if (instructions.begin() != instructions.end()) {
+ DexInstructionIterator last_instruction = instructions.begin();
+ for (auto dex_it = instructions.begin(); dex_it != instructions.end(); ++dex_it) {
+ last_instruction = dex_it;
+ }
+ if (last_instruction->SizeInCodeUnits() == 1) {
+ // Set the opcode to something that will go past the end of the code item.
+ const_cast<Instruction*>(last_instruction.Inst())->SetOpcode(
+ Instruction::CONST_STRING_JUMBO);
+ mutated_successfully = true;
+ }
+ }
+ }
+ it.Next();
+ }
+ }
+ CHECK(mutated_successfully)
+ << "Failed to find candidate code item with only one code unit in last instruction.";
+ });
+ std::string dexlayout = GetTestAndroidRoot() + "/bin/dexlayout";
+ EXPECT_TRUE(OS::FileExists(dexlayout.c_str())) << dexlayout << " should be a valid file path";
+ std::vector<std::string> dexlayout_exec_argv =
+ { dexlayout, "-i", "-o", "/dev/null", temp_dex.GetFilename() };
+ ASSERT_TRUE(DexLayoutExec(&temp_dex,
+ /*dex_filename*/ nullptr,
+ nullptr /* profile_file */,
+ dexlayout_exec_argv));
+}
+
} // namespace art
diff --git a/runtime/dex_instruction.cc b/runtime/dex_instruction.cc
index 99fe53b..e64c0f6 100644
--- a/runtime/dex_instruction.cc
+++ b/runtime/dex_instruction.cc
@@ -119,6 +119,26 @@
}
}
+size_t Instruction::CodeUnitsRequiredForSizeOfComplexOpcode() const {
+ const uint16_t* insns = reinterpret_cast<const uint16_t*>(this);
+ // Handle special NOP encoded variable length sequences.
+ switch (*insns) {
+ case kPackedSwitchSignature:
+ FALLTHROUGH_INTENDED;
+ case kSparseSwitchSignature:
+ return 2;
+ case kArrayDataSignature:
+ return 4;
+ default:
+ if ((*insns & 0xFF) == 0) {
+ return 1; // NOP.
+ } else {
+ LOG(FATAL) << "Unreachable: " << DumpString(nullptr);
+ UNREACHABLE();
+ }
+ }
+}
+
std::string Instruction::DumpHex(size_t code_units) const {
size_t inst_length = SizeInCodeUnits();
if (inst_length > code_units) {
diff --git a/runtime/dex_instruction.h b/runtime/dex_instruction.h
index 2f28dff..09c78b2 100644
--- a/runtime/dex_instruction.h
+++ b/runtime/dex_instruction.h
@@ -225,6 +225,12 @@
}
}
+ // Code units required to calculate the size of the instruction.
+ size_t CodeUnitsRequiredForSizeComputation() const {
+ const int8_t result = kInstructionDescriptors[Opcode()].size_in_code_units;
+ return UNLIKELY(result < 0) ? CodeUnitsRequiredForSizeOfComplexOpcode() : 1;
+ }
+
// Reads an instruction out of the stream at the specified address.
static const Instruction* At(const uint16_t* code) {
DCHECK(code != nullptr);
@@ -638,6 +644,9 @@
private:
size_t SizeInCodeUnitsComplexOpcode() const;
+ // Return how many code unit words are required to compute the size of the opcode.
+ size_t CodeUnitsRequiredForSizeOfComplexOpcode() const;
+
uint32_t Fetch32(size_t offset) const {
return (Fetch16(offset) | ((uint32_t) Fetch16(offset + 1) << 16));
}
diff --git a/runtime/dex_instruction_iterator.h b/runtime/dex_instruction_iterator.h
index 280746e..afa8d4a 100644
--- a/runtime/dex_instruction_iterator.h
+++ b/runtime/dex_instruction_iterator.h
@@ -24,19 +24,68 @@
namespace art {
-class DexInstructionIterator : public std::iterator<std::forward_iterator_tag, Instruction> {
+// Base helper class to prevent duplicated comparators.
+class DexInstructionIteratorBase : public std::iterator<std::forward_iterator_tag, Instruction> {
public:
using value_type = std::iterator<std::forward_iterator_tag, Instruction>::value_type;
using difference_type = std::iterator<std::forward_iterator_tag, value_type>::difference_type;
- DexInstructionIterator() = default;
- DexInstructionIterator(const DexInstructionIterator&) = default;
- DexInstructionIterator(DexInstructionIterator&&) = default;
- DexInstructionIterator& operator=(const DexInstructionIterator&) = default;
- DexInstructionIterator& operator=(DexInstructionIterator&&) = default;
+ DexInstructionIteratorBase() = default;
+ explicit DexInstructionIteratorBase(const value_type* inst) : inst_(inst) {}
- explicit DexInstructionIterator(const value_type* inst) : inst_(inst) {}
- explicit DexInstructionIterator(const uint16_t* inst) : inst_(value_type::At(inst)) {}
+ const value_type* Inst() const {
+ return inst_;
+ }
+
+ // Return the dex pc for an iterator compared to the code item begin.
+ uint32_t GetDexPC(const DexInstructionIteratorBase& code_item_begin) {
+ return reinterpret_cast<const uint16_t*>(inst_) -
+ reinterpret_cast<const uint16_t*>(code_item_begin.inst_);
+ }
+
+ protected:
+ const value_type* inst_ = nullptr;
+};
+
+
+static ALWAYS_INLINE inline bool operator==(const DexInstructionIteratorBase& lhs,
+ const DexInstructionIteratorBase& rhs) {
+ return lhs.Inst() == rhs.Inst();
+}
+
+static inline bool operator!=(const DexInstructionIteratorBase& lhs,
+ const DexInstructionIteratorBase& rhs) {
+ return !(lhs == rhs);
+}
+
+static inline bool operator<(const DexInstructionIteratorBase& lhs,
+ const DexInstructionIteratorBase& rhs) {
+ return lhs.Inst() < rhs.Inst();
+}
+
+static inline bool operator>(const DexInstructionIteratorBase& lhs,
+ const DexInstructionIteratorBase& rhs) {
+ return rhs < lhs;
+}
+
+static inline bool operator<=(const DexInstructionIteratorBase& lhs,
+ const DexInstructionIteratorBase& rhs) {
+ return !(rhs < lhs);
+}
+
+static inline bool operator>=(const DexInstructionIteratorBase& lhs,
+ const DexInstructionIteratorBase& rhs) {
+ return !(lhs < rhs);
+}
+
+class DexInstructionIterator : public DexInstructionIteratorBase {
+ public:
+ using value_type = std::iterator<std::forward_iterator_tag, Instruction>::value_type;
+ using difference_type = std::iterator<std::forward_iterator_tag, value_type>::difference_type;
+ using DexInstructionIteratorBase::DexInstructionIteratorBase;
+
+ explicit DexInstructionIterator(const uint16_t* inst)
+ : DexInstructionIteratorBase(value_type::At(inst)) {}
// Value after modification.
DexInstructionIterator& operator++() {
@@ -58,51 +107,63 @@
const value_type* operator->() const {
return &**this;
}
+};
- // Return the dex pc for an iterator compared to the code item begin.
- uint32_t GetDexPC(const DexInstructionIterator& code_item_begin) {
- return reinterpret_cast<const uint16_t*>(inst_) -
- reinterpret_cast<const uint16_t*>(code_item_begin.inst_);
+class SafeDexInstructionIterator : public DexInstructionIteratorBase {
+ public:
+ explicit SafeDexInstructionIterator(const DexInstructionIteratorBase& start,
+ const DexInstructionIteratorBase& end)
+ : DexInstructionIteratorBase(start.Inst())
+ , end_(end.Inst()) {}
+
+ // 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 = reinterpret_cast<const uint16_t*>(end_) -
+ reinterpret_cast<const uint16_t*>(Inst());
+ if (size_code_units > available) {
+ error_state_ = true;
+ } else {
+ inst_ = inst_->Next();
+ }
+ return *this;
}
- const value_type* Inst() const {
- return inst_;
+ // Value before modification.
+ SafeDexInstructionIterator operator++(int) {
+ SafeDexInstructionIterator temp = *this;
+ ++*this;
+ return temp;
+ }
+
+ const value_type& operator*() const {
+ AssertValid();
+ return *inst_;
+ }
+
+ const value_type* operator->() const {
+ AssertValid();
+ return &**this;
+ }
+
+ // 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:
- const value_type* inst_ = nullptr;
+ ALWAYS_INLINE void AssertValid() const {
+ DCHECK(!IsErrorState());
+ DCHECK_LT(Inst(), end_);
+ }
+
+ const value_type* end_ = nullptr;
+ bool error_state_ = false;
};
-static ALWAYS_INLINE inline bool operator==(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return lhs.Inst() == rhs.Inst();
-}
-
-static inline bool operator!=(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return !(lhs == rhs);
-}
-
-static inline bool operator<(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return lhs.Inst() < rhs.Inst();
-}
-
-static inline bool operator>(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return rhs < lhs;
-}
-
-static inline bool operator<=(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return !(rhs < lhs);
-}
-
-static inline bool operator>=(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return !(lhs < rhs);
-}
-
} // namespace art
#endif // ART_RUNTIME_DEX_INSTRUCTION_ITERATOR_H_
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 0033167..6555e14 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -987,9 +987,17 @@
size_t monitor_enter_count = 0;
IterationRange<DexInstructionIterator> instructions = code_item_->Instructions();
- DexInstructionIterator inst = instructions.begin();
- for ( ; inst < instructions.end(); ++inst) {
- Instruction::Code opcode = inst->Opcode();
+ // 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(instructions.begin(), instructions.end());
+ for ( ; !it.IsErrorState() && it < instructions.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() || next > instructions.end()) {
+ break;
+ }
+ Instruction::Code opcode = it->Opcode();
switch (opcode) {
case Instruction::APUT_OBJECT:
case Instruction::CHECK_CAST:
@@ -1010,13 +1018,13 @@
default:
break;
}
- GetInstructionFlags(inst.GetDexPC(instructions.begin())).SetIsOpcode();
+ GetInstructionFlags(it.GetDexPC(instructions.begin())).SetIsOpcode();
}
- if (inst != instructions.end()) {
+ if (it != instructions.end()) {
const size_t insns_size = code_item_->insns_size_in_code_units_;
Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "code did not end where expected ("
- << inst.GetDexPC(instructions.begin()) << " vs. "
+ << it.GetDexPC(instructions.begin()) << " vs. "
<< insns_size << ")";
return false;
}