Skip to content

Commit 495c043

Browse files
author
Alex Menkov
committed
7124710: interleaved RedefineClasses() and RetransformClasses() calls may have a problem
Reviewed-by: sspitsyn, dcubed
1 parent d39d8c8 commit 495c043

File tree

4 files changed

+585
-17
lines changed

4 files changed

+585
-17
lines changed

‎src/hotspot/share/classfile/klassFactory.cpp

+6-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -123,7 +123,10 @@ static ClassFileStream* check_class_file_load_hook(ClassFileStream* stream,
123123
Handle class_loader(THREAD, loader_data->class_loader());
124124

125125
// Get the cached class file bytes (if any) from the class that
126-
// is being redefined or retransformed. We use jvmti_thread_state()
126+
// is being retransformed. If class file load hook provides
127+
// modified class data during class loading or redefinition,
128+
// new cached class file buffer should be allocated.
129+
// We use jvmti_thread_state()
127130
// instead of JvmtiThreadState::state_for(jt) so we don't allocate
128131
// a JvmtiThreadState any earlier than necessary. This will help
129132
// avoid the bug described by 7126851.
@@ -132,8 +135,7 @@ static ClassFileStream* check_class_file_load_hook(ClassFileStream* stream,
132135

133136
if (state != NULL) {
134137
Klass* k = state->get_class_being_redefined();
135-
136-
if (k != NULL) {
138+
if (k != NULL && state->get_class_load_kind() == jvmti_class_load_kind_retransform) {
137139
InstanceKlass* class_being_redefined = InstanceKlass::cast(k);
138140
*cached_class_file = class_being_redefined->get_cached_class_file();
139141
}

‎src/hotspot/share/prims/jvmtiRedefineClasses.cpp

+11-13
Original file line numberDiff line numberDiff line change
@@ -4329,20 +4329,18 @@ void VM_RedefineClasses::redefine_single_class(Thread* current, jclass the_jclas
43294329
int emcp_method_count = check_methods_and_mark_as_obsolete();
43304330
transfer_old_native_function_registrations(the_class);
43314331

4332-
// The class file bytes from before any retransformable agents mucked
4333-
// with them was cached on the scratch class, move to the_class.
4334-
// Note: we still want to do this if nothing needed caching since it
4335-
// should get cleared in the_class too.
4336-
if (the_class->get_cached_class_file() == 0) {
4337-
// the_class doesn't have a cache yet so copy it
4338-
the_class->set_cached_class_file(scratch_class->get_cached_class_file());
4339-
}
4340-
else if (scratch_class->get_cached_class_file() !=
4341-
the_class->get_cached_class_file()) {
4342-
// The same class can be present twice in the scratch classes list or there
4332+
if (scratch_class->get_cached_class_file() != the_class->get_cached_class_file()) {
4333+
// 1. the_class doesn't have a cache yet, scratch_class does have a cache.
4334+
// 2. The same class can be present twice in the scratch classes list or there
43434335
// are multiple concurrent RetransformClasses calls on different threads.
4344-
// In such cases we have to deallocate scratch_class cached_class_file.
4345-
os::free(scratch_class->get_cached_class_file());
4336+
// the_class and scratch_class have the same cached bytes, but different buffers.
4337+
// In such cases we need to deallocate one of the buffers.
4338+
// 3. RedefineClasses and the_class has cached bytes from a previous transformation.
4339+
// In the case we need to use class bytes from scratch_class.
4340+
if (the_class->get_cached_class_file() != nullptr) {
4341+
os::free(the_class->get_cached_class_file());
4342+
}
4343+
the_class->set_cached_class_file(scratch_class->get_cached_class_file());
43464344
}
43474345

43484346
// NULL out in scratch class to not delete twice. The class to be redefined

‎test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java

+274
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,274 @@
1+
/*
2+
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test
26+
*
27+
* @bug 7124710
28+
*
29+
* @requires vm.jvmti
30+
* @modules java.base/jdk.internal.org.objectweb.asm
31+
* @library /test/lib
32+
*
33+
* @comment main/othervm/native -Xlog:redefine*=trace -agentlib:RedefineRetransform RedefineRetransform
34+
* @run main/othervm/native -agentlib:RedefineRetransform RedefineRetransform 1
35+
* @run main/othervm/native -agentlib:RedefineRetransform RedefineRetransform 2
36+
* @run main/othervm/native -agentlib:RedefineRetransform RedefineRetransform 3
37+
* @run main/othervm/native -agentlib:RedefineRetransform RedefineRetransform 4
38+
* @run main/othervm/native -agentlib:RedefineRetransform RedefineRetransform 5
39+
* @run main/othervm/native -agentlib:RedefineRetransform RedefineRetransform 6
40+
*/
41+
42+
import java.io.IOException;
43+
import java.lang.annotation.Retention;
44+
import java.lang.annotation.RetentionPolicy;
45+
46+
import jdk.internal.org.objectweb.asm.AnnotationVisitor;
47+
import jdk.internal.org.objectweb.asm.ClassReader;
48+
import jdk.internal.org.objectweb.asm.ClassVisitor;
49+
import jdk.internal.org.objectweb.asm.ClassWriter;
50+
import jdk.internal.org.objectweb.asm.Opcodes;
51+
import jdk.internal.org.objectweb.asm.Type;
52+
53+
/*
54+
* The test verifies that after interleaved RedefineClasses/RetransformClasses calls
55+
* JVMTI passes correct class bytes to ClassFileLoadHook (as per JVMTI spec).
56+
* To distinguish class version the test instruments test class overriding runtime-visible annotation.
57+
*/
58+
public class RedefineRetransform {
59+
static {
60+
System.loadLibrary("RedefineRetransform");
61+
}
62+
63+
@Retention(RetentionPolicy.RUNTIME)
64+
@interface ClassVersion {
65+
int value();
66+
}
67+
68+
// Use runtime-visible annotation to specify class version.
69+
@ClassVersion(0)
70+
static class TestClass {
71+
public TestClass() { }
72+
}
73+
74+
// Redefines testClass with classBytes, instruments with classLoadHookBytes (if != null).
75+
// Returns class bytes passed to ClassFileLoadHook or null on error.
76+
private static native byte[] nRedefine(Class testClass, byte[] classBytes, byte[] classLoadHookBytes);
77+
// Retransforms testClass, instruments with classBytes (if != null).
78+
// Returns class bytes passed to ClassFileLoadHook or null on error.
79+
private static native byte[] nRetransform(Class testClass, byte[] classBytes);
80+
81+
// Class bytes for initial TestClass (ClassVersion == 0).
82+
private static byte[] initialClassBytes;
83+
84+
private static class VersionScanner extends ClassVisitor {
85+
private Integer detectedVersion;
86+
private Integer versionToSet;
87+
// to get version
88+
public VersionScanner() {
89+
super(Opcodes.ASM7);
90+
}
91+
// to set version
92+
public VersionScanner(int verToSet, ClassVisitor classVisitor) {
93+
super(Opcodes.ASM7, classVisitor);
94+
versionToSet = verToSet;
95+
}
96+
97+
public int detectedVersion() {
98+
if (detectedVersion == null) {
99+
throw new RuntimeException("Version not detected");
100+
}
101+
return detectedVersion;
102+
}
103+
104+
@Override
105+
public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
106+
//log("visitAnnotation: descr = '" + descriptor + "', visible = " + visible);
107+
if (Type.getDescriptor(ClassVersion.class).equals(descriptor)) {
108+
return new AnnotationVisitor(Opcodes.ASM7, super.visitAnnotation(descriptor, visible)) {
109+
@Override
110+
public void visit(String name, Object value) {
111+
//log("visit: name = '" + name + "', value = " + value
112+
// + " (" + (value == null ? "N/A" : value.getClass()) + ")");
113+
if ("value".equals(name) && value instanceof Integer intValue) {
114+
detectedVersion = intValue;
115+
if (versionToSet != null) {
116+
//log("replace with " + versionToSet);
117+
value = versionToSet;
118+
}
119+
}
120+
super.visit(name, value);
121+
}
122+
};
123+
}
124+
return super.visitAnnotation(descriptor, visible);
125+
}
126+
}
127+
128+
// Generates TestClass class bytes with the specified ClassVersion value.
129+
private static byte[] getClassBytes(int ver) {
130+
if (ver < 0) {
131+
return null;
132+
}
133+
ClassWriter cw = new ClassWriter(0);
134+
ClassReader cr = new ClassReader(initialClassBytes);
135+
cr.accept(new VersionScanner(ver, cw), 0);
136+
return cw.toByteArray();
137+
}
138+
139+
// Extracts ClassVersion values from the provided class bytes.
140+
private static int getClassBytesVersion(byte[] classBytes) {
141+
ClassReader cr = new ClassReader(classBytes);
142+
VersionScanner scanner = new VersionScanner();
143+
cr.accept(scanner, 0);
144+
return scanner.detectedVersion();
145+
}
146+
147+
static void init() {
148+
try {
149+
initialClassBytes = TestClass.class.getClassLoader()
150+
.getResourceAsStream("RedefineRetransform$TestClass.class")
151+
.readAllBytes();
152+
log("Read TestClass bytes: " + initialClassBytes.length);
153+
} catch (IOException ex) {
154+
throw new RuntimeException("Failed to read class bytes", ex);
155+
}
156+
}
157+
158+
// Redefines TestClass to the version specified.
159+
static void redefine(int ver) {
160+
redefine(ver, -1);
161+
}
162+
163+
// Redefines TestClass to the version specified
164+
// instrumenting (from ClassFileLoadHook) with 'classLoadHookVer' class bytes (if >= 0).
165+
// Also verifies that class bytes passed to ClassFileLoadHook have correct version (ver).
166+
static void redefine(int ver, int classLoadHookVer) {
167+
byte[] classBytes = getClassBytes(ver);
168+
byte[] classLoadHookBytes = getClassBytes(classLoadHookVer);
169+
170+
byte[] hookClassBytes = nRedefine(TestClass.class, classBytes, classLoadHookBytes);
171+
if (hookClassBytes == null) {
172+
throw new RuntimeException("Redefine error (ver = " + ver + ")");
173+
}
174+
// verify ClassFileLoadHook gets the expected class bytes
175+
int hookVer = getClassBytesVersion(hookClassBytes);
176+
if (hookVer != ver) {
177+
throw new RuntimeException("CLFH got unexpected version: " + hookVer
178+
+ " (expected " + ver + ")");
179+
}
180+
}
181+
182+
// Retransforms TestClass instrumenting (from ClassFileLoadHook) with 'ver' class bytes (if >= 0).
183+
// Verifies that class bytes passed to ClassFileLoadHook have correct version (expectedVer).
184+
static void retransform(int ver, int expectedVer) {
185+
byte[] classBytes = getClassBytes(ver);
186+
byte[] hookClassBytes = nRetransform(TestClass.class, classBytes);
187+
int hookVer = getClassBytesVersion(hookClassBytes);
188+
if (hookVer != expectedVer) {
189+
throw new RuntimeException("CLFH got unexpected version: " + hookVer
190+
+ " (expected " + expectedVer + ")");
191+
}
192+
}
193+
194+
public static void main(String[] args) throws Exception {
195+
int testCase;
196+
try {
197+
testCase = Integer.valueOf(args[0]);
198+
} catch (Exception ex) {
199+
throw new RuntimeException("Single numeric argument expected", ex);
200+
}
201+
init();
202+
switch (testCase) {
203+
case 1:
204+
test("Redefine-Retransform-Retransform", () -> {
205+
redefine(1); // cached class bytes are not set
206+
retransform(2, 1); // sets cached class bytes to ver 1
207+
retransform(3, 1); // uses existing cache
208+
});
209+
break;
210+
211+
case 2:
212+
test("Redefine-Retransform-Redefine-Redefine", () -> {
213+
redefine(1); // cached class bytes are not set
214+
retransform(2, 1); // sets cached class bytes to ver 1
215+
redefine(3); // resets cached class bytes to nullptr
216+
redefine(4); // cached class bytes are not set
217+
});
218+
break;
219+
220+
case 3:
221+
test("Redefine-Retransform-Redefine-Retransform", () -> {
222+
redefine(1); // cached class bytes are not set
223+
retransform(2, 1); // sets cached class bytes to ver 1
224+
redefine(3); // resets cached class bytes to nullptr
225+
retransform(4, 3); // sets cached class bytes to ver 3
226+
});
227+
break;
228+
229+
case 4:
230+
test("Retransform-Redefine-Retransform-Retransform", () -> {
231+
retransform(1, 0); // sets cached class bytes to ver 0 (initially loaded)
232+
redefine(2); // resets cached class bytes to nullptr
233+
retransform(3, 2); // sets cached class bytes to ver 2
234+
retransform(4, 2); // uses existing cache
235+
});
236+
break;
237+
238+
case 5:
239+
test("Redefine-Retransform-Redefine-Retransform with CFLH", () -> {
240+
redefine(1, 5); // CFLH sets cached class bytes to ver 1
241+
retransform(2, 1); // uses existing cache
242+
redefine(3, 6); // resets cached class bytes to nullptr,
243+
// CFLH sets cached class bytes to ver 3
244+
retransform(4, 3); // uses existing cache
245+
});
246+
break;
247+
248+
case 6:
249+
test("Retransform-Redefine-Retransform-Retransform with CFLH", () -> {
250+
retransform(1, 0); // sets cached class bytes to ver 0 (initially loaded)
251+
redefine(2, 5); // resets cached class bytes to nullptr,
252+
// CFLH sets cached class bytes to ver 2
253+
retransform(3, 2); // uses existing cache
254+
retransform(4, 2); // uses existing cache
255+
});
256+
break;
257+
}
258+
}
259+
260+
private static void log(Object msg) {
261+
System.out.println(msg);
262+
}
263+
264+
private interface Test {
265+
void test();
266+
}
267+
268+
private static void test(String name, Test theTest) {
269+
log(">>Test: " + name);
270+
theTest.test();
271+
log("<<Test: " + name + " - OK");
272+
log("");
273+
}
274+
}

0 commit comments

Comments
 (0)