8153858: Clean up needed when obtaining the package name from a fully qualified class name
authorrprotacio
Tue, 07 Jun 2016 11:39:47 -0400
changeset 39216 40c3d66352ae
parent 39212 e46a559b4503
child 39217 ad613683c889
8153858: Clean up needed when obtaining the package name from a fully qualified class name Summary: Consolidated and refactored code parsing fully qualified names. Includes gtest. Reviewed-by: dholmes, coleenp
hotspot/src/share/vm/classfile/classLoader.cpp
hotspot/src/share/vm/classfile/classLoader.hpp
hotspot/src/share/vm/classfile/systemDictionary.cpp
hotspot/src/share/vm/classfile/systemDictionaryShared.hpp
hotspot/src/share/vm/oops/instanceKlass.cpp
hotspot/src/share/vm/oops/instanceKlass.hpp
hotspot/src/share/vm/oops/method.hpp
hotspot/test/native/runtime/test_classLoader.cpp
hotspot/test/native/runtime/test_instanceKlass.cpp
--- a/hotspot/src/share/vm/classfile/classLoader.cpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/classfile/classLoader.cpp	Tue Jun 07 11:39:47 2016 -0400
@@ -181,26 +181,59 @@
 }
 
 // Used to obtain the package name from a fully qualified class name.
