Re: [boost] [shared_ptr] dangerous implementation ofmove constructor

From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2007-04-14 19:50:03

I think that clearing px after a move isn't a good idea. It is true that we 
need more experience with move semantics to see what works best, but in my 
opinion anything beyond destruction and assignment on an object that has 
been "moved" should -- in principle -- be undefined behavior.

>From certain point of view this complicates the invariant a bit, but it's 
very simple to say that after you move an object you can only assign it a 
new value, or destroy it.

It's also worth mentioning that (much like a raw pointer) there are other 
ways to end up with a shared_ptr object such that *p is undefined  behavior 
despite that "if(p)" is true.

Emil Dotchevski

----- Original Message ----- 
From: "Howard Hinnant" <howard.hinnant_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Saturday, April 14, 2007 4:12 PM
Subject: Re: [boost] [shared_ptr] dangerous implementation ofmove 
constructor

> On Apr 14, 2007, at 1:28 PM, Peter Dimov wrote:
>
>> Joe Gottman wrote:
>>>    I see that rvalue reference support has just been added to the
>>> shared_ptr template, and I think that there's a subtle bug in the
>>> implementation.  It is implemented as follows:
>>>
>>> shared_ptr( shared_ptr && r ): px( r.px ), pn() // never throws
>>>        {
>>>            pn.swap( r.pn );
>>>        }
>>>
>>> The problem is that after the move constructor has been called the
>>> moved -from shared_ptr, r, is in a dangerously inconsistent state.
>>> Its has an empty shared count, but it contains a non-null pointer.
>>> Usually it
>>> should be either destructed or assigned to shortly after the move
>>> constructor is called, in which case all will be well.  However,
>>> if it
>>> remains unchanged then there is the danger that someone might try to
>>> dereference it after r.px has been deleted.  This, of course, could
>>> result in a hard-to-track access violation.  To see what I mean,
>>> consider the following rather silly code:
>>>
>>>     shared_ptr<int> r(new int(1));
>>>     shared_ptr<int> s(std::move(r));
>>>     s.reset();
>>>     if (r) { //r.get() != 0, so this returns true
>>>        cout << *r; // Uh-oh
>>>     }
>>
>> The problem with the above code does indeed exist, but it is not a
>> bug in
>> the implementation (let alone a subtle one) - in the sense that the
>> implementation is deliberately written in the way it's written, to not
>> provide a guarantee about the consistency of r after the move. You
>> are right
>> that we can clear r.px easily for shared_ptr, but it's more
>> important to
>> think about the general case: is the user allowed to assume
>> anything about
>> the state of an object after it has been moved from?
>>
>> void f( T t1 )
>> {
>>     T t2( std::move( t1 ) );
>>     // What happens if I use t1 here?
>> }
>>
>> Using t1 after the move looks like a programming error to me.
>
> Fwiw Peter asked me to look at this.  Imho after a move from an
> object it should be in a self-consistent state.  All internal
> invariants should hold.  This answer is purposefully vague, and could
> be used to support either outcome.
>
> A very reasonable invariant for a shared_ptr r would be that if (r)
> then *r is safe.
>
> Another very reasonable invariant for a shared_ptr r would be that
> after a move, if (r) does not imply that you can *r.
>
> There is nothing in the rvalue ref proposals that really nails this
> down.  It is up to each class to define its behavior after a move.
> At the very least it should support assignment and destruction in
> order to work with generic code.  Beyond that I'd say the class
> should be free to implement whatever semantics / interface it feels
> best.
>
> In this particular case, I think it might be worth the extra
> assignment to null r.px just to make the class invariant simpler.
> However that statement falls well short of saying that the standard
> should require such behavior, or even that this behavior isn't ok
> with respect to putting this shared_ptr into std::containers or using
> with std::algorithms.  Furthermore I'm anxious to see what "best
> practices" in this area are born out by field experience.  And boost
> is a great place to have such experiments! :-)
>
> -Howard
>
> _______________________________________________
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost
>