shared_ptr bug?

From: Greg Colvin (gcolvin_at_[hidden])
Date: 2001-10-18 00:50:15

I'd like some feedback on the appended bug report.

At first I thought it was a bug in Electric Fence.  I didn't see
how it be that "when line if (--*pn == 0) { checked_delete(px); }
executes shared_ptr object gets destroyed and line *pn = 1
accesses already freed memory", since px does not point at the
shared_ptr object.

But I now see that the test case contrives to make the destructor
foo destroy the very shared_ptr<foo> that calls the destructor.
Ugly, but not forbidden.  Maybe it should be?

If this  is a bug the fix below looks OK to me.  Does anyone see
a problem?  Can someone volunteer to make the patch in CVS?

Thanks.

Bugs item #465404, was opened at 2001-09-26 14:25
You can respond by visiting:
http://sourceforge.net/tracker/?func=detail&atid=107586&aid=465404&group_id=7586
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Nobody/Anonymous (nobody)
Summary: shared_ptr and self-owning objects

Initial Comment:
Consider following program:

#include <boost/smart_ptr.hpp>
#include <iostream>

class foo {
   public:
      foo(): m_self(this) { std::cout << "foo constructor\n"; }
      ~foo() { std::cout << "foo destructor\n"; }
      void suicide() { m_self.reset(0); }
   private:
      boost::shared_ptr<foo> m_self;
};

int main() {
        foo *foo_ptr = new foo;
        foo_ptr->suicide();
}

When linked with Electric Fence this code fails. This
code invokes undefined behavior in reset method of
shared_ptr: when line if (--*pn == 0) {
checked_delete(px); } executes shared_ptr object gets
destroyed and line *pn = 1 acceses already freed
memory.

Why use m_self?

foo's clients share foo by keeping shared_ptr to it.
foo handles some asynchronous event which may render
foo unneeded to itself (network connection close, for
example), but it still may be needed to clients if any.
OTOH, if no clients use foo at the time of the event
then foo needs to destroy itself. This situation is
best modeled with keeping shared_ptr in foo and letting
foo to reset it when foo thinks it's mission is
complete.

The problem with shared_ptr can be solved with small
modification to reset
method:

void reset(T* p=0) {
    T *old_px = px;
    if ( px == p ) return;  // fix: self-assignment safe
    px = p;
    if (--*pn == 0) {
        *pn = 1;
        checked_delete(old_px);
    }
    else { // allocate new reference counter
        try { pn = new long(1); }  // fix: prevent leak
if new throws
        catch (...) {
            ++*pn;  // undo effect of --*pn above to
meet effects guarantee
            px = old_px; // undo effect of px = p above
            checked_delete(p);
            throw;
        } // catch
    } // allocate new reference counter
} // reset

operator=(const shared_ptr&) and
operator=(std::auto_ptr&) have the same problem and fix
is very similiar.