Ensure when NotifyMethodRedefined is called the ArtMethod is valid

Previously we were calling Jit::JitCodeCache::NotifyMethodRedefined
with an ArtMethod that was not fully valid. This could cause OOB
memory accesses if NotifyMethodRedefined attempts to access the
incorrect dex-file associated with the method at that time. This
occurs if the method is a native method. By looking at the wrong dex
file the JIT will get an incorrect MethodID and Shorty meaning it is
unable to correctly update the jit-code-cache.

Test: ./test.py --host -j50
Test: Run WIP dexmaker tests that hit this issue.
Bug: 74116990
Change-Id: Ied035b01b07d595df4037352b4bd20b42d285cb9
(cherry picked from commit 035105ff976680f11fa4fb12f1d42e2b7e250503)

7 files changed, 319 insertions, 7 deletions

diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index 46734548a8..5d430d2073 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -1402,13 +1402,6 @@ void Redefiner::ClassRedefinition::UpdateMethods(art::ObjPtr<art::mirror::Class>
     method.SetCodeItemOffset(dex_file_->FindCodeItemOffset(class_def, dex_method_idx));
     // Clear all the intrinsics related flags.
     method.SetNotIntrinsic();
-    // Notify the jit that this method is redefined.
-    art::jit::Jit* jit = driver_->runtime_->GetJit();
-    // Non-invokable methods don't have any JIT data associated with them so we don't need to tell
-    // the jit about them.
-    if (jit != nullptr && method.IsInvokable()) {
-      jit->GetCodeCache()->NotifyMethodRedefined(&method);
-    }
   }
 }
 
@@ -1450,6 +1443,23 @@ void Redefiner::ClassRedefinition::UpdateClass(
   art::ObjPtr<art::mirror::ClassExt> ext(mclass->GetExtData());
   CHECK(!ext.IsNull());
   ext->SetOriginalDexFile(original_dex_file);
+
+  // Notify the jit that all the methods in this class were redefined. Need to do this last since
+  // the jit relies on the dex_file_ being correct (for native methods at least) to find the method
+  // meta-data.
+  art::jit::Jit* jit = driver_->runtime_->GetJit();
+  if (jit != nullptr) {
+    art::PointerSize image_pointer_size =
+        driver_->runtime_->GetClassLinker()->GetImagePointerSize();
+    auto code_cache = jit->GetCodeCache();
+    // Non-invokable methods don't have any JIT data associated with them so we don't need to tell
+    // the jit about them.
+    for (art::ArtMethod& method : mclass->GetDeclaredMethods(image_pointer_size)) {
+      if (method.IsInvokable()) {
+        code_cache->NotifyMethodRedefined(&method);
+      }
+    }
+  }
 }
 
 // Restores the old obsolete methods maps if it turns out they weren't needed (ie there were no new
