Project

General

Profile

Actions

Bug #8518

closed

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

Added by Dries Mys almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Roel Standaert
Target version:
Start date:
05/13/2021
Due date:
% Done:

100%

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.

Actions #1

Updated by Korneel Dumon almost 3 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?

Actions #2

Updated by Dries Mys almost 3 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.

Actions #3

Updated by Korneel Dumon almost 3 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.

Actions #4

Updated by Roel Standaert almost 3 years ago

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

Updated by Roel Standaert almost 3 years ago

  • Status changed from Review to Resolved
Actions #6

Updated by Roel Standaert over 2 years ago

  • % Done changed from 0 to 100
Actions #7

Updated by Roel Standaert over 2 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF