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

5 files changed, 74 insertions, 20 deletions

diff --git a/dex2oat/dex2oat_test.cc b/dex2oat/dex2oat_test.cc
index d3bfb57a0b..758e69f8e8 100644
--- a/dex2oat/dex2oat_test.cc
+++ b/dex2oat/dex2oat_test.cc
@@ -2152,34 +2152,67 @@ TEST_F(Dex2oatTest, AppImageResolveStrings) {
   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 @@ TEST_F(Dex2oatTest, AppImageResolveStrings) {
       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 520b455a7a..bcd573bf64 100644
--- a/dex2oat/driver/compiler_driver.cc
+++ b/dex2oat/driver/compiler_driver.cc
@@ -690,6 +690,12 @@ void CompilerDriver::ResolveConstStrings(const std::vector<const DexFile*>& dex_
           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 @@ void CompilerDriver::PreCompile(jobject class_loader,
     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 @@ void CompilerDriver::PreCompile(jobject class_loader,
     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 ed74947347..8e3e73f097 100644
--- a/libartbase/base/common_art_test.cc
+++ b/libartbase/base/common_art_test.cc
@@ -441,15 +441,19 @@ std::vector<std::unique_ptr<const DexFile>> CommonArtTestImpl::OpenDexFiles(cons
   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 f8cab92ffe..1748a9bb0b 100644
--- a/libartbase/base/common_art_test.h
+++ b/libartbase/base/common_art_test.h
@@ -185,6 +185,10 @@ class CommonArtTestImpl {
 
   // 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 9ab37ca3de..c2b518a435 100644
--- a/test/StringLiterals/StringLiterals.java
+++ b/test/StringLiterals/StringLiterals.java
@@ -33,6 +33,13 @@ class StringLiterals {
         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!");