diff --git a/test/1949-short-dex-file/expected.txt b/test/1949-short-dex-file/expected.txt
new file mode 100644
index 0000000000..863339fb8c
--- /dev/null
+++ b/test/1949-short-dex-file/expected.txt
@@ -0,0 +1 @@
+Passed
diff --git a/test/1949-short-dex-file/info.txt b/test/1949-short-dex-file/info.txt
new file mode 100644
index 0000000000..e924086e23
--- /dev/null
+++ b/test/1949-short-dex-file/info.txt
@@ -0,0 +1,30 @@
+Tests the fix for b/74116990
+
+The JIT was reading into incorrect dex files during class redefinition if a
+native method was present.
+
+The transformed dex file is specifically crafted to have exactly 4 methodIDs in
+it. They are (in order):
+  (0) Ljava/lang/Object;-><init>()V
+  (1) Lxyz/Transform;-><init>()V
+  (2) Lxyz/Transform;->bar()V
+  (3) Lxyz/Transform;->foo()V
+
+In the transformed version of the dex file there is a new method. The new list of methodIDs is:
+  (0) Lart/Test1949;->doNothing()V
+  (1) Ljava/lang/Object;-><init>()V
+  (2) Lxyz/Transform;-><init>()V
+  (3) Lxyz/Transform;->bar()V
+  (4) Lxyz/Transform;->foo()V
+
+This test tries to get the JIT to read out-of-bounds on the initial dex file by getting it to
+read the 5th method id of the new file (Lxyz/Transform;->foo()V) from the old dex file (which
+only has 4 method ids).
+
+To do this we need to make sure that the class being transformed is near the end of the
+alphabet (package xyz, method foo). If it is further forward than the other method-ids then the
+JIT will read an incorrect (but valid) method-id from the old-dex file. This is why the error
+wasn't caught in our other tests (package art is always at the front).
+
+The final method that causes the OOB read needs to be a native method because that is the only
+method-type the jit uses dex-file information to keep track of.
diff --git a/test/1949-short-dex-file/run b/test/1949-short-dex-file/run
new file mode 100755
index 0000000000..c6e62ae6cd
--- /dev/null
+++ b/test/1949-short-dex-file/run
@@ -0,0 +1,17 @@
+#!/bin/bash
+#
+# Copyright 2016 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+./default-run "$@" --jvmti
diff --git a/test/1949-short-dex-file/src/Main.java b/test/1949-short-dex-file/src/Main.java
new file mode 100644
index 0000000000..dbbaa861c9
--- /dev/null
+++ b/test/1949-short-dex-file/src/Main.java
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+  public static void main(String[] args) throws Exception {
+    art.Test1949.run();
+  }
+}
diff --git a/test/1949-short-dex-file/src/art/Redefinition.java b/test/1949-short-dex-file/src/art/Redefinition.java
new file mode 100644
index 0000000000..56d2938a01
--- /dev/null
+++ b/test/1949-short-dex-file/src/art/Redefinition.java
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package art;
+
+import java.util.ArrayList;
+// Common Redefinition functions. Placed here for use by CTS
+public class Redefinition {
+  public static final class CommonClassDefinition {
+    public final Class<?> target;
+    public final byte[] class_file_bytes;
+    public final byte[] dex_file_bytes;
+
+    public CommonClassDefinition(Class<?> target, byte[] class_file_bytes, byte[] dex_file_bytes) {
+      this.target = target;
+      this.class_file_bytes = class_file_bytes;
+      this.dex_file_bytes = dex_file_bytes;
+    }
+  }
+
+  // A set of possible test configurations. Test should set this if they need to.
+  // This must be kept in sync with the defines in ti-agent/common_helper.cc
+  public static enum Config {
+    COMMON_REDEFINE(0),
+    COMMON_RETRANSFORM(1),
+    COMMON_TRANSFORM(2);
+
+    private final int val;
+    private Config(int val) {
+      this.val = val;
+    }
+  }
+
+  public static void setTestConfiguration(Config type) {
+    nativeSetTestConfiguration(type.val);
+  }
+
+  private static native void nativeSetTestConfiguration(int type);
+
+  // Transforms the class
+  public static native void doCommonClassRedefinition(Class<?> target,
+                                                      byte[] classfile,
+                                                      byte[] dexfile);
+
+  public static void doMultiClassRedefinition(CommonClassDefinition... defs) {
+    ArrayList<Class<?>> classes = new ArrayList<>();
+    ArrayList<byte[]> class_files = new ArrayList<>();
+    ArrayList<byte[]> dex_files = new ArrayList<>();
+
+    for (CommonClassDefinition d : defs) {
+      classes.add(d.target);
+      class_files.add(d.class_file_bytes);
+      dex_files.add(d.dex_file_bytes);
+    }
+    doCommonMultiClassRedefinition(classes.toArray(new Class<?>[0]),
+                                   class_files.toArray(new byte[0][]),
+                                   dex_files.toArray(new byte[0][]));
+  }
+
+  public static void addMultiTransformationResults(CommonClassDefinition... defs) {
+    for (CommonClassDefinition d : defs) {
+      addCommonTransformationResult(d.target.getCanonicalName(),
+                                    d.class_file_bytes,
+                                    d.dex_file_bytes);
+    }
+  }
+
+  public static native void doCommonMultiClassRedefinition(Class<?>[] targets,
+                                                           byte[][] classfiles,
+                                                           byte[][] dexfiles);
+  public static native void doCommonClassRetransformation(Class<?>... target);
+  public static native void setPopRetransformations(boolean pop);
+  public static native void popTransformationFor(String name);
+  public static native void enableCommonRetransformation(boolean enable);
+  public static native void addCommonTransformationResult(String target_name,
+                                                          byte[] class_bytes,
+                                                          byte[] dex_bytes);
+}
diff --git a/test/1949-short-dex-file/src/art/Test1949.java b/test/1949-short-dex-file/src/art/Test1949.java
new file mode 100644
index 0000000000..98fa7fc2c1
--- /dev/null
+++ b/test/1949-short-dex-file/src/art/Test1949.java
@@ -0,0 +1,142 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package art;
+
+import java.lang.reflect.*;
+import java.util.Base64;
+import java.nio.ByteBuffer;
+
+public class Test1949 {
+  private final static boolean isDalvik = System.getProperty("java.vm.name").equals("Dalvik");
+
+  // This dex file is specifically crafted to have exactly 4 methodIDs in it. They are (in order):
+  //   (0) Ljava/lang/Object;-><init>()V
+  //   (1) Lxyz/Transform;-><init>()V
+  //   (2) Lxyz/Transform;->bar()V
+  //   (3) Lxyz/Transform;->foo()V
+  //
+  // In the transformed version of the dex file there is a new method. The new list of methodIDs is:
+  //   (0) Lart/Test1949;->doNothing()V
+  //   (1) Ljava/lang/Object;-><init>()V
+  //   (2) Lxyz/Transform;-><init>()V
+  //   (3) Lxyz/Transform;->bar()V
+  //   (4) Lxyz/Transform;->foo()V
+  //
+  // This test tries to get the JIT to read out-of-bounds on the initial dex file by getting it to
+  // read the 5th method id of the new file (Lxyz/Transform;->foo()V) from the old dex file (which
+  // only has 4 method ids).
+  //
+  // To do this we need to make sure that the class being transformed is near the end of the
+  // alphabet (package xyz, method foo). If it is further forward than the other method-ids then the
+  // JIT will read an incorrect (but valid) method-id from the old-dex file. This is why the error
+  // wasn't caught in our other tests (package art is always at the front).
+  //
+  // The final method that causes the OOB read needs to be a native method because that is the only
+  // method-type the jit uses dex-file information to keep track of.
+
+  /**
+   * base64 encoded class/dex file for
+   * package xyz;
+   * public class Transform {
+   *   public native void foo();
+   *   public void bar() {}
+   * }
+   */
+  private static final byte[] CLASS_BYTES_INIT = Base64.getDecoder().decode(
+    "yv66vgAAADUADwoAAwAMBwANBwAOAQAGPGluaXQ+AQADKClWAQAEQ29kZQEAD0xpbmVOdW1iZXJU" +
+    "YWJsZQEAA2ZvbwEAA2JhcgEAClNvdXJjZUZpbGUBAA5UcmFuc2Zvcm0uamF2YQwABAAFAQANeHl6" +
+    "L1RyYW5zZm9ybQEAEGphdmEvbGFuZy9PYmplY3QAIQACAAMAAAAAAAMAAQAEAAUAAQAGAAAAHQAB" +
+    "AAEAAAAFKrcAAbEAAAABAAcAAAAGAAEAAAACAQEACAAFAAAAAQAJAAUAAQAGAAAAGQAAAAEAAAAB" +
+    "sQAAAAEABwAAAAYAAQAAAAQAAQAKAAAAAgAL");
+  private static final byte[] DEX_BYTES_INIT = Base64.getDecoder().decode(
+    "ZGV4CjAzNQBDUutFJpeT+okk+aXah8NQ61q2XRtkmChwAgAAcAAAAHhWNBIAAAAAAAAAANwBAAAI" +
+    "AAAAcAAAAAMAAACQAAAAAQAAAJwAAAAAAAAAAAAAAAQAAACoAAAAAQAAAMgAAACIAQAA6AAAABwB" +
+    "AAAkAQAAOAEAAEkBAABZAQAAXAEAAGEBAABmAQAAAQAAAAIAAAAEAAAABAAAAAIAAAAAAAAAAAAA" +
+    "AAAAAAABAAAAAAAAAAEAAAAFAAAAAQAAAAYAAAABAAAAAQAAAAAAAAAAAAAAAwAAAAAAAADDAQAA" +
+    "AAAAAAEAAQABAAAAEgEAAAQAAABwEAAAAAAOAAEAAQAAAAAAFgEAAAEAAAAOAAIADgAEAA4AAAAG" +
+    "PGluaXQ+ABJMamF2YS9sYW5nL09iamVjdDsAD0x4eXovVHJhbnNmb3JtOwAOVHJhbnNmb3JtLmph" +
+    "dmEAAVYAA2JhcgADZm9vAFt+fkQ4eyJtaW4tYXBpIjoxLCJzaGEtMSI6IjkwZWYyMjkwNWMzZmVj" +
+    "Y2FiMjMwMzBhNmJkYzU2NTcwYTMzNWVmMDUiLCJ2ZXJzaW9uIjoidjEuMS44LWRldiJ9AAAAAQIB" +
+    "gYAE6AECAYACAYECAAAAAAAAAAAMAAAAAAAAAAEAAAAAAAAAAQAAAAgAAABwAAAAAgAAAAMAAACQ" +
+    "AAAAAwAAAAEAAACcAAAABQAAAAQAAACoAAAABgAAAAEAAADIAAAAASAAAAIAAADoAAAAAyAAAAIA" +
+    "AAASAQAAAiAAAAgAAAAcAQAAACAAAAEAAADDAQAAAxAAAAEAAADYAQAAABAAAAEAAADcAQAA");
+
+  /**
+   * base64 encoded class/dex file for
+   * package xyz;
+   * public class Transform {
+   *   public native void foo();
+   *   public void bar() {
+   *     // Make sure the methodID is before any of the ones in Transform
+   *     art.Test1949.doNothing();
+   *   }
+   * }
+   */
+  private static final byte[] CLASS_BYTES_FINAL = Base64.getDecoder().decode(
+    "yv66vgAAADUAFAoABAANCgAOAA8HABAHABEBAAY8aW5pdD4BAAMoKVYBAARDb2RlAQAPTGluZU51" +
+    "bWJlclRhYmxlAQADZm9vAQADYmFyAQAKU291cmNlRmlsZQEADlRyYW5zZm9ybS5qYXZhDAAFAAYH" +
+    "ABIMABMABgEADXh5ei9UcmFuc2Zvcm0BABBqYXZhL2xhbmcvT2JqZWN0AQAMYXJ0L1Rlc3QxOTQ5" +
+    "AQAJZG9Ob3RoaW5nACEAAwAEAAAAAAADAAEABQAGAAEABwAAAB0AAQABAAAABSq3AAGxAAAAAQAI" +
+    "AAAABgABAAAAAgEBAAkABgAAAAEACgAGAAEABwAAABwAAAABAAAABLgAArEAAAABAAgAAAAGAAEA" +
+    "AAAEAAEACwAAAAIADA==");
+  private static final byte[] DEX_BYTES_FINAL = Base64.getDecoder().decode(
+    "ZGV4CjAzNQBHXBiw7Hso1vnmaXE1VCV41f4+0aECixOgAgAAcAAAAHhWNBIAAAAAAAAAAAwCAAAK" +
+    "AAAAcAAAAAQAAACYAAAAAQAAAKgAAAAAAAAAAAAAAAUAAAC0AAAAAQAAANwAAACkAQAA/AAAADQB" +
+    "AAA8AQAATAEAAGABAABxAQAAgQEAAIQBAACJAQAAlAEAAJkBAAABAAAAAgAAAAMAAAAFAAAABQAA" +
+    "AAMAAAAAAAAAAAAAAAcAAAABAAAAAAAAAAIAAAAAAAAAAgAAAAYAAAACAAAACAAAAAIAAAABAAAA" +
+    "AQAAAAAAAAAEAAAAAAAAAPYBAAAAAAAAAQABAAEAAAAsAQAABAAAAHAQAQAAAA4AAQABAAAAAAAw" +
+    "AQAABAAAAHEAAAAAAA4AAgAOAAQADgAGPGluaXQ+AA5MYXJ0L1Rlc3QxOTQ5OwASTGphdmEvbGFu" +
+    "Zy9PYmplY3Q7AA9MeHl6L1RyYW5zZm9ybTsADlRyYW5zZm9ybS5qYXZhAAFWAANiYXIACWRvTm90" +
+    "aGluZwADZm9vAFt+fkQ4eyJtaW4tYXBpIjoxLCJzaGEtMSI6IjkwZWYyMjkwNWMzZmVjY2FiMjMw" +
+    "MzBhNmJkYzU2NTcwYTMzNWVmMDUiLCJ2ZXJzaW9uIjoidjEuMS44LWRldiJ9AAAAAQICgYAE/AED" +
+    "AZQCAYECAAAAAAAMAAAAAAAAAAEAAAAAAAAAAQAAAAoAAABwAAAAAgAAAAQAAACYAAAAAwAAAAEA" +
+    "AACoAAAABQAAAAUAAAC0AAAABgAAAAEAAADcAAAAASAAAAIAAAD8AAAAAyAAAAIAAAAsAQAAAiAA" +
+    "AAoAAAA0AQAAACAAAAEAAAD2AQAAAxAAAAEAAAAIAgAAABAAAAEAAAAMAgAA");
+
+  public static void run() throws Exception {
+    Redefinition.setTestConfiguration(Redefinition.Config.COMMON_REDEFINE);
+    doTest();
+  }
+
+  // A method with a methodID before anything in Transform.
+  public static void doNothing() {}
+
+  private static ClassLoader CreateClassLoader(byte[] clz, byte[] dex) throws Exception {
+    if (isDalvik) {
+      Class<?> class_loader_class = Class.forName("dalvik.system.InMemoryDexClassLoader");
+      Constructor<?> ctor = class_loader_class.getConstructor(ByteBuffer.class, ClassLoader.class);
+      /* on Dalvik, this is a DexFile; otherwise, it's null */
+      return (ClassLoader)ctor.newInstance(ByteBuffer.wrap(dex), Test1949.class.getClassLoader());
+    } else {
+      return new ClassLoader() {
+        public Class<?> findClass(String name) throws ClassNotFoundException {
+          if (name.equals("xyz.Transform")) {
+            return defineClass(name, clz, 0, clz.length);
+          } else {
+            throw new ClassNotFoundException("Couldn't find class: " + name);
+          }
+        }
+      };
+    }
+  }
+
+  public static void doTest() throws Exception {
+    Class c = CreateClassLoader(CLASS_BYTES_INIT, DEX_BYTES_INIT).loadClass("xyz.Transform");
+    Redefinition.doCommonClassRedefinition(c, CLASS_BYTES_FINAL, DEX_BYTES_FINAL);
+    System.out.println("Passed");
+  }
+}