8355960: JvmtiAgentList::Iterator dtor double free with -fno-elide-constructors

Reviewed-by: dholmes, sspitsyn

Author: Alex Menkov

src/hotspot/share/jfr/periodic/jfrPeriodic.cpp

@@ -291,7 +291,7 @@ static void send_agent_event(AgentEvent& event, const JvmtiAgent* agent) {
 }
 
 TRACE_REQUEST_FUNC(JavaAgent) {
-  const JvmtiAgentList::Iterator it =JvmtiAgentList::java_agents();
+  JvmtiAgentList::Iterator it = JvmtiAgentList::java_agents();
   while (it.has_next()) {
     const JvmtiAgent* agent = it.next();
     assert(agent->is_jplis(), "invariant");
@@ -300,7 +300,7 @@ TRACE_REQUEST_FUNC(JavaAgent) {
   }
 }
 
-static void send_native_agent_events(const JvmtiAgentList::Iterator& it) {
+static void send_native_agent_events(JvmtiAgentList::Iterator& it) {
   while (it.has_next()) {
     const JvmtiAgent* agent = it.next();
     assert(!agent->is_jplis(), "invariant");
@@ -311,9 +311,9 @@ static void send_native_agent_events(const JvmtiAgentList::Iterator& it) {
 }
 
 TRACE_REQUEST_FUNC(NativeAgent) {
-  const JvmtiAgentList::Iterator native_agents_it = JvmtiAgentList::native_agents();
+  JvmtiAgentList::Iterator native_agents_it = JvmtiAgentList::native_agents();
   send_native_agent_events(native_agents_it);
-  const JvmtiAgentList::Iterator xrun_agents_it = JvmtiAgentList::xrun_agents();
+  JvmtiAgentList::Iterator xrun_agents_it = JvmtiAgentList::xrun_agents();
   send_native_agent_events(xrun_agents_it);
 }
 #else  // INCLUDE_JVMTI

src/hotspot/share/prims/jvmtiAgent.cpp

@@ -31,6 +31,7 @@
 #include "prims/jvmtiEnvBase.hpp"
 #include "prims/jvmtiExport.hpp"
 #include "runtime/arguments.hpp"
+#include "runtime/atomic.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/interfaceSupport.inline.hpp"
 #include "runtime/java.hpp"
@@ -82,11 +83,7 @@ JvmtiAgent::JvmtiAgent(const char* name, const char* options, bool is_absolute_p
   _xrun(false) {}
 
 JvmtiAgent* JvmtiAgent::next() const {
-  return _next;
-}
-
-void JvmtiAgent::set_next(JvmtiAgent* agent) {
-  _next = agent;
+  return Atomic::load_acquire(&_next);
 }
 
 const char* JvmtiAgent::name() const {

src/hotspot/share/prims/jvmtiAgent.hpp

@@ -52,7 +52,6 @@ class JvmtiAgent : public CHeapObj<mtServiceability> {
   bool _xrun;
 
   JvmtiAgent* next() const;
-  void set_next(JvmtiAgent* agent);
   void convert_xrun_agent();
   void set_xrun();
 

src/hotspot/share/prims/jvmtiAgentList.cpp

@@ -31,7 +31,7 @@
 #include "runtime/atomic.hpp"
 #include "runtime/os.inline.hpp"
 
-JvmtiAgent* JvmtiAgentList::_list = nullptr;
+JvmtiAgent* JvmtiAgentList::_head = nullptr;
 
 // Selection as a function of the filter.
 JvmtiAgent* JvmtiAgentList::Iterator::select(JvmtiAgent* agent) const {
@@ -61,68 +61,59 @@ JvmtiAgent* JvmtiAgentList::Iterator::select(JvmtiAgent* agent) const {
   return nullptr;
 }
 
-static inline JvmtiAgent* head(JvmtiAgent** list) {
-  assert(list != nullptr, "invariant");
-  return Atomic::load_acquire(list);
-}
-
-// The storage list is a single cas-linked-list, to allow for concurrent iterations.
-// Especially during initial loading of agents, there exist an order requirement to iterate oldest -> newest.
-// Our concurrent storage linked-list is newest -> oldest.
-// The correct order is preserved by the iterator, by storing a filtered set of entries in a stack.
-JvmtiAgentList::Iterator::Iterator(JvmtiAgent** list, Filter filter) :
-  _stack(new GrowableArrayCHeap<JvmtiAgent*, mtServiceability>(16)), _filter(filter) {
-  JvmtiAgent* next = head(list);
-  while (next != nullptr) {
-    next = select(next);
-    if (next != nullptr) {
-      _stack->push(next);
-      next = next->next();
-    }
-  }
+JvmtiAgentList::Iterator::Iterator(JvmtiAgent* head, Filter filter) : _filter(filter), _next(select(head)) {
 }
 
 bool JvmtiAgentList::Iterator::has_next() const {
-  assert(_stack != nullptr, "invariant");
-  return _stack->is_nonempty();
-}
-
-const JvmtiAgent* JvmtiAgentList::Iterator::next() const {
-  assert(has_next(), "invariant");
-  return _stack->pop();
+  return _next != nullptr;
 }
 
 JvmtiAgent* JvmtiAgentList::Iterator::next() {
-  return const_cast<JvmtiAgent*>(const_cast<const Iterator*>(this)->next());
+  assert(_next != nullptr, "must be");
+  JvmtiAgent* result = _next;
+  _next = select(_next->next());
+  return result;
+
 }
 
 JvmtiAgentList::Iterator JvmtiAgentList::agents() {
-  return Iterator(&_list, Iterator::NOT_XRUN);
+  return Iterator(head(), Iterator::NOT_XRUN);
 }
 
 JvmtiAgentList::Iterator JvmtiAgentList::java_agents() {
-  return Iterator(&_list, Iterator::JAVA);
+  return Iterator(head(), Iterator::JAVA);
 }
 
 JvmtiAgentList::Iterator JvmtiAgentList::native_agents() {
-  return Iterator(&_list, Iterator::NATIVE);
+  return Iterator(head(), Iterator::NATIVE);
 }
 
 JvmtiAgentList::Iterator JvmtiAgentList::xrun_agents() {
-  return Iterator(&_list, Iterator::XRUN);
+  return Iterator(head(), Iterator::XRUN);
 }
 
 JvmtiAgentList::Iterator JvmtiAgentList::all() {
-  return Iterator(&_list, Iterator::ALL);
+  return Iterator(head(), Iterator::ALL);
 }
 
 void JvmtiAgentList::add(JvmtiAgent* agent) {
   assert(agent != nullptr, "invariant");
-  JvmtiAgent* next;
-  do {
-    next = head(&_list);
-    agent->set_next(next);
-  } while (Atomic::cmpxchg(&_list, next, agent) != next);
+
+  // address of the pointer to add new agent (&_head when the list is empty or &agent->_next of the last agent in the list)
+  JvmtiAgent** tail_ptr = &_head;
+  while (true) {
+    JvmtiAgent* next = Atomic::load(tail_ptr);
+    if (next == nullptr) {
+      // *tail_ptr == nullptr here
+      if (Atomic::cmpxchg(tail_ptr, (JvmtiAgent*)nullptr, agent) != nullptr) {
+        // another thread added an agent, reload next from tail_ptr
+        continue;
+      }
+      // successfully set, exit
+      break;
+    }
+    tail_ptr = &next->_next;
+  }
 }
 
 void JvmtiAgentList::add(const char* name, const char* options, bool absolute_path) {
@@ -143,6 +134,10 @@ static void assert_initialized(JvmtiAgentList::Iterator& it) {
 }
 #endif
 
+JvmtiAgent* JvmtiAgentList::head() {
+  return Atomic::load_acquire(&_head);
+}
+
 // In case an agent did not enable the VMInit callback, or if it is an -Xrun agent,
 // it gets an initializiation timestamp here.
 void JvmtiAgentList::initialize() {
@@ -283,6 +278,6 @@ void JvmtiAgentList::disable_agent_list() {
   assert(CDSConfig::is_dumping_final_static_archive(), "use this only for -XX:AOTMode=create!");
   assert(!Universe::is_bootstrapping() && !Universe::is_fully_initialized(), "must do this very early");
   log_info(aot)("Disabled all JVMTI agents during -XX:AOTMode=create");
-  _list = nullptr; // Pretend that no agents have been added.
+  _head = nullptr; // Pretend that no agents have been added.
 #endif
 }

src/hotspot/share/prims/jvmtiAgentList.hpp

@@ -25,15 +25,12 @@
 #ifndef SHARE_PRIMS_JVMTIAGENTLIST_HPP
 #define SHARE_PRIMS_JVMTIAGENTLIST_HPP
 
-#include "nmt/memTag.hpp"
 #include "prims/jvmtiAgent.hpp"
-#include "utilities/growableArray.hpp"
 
 class JvmtiEnv;
 
-// Maintains a single cas linked-list of JvmtiAgents.
+// Maintains thread-safe linked list of JvmtiAgents.
 class JvmtiAgentList : AllStatic {
-  friend class Iterator;
   friend class JvmtiExport;
  public:
   class Iterator {
@@ -46,20 +43,20 @@ class JvmtiAgentList : AllStatic {
       NOT_XRUN,
       ALL
     };
-    GrowableArrayCHeap<JvmtiAgent*, mtServiceability>* _stack;
     const Filter _filter;
-    Iterator() : _stack(nullptr), _filter(ALL) {}
-    Iterator(JvmtiAgent** list, Filter filter);
+    JvmtiAgent* _next;
+    Iterator(): _filter(ALL), _next(nullptr) {}
+    Iterator(JvmtiAgent* head, Filter filter);
     JvmtiAgent* select(JvmtiAgent* agent) const;
    public:
     bool has_next() const NOT_JVMTI_RETURN_(false);
     JvmtiAgent* next() NOT_JVMTI_RETURN_(nullptr);
-    const JvmtiAgent* next() const NOT_JVMTI_RETURN_(nullptr);
-    ~Iterator() { delete _stack; }
   };
 
  private:
-  static JvmtiAgent* _list;
+  static JvmtiAgent* _head;
+
+  static JvmtiAgent* head();
 
   static void initialize();
   static void convert_xrun_agents();

src/hotspot/share/runtime/os.cpp

@@ -1125,7 +1125,7 @@ void os::print_environment_variables(outputStream* st, const char** env_list) {
 
 void os::print_jvmti_agent_info(outputStream* st) {
 #if INCLUDE_JVMTI
-  const JvmtiAgentList::Iterator it = JvmtiAgentList::all();
+  JvmtiAgentList::Iterator it = JvmtiAgentList::all();
   if (it.has_next()) {
     st->print_cr("JVMTI agents:");
   } else {