Re: [boost] shared_ptr bug?

From: David Abrahams (david.abrahams_at_[hidden])
Date: 2001-10-18 10:24:57

I agree with the O.P. that we should make the fix. I think that path is far
superior to the one which restricts the legal usage of shared_ptr. I also
think that we should look anew at our implementation: is the
two-pointer-separate-count design causing so much complication that the
correctness of the implementation is hard to manage? It seems to me that the
implementation of shared_ptr is much more complicated than it should need to
be. Can we do better with a single-pointer-chained-count design?

+-ptr-+     +-count-+     +-target-+
|     |     |   |   |     |        |
|  *=======>| 1 | *======>|        |
|     |     |   |   |     |        |
+-----+     +---+---+     +--------+

===================================================
  David Abrahams, C++ library designer for hire
 resume: http://users.rcn.com/abrahams/resume.html

        C++ Booster (http://www.boost.org)
          email: david.abrahams_at_[hidden]
===================================================

----- Original Message -----
From: "Greg Colvin" <gcolvin_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Thursday, October 18, 2001 1:50 AM
Subject: [boost] shared_ptr bug?

> 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.
>
>
>
> Info: http://www.boost.org  Unsubscribe:
<mailto:boost-unsubscribe_at_[hidden]>
>
> Your use of Yahoo! Groups is subject to http://docs.yahoo.com/info/terms/
>
>
>