8195744: Avoid calling ClassLoader.checkPackageAccess if security man…

…ager is not installed

Reviewed-by: dholmes, iklam

src/hotspot/share/classfile/dictionary.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 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
@@ -132,12 +132,17 @@ bool Dictionary::resize_if_needed() {
   return (desired_size != 0);
 }
 
+bool DictionaryEntry::is_valid_protection_domain(Handle protection_domain) {
+
+  return protection_domain() == NULL || !java_lang_System::allow_security_manager()
+        ? true
+        : contains_protection_domain(protection_domain());
+}
+
 bool DictionaryEntry::contains_protection_domain(oop protection_domain) const {
-  // Lock the pd_set list.  This lock cannot safepoint since the caller holds
-  // a Dictionary entry, which can be moved if the Dictionary is resized.
-  MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
 #ifdef ASSERT
   if (protection_domain == instance_klass()->protection_domain()) {
+    MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
     // Ensure this doesn't show up in the pd_set (invariant)
     bool in_pd_set = false;
     for (ProtectionDomainEntry* current = pd_set();
@@ -160,10 +165,15 @@ bool DictionaryEntry::contains_protection_domain(oop protection_domain) const {
     return true;
   }
 
+  // Lock the pd_set list.  This lock cannot safepoint since the caller holds
+  // a Dictionary entry, which can be moved if the Dictionary is resized.
+  MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
   for (ProtectionDomainEntry* current = pd_set();
                               current != NULL;
                               current = current->next()) {
-    if (current->object_no_keepalive() == protection_domain) return true;
+    if (current->object_no_keepalive() == protection_domain) {
+      return true;
+    }
   }
   return false;
 }
@@ -283,6 +293,7 @@ DictionaryEntry* Dictionary::get_entry(int index, unsigned int hash,
 }
 
 
+
 InstanceKlass* Dictionary::find(unsigned int hash, Symbol* name,
                                 Handle protection_domain) {
   NoSafepointVerifier nsv;
@@ -307,11 +318,11 @@ InstanceKlass* Dictionary::find_class(unsigned int hash,
   return (entry != NULL) ? entry->instance_klass() : NULL;
 }
 
-
 void Dictionary::add_protection_domain(int index, unsigned int hash,
                                        InstanceKlass* klass,
                                        Handle protection_domain,
                                        TRAPS) {
+  assert(java_lang_System::allow_security_manager(), "only needed if security manager allowed");
   Symbol*  klass_name = klass->name();
   DictionaryEntry* entry = get_entry(index, hash, klass_name);
 

src/hotspot/share/classfile/dictionary.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 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
@@ -153,14 +153,7 @@ class DictionaryEntry : public HashtableEntry<InstanceKlass*, mtClass> {
   void set_pd_set(ProtectionDomainEntry* new_head) {  _pd_set = new_head; }
 
   // Tells whether the initiating class' protection domain can access the klass in this entry
-  bool is_valid_protection_domain(Handle protection_domain) {
-    if (!ProtectionDomainVerification) return true;
-
-    return protection_domain() == NULL
-         ? true
-         : contains_protection_domain(protection_domain());
-  }
-
+  inline bool is_valid_protection_domain(Handle protection_domain);
   void verify_protection_domain_set();
 
   bool equals(const Symbol* class_name) const {

src/hotspot/share/classfile/javaClasses.cpp

@@ -4391,18 +4391,41 @@ int java_lang_System::_static_in_offset;
 int java_lang_System::_static_out_offset;
 int java_lang_System::_static_err_offset;
 int java_lang_System::_static_security_offset;
+int java_lang_System::_static_allow_security_offset;
+int java_lang_System::_static_never_offset;
 
 #define SYSTEM_FIELDS_DO(macro) \
   macro(_static_in_offset,  k, "in",  input_stream_signature, true); \
   macro(_static_out_offset, k, "out", print_stream_signature, true); \
   macro(_static_err_offset, k, "err", print_stream_signature, true); \
-  macro(_static_security_offset, k, "security", security_manager_signature, true)
+  macro(_static_security_offset, k, "security", security_manager_signature, true); \
+  macro(_static_allow_security_offset, k, "allowSecurityManager", int_signature, true); \
+  macro(_static_never_offset, k, "NEVER", int_signature, true)
 
 void java_lang_System::compute_offsets() {
   InstanceKlass* k = vmClasses::System_klass();
   SYSTEM_FIELDS_DO(FIELD_COMPUTE_OFFSET);
 }
 
+// This field tells us that a security manager can never be installed so we
+// can completely skip populating the ProtectionDomainCacheTable.
+bool java_lang_System::allow_security_manager() {
+  static int initialized = false;
+  static bool allowed = true; // default
+  if (!initialized) {
+    oop base = vmClasses::System_klass()->static_field_base_raw();
+    int never = base->int_field(_static_never_offset);
+    allowed = (base->int_field(_static_allow_security_offset) != never);
+  }
+  return allowed;
+}
+
+// This field tells us that a security manager is installed.
+bool java_lang_System::has_security_manager() {
+  oop base = vmClasses::System_klass()->static_field_base_raw();
+  return base->obj_field(_static_security_offset) != NULL;
+}
+
 #if INCLUDE_CDS
 void java_lang_System::serialize_offsets(SerializeClosure* f) {
    SYSTEM_FIELDS_DO(FIELD_SERIALIZE_OFFSET);

src/hotspot/share/classfile/javaClasses.hpp

@@ -1372,11 +1372,15 @@ class java_lang_System : AllStatic {
   static int _static_out_offset;
   static int _static_err_offset;
   static int _static_security_offset;
+  static int _static_allow_security_offset;
+  static int _static_never_offset;
 
  public:
   static int  in_offset() { CHECK_INIT(_static_in_offset); }
   static int out_offset() { CHECK_INIT(_static_out_offset); }
   static int err_offset() { CHECK_INIT(_static_err_offset); }
+  static bool allow_security_manager();
+  static bool has_security_manager();
 
   static void compute_offsets();
   static void serialize_offsets(SerializeClosure* f) NOT_CDS_RETURN;

src/hotspot/share/classfile/protectionDomainCache.cpp

@@ -25,6 +25,7 @@
 #include "precompiled.hpp"
 #include "classfile/classLoaderDataGraph.hpp"
 #include "classfile/dictionary.hpp"
+#include "classfile/javaClasses.hpp"
 #include "classfile/protectionDomainCache.hpp"
 #include "logging/log.hpp"
 #include "logging/logStream.hpp"
@@ -66,6 +67,9 @@ class CleanProtectionDomainEntries : public CLDClosure {
 };
 
 void ProtectionDomainCacheTable::unlink() {
+  // The dictionary entries _pd_set field should be null also, so nothing to do.
+  assert(java_lang_System::allow_security_manager(), "should not be called otherwise");
+
   {
     // First clean cached pd lists in loaded CLDs
     // It's unlikely, but some loaded classes in a dictionary might

src/hotspot/share/classfile/systemDictionary.cpp

@@ -460,54 +460,54 @@ void SystemDictionary::validate_protection_domain(InstanceKlass* klass,
                                                   Handle protection_domain,
                                                   TRAPS) {
   // Now we have to call back to java to check if the initating class has access
-  JavaValue result(T_VOID);
-  LogTarget(Debug, protectiondomain) lt;
-  if (lt.is_enabled()) {
-    ResourceMark rm(THREAD);
-    // Print out trace information
-    LogStream ls(lt);
-    ls.print_cr("Checking package access");
-    if (class_loader() != NULL) {
+  assert(class_loader() != NULL, "Should not call this");
+  assert(protection_domain() != NULL, "Should not call this");
+
+  // We only have to call checkPackageAccess if there's a security manager installed.
+  if (java_lang_System::has_security_manager()) {
+
+    // This handle and the class_loader handle passed in keeps this class from
+    // being unloaded through several GC points.
+    // The class_loader handle passed in is the initiating loader.
+    Handle mirror(THREAD, klass->java_mirror());
+
+    InstanceKlass* system_loader = vmClasses::ClassLoader_klass();
+    JavaValue result(T_VOID);
+    JavaCalls::call_special(&result,
+                           class_loader,
+                           system_loader,
+                           vmSymbols::checkPackageAccess_name(),
+                           vmSymbols::class_protectiondomain_signature(),
+                           mirror,
+                           protection_domain,
+                           THREAD);
+
+    LogTarget(Debug, protectiondomain) lt;
+    if (lt.is_enabled()) {
+      ResourceMark rm(THREAD);
+      // Print out trace information
+      LogStream ls(lt);
+      ls.print_cr("Checking package access");
       ls.print("class loader: ");
       class_loader()->print_value_on(&ls);
-    } else {
-      ls.print_cr("class loader: NULL");
-    }
-    if (protection_domain() != NULL) {
       ls.print(" protection domain: ");
       protection_domain()->print_value_on(&ls);
-    } else {
-      ls.print_cr(" protection domain: NULL");
+      ls.print(" loading: "); klass->print_value_on(&ls);
+      if (HAS_PENDING_EXCEPTION) {
+        ls.print_cr(" DENIED !!!!!!!!!!!!!!!!!!!!!");
+      } else {
+        ls.print_cr(" granted");
+      }
     }
-    ls.print(" loading: "); klass->print_value_on(&ls);
-    ls.cr();
-  }
-
-  // This handle and the class_loader handle passed in keeps this class from
-  // being unloaded through several GC points.
-  // The class_loader handle passed in is the initiating loader.
-  Handle mirror(THREAD, klass->java_mirror());
 
-  InstanceKlass* system_loader = vmClasses::ClassLoader_klass();
-  JavaCalls::call_special(&result,
-                         class_loader,
-                         system_loader,
-                         vmSymbols::checkPackageAccess_name(),
-                         vmSymbols::class_protectiondomain_signature(),
-                         mirror,
-                         protection_domain,
-                         THREAD);
-
-  if (HAS_PENDING_EXCEPTION) {
-    log_debug(protectiondomain)("DENIED !!!!!!!!!!!!!!!!!!!!!");
-  } else {
-   log_debug(protectiondomain)("granted");
+    if (HAS_PENDING_EXCEPTION) return;
   }
 
-  if (HAS_PENDING_EXCEPTION) return;
-
   // If no exception has been thrown, we have validated the protection domain
   // Insert the protection domain of the initiating class into the set.
+  // We still have to add the protection_domain to the dictionary in case a new
+  // security manager is installed later. Calls to load the same class with class loader
+  // and protection domain are expected to succeed.
   {
     ClassLoaderData* loader_data = class_loader_data(class_loader);
     Dictionary* dictionary = loader_data->dictionary();
@@ -889,18 +889,15 @@ InstanceKlass* SystemDictionary::resolve_instance_class_or_null(Symbol* name,
   // Make sure we have the right class in the dictionary
   DEBUG_ONLY(verify_dictionary_entry(name, loaded_class));
 
-  // return if the protection domain in NULL
-  if (protection_domain() == NULL) return loaded_class;
-
-  // Check the protection domain has the right access
-  if (dictionary->is_valid_protection_domain(name_hash, name,
+  // Check if the protection domain is present it has the right access
+  if (protection_domain() != NULL &&
+     java_lang_System::allow_security_manager() &&
+     !dictionary->is_valid_protection_domain(name_hash, name,
                                              protection_domain)) {
-    return loaded_class;
+    // Verify protection domain. If it fails an exception is thrown
+    validate_protection_domain(loaded_class, class_loader, protection_domain, CHECK_NULL);
   }
 
-  // Verify protection domain. If it fails an exception is thrown
-  validate_protection_domain(loaded_class, class_loader, protection_domain, CHECK_NULL);
-
   return loaded_class;
 }
 
@@ -1773,12 +1770,16 @@ bool SystemDictionary::do_unloading(GCTimer* gc_timer) {
   if (unloading_occurred) {
     SymbolTable::trigger_cleanup();
 
-    // Oops referenced by the protection domain cache table may get unreachable independently
-    // of the class loader (eg. cached protection domain oops). So we need to
-    // explicitly unlink them here.
-    // All protection domain oops are linked to the caller class, so if nothing
-    // unloads, this is not needed.
-    _pd_cache_table->trigger_cleanup();
+    if (java_lang_System::allow_security_manager()) {
+      // Oops referenced by the protection domain cache table may get unreachable independently
+      // of the class loader (eg. cached protection domain oops). So we need to
+      // explicitly unlink them here.
+      // All protection domain oops are linked to the caller class, so if nothing
+      // unloads, this is not needed.
+      _pd_cache_table->trigger_cleanup();
+    } else {
+      assert(_pd_cache_table->number_of_entries() == 0, "should be empty");
+    }
   }
 
   return unloading_occurred;

src/hotspot/share/runtime/globals.hpp

@@ -677,9 +677,6 @@ const intx ObjectAlignmentInBytes = 8;
   develop(bool, UsePrivilegedStack, true,                                   \
           "Enable the security JVM functions")                              \
                                                                             \
-  develop(bool, ProtectionDomainVerification, true,                         \
-          "Verify protection domain before resolution in system dictionary")\
-                                                                            \
   product(bool, ClassUnloading, true,                                       \
           "Do unloading of classes")                                        \
                                                                             \

test/hotspot/jtreg/runtime/logging/ProtectionDomainVerificationTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -40,22 +40,38 @@ public static void main(String... args) throws Exception {
         // -Xlog:protectiondomain=trace
         ProcessBuilder pb = ProcessTools.createJavaProcessBuilder("-Xlog:protectiondomain=trace",
                                                                   "-Xmx128m",
-                                                                  Hello.class.getName());
-        OutputAnalyzer out = new OutputAnalyzer(pb.start());
-        out.shouldContain("[protectiondomain] Checking package access");
-        out.shouldContain("[protectiondomain] pd set count = #");
+                                                                  Hello.class.getName(), "security_manager");
+        new OutputAnalyzer(pb.start())
+        .shouldHaveExitValue(0)
+        .shouldContain("[protectiondomain] Checking package access")
+        .shouldContain("[protectiondomain] pd set count = #");
 
         // -Xlog:protectiondomain=debug
         pb = ProcessTools.createJavaProcessBuilder("-Xlog:protectiondomain=debug",
                                                                   "-Xmx128m",
-                                                                  Hello.class.getName());
-        out = new OutputAnalyzer(pb.start());
-        out.shouldContain("[protectiondomain] Checking package access");
-        out.shouldNotContain("pd set count = #");
+                                                                  Hello.class.getName(), "security_manager");
+        new OutputAnalyzer(pb.start())
+        .shouldHaveExitValue(0)
+        .shouldContain("[protectiondomain] Checking package access")
+        .shouldNotContain("pd set count = #");
+
+        // -Xlog:protectiondomain=debug
+        pb = ProcessTools.createJavaProcessBuilder("-Xlog:protectiondomain=trace",
+                                                   "-Xmx128m",
+                                                   "-Djava.security.manager=disallow",
+                                                   Hello.class.getName());
+        new OutputAnalyzer(pb.start())
+        .shouldHaveExitValue(0)
+        .shouldNotContain("[protectiondomain] Checking package access")
+        .shouldNotContain("pd set count = #");
     }
 
     public static class Hello {
         public static void main(String[] args) {
+            if (args.length == 1) {
+              // Need a security manager to trigger logging.
+              System.setSecurityManager(new SecurityManager());
+            }
             System.out.print("Hello!");
         }
     }