Fix dealii#15111. ParameterAcceptor now uses a set.

doc/news/changes/minor/20230420LucaHeltai

@@ -0,0 +1,3 @@
+Fixed: ParameterAcceptor::clear() had issues with 'use after free'.
+<br>
+(Luca Heltai, 2023/04/20)

include/deal.II/base/parameter_acceptor.h

@@ -25,10 +25,22 @@
 
 #include <boost/signals2/signal.hpp>
 
+#include <mutex>
 #include <typeinfo>
 
 DEAL_II_NAMESPACE_OPEN
 
+class ParameterAcceptor;
+
+namespace internal
+{
+  /**
+   * Compare operator between two parameter acceptors. Same as std::less applied
+   * to the acceptor ids of each instance.
+   */
+  struct ParameterAcceptorCompare;
+} // namespace internal
+
 /**
  * A parameter acceptor base class. This class is used to define a public
  * interface for classes which want to use a single global ParameterHandler to
@@ -356,6 +368,12 @@ class ParameterAcceptor : public Subscriptor
    */
   ParameterAcceptor(const std::string &section_name = "");
 
+  /**
+   * Get the acceptor id of this object.
+   */
+  unsigned int
+  get_acceptor_id() const;
+
   /**
    * Destructor.
    */
@@ -592,12 +610,18 @@ class ParameterAcceptor : public Subscriptor
 
 private:
   /**
-   * A list containing all constructed classes of type
+   * A mutex to prevent writing on the class_list set from multiple threads.
+   */
+  static std::mutex class_list_mutex;
+
+  /**
+   * A set containing the address of all constructed classes of type
    * ParameterAcceptor.
    */
-  static std::vector<SmartPointer<ParameterAcceptor>> class_list;
+  static std::set<ParameterAcceptor *, internal::ParameterAcceptorCompare>
+    class_list;
 