-// It is the responsibility of the caller to establish ResourceMark.
-const char* ClassLoader::package_from_name(const char* class_name) {
-  const char* last_slash = strrchr(class_name, '/');
+// It is the responsibility of the caller to establish a ResourceMark.
+const char* ClassLoader::package_from_name(const char* const class_name, bool* bad_class_name) {
+  if (class_name == NULL) {
+    if (bad_class_name != NULL) {
+      *bad_class_name = true;
+    }
+    return NULL;
+  }
+
+  if (bad_class_name != NULL) {
+    *bad_class_name = false;
+  }
+
+  const char* const last_slash = strrchr(class_name, '/');
   if (last_slash == NULL) {
     // No package name
     return NULL;
   }
-  int length = last_slash - class_name;
+
+  char* class_name_ptr = (char*) class_name;
+  // Skip over '['s
+  if (*class_name_ptr == '[') {
+    do {
+      class_name_ptr++;
+    } while (*class_name_ptr == '[');
 
-  // A class name could have just the slash character in the name,
-  // resulting in a negative length.
+    // Fully qualified class names should not contain a 'L'.
+    // Set bad_class_name to true to indicate that the package name
+    // could not be obtained due to an error condition.
+    // In this situation, is_same_class_package returns false.
+    if (*class_name_ptr == 'L') {
+      if (bad_class_name != NULL) {
+        *bad_class_name = true;
+      }
+      return NULL;
+    }
+  }
+
+  int length = last_slash - class_name_ptr;
+
+  // A class name could have just the slash character in the name.
   if (length <= 0) {
     // No package name
+    if (bad_class_name != NULL) {
+      *bad_class_name = true;
+    }
     return NULL;
   }
 
   // drop name after last slash (including slash)
   // Ex., "java/lang/String.class" => "java/lang"
   char* pkg_name = NEW_RESOURCE_ARRAY(char, length + 1);
-  strncpy(pkg_name, class_name, length);
+  strncpy(pkg_name, class_name_ptr, length);
   *(pkg_name+length) = '\0';
 
   return (const char *)pkg_name;
@@ -1117,13 +1150,11 @@
   assert(fullq_class_name != NULL, "just checking");
 
   // Get package name from fully qualified class name.
-  const char *cp = strrchr(fullq_class_name, '/');
+  ResourceMark rm;
+  const char *cp = package_from_name(fullq_class_name);
   if (cp != NULL) {
-    int len = cp - fullq_class_name;
-    PackageEntryTable* pkg_entry_tbl =
-      ClassLoaderData::the_null_class_loader_data()->packages();
-    TempNewSymbol pkg_symbol =
-      SymbolTable::new_symbol(fullq_class_name, len, CHECK_false);
+    PackageEntryTable* pkg_entry_tbl = ClassLoaderData::the_null_class_loader_data()->packages();
+    TempNewSymbol pkg_symbol = SymbolTable::new_symbol(cp, CHECK_false);
     PackageEntry* pkg_entry = pkg_entry_tbl->lookup_only(pkg_symbol);
     if (pkg_entry != NULL) {
       assert(classpath_index != -1, "Unexpected classpath_index");
@@ -1231,11 +1262,9 @@
   // jimage, it is determined by the class path entry.
   jshort loader_type = ClassLoader::APP_LOADER;
   if (e->is_jrt()) {
-    int length = 0;
-    const jbyte* pkg_string = InstanceKlass::package_from_name(class_name, length);
-    if (pkg_string != NULL) {
-      ResourceMark rm;
-      TempNewSymbol pkg_name = SymbolTable::new_symbol((const char*)pkg_string, length, THREAD);
+    ResourceMark rm;
+    TempNewSymbol pkg_name = InstanceKlass::package_from_name(class_name, CHECK_0);
+    if (pkg_name != NULL) {
       const char* pkg_name_C_string = (const char*)(pkg_name->as_C_string());
       ClassPathImageEntry* cpie = (ClassPathImageEntry*)e;
       JImageFile* jimage = cpie->jimage();
--- a/hotspot/src/share/vm/classfile/classLoader.hpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/classfile/classLoader.hpp	Tue Jun 07 11:39:47 2016 -0400
@@ -444,7 +444,9 @@
   static bool string_ends_with(const char* str, const char* str_to_find);
 
   // obtain package name from a fully qualified class name
-  static const char* package_from_name(const char* class_name);
+  // *bad_class_name is set to true if there's a problem with parsing class_name, to
+  // distinguish from a class_name with no package name, as both cases have a NULL return value
+  static const char* package_from_name(const char* const class_name, bool* bad_class_name = NULL);
 
   static bool is_jrt(const char* name) { return string_ends_with(name, MODULES_IMAGE_NAME); }
 
--- a/hotspot/src/share/vm/classfile/systemDictionary.cpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/classfile/systemDictionary.cpp	Tue Jun 07 11:39:47 2016 -0400
@@ -70,6 +70,7 @@
 #include "services/threadService.hpp"
 #include "trace/traceMacros.hpp"
 #include "utilities/macros.hpp"
+#include "utilities/stringUtils.hpp"
 #include "utilities/ticks.hpp"
 #if INCLUDE_CDS
 #include "classfile/sharedClassUtil.hpp"
@@ -1154,12 +1155,10 @@
     // It is illegal to define classes in the "java." package from
     // JVM_DefineClass or jni_DefineClass unless you're the bootclassloader
     ResourceMark rm(THREAD);
-    char* name = parsed_name->as_C_string();
-    char* index = strrchr(name, '/');
-    *index = '\0'; // chop to just the package name
-    while ((index = strchr(name, '/')) != NULL) {
-      *index = '.'; // replace '/' with '.' in package name
-    }
+    TempNewSymbol pkg_name = InstanceKlass::package_from_name(parsed_name, CHECK_NULL);
+    assert(pkg_name != NULL, "Error in parsing package name starting with 'java/'");
+    char* name = pkg_name->as_C_string();
+    StringUtils::replace_no_expand(name, "/", ".");
     const char* msg_text = "Prohibited package name: ";
     size_t len = strlen(msg_text) + strlen(name) + 1;
     char* message = NEW_RESOURCE_ARRAY(char, len);
@@ -1257,6 +1256,7 @@
 bool SystemDictionary::is_shared_class_visible(Symbol* class_name,
                                                instanceKlassHandle ik,
                                                Handle class_loader, TRAPS) {
+  ResourceMark rm;
   int path_index = ik->shared_classpath_index();
   SharedClassPathEntry* ent =
             (SharedClassPathEntry*)FileMapInfo::shared_classpath(path_index);
@@ -1270,12 +1270,11 @@
   TempNewSymbol pkg_name = NULL;
   PackageEntry* pkg_entry = NULL;
   ModuleEntry* mod_entry = NULL;
-  int length = 0;
+  const char* pkg_string = NULL;
   ClassLoaderData* loader_data = class_loader_data(class_loader);
-  const jbyte* pkg_string = InstanceKlass::package_from_name(class_name, length);
-  if (pkg_string != NULL) {
-    pkg_name = SymbolTable::new_symbol((const char*)pkg_string,
-                                       length, CHECK_(false));
+  pkg_name = InstanceKlass::package_from_name(class_name, CHECK_false);
+  if (pkg_name != NULL) {
+    pkg_string = pkg_name->as_C_string();
     if (loader_data != NULL) {
       pkg_entry = loader_data->packages()->lookup_only(pkg_name);
     }
@@ -1432,15 +1431,14 @@
   instanceKlassHandle nh = instanceKlassHandle(); // null Handle
 
   if (class_loader.is_null()) {
-    int length = 0;
+    ResourceMark rm;
     PackageEntry* pkg_entry = NULL;
     bool search_only_bootloader_append = false;
     ClassLoaderData *loader_data = class_loader_data(class_loader);
 
     // Find the package in the boot loader's package entry table.
-    const jbyte* pkg_string = InstanceKlass::package_from_name(class_name, length);
-    if (pkg_string != NULL) {
-      TempNewSymbol pkg_name = SymbolTable::new_symbol((const char*)pkg_string, length, CHECK_(nh));
+    TempNewSymbol pkg_name = InstanceKlass::package_from_name(class_name, CHECK_NULL);
+    if (pkg_name != NULL) {
       pkg_entry = loader_data->packages()->lookup_only(pkg_name);
     }
 
@@ -1477,7 +1475,7 @@
       assert(!DumpSharedSpaces, "Archive dumped after module system initialization");
       // After the module system has been initialized, check if the class'
       // package is in a module defined to the boot loader.
-      if (pkg_string == NULL || pkg_entry == NULL || pkg_entry->in_unnamed_module()) {
+      if (pkg_name == NULL || pkg_entry == NULL || pkg_entry->in_unnamed_module()) {
         // Class is either in the unnamed package, in a named package
         // within a module not defined to the boot loader or in a
         // a named package within the unnamed module.  In all cases,
--- a/hotspot/src/share/vm/classfile/systemDictionaryShared.hpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/classfile/systemDictionaryShared.hpp	Tue Jun 07 11:39:47 2016 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2016, 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
@@ -48,7 +48,7 @@
   static bool is_shared_class_visible_for_classloader(
                                       instanceKlassHandle ik,
                                       Handle class_loader,
-                                      const jbyte* pkg_string,
+                                      const char* pkg_string,
                                       Symbol* pkg_name,
                                       PackageEntry* pkg_entry,
                                       ModuleEntry* mod_entry,
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp	Tue Jun 07 11:39:47 2016 -0400
@@ -2182,39 +2182,21 @@
   return dest;
 }
 
-const jbyte* InstanceKlass::package_from_name(const Symbol* name, int& length) {
-  ResourceMark rm;
-  length = 0;
+// Used to obtain the package name from a fully qualified class name.
+Symbol* InstanceKlass::package_from_name(const Symbol* name, TRAPS) {
   if (name == NULL) {
     return NULL;
   } else {
-    const jbyte* base_name = name->base();
-    const jbyte* last_slash = UTF8::strrchr(base_name, name->utf8_length(), '/');
-
-    if (last_slash == NULL) {
-      // No package name
+    if (name->utf8_length() <= 0) {
       return NULL;
-    } else {
-      // Skip over '['s
-      if (*base_name == '[') {
-        do {
-          base_name++;
-        } while (*base_name == '[');
-        if (*base_name != 'L') {
-          // Fully qualified class names should not contain a 'L'.
-          // Set length to -1 to indicate that the package name
-          // could not be obtained due to an error condition.
-          // In this situtation, is_same_class_package returns false.
-          length = -1;
-          return NULL;
-        }
-      }
-
-      // Found the package name, look it up in the symbol table.
-      length = last_slash - base_name;
-      assert(length > 0, "Bad length for package name");
-      return base_name;
     }
+    ResourceMark rm;
+    const char* package_name = ClassLoader::package_from_name((const char*) name->as_C_string());
+    if (package_name == NULL) {
+      return NULL;
+    }
+    Symbol* pkg_name = SymbolTable::new_symbol(package_name, THREAD);
+    return pkg_name;
   }
 }
 
@@ -2230,12 +2212,9 @@
 }
 
 void InstanceKlass::set_package(ClassLoaderData* loader_data, TRAPS) {
-  int length = 0;
-  const jbyte* base_name = package_from_name(name(), length);
-
-  if (base_name != NULL && loader_data != NULL) {
-    TempNewSymbol pkg_name = SymbolTable::new_symbol((const char*)base_name, length, CHECK);
-
+  TempNewSymbol pkg_name = package_from_name(name(), CHECK);
+
+  if (pkg_name != NULL && loader_data != NULL) {
     // Find in class loader's package entry table.
     _package_entry = loader_data->packages()->lookup_only(pkg_name);
 
@@ -2331,20 +2310,18 @@
   if (class_loader1 != class_loader2) {
     return false;
   } else if (class_name1 == class_name2) {
-    return true;                // skip painful bytewise comparison
+    return true;
   } else {
     ResourceMark rm;
 
-    // The Symbol*'s are in UTF8 encoding. Since we only need to check explicitly
-    // for ASCII characters ('/', 'L', '['), we can keep them in UTF8 encoding.
-    // Otherwise, we just compare jbyte values between the strings.
-    int length1 = 0;
-    int length2 = 0;
-    const jbyte *name1 = package_from_name(class_name1, length1);
-    const jbyte *name2 = package_from_name(class_name2, length2);
-
-    if ((length1 < 0) || (length2 < 0)) {
-      // error occurred parsing package name.
+    bool bad_class_name = false;
+    const char* name1 = ClassLoader::package_from_name((const char*) class_name1->as_C_string(), &bad_class_name);
+    if (bad_class_name) {
+      return false;
+    }
+
+    const char* name2 = ClassLoader::package_from_name((const char*) class_name2->as_C_string(), &bad_class_name);
+    if (bad_class_name) {
       return false;
     }
 
@@ -2354,13 +2331,13 @@
       return name1 == name2;
     }
 
-    // Check that package part is identical
-    return UTF8::equal(name1, length1, name2, length2);
+    // Check that package is identical
+    return (strcmp(name1, name2) == 0);
   }
 }
 
 // Returns true iff super_method can be overridden by a method in targetclassname
-// See JSL 3rd edition 8.4.6.1
+// See JLS 3rd edition 8.4.6.1
 // Assumes name-signature match
 // "this" is InstanceKlass of super_method which must exist
 // note that the InstanceKlass of the method in the targetclassname has not always been created yet
--- a/hotspot/src/share/vm/oops/instanceKlass.hpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp	Tue Jun 07 11:39:47 2016 -0400
@@ -1108,7 +1108,7 @@
 
   // Naming
   const char* signature_name() const;
-  static const jbyte* package_from_name(const Symbol* name, int& length);
+  static Symbol* package_from_name(const Symbol* name, TRAPS);
 
   // GC specific object visitors
   //
--- a/hotspot/src/share/vm/oops/method.hpp	Tue Jun 07 00:57:23 2016 +0000
+++ b/hotspot/src/share/vm/oops/method.hpp	Tue Jun 07 11:39:47 2016 -0400
@@ -246,7 +246,7 @@
   int code_size() const                  { return constMethod()->code_size(); }
 
   // method size in words
-  int method_size() const                { return sizeof(Method)/wordSize + is_native() ? 2 : 0; }
+  int method_size() const                { return sizeof(Method)/wordSize + ( is_native() ? 2 : 0 ); }
 
   // constant pool for Klass* holding this method
   ConstantPool* constants() const              { return constMethod()->constants(); }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/native/runtime/test_classLoader.cpp	Tue Jun 07 11:39:47 2016 -0400
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2016, 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 "classfile/classLoader.hpp"
+#include "memory/resourceArea.hpp"
+#include "unittest.hpp"
+
+// Tests ClassLoader::package_from_name()
+TEST_VM(classLoader, null_class_name) {
+  ResourceMark rm;
+  bool bad_class_name = false;
+  const char* retval= ClassLoader::package_from_name(NULL, &bad_class_name);
+  ASSERT_TRUE(bad_class_name) << "Function did not set bad_class_name with NULL class name";
+  ASSERT_STREQ(retval, NULL) << "Wrong package for NULL class name pointer";
+}
+
+TEST_VM(classLoader, empty_class_name) {
+  ResourceMark rm;
+  const char* retval = ClassLoader::package_from_name("");
+  ASSERT_STREQ(retval, NULL) << "Wrong package for empty string";
+}
+
+TEST_VM(classLoader, no_slash) {
+  ResourceMark rm;
+  const char* retval = ClassLoader::package_from_name("L");
+  ASSERT_STREQ(retval, NULL) << "Wrong package for class with no slashes";
+}
+
+TEST_VM(classLoader, just_slash) {
+  ResourceMark rm;
+  bool bad_class_name = false;
+  const char* retval = ClassLoader::package_from_name("/", &bad_class_name);
+  ASSERT_TRUE(bad_class_name) << "Function did not set bad_class_name with package of length 0";
+  ASSERT_STREQ(retval, NULL) << "Wrong package for class with just slash";
+}
+
+TEST_VM(classLoader, multiple_slashes) {
+  ResourceMark rm;
+  const char* retval = ClassLoader::package_from_name("///");
+  ASSERT_STREQ(retval, "//") << "Wrong package for class with just slashes";
+}
+
+TEST_VM(classLoader, standard_case_1) {
+  ResourceMark rm;
+  bool bad_class_name = true;
+  const char* retval = ClassLoader::package_from_name("package/class", &bad_class_name);
+  ASSERT_FALSE(bad_class_name) << "Function did not reset bad_class_name";
+  ASSERT_STREQ(retval, "package") << "Wrong package for class with one slash";
+}
+
+TEST_VM(classLoader, standard_case_2) {
+  ResourceMark rm;
+  const char* retval = ClassLoader::package_from_name("package/folder/class");
+  ASSERT_STREQ(retval, "package/folder") << "Wrong package for class with multiple slashes";
+}
+
+TEST_VM(classLoader, class_array) {
+  ResourceMark rm;
+  bool bad_class_name = false;
+  const char* retval = ClassLoader::package_from_name("[package/class", &bad_class_name);
+  ASSERT_FALSE(bad_class_name) << "Function set bad_class_name with class array";
+  ASSERT_STREQ(retval, "package") << "Wrong package for class with leading bracket";
+}
+
+TEST_VM(classLoader, class_object_array) {
+  ResourceMark rm;
+  bool bad_class_name = false;
+  const char* retval = ClassLoader::package_from_name("[Lpackage/class", &bad_class_name);
+  ASSERT_TRUE(bad_class_name) << "Function did not set bad_class_name with array of class objects";
+  ASSERT_STREQ(retval, NULL) << "Wrong package for class with leading '[L'";
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/native/runtime/test_instanceKlass.cpp	Tue Jun 07 11:39:47 2016 -0400
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2016, 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 "classfile/symbolTable.hpp"
+#include "memory/resourceArea.hpp"
+#include "oops/instanceKlass.hpp"
+#include "unittest.hpp"
+
+// Tests InstanceKlass::package_from_name()
+TEST_VM(instanceKlass, null_symbol) {
+  ResourceMark rm;
+  TempNewSymbol package_sym = InstanceKlass::package_from_name(NULL, NULL);
+  ASSERT_TRUE(package_sym == NULL) << "Wrong package for NULL symbol";
+}