8277444: Data race between JvmtiClassFileReconstituter::copy_bytecodes and class linking

src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp

@@ -960,6 +960,11 @@ void JvmtiClassFileReconstituter::write_u8(u8 x) {
 
 void JvmtiClassFileReconstituter::copy_bytecodes(const methodHandle& mh,
                                                  unsigned char* bytecodes) {
+  // We must copy bytecodes only from linked classes.
+  // Being linked guarantees we are not getting bytecodes at
+  // the same time the linking process is rewriting them.
+  guarantee(mh->method_holder()->is_linked(), "Bytecodes must be copied from a linked class");
+
   // use a BytecodeStream to iterate over the bytecodes. JVM/fast bytecodes
   // and the breakpoint bytecode are converted to their original bytecodes.
 

src/hotspot/share/prims/jvmtiEnv.cpp

@@ -425,6 +425,18 @@ JvmtiEnv::RetransformClasses(jint class_count, const jclass* classes) {
 
     InstanceKlass* ik = InstanceKlass::cast(klass);
     if (ik->get_cached_class_file_bytes() == NULL) {
+      // Link the class to avoid races with the rewriter. This will call the verifier also
+      // on the class. Linking is also done in VM_RedefineClasses below, but we need
+      // to keep that for other VM_RedefineClasses callers.
+      JavaThread* THREAD = current_thread;
+      ik->link_class(THREAD);
+      if (HAS_PENDING_EXCEPTION) {
+        // Retransform/JVMTI swallows error messages. Using this class will rerun the verifier in a context
+        // that propagates the VerifyError, if thrown.
+        CLEAR_PENDING_EXCEPTION;
+        return JVMTI_ERROR_INVALID_CLASS;
+      }
+
       // Not cached, we need to reconstitute the class file from the
       // VM representation. We don't attach the reconstituted class
       // bytes to the InstanceKlass here because they have not been
@@ -3013,7 +3025,8 @@ jvmtiError
 JvmtiEnv::GetBytecodes(Method* method, jint* bytecode_count_ptr, unsigned char** bytecodes_ptr) {
   NULL_CHECK(method, JVMTI_ERROR_INVALID_METHODID);
 
-  methodHandle mh(Thread::current(), method);
+  JavaThread* current_thread = JavaThread::current();
+  methodHandle mh(current_thread, method);
   jint size = (jint)mh->code_size();
   jvmtiError err = allocate(size, bytecodes_ptr);
   if (err != JVMTI_ERROR_NONE) {
@@ -3022,6 +3035,13 @@ JvmtiEnv::GetBytecodes(Method* method, jint* bytecode_count_ptr, unsigned char**
 
   (*bytecode_count_ptr) = size;
   // get byte codes
+  // Make sure the class is verified and rewritten first.
+  JavaThread* THREAD = current_thread;
+  mh->method_holder()->link_class(THREAD);
+  if (HAS_PENDING_EXCEPTION) {
+    CLEAR_PENDING_EXCEPTION;
+    return JVMTI_ERROR_INVALID_CLASS;
+  }
   JvmtiClassFileReconstituter::copy_bytecodes(mh, *bytecodes_ptr);
 
   return JVMTI_ERROR_NONE;

test/jdk/java/lang/instrument/RetransformBigClassTest.java

@@ -0,0 +1,161 @@
+/*
+ * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/*
+ * @test
+ * @bug 8277444
+ *
+ * @library /test/lib
+ * @compile SimpleIdentityTransformer.java
+ * @run shell MakeJAR.sh retransformAgent
+ * @run main/othervm -javaagent:retransformAgent.jar RetransformBigClassTest
+ */
+
+import jdk.test.lib.compiler.InMemoryJavaCompiler;
+
+/*
+ * JvmtiClassFileReconstituter::copy_bytecodes restores bytecodes rewritten
+ * by the linking process. It is used by RetransformClasses.
+ * JDK-8277444 is a data race between copy_bytecodes and the linking process.
+ * This test puts the linking process in one thread and the retransforming process
+ * in another thread. The test uses Class.forName("BigClass", false, classLoader)
+ * which does not link the class. When the class is used, the linking process starts.
+ * In another thread retransforming of the class is happening.
+ * We generate a class with big methods. A number of methods and their size are
+ * chosen to make the linking and retransforming processes run concurrently.
+ * We delay the retransforming process to follow the linking process.
+ * If there is no synchronization between the processes, a data race will happen.
+ */
+public class RetransformBigClassTest extends AInstrumentationTestCase {
+
+    private static final Object LOCK = new Object();
+    private static final int COUNTER_INC_COUNT            = 2000; // A number of 'c+=1;' statements in methods of a class.
+    private static final int MIN_LINK_TIME_MS             = 60;   // Large enough so the linking and retransforming processes run in parallel.
+    private static final int RETRANSFORM_CLASSES_DELAY_MS = 37;   // We manage to create a data race when a delay is in the range 0.52x - 0.62x of MIN_LINK_TIME_MS.
+
+    private static Class<?> bigClass;
+    private static byte[] bigClassBytecode;
+
+    private Thread retransformThread;
+
+    RetransformBigClassTest() {
+        super("RetransformBigClassTest");
+    }
+
+    public static void main(String[] args) throws Throwable {
+        new RetransformBigClassTest().runTest();
+    }
+
+    protected final void doRunTest() throws Throwable {
+        ClassLoader classLoader = new ClassLoader() {
+                @Override
+                protected Class<?> findClass(String name) throws ClassNotFoundException {
+                    if (name.equals("BigClass")) {
+                        return defineClass(name, bigClassBytecode, 0, bigClassBytecode.length);
+                    }
+
+                    return super.findClass(name);
+                }
+        };
+        synchronized (LOCK) {
+            bigClass = Class.forName("BigClass", false, classLoader);
+            LOCK.notify();
+        }
+        // Make a use of the BigClass
+        assertTrue(bigClass.getConstructor().newInstance().hashCode() != 0);
+        retransformThread.join();
+    }
+
+    private byte[] createClassBytecode(String className, int methodCount) throws Exception {
+        String methodBody = "";
+        for (int j = 0; j < COUNTER_INC_COUNT; j++) {
+            methodBody += "c+=1;";
+        }
+
+        String classSrc = "public class " + className + " { int c;";
+
+        for (int i = 0; i < methodCount; i++) {
+            classSrc += "\npublic void m" + i + "(){";
+            classSrc += methodBody;
+            classSrc += "\n}";
+        }
+        classSrc += "\n}";
+
+        return InMemoryJavaCompiler.compile(className, classSrc);
+    }
+
+    // We need a number of methods such that the linking time is greater than
+    // or equal to MIN_LINK_TIME_MS.
+    // We create a class having 5 methods and trigger the linking process.
+    // We measure the time taken and use it to calculate the needed number.
+    private int findMethodCount() throws Exception {
+        int methodCount = 5;
+        final String className = "BigClass" + methodCount;
+        final byte[] bytecode = createClassBytecode(className, methodCount);
+        ClassLoader classLoader = new ClassLoader() {
+            @Override
+            protected Class<?> findClass(String name) throws ClassNotFoundException {
+                if (name.equals(className)) {
+                    return defineClass(name, bytecode, 0, bytecode.length);
+                }
+
+                return super.findClass(name);
+            }
+        };
+        var bigClass = Class.forName(className, false, classLoader);
+        long startTime = System.nanoTime();
+        assertTrue(bigClass.getConstructor().newInstance().hashCode() != 0);
+        double linkTimeMs = (System.nanoTime() - startTime) / 1000000.0;
+        System.out.println("Link time for a class with " + methodCount + " methods each having " + COUNTER_INC_COUNT + " counter increments: " + Math.round(linkTimeMs));
+        if (linkTimeMs < MIN_LINK_TIME_MS) {
+          methodCount = (int)Math.round((MIN_LINK_TIME_MS * methodCount) / linkTimeMs);
+        }
+        System.out.println("The number of methods to exceed " + MIN_LINK_TIME_MS + " ms linking time: " + methodCount);
+        return methodCount;
+    }
+
+    @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+
+        bigClassBytecode = createClassBytecode("BigClass", findMethodCount());
+        fInst.addTransformer(new SimpleIdentityTransformer());
+        retransformThread = new Thread(() -> {
+            try {
+                synchronized (LOCK) {
+                    while (bigClass == null) {
+                        System.out.println("[retransformThread]: Waiting for bigClass");
+                        LOCK.wait();
+                    }
+                }
+                Thread.sleep(RETRANSFORM_CLASSES_DELAY_MS);
+                fInst.retransformClasses(bigClass);
+            } catch (Exception e) {
+                e.printStackTrace();
+            }
+        });
+        retransformThread.start();
+        Thread.sleep(100);
+    }
+}