Bug #8518
Accessing freed memory when dirty Wt::Dbo::ptr exists at destruction of its session.
100%
Description
Consider the following pseudo example:
// construct Wt::Dbo::Session // construct Wt::Dbo::Transaction // load a Wt::Dbo::ptr from database // destruct/commit the transaction // modify the Wt::Dbo::ptr object => adds the object to Session::dirtyObjects_ // destruct the Wt::Dbo::Session => calls decRef on all dirtyObjects_ untill they are really destructed (although a Wt::Dbo::ptr still exists), in ~MetaDbo decRef will be called on a being destructed object. // destruct the Wt::Dbo::ptr object => will call decRef on an already destructed object (see previous step)
Note that this problem may occur especially in case of manual flushing.
Possible solution:
Change the ~Session from
while (!dirtyObjects_->empty()) { MetaDboBase *b = *dirtyObjects_->begin(); b->decRef(); }
to
while (!dirtyObjects_->empty()) { MetaDboBase *b = *dirtyObjects_->begin(); discardChanges(b); }
Possible solution v2:
Session.C:
Session::~Session() { if (!dirtyObjects_->empty()) { LOG_WARN("Session exiting with " << dirtyObjects_->size() << " dirty objects"); } - while (!dirtyObjects_->empty()) { - MetaDboBase *b = *dirtyObjects_->begin(); - b->decRef(); - } - - dirtyObjects_->clear(); - delete dirtyObjects_; for (ClassRegistry::iterator i = classRegistry_.begin(); i != classRegistry_.end(); ++i) delete i->second; + + delete dirtyObjects_; // dirtyObjects_ will be empty by now due to delete i->second (which calls rereadAll()) }
Session_impl.h:
template <class C> Session::Mapping<C>::~Mapping() { + rereadAll(); for (typename Registry::iterator i = registry_.begin(); i != registry_.end(); ++i) { i->second->setState(MetaDboBase::Orphaned); } }
Advantage of this solution is that all circular references between Wt::Dbo::ptr (in case Wt::Dbo::weak_ptr is not sufficiently/correctly used) are cleaned up at session destruction at least. This could for ex occur in case of the following situation of three classes refencing to each other using one-to-one are one-to-many relations:
a <- b <- | | -> c ----|
Disadvantage is that rereadAll introduces a copy of the registry_ array. However, this array should be small (or empty) during Session destruction.
Extra: Alternative implementation of rereadAll without copying array
To overcome the disadvantage of solution 2, rereadAll could be rewritten without copying the whole registry_ array.
template <class C> void Session::Mapping<C>::rereadAll() { if (registry_.empty()) return; typename Registry::iterator i = registry_.begin(); ptr<C> previous = i->second; for (++i; i != registry_.end(); previous = i->second, ++i) previous.reread(); previous.reread(); }
Note that it is safe to reread the previous item as std::map::erase only invalidates the iterators pointing to the item being erased: References and iterators to the erased elements are invalidated. Other references and iterators are not affected.
Updated by Korneel Dumon almost 2 years ago
- Status changed from New to InProgress
- Assignee set to Korneel Dumon
Hi,
thanks for figuring this out. I will add the first change. Can you give an example of when the circular reference issue would occur?
Updated by Dries Mys almost 2 years ago
Hi,
Thanks for including the fix in Wt.
For the circular references, consider the following (theoretical example):
#include <Wt/Dbo/Dbo.h> #include <string> namespace dbo = Wt::Dbo; class A; class B; class C; class A { public: dbo::weak_ptr<C> c_; dbo::ptr<B> b_; template<class Action> void persist(Action& a) { dbo::hasOne(a, c_); dbo::belongsTo(a, b_); } }; class B { public: dbo::weak_ptr<A> a_; dbo::ptr<C> c_; template<class Action> void persist(Action& a) { dbo::hasOne(a, a_); dbo::belongsTo(a, c_); } }; class C { public: dbo::weak_ptr<B> b_; dbo::ptr<A> a_; template<class Action> void persist(Action& a) { dbo::hasOne(a, b_); dbo::belongsTo(a, a_); } };
Circular reference: A -> B -> C -> A
I'm aware the programmer could rewrite this to A <- B -> C -> A. Now A contains only weak_ptr
's, and B only ptr
's. However, it would be nice/convenient if Wt would at least clean up possible circular references during application (session) destruction, which would limit the memory consumption in case of such a circular reference is not detected by the programmer.
Note that it would be harder to destruct this circular reference in case of one-to-many relationships, but this situation is probably even rarer in real situations.
Another possibility for circular references occurs when the programmer 'manually' stores Wt::Dbo::ptr's to other objects (f.ex. the standard relationship could not be used if the current relationships depends on the current date):
class A { mutable dbo::ptr<B> b_; public: dbo::ptr<B> GetB () const {if (!b_) b_ = /*cache b_ in some way*/; return b_; } template<class Action> void persist(Action& a) { // b_ is not set by persist } }; class B { mutable dbo::ptr<A> a_; public: dbo::ptr<A> GetA () const {if (!a_) a_ = /*cache a_ in some way*/; return a_; } template<class Action> void persist(Action& a) { // a_ is not set by persist } };
This would introduce a circular reference if a_->b_->a_ == a_. Off course, the programmer could manually resolve this circular references by using weak_ptr
at an appropriate place.
Updated by Korneel Dumon almost 2 years ago
Since it's all kind of theoretical we will keep it as is for now (except for the discardChanges() fix of course). As you say, a user can use weak_ptr
or have a different table design.
Updated by Roel Standaert almost 2 years ago
- Status changed from InProgress to Review
- Assignee changed from Korneel Dumon to Roel Standaert
- Target version set to 4.6.0