summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
author Mathieu Chartier <mathieuc@google.com> 2017-10-27 09:42:46 -0700
committer Mathieu Chartier <mathieuc@google.com> 2017-10-30 16:12:55 -0700
commitaf7c9028905ccc0bb876e07dbe06921a80be9ebd (patch)
tree53d7b80fb299e0a34b9951610740935f6f3ca802
parent460e09c5a5a09937825fe101d260d5c917144369 (diff)
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
-rw-r--r--dexlayout/dex_ir.cc13
-rw-r--r--dexlayout/dexlayout_test.cc77
-rw-r--r--runtime/dex_instruction.cc20
-rw-r--r--runtime/dex_instruction.h9
-rw-r--r--runtime/dex_instruction_iterator.h147
-rw-r--r--runtime/verifier/method_verifier.cc20
6 files changed, 232 insertions, 54 deletions
diff --git a/dexlayout/dex_ir.cc b/dexlayout/dex_ir.cc
index 8c4ee6e9a1..23c3a5ca93 100644
--- a/dexlayout/dex_ir.cc
+++ b/dexlayout/dex_ir.cc
@@ -167,10 +167,17 @@ static bool GetIdsFromByteCode(Collections& collections,
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 f34e7ecd4b..cdd0219ef3 100644
--- a/dexlayout/dexlayout_test.cc
+++ b/dexlayout/dexlayout_test.cc
@@ -317,6 +317,30 @@ class DexLayoutTest : public CommonRuntimeTest {
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 @@ class DexLayoutTest : public CommonRuntimeTest {
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 @@ TEST_F(DexLayoutTest, DuplicateCodeItem) {
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 99fe53bb71..e64c0f62c2 100644
--- a/runtime/dex_instruction.cc
+++ b/runtime/dex_instruction.cc
@@ -119,6 +119,26 @@ size_t Instruction::SizeInCodeUnitsComplexOpcode() const {
}
}
+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 2f28dffa2b..09c78b2428 100644
--- a/runtime/dex_instruction.h
+++ b/runtime/dex_instruction.h
@@ -225,6 +225,12 @@ class Instruction {
}
}
+ // 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 @@ class Instruction {
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 280746e9dc..afa8d4aad3 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,50 +107,62 @@ class DexInstructionIterator : public std::iterator<std::forward_iterator_tag, I
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;
}
- private:
- const value_type* inst_ = nullptr;
-};
-
-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);
-}
+ const value_type& operator*() const {
+ AssertValid();
+ return *inst_;
+ }
-static inline bool operator<(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return lhs.Inst() < rhs.Inst();
-}
+ const value_type* operator->() const {
+ AssertValid();
+ return &**this;
+ }
-static inline bool operator>(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return rhs < lhs;
-}
+ // 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_;
+ }
-static inline bool operator<=(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return !(rhs < lhs);
-}
+ private:
+ ALWAYS_INLINE void AssertValid() const {
+ DCHECK(!IsErrorState());
+ DCHECK_LT(Inst(), end_);
+ }
-static inline bool operator>=(const DexInstructionIterator& lhs,
- const DexInstructionIterator& rhs) {
- return !(lhs < rhs);
-}
+ const value_type* end_ = nullptr;
+ bool error_state_ = false;
+};
} // namespace art
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 0033167160..6555e14358 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -987,9 +987,17 @@ bool MethodVerifier::ComputeWidthsAndCountOps() {
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 @@ bool MethodVerifier::ComputeWidthsAndCountOps() {
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;
}