8171971: Fix timing bug in JVM management of package export lists

Reduce the number of fields that maintain export state and use Module_lock to access these fields

Reviewed-by: acorn, sspitsyn, lfoltan

hotspot/src/share/vm/classfile/modules.cpp

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+* Copyright (c) 2016, 2017, 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
@@ -823,10 +823,7 @@ void Modules::add_module_exports_to_all_unnamed(jobject module, const char* pack
                        package_entry->name()->as_C_string(),
                        module_entry->name()->as_C_string());
 
-    // Mark package as exported to all unnamed modules, unless already
+    // Mark package as exported to all unnamed modules.
-    // unqualifiedly exported.
-    if (!package_entry->is_unqual_exported()) {
     package_entry->set_is_exported_allUnnamed();
   }
 }
-}

hotspot/src/share/vm/classfile/packageEntry.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2017, 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
@@ -37,8 +37,8 @@
 
 // Returns true if this package specifies m as a qualified export, including through an unnamed export
 bool PackageEntry::is_qexported_to(ModuleEntry* m) const {
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
   assert(m != NULL, "No module to lookup in this package's qualified exports list");
-  MutexLocker m1(Module_lock);
   if (is_exported_allUnnamed() && !m->is_named()) {
     return true;
   } else if (!has_qual_exports_list()) {
@@ -98,15 +98,8 @@ void PackageEntry::set_exported(ModuleEntry* m) {
   }
 
   if (m == NULL) {
-    // NULL indicates the package is being unqualifiedly exported
+    // NULL indicates the package is being unqualifiedly exported.  Clean up
-    if (has_qual_exports_list()) {
+    // the qualified list at the next safepoint.
-      // Legit to transition a package from being qualifiedly exported
-      // to unqualified.  Clean up the qualified lists at the next
-      // safepoint.
-      _exported_pending_delete = _qualified_exports;
-    }
-
-    // Mark package as unqualifiedly exported
     set_unqual_exported();
 
   } else {
@@ -115,14 +108,19 @@ void PackageEntry::set_exported(ModuleEntry* m) {
   }
 }
 
+// Set the package as exported to all unnamed modules unless the package is
+// already unqualifiedly exported.
 void PackageEntry::set_is_exported_allUnnamed() {
   MutexLocker m1(Module_lock);
   if (!is_unqual_exported()) {
-   _is_exported_allUnnamed = true;
+   _export_flags = PKG_EXP_ALLUNNAMED;
   }
 }
 
-// Remove dead module entries within the package's exported list.
+// Remove dead module entries within the package's exported list.  Note that
+// if all of the modules on the _qualified_exports get purged the list does not
+// get deleted.  This prevents the package from illegally transitioning from
+// exported to non-exported.
 void PackageEntry::purge_qualified_exports() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
   if (_must_walk_exports &&
@@ -160,18 +158,9 @@ void PackageEntry::purge_qualified_exports() {
 
 void PackageEntry::delete_qualified_exports() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
-  if (_exported_pending_delete != NULL) {
-    // If a transition occurred from qualified to unqualified, the _qualified_exports
-    // field should have been NULL'ed out.
-    assert(_qualified_exports == NULL, "Package's exported pending delete, exported list should not be active");
-    delete _exported_pending_delete;
-  }
-
   if (_qualified_exports != NULL) {
     delete _qualified_exports;
   }
-
-  _exported_pending_delete = NULL;
   _qualified_exports = NULL;
 }
 
@@ -314,6 +303,11 @@ void PackageEntry::package_exports_do(ModuleClosure* const f) {
   }
 }
 
+bool PackageEntry::exported_pending_delete() const {
+  assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
+  return (is_unqual_exported() && _qualified_exports != NULL);
+}
+
 // Remove dead entries from all packages' exported list
 void PackageEntryTable::purge_all_package_exports() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
@@ -344,13 +338,17 @@ void PackageEntryTable::print(outputStream* st) {
   }
 }
 
+// This function may be called from debuggers so access private fields directly
+// to prevent triggering locking-related asserts that could result from calling
+// getter methods.
 void PackageEntry::print(outputStream* st) {
   ResourceMark rm;
   st->print_cr("package entry " PTR_FORMAT " name %s module %s classpath_index "
                INT32_FORMAT " is_exported_unqualified %d is_exported_allUnnamed %d " "next " PTR_FORMAT,
                p2i(this), name()->as_C_string(),
                (module()->is_named() ? module()->name()->as_C_string() : UNNAMED_MODULE),
-               _classpath_index, _is_exported_unqualified, _is_exported_allUnnamed, p2i(next()));
+               _classpath_index, _export_flags == PKG_EXP_UNQUALIFIED,
+               _export_flags == PKG_EXP_ALLUNNAMED, p2i(next()));
 }
 
 void PackageEntryTable::verify() {

hotspot/src/share/vm/classfile/packageEntry.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2017, 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
@@ -34,8 +34,8 @@
 // A PackageEntry basically represents a Java package.  It contains:
 //   - Symbol* containing the package's name.
 //   - ModuleEntry* for this package's containing module.
-//   - a flag indicating if package is exported unqualifiedly
+//   - a field indicating if the package is exported unqualifiedly or to all
-//   - a flag indicating if this package is exported to all unnamed modules.
+//     unnamed modules.
 //   - a growable array containing other module entries that this
 //     package is exported to.
 //
@@ -44,9 +44,9 @@
 //   - qualified exports:   the package has been explicitly qualified to at least
 //                            one particular module or has been qualifiedly exported
 //                            to all unnamed modules.
-//                            Note: _is_exported_allUnnamed is a form of a qualified
+//                            Note: being exported to all unnamed is a form of a qualified
-//                            export. It is equivalent to the package being
+//                            export. It is equivalent to the package being explicitly
-//                            explicitly exported to all current and future unnamed modules.
+//                            exported to all current and future unnamed modules.
 //   - unqualified exports: the package is exported to all modules.
 //
 // A package can transition from:
@@ -56,21 +56,53 @@
 // A package cannot transition from:
 //   - being unqualifiedly exported, to exported qualifiedly to a specific module.
 //       This transition attempt is silently ignored in set_exported.
+//   - being qualifiedly exported to not exported.
+//       Because transitions are only allowed from less exposure to greater exposure,
+//       the transition from qualifiedly exported to not exported would be considered
+//       a backward direction.  Therefore the implementation considers a package as
+//       qualifiedly exported even if its export-list exists but is empty.
 //
 // The Mutex Module_lock is shared between ModuleEntry and PackageEntry, to lock either
 // data structure.
+
+// PKG_EXP_UNQUALIFIED and PKG_EXP_ALLUNNAMED indicate whether the package is
+// exported unqualifiedly or exported to all unnamed modules.  They are used to
+// set the value of _export_flags.  Field _export_flags and the _qualified_exports
+// list are used to determine a package's export state.
+// Valid states are:
+//
+//   1. Package is not exported
+//      _export_flags is zero and _qualified_exports is null
+//   2. Package is unqualifiedly exported
+//      _export_flags is set to PKG_EXP_UNQUALIFIED
+//      _qualified_exports may or may not be null depending on whether the package
+//        transitioned from qualifiedly exported to unqualifiedly exported.
+//   3. Package is qualifiedly exported
+//      _export_flags may be set to PKG_EXP_ALLUNNAMED if the package is also
+//        exported to all unnamed modules
+//      _qualified_exports will be non-null
+//   4. Package is exported to all unnamed modules
+//      _export_flags is set to PKG_EXP_ALLUNNAMED
+//      _qualified_exports may or may not be null depending on whether the package
+//        is also qualifiedly exported to one or more named modules.
+#define PKG_EXP_UNQUALIFIED  0x0001
+#define PKG_EXP_ALLUNNAMED   0x0002
+#define PKG_EXP_UNQUALIFIED_OR_ALL_UNAMED (PKG_EXP_UNQUALIFIED | PKG_EXP_ALLUNNAMED)
+
 class PackageEntry : public HashtableEntry<Symbol*, mtModule> {
 private:
   ModuleEntry* _module;
+  // Indicates if package is exported unqualifiedly or to all unnamed. Access to
+  // this field is protected by the Module_lock.
+  int _export_flags;
   // Used to indicate for packages with classes loaded by the boot loader that
   // a class in that package has been loaded.  And, for packages with classes
   // loaded by the boot loader from -Xbootclasspath/a in an unnamed module, it
   // indicates from which class path entry.
   s2 _classpath_index;
-  bool _is_exported_unqualified;
-  bool _is_exported_allUnnamed;
   bool _must_walk_exports;
-  GrowableArray<ModuleEntry*>* _exported_pending_delete; // transitioned from qualified to unqualified, delete at safepoint
+  // Contains list of modules this package is qualifiedly exported to.  Access
+  // to this list is protected by the Module_lock.
   GrowableArray<ModuleEntry*>* _qualified_exports;
   TRACE_DEFINE_TRACE_ID_FIELD;
 
@@ -80,17 +112,14 @@ private:
 public:
   void init() {
     _module = NULL;
+    _export_flags = 0;
     _classpath_index = -1;
-    _is_exported_unqualified = false;
-    _is_exported_allUnnamed = false;
     _must_walk_exports = false;
-    _exported_pending_delete = NULL;
     _qualified_exports = NULL;
   }
 
   // package name
   Symbol*            name() const               { return literal(); }
-  void               set_name(Symbol* n)        { set_literal(n); }
 
   // the module containing the package definition
   ModuleEntry*       module() const             { return _module; }
@@ -98,37 +127,39 @@ public:
 
   // package's export state
   bool is_exported() const { // qualifiedly or unqualifiedly exported
-      return (is_unqual_exported() || has_qual_exports_list() || is_exported_allUnnamed());
+    assert_locked_or_safepoint(Module_lock);
+    return ((_export_flags & PKG_EXP_UNQUALIFIED_OR_ALL_UNAMED) != 0) || has_qual_exports_list();
   }
   // Returns true if the package has any explicit qualified exports or is exported to all unnamed
   bool is_qual_exported() const {
+    assert_locked_or_safepoint(Module_lock);
     return (has_qual_exports_list() || is_exported_allUnnamed());
   }
-  // Returns true if there are any explicit qualified exports
+  // Returns true if there are any explicit qualified exports.  Note that even
+  // if the _qualified_exports list is now empty (because the modules that were
+  // on the list got gc-ed and deleted from the list) this method may still
+  // return true.
   bool has_qual_exports_list() const {
-    assert(!(_qualified_exports != NULL && _is_exported_unqualified),
+    assert_locked_or_safepoint(Module_lock);
-           "_qualified_exports set at same time as _is_exported_unqualified");
+    return (!is_unqual_exported() && _qualified_exports != NULL);
-    return (_qualified_exports != NULL);
   }
   bool is_exported_allUnnamed() const {
-    assert(!(_is_exported_allUnnamed && _is_exported_unqualified),
+    assert_locked_or_safepoint(Module_lock);
-           "_is_exported_allUnnamed set at same time as _is_exported_unqualified");
+    return (_export_flags == PKG_EXP_ALLUNNAMED);
-    return _is_exported_allUnnamed;
   }
   bool is_unqual_exported() const {
-    assert(!(_qualified_exports != NULL && _is_exported_unqualified),
+    assert_locked_or_safepoint(Module_lock);
-           "_qualified_exports set at same time as _is_exported_unqualified");
+    return (_export_flags == PKG_EXP_UNQUALIFIED);
-    assert(!(_is_exported_allUnnamed && _is_exported_unqualified),
-           "_is_exported_allUnnamed set at same time as _is_exported_unqualified");
-    return _is_exported_unqualified;
   }
+
+  // Explicitly set _export_flags to PKG_EXP_UNQUALIFIED and clear
+  // PKG_EXP_ALLUNNAMED, if it was set.
   void set_unqual_exported() {
     assert(Module_lock->owned_by_self(), "should have the Module_lock");
-    _is_exported_unqualified = true;
+    _export_flags = PKG_EXP_UNQUALIFIED;
-    _is_exported_allUnnamed = false;
-    _qualified_exports = NULL;
   }
-  bool exported_pending_delete() const     { return (_exported_pending_delete != NULL); }
+
+  bool exported_pending_delete() const;
 
   void set_exported(ModuleEntry* m);
 

hotspot/src/share/vm/runtime/reflection.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -534,6 +534,9 @@ Reflection::VerifyClassAccessResults Reflection::verify_class_access(
     PackageEntry* package_to = new_class->package();
     assert(package_to != NULL, "can not obtain new_class' package");
 
+    {
+      MutexLocker m1(Module_lock);
+
       // Once readability is established, if module_to exports T unqualifiedly,
       // (to all modules), than whether module_from is in the unnamed module
       // or not does not matter, access is allowed.
@@ -551,6 +554,7 @@ Reflection::VerifyClassAccessResults Reflection::verify_class_access(
       if (!package_to->is_qexported_to(module_from)) {
         return TYPE_NOT_EXPORTED;
       }
+    }
     return ACCESS_OK;
   }