]> https://gitweb.dealii.org/ - dealii.git/commitdiff
Fix #15111. ParameterAcceptor now uses a set. 15122/head
authorLuca Heltai <luca.heltai@sissa.it>
Thu, 20 Apr 2023 20:37:41 +0000 (20:37 +0000)
committerLuca Heltai <luca.heltai@sissa.it>
Sat, 22 Apr 2023 06:09:25 +0000 (08:09 +0200)
doc/news/changes/minor/20230420LucaHeltai [new file with mode: 0644]
include/deal.II/base/parameter_acceptor.h
source/base/parameter_acceptor.cc
tests/parameter_handler/parameter_acceptor_11.cc [new file with mode: 0644]
tests/parameter_handler/parameter_acceptor_11.output [new file with mode: 0644]

diff --git a/doc/news/changes/minor/20230420LucaHeltai b/doc/news/changes/minor/20230420LucaHeltai
new file mode 100644 (file)
index 0000000..9bdfa8d
--- /dev/null
@@ -0,0 +1,3 @@
+Fixed: ParameterAcceptor::clear() had issues with 'use after free'.
+<br>
+(Luca Heltai, 2023/04/20)
index f629debdba517e6dbc69c4236811cc8d84bc8fb3..b26853fa117bde911a33c453542227658eb094c2 100644 (file)
 
 #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 @@ public:
    */
   ParameterAcceptor(const std::string &section_name = "");
 
+  /**
+   * Get the acceptor id of this object.
+   */
+  unsigned int
+  get_acceptor_id() const;
+
   /**
    * Destructor.
    */
@@ -592,12 +610,24 @@ public:
 
 private:
   /**
-   * A list containing all constructed classes of type
+   * Get the next free id for this class.
+   */
+  static unsigned int
+  get_next_free_id();
+
+  /**
+   * 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;
 
   /**
index 215286d8f3218a8aa2a0a2c44f7c63d03379fa12..cb9c7ee0d1f5d111a38ee10786ce4910cb83d7ef 100644 (file)
 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(ParameterAcceptor::get_next_free_id())
   , section_name(name)
 {
-  SmartPointer<ParameterAcceptor> pt(
-    this, boost::core::demangle(typeid(*this).name()).c_str());
-  class_list.push_back(pt);
+  std::lock_guard<std::mutex> l(class_list_mutex);
+  class_list.insert(this);
 }
 
 
 
 ParameterAcceptor::~ParameterAcceptor()
 {
-  class_list[acceptor_id] = nullptr;
+  std::lock_guard<std::mutex> l(class_list_mutex);
+  // Notice that it is possible that the class is no longer in the static list.
+  // This happens when the clear() method has been called. We must guard that
+  // this is not the case, and therefore we only remove ourselves from the list
+  // if we are actually there.
+  if (class_list.find(this) != class_list.end())
+    class_list.erase(this);
 }
 
 
@@ -105,6 +128,7 @@ ParameterAcceptor::initialize(std::istream &input_stream, ParameterHandler &prm)
 void
 ParameterAcceptor::clear()
 {
+  std::lock_guard<std::mutex> l(class_list_mutex);
   class_list.clear();
   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,13 +165,12 @@ 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);
+    }
 }
 
 
@@ -156,7 +178,6 @@ ParameterAcceptor::declare_all_parameters(ParameterHandler &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());
@@ -247,4 +272,23 @@ ParameterAcceptor::leave_my_subsection(
     }
 }
 
+
+
+inline unsigned int
+ParameterAcceptor::get_acceptor_id() const
+{
+  return acceptor_id;
+}
+
+
+
+unsigned int
+ParameterAcceptor::get_next_free_id()
+{
+  static std::mutex           id_mutex;
+  std::lock_guard<std::mutex> lock(id_mutex);
+  static int                  current_id = 0;
+  return current_id++;
+}
+
 DEAL_II_NAMESPACE_CLOSE
diff --git a/tests/parameter_handler/parameter_acceptor_11.cc b/tests/parameter_handler/parameter_acceptor_11.cc
new file mode 100644 (file)
index 0000000..d7dc88a
--- /dev/null
@@ -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);
+  }
+}
diff --git a/tests/parameter_handler/parameter_acceptor_11.output b/tests/parameter_handler/parameter_acceptor_11.output
new file mode 100644 (file)
index 0000000..82a5e94
--- /dev/null
@@ -0,0 +1,2 @@
+
+DEAL:parameters:Bar::A parameter: 1

In the beginning the Universe was created. This has made a lot of people very angry and has been widely regarded as a bad move.

Douglas Adams


Typeset in Trocchi and Trocchi Bold Sans Serif.