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
diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index 4673454..5d430d2 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -1402,13 +1402,6 @@
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 @@
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 0000000..863339f
--- /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 0000000..e924086
--- /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 0000000..c6e62ae
--- /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 0000000..dbbaa86
--- /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 0000000..56d2938
--- /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 0000000..98fa7fc
--- /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");
+ }
+}