-  /** The index of this specific class within the class list. */
+  /** The id of this specific class instance. */
   const unsigned int acceptor_id;
 
   /**
@@ -736,6 +760,14 @@ ParameterAcceptorProxy<SourceClass>::parse_parameters(ParameterHandler &prm)
   SourceClass::parse_parameters(prm);
 }
 
+
+
+inline unsigned int
+ParameterAcceptor::get_acceptor_id() const
+{
+  return acceptor_id;
+}
+
 DEAL_II_NAMESPACE_CLOSE
 
 #endif

source/base/parameter_acceptor.cc

@@ -24,25 +24,47 @@
 DEAL_II_NAMESPACE_OPEN
 
 
-// Static empty class list
-std::vector<SmartPointer<ParameterAcceptor>> ParameterAcceptor::class_list;
+namespace internal
+{
+  struct ParameterAcceptorCompare
+  {
+    bool
+    operator()(const ParameterAcceptor *p1, const ParameterAcceptor *p2) const
+    {
+      return p1->get_acceptor_id() < p2->get_acceptor_id();
+    }
+  };
+} // namespace internal
+
+
+// Static mutex for the class list
+std::mutex ParameterAcceptor::class_list_mutex;
+
+// Static empty class set
+std::set<ParameterAcceptor *, internal::ParameterAcceptorCompare>
+  ParameterAcceptor::class_list;
+
 // Static parameter handler
 ParameterHandler ParameterAcceptor::prm;
 
 ParameterAcceptor::ParameterAcceptor(const std::string &name)
-  : acceptor_id(class_list.size())
+  : acceptor_id(
+      class_list.empty() ? 0 : (*(class_list.rbegin()))->get_acceptor_id() + 1)
   , section_name(name)
 {
-  SmartPointer<ParameterAcceptor> pt(
-    this, boost::core::demangle(typeid(*this).name()).c_str());
-  class_list.push_back(pt);
+  class_list_mutex.lock();
+  class_list.insert(this);
+  class_list_mutex.unlock();
 }
 
 
 
 ParameterAcceptor::~ParameterAcceptor()
 {
-  class_list[acceptor_id] = nullptr;
+  class_list_mutex.lock();
+  if (class_list.find(this) != class_list.end())
+    class_list.erase(this);
+  class_list_mutex.unlock();
 }
 
 
@@ -105,7 +127,9 @@ ParameterAcceptor::initialize(std::istream &input_stream, ParameterHandler &prm)
 void
 ParameterAcceptor::clear()
 {
+  class_list_mutex.lock();
   class_list.clear();
+  class_list_mutex.unlock();
   prm.clear();
 }
 
@@ -127,13 +151,12 @@ void
 ParameterAcceptor::parse_all_parameters(ParameterHandler &prm)
 {
   for (const auto &instance : class_list)
-    if (instance != nullptr)
-      {
-        instance->enter_my_subsection(prm);
-        instance->parse_parameters(prm);
-        instance->parse_parameters_call_back();
-        instance->leave_my_subsection(prm);
-      }
+    {
+      instance->enter_my_subsection(prm);
+      instance->parse_parameters(prm);
+      instance->parse_parameters_call_back();
+      instance->leave_my_subsection(prm);
+    }
 }
 
 
@@ -142,21 +165,19 @@ void
 ParameterAcceptor::declare_all_parameters(ParameterHandler &prm)
 {
   for (const auto &instance : class_list)
-    if (instance != nullptr)
-      {
-        instance->enter_my_subsection(prm);
-        instance->declare_parameters(prm);
-        instance->declare_parameters_call_back();
-        instance->leave_my_subsection(prm);
-      }
+    {
+      instance->enter_my_subsection(prm);
+      instance->declare_parameters(prm);
+      instance->declare_parameters_call_back();
+      instance->leave_my_subsection(prm);
+    }
 }
 
 
 
 std::vector<std::string>
 ParameterAcceptor::get_section_path() const
 {
-  Assert(acceptor_id < class_list.size(), ExcInternalError());
   const auto my_section_name = get_section_name();
   const bool is_absolute     = (my_section_name.front() == sep);
 
@@ -174,22 +195,26 @@ ParameterAcceptor::get_section_path() const
       // to ours. This is tricky. If the previous class has a path with a
       // trailing /, then the full path is used, else only the path except the
       // last one
-      for (int i = acceptor_id - 1; i >= 0; --i)
-        if (class_list[i] != nullptr)
-          {
-            bool has_trailing = class_list[i]->get_section_name().back() == sep;
-            auto previous_path = class_list[i]->get_section_path();
-
-            // See if we need to remove last piece of the path
-            if ((previous_path.size() > 0) && has_trailing == false)
-              previous_path.resize(previous_path.size() - 1);
-
-            sections.insert(sections.begin(),
-                            previous_path.begin(),
-                            previous_path.end());
-            // Exit the for cycle
-            break;
-          }
+      for (auto acceptor_it = class_list.rbegin();
+           acceptor_it != class_list.rend();
+           ++acceptor_it)
+        {
+          const auto acceptor = *acceptor_it;
+          if (acceptor->get_acceptor_id() >= get_acceptor_id())
+            continue;
+          bool has_trailing  = acceptor->get_section_name().back() == sep;
+          auto previous_path = acceptor->get_section_path();
+
+          // See if we need to remove last piece of the path
+          if ((previous_path.size() > 0) && has_trailing == false)
+            previous_path.resize(previous_path.size() - 1);
+
+          sections.insert(sections.begin(),
+                          previous_path.begin(),
+                          previous_path.end());
+          // Exit the for cycle
+          break;
+        }
     }
   // Finally, insert the remaining subsections
   sections.insert(sections.end(), subsections.begin(), subsections.end());

tests/parameter_handler/parameter_acceptor_11.cc

@@ -0,0 +1,52 @@
+//-----------------------------------------------------------
+//
+//    Copyright (C) 2023 by the deal.II authors
+//
+//    This file is part of the deal.II library.
+//
+//    The deal.II library is free software; you can use it, redistribute
+//    it, and/or modify it under the terms of the GNU Lesser General
+//    Public License as published by the Free Software Foundation; either
+//    version 2.1 of the License, or (at your option) any later version.
+//    The full text of the license can be found in the file LICENSE.md at
+//    the top level directory of deal.II.
+//
+//-----------------------------------------------------------
+
+
+// Test that clear works as expected.
+
+#include <deal.II/base/parameter_acceptor.h>
+
+#include "../tests.h"
+
+struct Foo : public dealii::ParameterAcceptor
+{};
+
+
+struct Bar : public dealii::ParameterAcceptor
+{
+  Bar()
+  {
+    add_parameter("A parameter", a);
+  }
+  int a = 1;
+};
+
+int
+main()
+{
+  initlog();
+  {
+    Foo foo;
+    ParameterAcceptor::clear();
+    // <-- foo goes out of scope here
+  }
+
+  {
+    Bar bar;
+    ParameterAcceptor::prm.log_parameters(deallog);
+    ParameterAcceptor::clear();
+    ParameterAcceptor::prm.log_parameters(deallog);
+  }
+}

tests/parameter_handler/parameter_acceptor_11.output

@@ -0,0 +1,2 @@
+
+DEAL:parameters:Bar::A parameter: 1