ParameterAcceptor::clear() leads to use after free

[tamiko]

The following, admittedly, not very practical piece of code triggers a use after free:

#include <deal.II/base/parameter_acceptor.h>

struct Foo : public dealii::ParameterAcceptor {};

int main()
{
  Foo foo;
  dealii::ParameterAcceptor::clear();
  // <-- foo goes out of scope here
}

The triggered assertion reads:

/usr/lib/gcc/x86_64-pc-linux-gnu/12/include/g++-v12/bits/stl_vector.h:1123: reference std::vector<dealii::SmartPointer<dealii::ParameterAcceptor>>::operator[](size_type) [_Tp = dealii::SmartPointer<dealii::ParameterAcceptor>, _Alloc = std::allocator<dealii::SmartPointer<dealii::ParameterAcceptor>>]: Assertion '__n < this->size()' failed.

The offending line of code in source/base/parameter_acceptor.cc is:

   43 ParameterAcceptor::~ParameterAcceptor()                                          
   44 {                                                                                
   45   class_list[acceptor_id] = nullptr;                                             
   46 }  

The underlying problem is the fact that a call to clear() resets the class_list, which is a std::vector of SmartPointer<ParameterAcceptor> objects that gets reset.

But every instance of a ParameterAcceptor object stores an index acceptor_id to it's pointer's position in the vector.
Naturally, after the ParameterAcceptor class has been cleared this position is no longer valid, hence the assert.

I think, this approach is fundamentally broken.

The class_list object should probably be a std::set<dealii::ParameterAcceptor*> and not a std::vector of a SmartPointer.
A SmartPointer is of very little utility anyway: The desctructor of dealii::ParameterAcceptor will already remove the instance from the std::vector/std::set, which basically guarantees that a use-after-free that SmartPointer guards against cannot happen. (*)

So I think the way to go is to remove the acceptor_id and simply store the address ot the object in a std::set.

(*) None of this is thread-safe, which is a story in itself.

[bangerth]

Maybe a better design is to use boost::signals2::connection. Connections can be severed from both sides I believe.

[masterleinad]

Is there a good reason for having ParameterAcceptor::clear() anyway? If so, I would also prefer a solution where the ParameterAcceptor tries to unregister itself from the static variable (but doesn't fail if it can't).

[tamiko]

@masterleinad I currently have a use case: I want to get all parameters associated to one class (and all dependent objects), after that clear the global ParameterAcceptor::prm and then do the same for an unrelated class.

[tamiko]

@bangerth At first glance I believe this should be doable (meaning to use signals for all of these operations). We will have to ensure that slots are called sequentially, though (because our ParameterHandler class interface is neither reentrant nor thread-safe).

[luca-heltai]

I'm working on a fix.