8262913: KlassFactory::create_from_stream should never return NULL

Reviewed-by: hseigel, iklam

Author: coleenp

src/hotspot/share/classfile/classFileStream.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2021, 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
@@ -45,7 +45,9 @@ ClassFileStream::ClassFileStream(const u1* buffer,
   _current(buffer),
   _source(source),
   _need_verify(verify_stream),
-  _from_boot_loader_modules_image(from_boot_loader_modules_image) {}
+  _from_boot_loader_modules_image(from_boot_loader_modules_image) {
+    assert(buffer != NULL, "caller should throw NPE");
+}
 
 const u1* ClassFileStream::clone_buffer() const {
   u1* const new_buffer_start = NEW_RESOURCE_ARRAY(u1, length());

src/hotspot/share/classfile/klassFactory.cpp

@@ -205,10 +205,7 @@ InstanceKlass* KlassFactory::create_from_stream(ClassFileStream* stream,
 
   const ClassInstanceInfo* cl_inst_info = cl_info.class_hidden_info_ptr();
   InstanceKlass* result = parser.create_instance_klass(old_stream != stream, *cl_inst_info, CHECK_NULL);
-
-  if (result == NULL) {
-    return NULL;
-  }
+  assert(result != NULL, "result cannot be null with no pending exception");
 
   if (cached_class_file != NULL) {
     // JVMTI: we have an InstanceKlass now, tell it about the cached bytes

src/hotspot/share/classfile/systemDictionary.cpp

@@ -990,8 +990,9 @@ InstanceKlass* SystemDictionary::parse_stream(Symbol* class_name,
                                                       loader_data,
                                                       cl_info,
                                                       CHECK_NULL);
+  assert(k != NULL, "no klass created");
 
-  if ((cl_info.is_hidden() || is_unsafe_anon_class) && k != NULL) {
+  if (cl_info.is_hidden() || is_unsafe_anon_class) {
     // Hidden classes that are not strong and unsafe anonymous classes must update
     // ClassLoaderData holder so that they can be unloaded when the mirror is no
     // longer referenced.
@@ -1035,7 +1036,8 @@ InstanceKlass* SystemDictionary::parse_stream(Symbol* class_name,
 // JVM_DefineClass).
 // Note: class_name can be NULL. In that case we do not know the name of
 // the class until we have parsed the stream.
-
+// This function either returns an InstanceKlass or throws an exception.  It does
+// not return NULL without a pending exception.
 InstanceKlass* SystemDictionary::resolve_from_stream(Symbol* class_name,
                                                      Handle class_loader,
                                                      Handle protection_domain,
@@ -1068,9 +1070,6 @@ InstanceKlass* SystemDictionary::resolve_from_stream(Symbol* class_name,
 #endif
 
   if (k == NULL) {
-    if (st->buffer() == NULL) {
-      return NULL;
-    }
     ClassLoadInfo cl_info(protection_domain);
     k = KlassFactory::create_from_stream(st, class_name, loader_data, cl_info, CHECK_NULL);
   }

src/hotspot/share/prims/jni.cpp

@@ -288,7 +288,7 @@ JNI_ENTRY(jclass, jni_DefineClass(JNIEnv *env, const char *name, jobject loaderR
                                                    &st,
                                                    CHECK_NULL);
 
-  if (log_is_enabled(Debug, class, resolve) && k != NULL) {
+  if (log_is_enabled(Debug, class, resolve)) {
     trace_class_resolution(k);
   }
 

src/hotspot/share/prims/jvm.cpp

@@ -866,7 +866,7 @@ static jclass jvm_define_class_common(const char *name,
                                                    &st,
                                                    CHECK_NULL);
 
-  if (log_is_enabled(Debug, class, resolve) && k != NULL) {
+  if (log_is_enabled(Debug, class, resolve)) {
     trace_class_resolution(k);
   }
 
@@ -945,19 +945,17 @@ static jclass jvm_lookup_define_class(jclass lookup, const char *name,
   const char* source = is_nestmate ? host_class->external_name() : "__JVM_LookupDefineClass__";
   ClassFileStream st((u1*)buf, len, source, ClassFileStream::verify);
 
-  Klass* defined_k;
   InstanceKlass* ik = NULL;
   if (!is_hidden) {
-    defined_k = SystemDictionary::resolve_from_stream(class_name,
-                                                      class_loader,
-                                                      protection_domain,
-                                                      &st,
-                                                      CHECK_NULL);
-
-    if (log_is_enabled(Debug, class, resolve) && defined_k != NULL) {
-      trace_class_resolution(defined_k);
+    ik = SystemDictionary::resolve_from_stream(class_name,
+                                               class_loader,
+                                               protection_domain,
+                                               &st,
+                                               CHECK_NULL);
+
+    if (log_is_enabled(Debug, class, resolve)) {
+      trace_class_resolution(ik);
     }
-    ik = InstanceKlass::cast(defined_k);
   } else { // hidden
     Handle classData_h(THREAD, JNIHandles::resolve(classData));
     ClassLoadInfo cl_info(protection_domain,
@@ -968,16 +966,11 @@ static jclass jvm_lookup_define_class(jclass lookup, const char *name,
                           is_hidden,
                           is_strong,
                           vm_annotations);
-    defined_k = SystemDictionary::parse_stream(class_name,
-                                               class_loader,
-                                               &st,
-                                               cl_info,
-                                               CHECK_NULL);
-    if (defined_k == NULL) {
-      THROW_MSG_0(vmSymbols::java_lang_Error(), "Failure to define a hidden class");
-    }
-
-    ik = InstanceKlass::cast(defined_k);
+    ik = SystemDictionary::parse_stream(class_name,
+                                        class_loader,
+                                        &st,
+                                        cl_info,
+                                        CHECK_NULL);
 
     // The hidden class loader data has been artificially been kept alive to
     // this point. The mirror and any instances of this class have to keep
@@ -994,7 +987,7 @@ static jclass jvm_lookup_define_class(jclass lookup, const char *name,
                                   ik->is_hidden() ? "is hidden" : "is not hidden");
     }
   }
-  assert(Reflection::is_same_class_package(lookup_k, defined_k),
+  assert(Reflection::is_same_class_package(lookup_k, ik),
          "lookup class and defined class are in different packages");
 
   if (init) {
@@ -1003,7 +996,7 @@ static jclass jvm_lookup_define_class(jclass lookup, const char *name,
     ik->link_class(CHECK_NULL);
   }
 
-  return (jclass) JNIHandles::make_local(THREAD, defined_k->java_mirror());
+  return (jclass) JNIHandles::make_local(THREAD, ik->java_mirror());
 }
 
 JVM_ENTRY(jclass, JVM_DefineClass(JNIEnv *env, const char *name, jobject loader, const jbyte *buf, jsize len, jobject pd))

src/hotspot/share/prims/unsafe.cpp

@@ -862,16 +862,13 @@ Unsafe_DefineAnonymousClass_impl(JNIEnv *env,
                         false,    // is_strong_hidden
                         true);    // can_access_vm_annotations
 
-  Klass* anonk = SystemDictionary::parse_stream(no_class_name,
-                                                host_loader,
-                                                &st,
-                                                cl_info,
-                                                CHECK_NULL);
-  if (anonk == NULL) {
-    return NULL;
-  }
-
-  return InstanceKlass::cast(anonk);
+  InstanceKlass* anonk = SystemDictionary::parse_stream(no_class_name,
+                                                        host_loader,
+                                                        &st,
+                                                        cl_info,
+                                                        CHECK_NULL);
+  assert(anonk != NULL, "no klass created");
+  return anonk;
 }
 
 UNSAFE_ENTRY(jclass, Unsafe_DefineAnonymousClass0(JNIEnv *env, jobject unsafe, jclass host_class, jbyteArray data, jobjectArray cp_patches_jh)) {

test/hotspot/jtreg/runtime/DefineClass/A.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2021, 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.
+ */
+
+public class A { public A() { System.out.println("A called"); } }

test/hotspot/jtreg/runtime/DefineClass/NullClassBytesTest.java

@@ -0,0 +1,124 @@
+/*
+ * Copyright (c) 2021, 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 8262913
+ * @summary Verifies DefineClass with null or truncate bytes gets appropriate exception
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ * @compile A.java
+ * @run main/native NullClassBytesTest
+ */
+
+import java.io.*;
+
+public class NullClassBytesTest {
+
+    static native Class<?> nativeDefineClass(String name, ClassLoader ldr, byte[] class_bytes, int length);
+
+    static {
+        System.loadLibrary("NullClassBytesTest");
+    }
+
+    static class SimpleLoader extends ClassLoader {
+
+        public Class<?> loadClass(String name) throws ClassNotFoundException {
+            synchronized(getClassLoadingLock(name)) {
+                Class<?> c = findLoadedClass(name);
+                if (c != null) return c;
+
+                // load the class data from the connection
+                if (name.equals("A")) {
+                    byte[] b = getClassData("A");
+                    return defineClass(name, b, 0, b.length);
+                } else if (name.equals("B")) {
+                    byte[] b = new byte[0];
+                    return defineClass(name, b, 0, b.length);
+                } else if (name.equals("C")) {
+                    byte[] b = null;
+                    return defineClass(name, b, 0, 0);
+                } else if (name.equals("D")) {
+                    byte[] b = new byte[0];
+                    return nativeDefineClass(name, this, b, b.length);
+                } else if (name.equals("E")) {
+                    byte[] b = null;
+                    return nativeDefineClass(name, this, b, 0);
+                } else {
+                    return super.loadClass(name);
+                }
+            }
+        }
+
+        byte[] getClassData(String name) {
+           try {
+             return SimpleLoader.class.getClassLoader().getResourceAsStream(name + ".class").readAllBytes();
+           } catch (IOException e) {
+              return null;
+           }
+        }
+    }
+
+    public static void main(java.lang.String[] unused) throws Exception {
+        SimpleLoader ldr = new SimpleLoader();
+        Class<?> a = Class.forName("A", true, ldr);
+        Object obj = a.getConstructor().newInstance();
+
+        // If byte array points to null, the JVM throws ClassFormatError("Truncated class file")
+        try {
+            Class<?> b = Class.forName("B", true, ldr);
+        } catch (ClassFormatError cfe) {
+            if (!cfe.getMessage().contains("Truncated class file")) {
+                cfe.printStackTrace();
+                throw new RuntimeException("Wrong message");
+            }
+        }
+
+        // If byte array is null, ClassLoader native detects this and throws NPE
+        // before calling JVM_DefineClassWithSource
+        try {
+            Class<?> c = Class.forName("C", true, ldr);
+        } catch (NullPointerException npe) {}
+
+        // Test JNI_DefineClass with truncated bytes
+        try {
+            Class<?> c = Class.forName("D", true, ldr);
+        } catch (ClassFormatError cfe) {
+            if (!cfe.getMessage().contains("Truncated class file")) {
+                cfe.printStackTrace();
+                throw new RuntimeException("Wrong message");
+            }
+        }
+
+        // Native methods must throw their own NPE
+        try {
+            Class<?> c = Class.forName("E", true, ldr);
+        } catch (NullPointerException npe) {
+            if (!npe.getMessage().equals("class_bytes are null")) {
+                npe.printStackTrace();
+                throw new RuntimeException("Wrong message");
+            }
+        }
+        System.out.println("TEST PASSED");
+    }
+}

test/hotspot/jtreg/runtime/DefineClass/libNullClassBytesTest.c

@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2021, 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.
+ */
+
+#include <jni.h>
+
+JNIEXPORT void JNICALL
+Java_NullClassBytesTest_nativeDefineClass(JNIEnv *env, jclass klass, jstring className, jobject ldr,
+                                          jbyte* class_bytes, jint length) {
+    if (class_bytes == NULL) {
+        jclass cls = (*env)->FindClass(env, "java/lang/NullPointerException");
+
+        if (cls != 0) {
+            (*env)->ThrowNew(env, cls, "class_bytes are null");
+        }
+        return;
+    }
+    const char* c_name = (*env)->GetStringUTFChars(env, className, NULL);
+    (*env)->DefineClass(env, c_name, ldr, class_bytes, length);
+}