Ensure IsStructurallyRedefinable takes into account subtypes
Structural-redefinition changes the addresses of the ArtFields and
ArtMethods of the class being redefined and all of its subclasses. For
this reason if there are JNI-ids that are pointers to these anywhere
we cannot allow structural redefinition of the class, even if the
pointers are for subtypes. Previously we didn't check the
redefinability of sub-types so we could incorrectly invalidate jmethod
or jfield IDs.
Test: ./test.py --host
Bug: 148414638
Change-Id: I8fc809a43e16d5e7938b90b00c531573a2edca00
diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index a9c62c6..28018d7 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -465,17 +465,44 @@
"safe to structurally redefine it.";
return ERR(UNMODIFIABLE_CLASS);
}
- // Check for fields/methods which were returned before moving to index jni id type.
- // TODO We might want to rework how this is done. Once full redefinition is implemented we will
- // need to check any subtypes too.
- art::ObjPtr<art::mirror::ClassExt> ext(klass->GetExtData());
- if (!ext.IsNull()) {
- if (ext->HasInstanceFieldPointerIdMarker() ||
- ext->HasMethodPointerIdMarker() ||
- ext->HasStaticFieldPointerIdMarker()) {
- return ERR(UNMODIFIABLE_CLASS);
- }
+ auto has_pointer_marker =
+ [](art::ObjPtr<art::mirror::Class> k) REQUIRES_SHARED(art::Locks::mutator_lock_) {
+ // Check for fields/methods which were returned before moving to index jni id type.
+ // TODO We might want to rework how this is done. Once full redefinition is implemented we
+ // will need to check any subtypes too.
+ art::ObjPtr<art::mirror::ClassExt> ext(k->GetExtData());
+ if (!ext.IsNull()) {
+ if (ext->HasInstanceFieldPointerIdMarker() || ext->HasMethodPointerIdMarker() ||
+ ext->HasStaticFieldPointerIdMarker()) {
+ return true;
+ }
+ }
+ return false;
+ };
+ if (has_pointer_marker(klass.Get())) {
+ *error_msg =
+ StringPrintf("%s has active pointer jni-ids and cannot be redefined structurally",
+ klass->PrettyClass().c_str());
+ return ERR(UNMODIFIABLE_CLASS);
}
+ jvmtiError res = OK;
+ art::ClassFuncVisitor cfv(
+ [&](art::ObjPtr<art::mirror::Class> k) REQUIRES_SHARED(art::Locks::mutator_lock_) {
+ // if there is any class 'K' that is a subtype (i.e. extends) klass and has pointer-jni-ids
+ // we cannot structurally redefine the class 'k' since we would structurally redefine the
+ // subtype.
+ if (k->IsLoaded() && klass->IsAssignableFrom(k) && has_pointer_marker(k)) {
+ *error_msg = StringPrintf(
+ "%s has active pointer jni-ids from subtype %s and cannot be redefined structurally",
+ klass->PrettyClass().c_str(),
+ k->PrettyClass().c_str());
+ res = ERR(UNMODIFIABLE_CLASS);
+ return false;
+ }
+ return true;
+ });
+ art::Runtime::Current()->GetClassLinker()->VisitClasses(&cfv);
+ return res;
}
return OK;
}
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index 5456e9b..4ce8c43 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -103,6 +103,18 @@
virtual bool operator()(ObjPtr<mirror::Class> klass) = 0;
};
+template <typename Func>
+class ClassFuncVisitor final : public ClassVisitor {
+ public:
+ explicit ClassFuncVisitor(Func func) : func_(func) {}
+ bool operator()(ObjPtr<mirror::Class> klass) override REQUIRES_SHARED(Locks::mutator_lock_) {
+ return func_(klass);
+ }
+
+ private:
+ Func func_;
+};
+
class ClassLoaderVisitor {
public:
virtual ~ClassLoaderVisitor() {}
diff --git a/test/2012-structural-redefinition-failures-jni-id/expected.txt b/test/2012-structural-redefinition-failures-jni-id/expected.txt
new file mode 100644
index 0000000..4c3dd98
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/expected.txt
@@ -0,0 +1,8 @@
+Checking classes
+Is Structurally modifiable class Main$C1 true
+Is Structurally modifiable class Main$C2 true
+Is Structurally modifiable class Main$C3 true
+Setting C2 as having pointer-ids used and checking classes
+Is Structurally modifiable class Main$C1 false
+Is Structurally modifiable class Main$C2 false
+Is Structurally modifiable class Main$C3 true
diff --git a/test/2012-structural-redefinition-failures-jni-id/info.txt b/test/2012-structural-redefinition-failures-jni-id/info.txt
new file mode 100644
index 0000000..68520bf
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/info.txt
@@ -0,0 +1,3 @@
+Sanity check for isStructurallyModifiable.
+
+Ensures that types being not-modifiable makes their supertypes not-modifiable.
diff --git a/test/2012-structural-redefinition-failures-jni-id/run b/test/2012-structural-redefinition-failures-jni-id/run
new file mode 100755
index 0000000..03e41a5
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/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 --runtime-option -Xopaque-jni-ids:true
diff --git a/test/2012-structural-redefinition-failures-jni-id/set-jni-id-used.cc b/test/2012-structural-redefinition-failures-jni-id/set-jni-id-used.cc
new file mode 100644
index 0000000..4b3dac9
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/set-jni-id-used.cc
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2020 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.
+ */
+
+#include <stdio.h>
+
+#include <vector>
+
+#include "android-base/logging.h"
+#include "android-base/macros.h"
+#include "handle_scope-inl.h"
+#include "jni.h"
+#include "jvmti.h"
+
+// Test infrastructure
+#include "jvmti_helper.h"
+#include "mirror/class.h"
+#include "mirror/class_ext.h"
+#include "scoped_local_ref.h"
+#include "scoped_thread_state_change-inl.h"
+#include "test_env.h"
+
+namespace art {
+namespace Test2012SetJniIdUsed {
+
+extern "C" JNIEXPORT void JNICALL Java_Main_SetPointerIdsUsed(
+ JNIEnv* env, jclass klass ATTRIBUTE_UNUSED, jclass target) {
+ ScopedObjectAccess soa(env);
+ StackHandleScope<1> hs(soa.Self());
+ Handle<mirror::Class> h(hs.NewHandle(soa.Decode<mirror::Class>(target)));
+ ObjPtr<mirror::ClassExt> ext(h->EnsureExtDataPresent(h, soa.Self()));
+ CHECK(!ext.IsNull());
+ ext->SetIdsArraysForClassExtExtData(Runtime::Current()->GetJniIdManager()->GetPointerMarker());
+}
+
+} // namespace Test2012SetJniIdUsed
+} // namespace art
diff --git a/test/2012-structural-redefinition-failures-jni-id/src-art/Main.java b/test/2012-structural-redefinition-failures-jni-id/src-art/Main.java
new file mode 100644
index 0000000..4f39cde
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/src-art/Main.java
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+
+import art.*;
+
+public class Main {
+ public static void Check(Class[] klasses) {
+ for (Class k : klasses) {
+ try {
+ boolean res = Redefinition.isStructurallyModifiable(k);
+ System.out.println("Is Structurally modifiable " + k + " " + res);
+ } catch (Exception e) {
+ System.out.println("Got exception " + e + " during check modifiablity of " + k);
+ e.printStackTrace(System.out);
+ }
+ }
+ }
+
+ public static class C1 {
+ public Object o;
+ public void foobar() {}
+ }
+ public static class C2 extends C1 {
+ public static Object o;
+ public static void foo() {}
+ }
+ public static class C3 extends C2 {
+ public Object j;
+ public void bar() {}
+ }
+
+ public static void doTest() throws Exception {
+ Class[] classes = new Class[] {
+ C1.class,
+ C2.class,
+ C3.class,
+ };
+ System.out.println("Checking classes");
+ Check(classes);
+ System.out.println("Setting C2 as having pointer-ids used and checking classes");
+ SetPointerIdsUsed(C2.class);
+ Check(classes);
+ }
+ public static native void SetPointerIdsUsed(Class<?> k);
+ public static void main(String[] args) throws Exception {
+ // Redefinition.setTestConfiguration(Redefinition.Config.COMMON_REDEFINE);
+ doTest();
+ }
+}
diff --git a/test/2012-structural-redefinition-failures-jni-id/src-art/art/Redefinition.java b/test/2012-structural-redefinition-failures-jni-id/src-art/art/Redefinition.java
new file mode 120000
index 0000000..81eaf31
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/src-art/art/Redefinition.java
@@ -0,0 +1 @@
+../../../jvmti-common/Redefinition.java
\ No newline at end of file
diff --git a/test/2012-structural-redefinition-failures-jni-id/src-art/art/Test1983.java b/test/2012-structural-redefinition-failures-jni-id/src-art/art/Test1983.java
new file mode 100644
index 0000000..0576349
--- /dev/null
+++ b/test/2012-structural-redefinition-failures-jni-id/src-art/art/Test1983.java
@@ -0,0 +1,120 @@
+/*
+ * 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.invoke.*;
+import java.lang.ref.*;
+import java.lang.reflect.*;
+import java.util.*;
+
+public class Test1983 {
+ public static void runNonCts() throws Exception {
+ Redefinition.setTestConfiguration(Redefinition.Config.COMMON_REDEFINE);
+ doTest();
+ doTestNonCts();
+ }
+ public static void run() throws Exception {
+ Redefinition.setTestConfiguration(Redefinition.Config.COMMON_REDEFINE);
+ doTest();
+ }
+
+ public static void Check(Class[] klasses) {
+ for (Class k : klasses) {
+ try {
+ boolean res = Redefinition.isStructurallyModifiable(k);
+ System.out.println("Is Structurally modifiable " + k + " " + res);
+ } catch (Exception e) {
+ System.out.println("Got exception " + e + " during check modifiablity of " + k);
+ e.printStackTrace(System.out);
+ }
+ }
+ }
+
+ public static class WithVirtuals {
+ public Object o;
+ public void foobar() {}
+ }
+ public static class NoVirtuals extends WithVirtuals {
+ public static Object o;
+ public static void foo() {}
+ }
+ public static class SubWithVirtuals extends NoVirtuals {
+ public Object j;
+ public void bar() {}
+ }
+
+ public static void doTest() throws Exception {
+ Class[] mirrord_classes = new Class[] {
+ AccessibleObject.class,
+ CallSite.class,
+ // ClassExt is not on the compile classpath.
+ Class.forName("dalvik.system.ClassExt"),
+ ClassLoader.class,
+ Class.class,
+ Constructor.class,
+ // DexCache is not on the compile classpath
+ Class.forName("java.lang.DexCache"),
+ // EmulatedStackFrame is not on the compile classpath
+ Class.forName("dalvik.system.EmulatedStackFrame"),
+ Executable.class,
+ Field.class,
+ // @hide on CTS
+ Class.forName("java.lang.ref.FinalizerReference"),
+ MethodHandle.class,
+ MethodHandles.Lookup.class,
+ MethodType.class,
+ Method.class,
+ Object.class,
+ Proxy.class,
+ Reference.class,
+ StackTraceElement.class,
+ String.class,
+ Thread.class,
+ Throwable.class,
+ // @hide on CTS
+ Class.forName("java.lang.invoke.VarHandle"),
+ // TODO all the var handle types.
+ // @hide on CTS
+ Class.forName("java.lang.invoke.FieldVarHandle"),
+ };
+ System.out.println("Checking mirror'd classes");
+ Check(mirrord_classes);
+ // The results of some of these will change as we improve structural class redefinition. Any
+ // that are true should always remain so though.
+ Class[] non_mirrord_classes = new Class[] {
+ new Object[0].getClass(),
+ NoVirtuals.class,
+ WithVirtuals.class,
+ SubWithVirtuals.class,
+ };
+ System.out.println("Checking non-mirror'd classes");
+ Check(non_mirrord_classes);
+ }
+
+ public static void doTestNonCts() throws Exception {
+ System.out.println("Checking non-mirror'd classes (non-cts)");
+ Class[] non_mirrord_classes = new Class[] {
+ ArrayList.class,
+ Objects.class,
+ Arrays.class,
+ Integer.class,
+ Number.class,
+ MethodHandles.class,
+ };
+ Check(non_mirrord_classes);
+ }
+}
diff --git a/test/Android.bp b/test/Android.bp
index 125753f..3c10d1e 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -406,6 +406,7 @@
"1960-obsolete-jit-multithread-native/native_say_hi.cc",
"1964-add-to-dex-classloader-file/add_to_loader.cc",
"1963-add-to-dex-classloader-in-memory/check_memfd_create.cc",
+ "2012-structural-redefinition-failures-jni-id/set-jni-id-used.cc",
],
static_libs: [
"libz",