[Ipopt-tickets] [Ipopt] #162: unsave SmartPtr copy semantics, meaningless comparisons, missing conversions
Ipopt
coin-trac at coin-or.org
Sat Mar 30 14:03:19 EDT 2013
#162: unsave SmartPtr copy semantics, meaningless comparisons, missing
conversions
---------------------+----------------------
Reporter: guest | Owner: stefan
Type: defect | Status: assigned
Priority: normal | Component: Ipopt
Version: 3.9 | Severity: normal
Resolution: | Keywords: SmartPtr
---------------------+----------------------
Changes (by stefan):
* owner: ipopt-team => stefan
* status: new => assigned
Old description:
> Hi,
>
> there seem to exist several issues with the implementation of SmartPtr
> that make it unsafe to use in certain contexts.
> I have not actually experienced any bug myself, this is solely derived
> from studying the source code, triggered by this post:
> http://www.c-plusplus.de/forum/288762 (in german).
> ---------------------------------------------------------------------------------------------------------------------------------
> 1. SmartPtr does not handle self assignment properly if it is the sole
> referent to an object.
> Consider
>
> struct foo : ReferencedObject { ... };
>
> SmartPtr<foo> p = new foo;
> p = p; // (1)
> p = GetRawPointer( p ); // (2)
>
> Both (1) and (2) will cause p to be a null pointer afterwards. This is
> because in SmartPtr<T>::SetFromSmartPtr_ the old pointer is released
> before the reference count for the right hand side is incremented.
> This makes SmartPtr incompatible with standard containers and mutating
> standard algorithms.
> The fix would be straight forward by changing the order:
>
> template <class T>
> SmartPtr<T>& SmartPtr<T>::SetFromRawPtr_(T* rhs)
> {
> #ifdef IP_DEBUG_SMARTPTR
> DBG_START_METH(
> "SmartPtr<T>& SmartPtr<T>::SetFromRawPtr_(T* rhs)",
> dbg_smartptr_verbosity);
> #endif
>
> if (rhs != 0)
> rhs->AddRef(this);
>
> // Release any old pointer
> ReleasePointer_();
>
> ptr_ = rhs;
>
> return *this;
> }
> SmartPtr<T>::ReleasePointer_ must be patched to not modify ptr_ - since
> the only other function that calles it is the destructor, this is not a
> problem:
> template <class T>
> void SmartPtr<T>::ReleasePointer_()
> {
> #ifdef IP_DEBUG_SMARTPTR
> DBG_START_METH(
> "void SmartPtr<T>::ReleasePointer()",
> dbg_smartptr_verbosity);
> #endif
>
> if (ptr_) {
> ptr_->ReleaseRef(this);
> if (ptr_->ReferenceCount() == 0) {
> delete ptr_;
> }
> }
> }
> An explicit test for self assignment could be used but is not the best
> choice since it only conceals the logic error and it might mean an
> unnecessary performance hit since self assignment ist quite rare but the
> test would have to be done during each assignment.
> ---------------------------------------------------------------------------------------------------------------------------------
> 2. The comparison done in
> template <class U1, class U2>
> bool ComparePointers(const U1* lhs, const U2* rhs)
> either does not work as intended or is meaningless if different types are
> involved, this affects the overloaded operators == and !=
>
> consider the current code in trunk:
> 561 template <class U1, class U2>
> 562 bool ComparePointers(const U1* lhs, const U2* rhs)
> 563 {
> 564 #ifdef IP_DEBUG_SMARTPTR
> 565 DBG_START_FUN(
> 566 "bool ComparePtrs(const U1* lhs, const U2* rhs)",
> 567 dbg_smartptr_verbosity);
> 568 #endif
> 569
> 570 if (lhs == rhs) {
> 571 return true;
> 572 }
> 573
> 574 // Even if lhs and rhs point to the same object
> 575 // with different interfaces U1 and U2, we cannot guarantee
> that
> 576 // the value of the pointers will be equivalent. We can
> 577 // guarantee this if we convert to void*
> 578 const void* v_lhs = static_cast<const void*>(lhs);
> 579 const void* v_rhs = static_cast<const void*>(rhs);
> 580 if (v_lhs == v_rhs) {
> 581 return true;
> 582 }
> 583
> 584 // They must not be the same
> 585 return false;
> 586 }
> line 570 requires the template arguments to be compatible, that is one of
> them must be convertible to the other. Specifically both are of the same
> type (safe for const qualification), one of them is (const) void or one
> is a public and unambigous base class of the other.
> In all these cases the comparision in line 580 is meaningless, since it
> will never yield any result that differs from that in line 570.
> If line 580 is intended to handle the case where there existist multiple
> interfaces to the same object that are not derived from each other (i.e.
> multiple inheritance) it will not work as intended, because the
> conversion to const void* by means of a static_cast will not result in
> pointers to the complete objects. And line 570 would prevent the compiler
> to instantiate the function for such a combination of template parameters
> in the first place.
> A conversion to a pointer that points to the most derived object can be
> done by means of dynamic_cast, thus:
> template <class U1, class U2>
> bool ComparePointers(const U1* lhs, const U2* rhs)
> {
> #ifdef IP_DEBUG_SMARTPTR
> DBG_START_FUN(
> "bool ComparePtrs(const U1* lhs, const U2* rhs)",
> dbg_smartptr_verbosity);
> #endif
>
> return dynamic_cast<const void*>(lhs) == dynamic_cast<const
> void*>(rhs);
> }
> This requires lhs and rhs to be pointers to polymorphic types which is
> not a problem since pointees must be derived from ReferencedObject.
> Using dynamic_cast always might result in inacceptable performance since
> most comparisions are probably using compatible types, therefore several
> overloads could be used, for example:
> template <typename T>
> struct identity
> {
> typedef T type;
> };
> // the special case if both arguments are of the same type safe for const
> qualification
> template <typename T>
> bool ComparePointers(const T* lhs, const T* rhs)
> {
> return lhs == rhs;
> }
> // called if the right hand side is convertible to the type of the left
> hand side
> template <typename T>
> bool ComparePointers(const T* lhs, typename identity<const T>::type* rhs)
> {
> return ComparePointers(lhs, rhs);
> }
> // called if the left hand side is convertible to the type of the right
> hand side
> template <typename T>
> bool ComparePointers(typename identity<const T>::type* lhs, const T* rhs)
> {
> return ComparePointers(lhs, rhs);
> }
> // the general case
> template <typename T, typename U>
> bool ComparePointers(T* lhs, U* rhs)
> {
> return dynamic_cast<const void*>(lhs) == dynamic_cast<const
> void*>(rhs);
> }
>
> There is a simpler option by using the fact that all types used to
> instantiate a SmartPtr must be derived from ReferencedObject:
> template <typename T, typename U>
> bool ComparePointers(T* lhs, U* rhs)
> {
> const ReferencedObject* lhs_ = lhs;
> const ReferencedObject* rhs_ = rhs;
> return lhs_ == rhs_;
> }
> No other overloads are needed in this case.
> It should be made explicit in the documentation that only one
> ReferencedObject may exist for every most derived object, so multiple
> inheritance requires virtual inheritance.
> ---------------------------------------------------------------------------------------------------------------------------------
> 3. SmartPtr is missing implicit conversions to SmartPtr to bases. This is
> inconvenient since it forces user code to either only use SmartPtr to
> base classes and explicit casts, or rely on the intrusive nature of
> SmartPtr (which should be an implementation detail).
> template <class T>
> template <class U>
> SmartPtr<T>::SmartPtr(const SmartPtr<U>& copy)
> :
> ptr_(0)
> {
> (void) SetFromRawPtr_(GetRawPtr(copy));
> }
>
> template <class T>
> template <class U>
> SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<U>& rhs)
> {
> return SetFromRawPtr_(GetRawPtr(copy));
> }
> should be provided.
> explicit conversion functions that model static_cast and dynamic_cast
> might be convenient as there exists already the member function ConstPtr:
>
> template <class T, class U>
> SmartPtr<T> StaticPtr(const SmartPtr<U>& smart_ptr)
> {
> return static_cast<T*>(GetRawPtr(smart_ptr));
> }
> template <class T, class U>
> SmartPtr<T> DynamicPtr(const SmartPtr<U>& smart_ptr)
> {
> return dynamic_cast<T*>(GetRawPtr(smart_ptr));
> }
>
> ---------------------------------------------------------------------------------------------------------------------------------
> 4. It may be convenient to additionally explicitily define swap, and
> overloaded relational operators in order to make SmartPtr usable with
> associative containers like std::set (it probably does not really make
> sense to define ordering for different SmartPtr types, but it could be
> done by relying on the unique ReferencedObject subobject):
>
> // as member function
> template <class T>
> void SmartPtr<T>::swap(SmartPtr<T>& other)
> {
> using std::swap;
> swap(ptr_, other.ptr_);
> }
>
> // as free function
> template <class T>
> void swap(SmartPtr<T>& a, SmartPtr<T>& b)
> {
> a.swap(b);
> }
>
> template <class T>
> bool operator<(const SmartPtr<T>& lhs, const SmartPtr<T>& rhs)
> {
> return std::less<T*>()(lhs.ptr_, rhs.ptr_);
> }
> template <class T> bool operator> (const SmartPtr<T>& lhs, const
> SmartPtr<T>& rhs) { return rhs < lhs; }
> template <class T> bool operator<=(const SmartPtr<T>& lhs, const
> SmartPtr<T>& rhs) { return !( rhs < lhs ); }
> template <class T> bool operator>=(const SmartPtr<T>& lhs, const
> SmartPtr<T>& rhs) { return !( lhs < rhs ); }
New description:
Hi,
there seem to exist several issues with the implementation of !SmartPtr
that make it unsafe to use in certain contexts.
I have not actually experienced any bug myself, this is solely derived
from studying the source code, triggered by this post:
http://www.c-plusplus.de/forum/288762 (in german).
=== 1. !SmartPtr does not handle self assignment properly if it is the
sole referent to an object. ===
Consider
{{{
struct foo : ReferencedObject { ... };
SmartPtr<foo> p = new foo;
p = p; // (1)
p = GetRawPointer( p ); // (2)
}}}
Both (1) and (2) will cause p to be a null pointer afterwards. This is
because in {{{SmartPtr<T>::SetFromSmartPtr_}}} the old pointer is released
before the reference count for the right hand side is incremented.
This makes !SmartPtr incompatible with standard containers and mutating
standard algorithms.
The fix would be straight forward by changing the order:
{{{
template <class T>
SmartPtr<T>& SmartPtr<T>::SetFromRawPtr_(T* rhs)
{
#ifdef IP_DEBUG_SMARTPTR
DBG_START_METH(
"SmartPtr<T>& SmartPtr<T>::SetFromRawPtr_(T* rhs)",
dbg_smartptr_verbosity);
#endif
if (rhs != 0)
rhs->AddRef(this);
// Release any old pointer
ReleasePointer_();
ptr_ = rhs;
return *this;
}
}}}
{{{SmartPtr<T>::ReleasePointer_}}} must be patched to not modify ptr_ -
since the only other function that calles it is the destructor, this is
not a problem:
{{{
template <class T>
void SmartPtr<T>::ReleasePointer_()
{
#ifdef IP_DEBUG_SMARTPTR
DBG_START_METH(
"void SmartPtr<T>::ReleasePointer()",
dbg_smartptr_verbosity);
#endif
if (ptr_) {
ptr_->ReleaseRef(this);
if (ptr_->ReferenceCount() == 0) {
delete ptr_;
}
}
}
}}}
An explicit test for self assignment could be used but is not the best
choice since it only conceals the logic error and it might mean an
unnecessary performance hit since self assignment ist quite rare but the
test would have to be done during each assignment.
=== 2. The comparison done in {{{template <class U1, class U2> bool
ComparePointers(const U1* lhs, const U2* rhs)}}} either does not work as
intended or is meaningless if different types are involved, this affects
the overloaded operators == and !=. ===
Consider the current code in trunk:
{{{
561 template <class U1, class U2>
562 bool ComparePointers(const U1* lhs, const U2* rhs)
563 {
564 #ifdef IP_DEBUG_SMARTPTR
565 DBG_START_FUN(
566 "bool ComparePtrs(const U1* lhs, const U2* rhs)",
567 dbg_smartptr_verbosity);
568 #endif
569
570 if (lhs == rhs) {
571 return true;
572 }
573
574 // Even if lhs and rhs point to the same object
575 // with different interfaces U1 and U2, we cannot guarantee
that
576 // the value of the pointers will be equivalent. We can
577 // guarantee this if we convert to void*
578 const void* v_lhs = static_cast<const void*>(lhs);
579 const void* v_rhs = static_cast<const void*>(rhs);
580 if (v_lhs == v_rhs) {
581 return true;
582 }
583
584 // They must not be the same
585 return false;
586 }
}}}
line 570 requires the template arguments to be compatible, that is one of
them must be convertible to the other. Specifically both are of the same
type (safe for const qualification), one of them is (const) void or one is
a public and unambigous base class of the other.
In all these cases the comparision in line 580 is meaningless, since it
will never yield any result that differs from that in line 570.
If line 580 is intended to handle the case where there existist multiple
interfaces to the same object that are not derived from each other (i.e.
multiple inheritance) it will not work as intended, because the conversion
to const void* by means of a static_cast will not result in pointers to
the complete objects. And line 570 would prevent the compiler to
instantiate the function for such a combination of template parameters in
the first place.
A conversion to a pointer that points to the most derived object can be
done by means of dynamic_cast, thus:
{{{
template <class U1, class U2>
bool ComparePointers(const U1* lhs, const U2* rhs)
{
#ifdef IP_DEBUG_SMARTPTR
DBG_START_FUN(
"bool ComparePtrs(const U1* lhs, const U2* rhs)",
dbg_smartptr_verbosity);
#endif
return dynamic_cast<const void*>(lhs) == dynamic_cast<const
void*>(rhs);
}
}}}
This requires lhs and rhs to be pointers to polymorphic types which is not
a problem since pointees must be derived from !ReferencedObject.
Using dynamic_cast always might result in inacceptable performance since
most comparisions are probably using compatible types, therefore several
overloads could be used, for example:
{{{
template <typename T>
struct identity
{
typedef T type;
};
// the special case if both arguments are of the same type safe for const
qualification
template <typename T>
bool ComparePointers(const T* lhs, const T* rhs)
{
return lhs == rhs;
}
// called if the right hand side is convertible to the type of the left
hand side
template <typename T>
bool ComparePointers(const T* lhs, typename identity<const T>::type* rhs)
{
return ComparePointers(lhs, rhs);
}
// called if the left hand side is convertible to the type of the right
hand side
template <typename T>
bool ComparePointers(typename identity<const T>::type* lhs, const T* rhs)
{
return ComparePointers(lhs, rhs);
}
// the general case
template <typename T, typename U>
bool ComparePointers(T* lhs, U* rhs)
{
return dynamic_cast<const void*>(lhs) == dynamic_cast<const
void*>(rhs);
}
}}}
There is a simpler option by using the fact that all types used to
instantiate a !SmartPtr must be derived from !ReferencedObject:
{{{
template <typename T, typename U>
bool ComparePointers(T* lhs, U* rhs)
{
const ReferencedObject* lhs_ = lhs;
const ReferencedObject* rhs_ = rhs;
return lhs_ == rhs_;
}
}}}
No other overloads are needed in this case.
It should be made explicit in the documentation that only one
!ReferencedObject may exist for every most derived object, so multiple
inheritance requires virtual inheritance.
=== 3. !SmartPtr is missing implicit conversions to !SmartPtr to bases.
===
This is inconvenient since it forces user code to either only use
!SmartPtr to base classes and explicit casts, or rely on the intrusive
nature of !SmartPtr (which should be an implementation detail).
{{{
template <class T>
template <class U>
SmartPtr<T>::SmartPtr(const SmartPtr<U>& copy)
:
ptr_(0)
{
(void) SetFromRawPtr_(GetRawPtr(copy));
}
template <class T>
template <class U>
SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<U>& rhs)
{
return SetFromRawPtr_(GetRawPtr(copy));
}
}}}
should be provided.
explicit conversion functions that model static_cast and dynamic_cast
might be convenient as there exists already the member function ConstPtr:
{{{
template <class T, class U>
SmartPtr<T> StaticPtr(const SmartPtr<U>& smart_ptr)
{
return static_cast<T*>(GetRawPtr(smart_ptr));
}
template <class T, class U>
SmartPtr<T> DynamicPtr(const SmartPtr<U>& smart_ptr)
{
return dynamic_cast<T*>(GetRawPtr(smart_ptr));
}
}}}
=== 4. It may be convenient to additionally explicitly define swap, and
overloaded relational operators in order to make !SmartPtr usable with
associative containers like std::set ===
It probably does not really make sense to define ordering for different
!SmartPtr types, but it could be done by relying on the unique
!ReferencedObject subobject:
{{{
// as member function
template <class T>
void SmartPtr<T>::swap(SmartPtr<T>& other)
{
using std::swap;
swap(ptr_, other.ptr_);
}
// as free function
template <class T>
void swap(SmartPtr<T>& a, SmartPtr<T>& b)
{
a.swap(b);
}
template <class T>
bool operator<(const SmartPtr<T>& lhs, const SmartPtr<T>& rhs)
{
return std::less<T*>()(lhs.ptr_, rhs.ptr_);
}
template <class T> bool operator> (const SmartPtr<T>& lhs, const
SmartPtr<T>& rhs) { return rhs < lhs; }
template <class T> bool operator<=(const SmartPtr<T>& lhs, const
SmartPtr<T>& rhs) { return !( rhs < lhs ); }
template <class T> bool operator>=(const SmartPtr<T>& lhs, const
SmartPtr<T>& rhs) { return !( lhs < rhs ); }
}}}
--
Comment:
Formatted description for better readability.
--
Ticket URL: <https://projects.coin-or.org/ticket/162#comment:1>
Ipopt <http://projects.coin-or.org/Ipopt>
Interior-point optimizer for nonlinear programs.
More information about the Ipopt-tickets
mailing list