Use-after-free in SharedArray.hpp
I'm building with gcc v12.2.0 and getting a lot of warnings like so:
In destructor ‘Nektar::Array<Nektar::OneD, const DataType>::~Array() [with DataType = double]’,
inlined from ‘Nektar::Array<Nektar::OneD, double>::~Array()’ at /home/alex/code/nektar/library/LibUtilities/BasicUtils/SharedArray.hpp:582:7,
inlined from ‘void Nektar::NavierStokesCFE::SpecialBndTreat(Nektar::Array<Nektar::OneD, Nektar::Array<Nektar::OneD, double> >&)’ at /home/alex/code/nektar/solvers/CompressibleFlowSolver/EquationSystems/NavierStokesCFE.cpp:556:13:
/home/alex/code/nektar/library/LibUtilities/BasicUtils/SharedArray.hpp:211:18: warning: pointer used after ‘void operator delete(void*, std::size_t)’ [-Wuse-after-free]
211 | *m_count -= 1;
| ~~~~~~~~~^~~~
In destructor ‘Nektar::Array<Nektar::OneD, const DataType>::~Array() [with DataType = double]’,
inlined from ‘Nektar::Array<Nektar::OneD, double>::~Array()’ at /home/alex/code/nektar/library/LibUtilities/BasicUtils/SharedArray.hpp:582:7,
inlined from ‘void Nektar::NavierStokesCFE::SpecialBndTreat(Nektar::Array<Nektar::OneD, Nektar::Array<Nektar::OneD, double> >&)’ at /home/alex/code/nektar/solvers/CompressibleFlowSolver/EquationSystems/NavierStokesCFE.cpp:555:51:
/home/alex/code/nektar/library/LibUtilities/BasicUtils/SharedArray.hpp:235:13: note: call to ‘void operator delete(void*, std::size_t)’ here
235 | delete m_count; // Clean up the memory used for the reference count.
|
I assume what is happening is that there are sequences of code where the destructor ~Array()
is (implicitly) called multiple times in succession, in which case, m_count
would indeed be being used after being delete
d. There is a guard checking whether m_count
is null, but this doesn't achieve anything as delete
doesn't set the pointer to null.
Frankly I think you'd be much better off using a shared_ptr
(of either the boost
or std
variety) under the hood for this. In addition to the use-after-free bug, this code is also decidedly thread-unsafe as there are potential race conditions in the destructor. To do it properly you'd need m_count
to be atomic among other things. Writing reference-counting code correctly is finicky and easy to get wrong so rolling your own implementation is always a dicey prospect.