From 699abf966a7af1ea90e906d803678dcbd0696690 Mon Sep 17 00:00:00 2001 From: Luca Heltai Date: Thu, 20 Apr 2023 20:37:41 +0000 Subject: [PATCH] Fix #15111. ParameterAcceptor now uses a set. --- doc/news/changes/minor/20230420LucaHeltai | 3 + include/deal.II/base/parameter_acceptor.h | 36 +++++- source/base/parameter_acceptor.cc | 120 ++++++++++++------ .../parameter_acceptor_11.cc | 52 ++++++++ .../parameter_acceptor_11.output | 2 + 5 files changed, 172 insertions(+), 41 deletions(-) create mode 100644 doc/news/changes/minor/20230420LucaHeltai create mode 100644 tests/parameter_handler/parameter_acceptor_11.cc create mode 100644 tests/parameter_handler/parameter_acceptor_11.output diff --git a/doc/news/changes/minor/20230420LucaHeltai b/doc/news/changes/minor/20230420LucaHeltai new file mode 100644 index 0000000000..9bdfa8db22 --- /dev/null +++ b/doc/news/changes/minor/20230420LucaHeltai @@ -0,0 +1,3 @@ +Fixed: ParameterAcceptor::clear() had issues with 'use after free'. +
+(Luca Heltai, 2023/04/20) diff --git a/include/deal.II/base/parameter_acceptor.h b/include/deal.II/base/parameter_acceptor.h index f629debdba..b26853fa11 100644 --- a/include/deal.II/base/parameter_acceptor.h +++ b/include/deal.II/base/parameter_acceptor.h @@ -25,10 +25,22 @@ #include +#include #include 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 §ion_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> class_list; + static std::set + class_list; - /** The index of this specific class within the class list. */ + /** The id of this specific class instance. */ const unsigned int acceptor_id; /** diff --git a/source/base/parameter_acceptor.cc b/source/base/parameter_acceptor.cc index 215286d8f3..cb9c7ee0d1 100644 --- a/source/base/parameter_acceptor.cc +++ b/source/base/parameter_acceptor.cc @@ -24,25 +24,48 @@ DEAL_II_NAMESPACE_OPEN -// Static empty class list -std::vector> 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::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 pt( - this, boost::core::demangle(typeid(*this).name()).c_str()); - class_list.push_back(pt); + std::lock_guard l(class_list_mutex); + class_list.insert(this); } ParameterAcceptor::~ParameterAcceptor() { - class_list[acceptor_id] = nullptr; + std::lock_guard 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 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 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 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 index 0000000000..d7dc88a1be --- /dev/null +++ b/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 + +#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 index 0000000000..82a5e94093 --- /dev/null +++ b/tests/parameter_handler/parameter_acceptor_11.output @@ -0,0 +1,2 @@ + +DEAL:parameters:Bar::A parameter: 1 -- 2.39.5