8330606: Redefinition doesn't but should verify the new klass

Reviewed-by: dholmes, jsjolen

src/hotspot/share/classfile/verifier.cpp

@@ -105,11 +105,13 @@ static verify_byte_codes_fn_t verify_byte_codes_fn() {
 
 // Methods in Verifier
 
+// This method determines whether we run the verifier and class file format checking code.
 bool Verifier::should_verify_for(oop class_loader) {
   return class_loader == nullptr ?
     BytecodeVerificationLocal : BytecodeVerificationRemote;
 }
 
+// This method determines whether we allow package access in access checks in reflection.
 bool Verifier::relax_access_for(oop loader) {
   bool trusted = java_lang_ClassLoader::is_trusted_loader(loader);
   bool need_verify =
@@ -120,6 +122,21 @@ bool Verifier::relax_access_for(oop loader) {
   return !need_verify;
 }
 
+// Callers will pass should_verify_class as true, depending on the results of should_verify_for() above,
+// or pass true for redefinition of any class.
+static bool is_eligible_for_verification(InstanceKlass* klass, bool should_verify_class) {
+  Symbol* name = klass->name();
+
+  return (should_verify_class &&
+    // Can not verify the bytecodes for shared classes because they have
+    // already been rewritten to contain constant pool cache indices,
+    // which the verifier can't understand.
+    // Shared classes shouldn't have stackmaps either.
+    // However, bytecodes for shared old classes can be verified because
+    // they have not been rewritten.
+    !(klass->is_shared() && klass->is_rewritten()));
+}
+
 void Verifier::trace_class_resolution(Klass* resolve_class, InstanceKlass* verify_class) {
   assert(verify_class != nullptr, "Unexpected null verify_class");
   ResourceMark rm;
@@ -273,27 +290,6 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
   }
 }
 
-bool Verifier::is_eligible_for_verification(InstanceKlass* klass, bool should_verify_class) {
-  Symbol* name = klass->name();
-
-  return (should_verify_class &&
-    // return if the class is a bootstrapping class
-    // or defineClass specified not to verify by default (flags override passed arg)
-    // We need to skip the following four for bootstraping
-    name != vmSymbols::java_lang_Object() &&
-    name != vmSymbols::java_lang_Class() &&
-    name != vmSymbols::java_lang_String() &&
-    name != vmSymbols::java_lang_Throwable() &&
-
-    // Can not verify the bytecodes for shared classes because they have
-    // already been rewritten to contain constant pool cache indices,
-    // which the verifier can't understand.
-    // Shared classes shouldn't have stackmaps either.
-    // However, bytecodes for shared old classes can be verified because
-    // they have not been rewritten.
-    !(klass->is_shared() && klass->is_rewritten()));
-}
-
 Symbol* Verifier::inference_verify(
     InstanceKlass* klass, char* message, size_t message_len, TRAPS) {
   JavaThread* thread = THREAD;

src/hotspot/share/classfile/verifier.hpp

@@ -61,7 +61,6 @@ class Verifier : AllStatic {
   static void trace_class_resolution(Klass* resolve_class, InstanceKlass* verify_class);
 
  private:
-  static bool is_eligible_for_verification(InstanceKlass* klass, bool should_verify_class);
   static Symbol* inference_verify(
     InstanceKlass* klass, char* msg, size_t msg_len, TRAPS);
 };

src/hotspot/share/oops/method.cpp

@@ -335,7 +335,8 @@ int Method::bci_from(address bcp) const {
 
 
 int Method::validate_bci(int bci) const {
-  return (bci == 0 || bci < code_size()) ? bci : -1;
+  // Called from the verifier, and should return -1 if not valid.
+  return ((is_native() && bci == 0) || (!is_native() && 0 <= bci && bci < code_size())) ? bci : -1;
 }
 
 // Return bci if it appears to be a valid bcp

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineVerifyError.java

@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2024, Oracle and/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 6402717 8330606
+ * @summary Redefine VerifyError to get a VerifyError should not throw SOE
+ * @requires vm.jvmti
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.base/jdk.internal.org.objectweb.asm
+ *          java.compiler
+ *          java.instrument
+ *          jdk.jartool/sun.tools.jar
+ * @run main RedefineClassHelper
+ * @run main/othervm/timeout=180
+ *         -javaagent:redefineagent.jar
+ *         -Xlog:class+init,exceptions
+ *         RedefineVerifyError
+ */
+
+import jdk.internal.org.objectweb.asm.AnnotationVisitor;
+import jdk.internal.org.objectweb.asm.Attribute;
+import jdk.internal.org.objectweb.asm.ClassReader;
+import jdk.internal.org.objectweb.asm.ClassWriter;
+import jdk.internal.org.objectweb.asm.ConstantDynamic;
+import jdk.internal.org.objectweb.asm.FieldVisitor;
+import jdk.internal.org.objectweb.asm.Handle;
+import jdk.internal.org.objectweb.asm.Label;
+import jdk.internal.org.objectweb.asm.MethodVisitor;
+import jdk.internal.org.objectweb.asm.Opcodes;
+import jdk.internal.org.objectweb.asm.RecordComponentVisitor;
+import jdk.internal.org.objectweb.asm.Type;
+import jdk.internal.org.objectweb.asm.TypePath;
+
+public class RedefineVerifyError implements Opcodes {
+
+    // This is a redefinition of java.lang.VerifyError with two broken init methods (no bytecodes)
+    public static byte[] dump () throws Exception {
+
+        ClassWriter classWriter = new ClassWriter(0);
+        FieldVisitor fieldVisitor;
+        RecordComponentVisitor recordComponentVisitor;
+        MethodVisitor methodVisitor;
+        AnnotationVisitor annotationVisitor0;
+
+        classWriter.visit(52, ACC_SUPER | ACC_PUBLIC, "java/lang/VerifyError", null, "java/lang/LinkageError", null);
+        {
+            fieldVisitor = classWriter.visitField(ACC_PRIVATE | ACC_FINAL | ACC_STATIC, "serialVersionUID", "J", null, new Long(7001962396098498785L));
+            fieldVisitor.visitEnd();
+        }
+        {
+            methodVisitor = classWriter.visitMethod(ACC_PUBLIC, "<init>", "()V", null, null);
+            methodVisitor.visitCode();
+            methodVisitor.visitEnd();
+        }
+
+        {
+            methodVisitor = classWriter.visitMethod(ACC_PUBLIC, "<init>", "(Ljava/lang/String;)V", null, null);
+            methodVisitor.visitCode();
+            classWriter.visitEnd();
+        }
+
+        return classWriter.toByteArray();
+    }
+
+    public static void main(String[] args) throws Exception {
+
+        Class<?> verifyErrorMirror = java.lang.VerifyError.class;
+
+        try {
+            // The Verifier is called for the redefinition, which will fail because of the broken <init> method above.
+            RedefineClassHelper.redefineClass(verifyErrorMirror, dump());
+            throw new RuntimeException("This should throw VerifyError");
+        } catch (VerifyError e) {
+            // JVMTI recreates the VerifyError so the verification message is lost.
+            System.out.println("Passed");
+        }
+    }
+}