Fix possible overrun bug for resolving startup strings
Moved the ResolveConstStrings after verification and added logic to
only resolve strings for classes that verify. This fixes a bug
where invalid Dex bytecode could cause dex2oat to crash.
Bug: 128915540
Test: test-art-host
Change-Id: Id2e5e4b10e5afbb8955e805d199754bc255a2f42
diff --git a/dex2oat/dex2oat_test.cc b/dex2oat/dex2oat_test.cc
index d3bfb57..758e69f 100644
--- a/dex2oat/dex2oat_test.cc
+++ b/dex2oat/dex2oat_test.cc
@@ -2152,34 +2152,67 @@
using Hotness = ProfileCompilationInfo::MethodHotness;
// Create a profile with the startup method marked.
ScratchFile profile_file;
+ ScratchFile temp_dex;
+ const std::string& dex_location = temp_dex.GetFilename();
std::vector<uint16_t> methods;
std::vector<dex::TypeIndex> classes;
- std::unique_ptr<const DexFile> dex(OpenTestDexFile("StringLiterals"));
{
- for (ClassAccessor accessor : dex->GetClasses()) {
- if (accessor.GetDescriptor() == std::string("LStringLiterals$StartupClass;")) {
- classes.push_back(accessor.GetClassIdx());
- }
- for (const ClassAccessor::Method& method : accessor.GetMethods()) {
- std::string method_name(dex->GetMethodName(dex->GetMethodId(method.GetIndex())));
- if (method_name == "startUpMethod") {
- methods.push_back(method.GetIndex());
+ MutateDexFile(temp_dex.GetFile(), GetTestDexFileName("StringLiterals"), [&] (DexFile* dex) {
+ bool mutated_successfully = false;
+ // Change the dex instructions to make an opcode that spans past the end of the code item.
+ for (ClassAccessor accessor : dex->GetClasses()) {
+ if (accessor.GetDescriptor() == std::string("LStringLiterals$StartupClass;")) {
+ classes.push_back(accessor.GetClassIdx());
+ }
+ for (const ClassAccessor::Method& method : accessor.GetMethods()) {
+ std::string method_name(dex->GetMethodName(dex->GetMethodId(method.GetIndex())));
+ CodeItemInstructionAccessor instructions = method.GetInstructions();
+ if (method_name == "startUpMethod2") {
+ // Make an instruction that runs past the end of the code item and verify that it
+ // doesn't cause dex2oat to crash.
+ ASSERT_TRUE(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;
+ }
+ ASSERT_EQ(last_instruction->SizeInCodeUnits(), 1u);
+ // 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;
+ // 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") {
+ methods.push_back(method.GetIndex());
+ }
}
}
- }
+ CHECK(mutated_successfully)
+ << "Failed to find candidate code item with only one code unit in last instruction.";
+ });
+ }
+ std::unique_ptr<const DexFile> dex_file(OpenDexFile(temp_dex.GetFilename().c_str()));
+ {
ASSERT_GT(classes.size(), 0u);
ASSERT_GT(methods.size(), 0u);
// Here, we build the profile from the method lists.
ProfileCompilationInfo info;
- info.AddClassesForDex(dex.get(), classes.begin(), classes.end());
- info.AddMethodsForDex(Hotness::kFlagStartup, dex.get(), methods.begin(), methods.end());
+ info.AddClassesForDex(dex_file.get(), classes.begin(), classes.end());
+ info.AddMethodsForDex(Hotness::kFlagStartup, dex_file.get(), methods.begin(), methods.end());
// Save the profile since we want to use it with dex2oat to produce an oat file.
ASSERT_TRUE(info.Save(profile_file.GetFd()));
}
const std::string out_dir = GetScratchDir();
const std::string odex_location = out_dir + "/base.odex";
const std::string app_image_location = out_dir + "/base.art";
- ASSERT_TRUE(GenerateOdexForTest(GetTestDexFileName("StringLiterals"),
+ ASSERT_TRUE(GenerateOdexForTest(dex_location,
odex_location,
CompilerFilter::Filter::kSpeedProfile,
{ "--app-image-file=" + app_image_location,
@@ -2223,7 +2256,7 @@
if (obj->IsDexCache<kVerifyNone>()) {
ObjPtr<mirror::DexCache> dex_cache = obj->AsDexCache();
GcRoot<mirror::String>* preresolved_strings = dex_cache->GetPreResolvedStrings();
- ASSERT_EQ(dex->NumStringIds(), dex_cache->NumPreResolvedStrings());
+ ASSERT_EQ(dex_file->NumStringIds(), dex_cache->NumPreResolvedStrings());
for (size_t i = 0; i < dex_cache->NumPreResolvedStrings(); ++i) {
ObjPtr<mirror::String> string = preresolved_strings[i].Read<kWithoutReadBarrier>();
if (string != nullptr) {
diff --git a/dex2oat/driver/compiler_driver.cc b/dex2oat/driver/compiler_driver.cc
index 520b455..bcd573b 100644
--- a/dex2oat/driver/compiler_driver.cc
+++ b/dex2oat/driver/compiler_driver.cc
@@ -690,6 +690,12 @@
profile_compilation_info != nullptr &&
profile_compilation_info->ContainsClass(*dex_file, accessor.GetClassIdx());
+ // Skip methods that failed to verify since they may contain invalid Dex code.
+ if (GetClassStatus(ClassReference(dex_file, accessor.GetClassDefIndex())) <
+ ClassStatus::kRetryVerificationAtRuntime) {
+ continue;
+ }
+
for (const ClassAccessor::Method& method : accessor.GetMethods()) {
const bool is_clinit = (method.GetAccessFlags() & kAccConstructor) != 0 &&
(method.GetAccessFlags() & kAccStatic) != 0;
@@ -873,6 +879,9 @@
return;
}
+ Verify(class_loader, dex_files, timings, verification_results);
+ VLOG(compiler) << "Verify: " << GetMemoryUsageString(false);
+
if (GetCompilerOptions().IsForceDeterminism() && GetCompilerOptions().IsBootImage()) {
// Resolve strings from const-string. Do this now to have a deterministic image.
ResolveConstStrings(dex_files, /*only_startup_strings=*/ false, timings);
@@ -881,9 +890,6 @@
ResolveConstStrings(dex_files, /*only_startup_strings=*/ true, timings);
}
- Verify(class_loader, dex_files, timings, verification_results);
- VLOG(compiler) << "Verify: " << GetMemoryUsageString(false);
-
if (had_hard_verifier_failure_ && GetCompilerOptions().AbortOnHardVerifierFailure()) {
// Avoid dumping threads. Even if we shut down the thread pools, there will still be three
// instances of this thread's stack.
diff --git a/libartbase/base/common_art_test.cc b/libartbase/base/common_art_test.cc
index ed74947..8e3e73f 100644
--- a/libartbase/base/common_art_test.cc
+++ b/libartbase/base/common_art_test.cc
@@ -441,15 +441,19 @@
return dex_files;
}
+std::unique_ptr<const DexFile> CommonArtTestImpl::OpenDexFile(const char* filename) {
+ std::vector<std::unique_ptr<const DexFile>> dex_files(OpenDexFiles(filename));
+ CHECK_EQ(dex_files.size(), 1u) << "Expected only one dex file";
+ return std::move(dex_files[0]);
+}
+
std::vector<std::unique_ptr<const DexFile>> CommonArtTestImpl::OpenTestDexFiles(
const char* name) {
return OpenDexFiles(GetTestDexFileName(name).c_str());
}
std::unique_ptr<const DexFile> CommonArtTestImpl::OpenTestDexFile(const char* name) {
- std::vector<std::unique_ptr<const DexFile>> vector = OpenTestDexFiles(name);
- EXPECT_EQ(1U, vector.size());
- return std::move(vector[0]);
+ return OpenDexFile(GetTestDexFileName(name).c_str());
}
std::string CommonArtTestImpl::GetCoreFileLocation(const char* suffix) {
diff --git a/libartbase/base/common_art_test.h b/libartbase/base/common_art_test.h
index f8cab92..1748a9b 100644
--- a/libartbase/base/common_art_test.h
+++ b/libartbase/base/common_art_test.h
@@ -185,6 +185,10 @@
// Open a file (allows reading of framework jars).
std::vector<std::unique_ptr<const DexFile>> OpenDexFiles(const char* filename);
+
+ // Open a single dex file (aborts if there are more than one).
+ std::unique_ptr<const DexFile> OpenDexFile(const char* filename);
+
// Open a test file (art-gtest-*.jar).
std::vector<std::unique_ptr<const DexFile>> OpenTestDexFiles(const char* name);
diff --git a/test/StringLiterals/StringLiterals.java b/test/StringLiterals/StringLiterals.java
index 9ab37ca..c2b518a 100644
--- a/test/StringLiterals/StringLiterals.java
+++ b/test/StringLiterals/StringLiterals.java
@@ -33,6 +33,13 @@
System.out.println("Loading " + resource);
}
+ static class InnerClass {
+ void startUpMethod2() {
+ String resource = "ab11.apk";
+ System.out.println("Start up method 2");
+ }
+ }
+
void otherMethod() {
System.out.println("Unexpected error");
System.out.println("Shutting down!");