Project

General

Profile

Bug #8518

Accessing freed memory when dirty Wt::Dbo::ptr exists at destruction of its session.

Added by Dries Mys about 1 month ago. Updated 9 days ago.

Status:
Resolved
Priority:
Normal
Target version:
Start date:
05/13/2021
Due date:
% Done:

0%

Estimated time:

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.

#1

Updated by Korneel Dumon about 1 month 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?

#2

Updated by Dries Mys about 1 month 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.

#3

Updated by Korneel Dumon 30 days 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.

#4

Updated by Roel Standaert 16 days ago

  • Status changed from InProgress to Review
  • Assignee changed from Korneel Dumon to Roel Standaert
  • Target version set to 4.6.0
#5

Updated by Roel Standaert 9 days ago

  • Status changed from Review to Resolved

Also available in: Atom PDF