Menu

#973 bad SwigValueWrapper::operator=

closed-fixed
5
2008-12-22
2008-12-16
No

Hi all,

Please take a look at the operator= in the class SwigValueWrapper:

SwigValueWrapper& operator=(const T& t) { delete tt; tt = new T(t); return *this; }

As you see, tt is being deleted first, then we are copy-constructing a new object of the class T.

If this constructor throws an exception, tt points to the deleted object, so wrapper object becomes unusable.

What's worse, when the wrapper object is being destructed, tt will be deleted second time:

~SwigValueWrapper() { delete tt; }

The version 1.1.36 still contains this bug.

The bugfix is obvious:
SwigValueWrapper& operator=(const T& t) { T *new_tt = new T(t); delete tt; tt = new_tt; return *this; }

Thanks,
Maxim

Discussion

  • Justin Vallon

    Justin Vallon - 2008-12-16

    Not so obvious, though. In the patched code, if delete throws, it will leak new_tt.

    I think the most straightforward would be:

    delete tt; tt = 0; tt = new T(t); return *this;

    Nitpick C++, though: once deleted, you should never delete again. So, if you are trying to handle exceptions from destructors, you should protect against it:

    T *save = tt; tt = 0; delete save; tt = new(T); return *this;

    However, even my std::auto_ptr<T> just does: "delete ptr; ptr = newptr;" and is declared as throw(), meaning it will never throw exceptions (abort if T::~T() throws).

    Destructors that throw are a real can-of-worms.

     
  • David Piepgrass

    David Piepgrass - 2008-12-16

    I wouldn't worry about destructors that throw, as it is very bad practice:

    http://www.parashift.com/c++-faq-lite/exceptions.html#faq-17.3

    However, I often write constructors that throw (not usually copy constructors that throw, mind you). auto_ptr's "delete ptr; ptr = newptr;" would be safe, as newptr is a pointer to an object that was constructed before the delete statement executed.

     
  • William Fulton

    William Fulton - 2008-12-22
    • assigned_to: nobody --> wsfulton
    • status: open --> closed-fixed
     
  • William Fulton

    William Fulton - 2008-12-22

    Added justinvallon's version 2nd version for 1.3.37. In theory I can see the problem, but I'm intrigued about a real life testcase to highlight the issue as I couldn't create one that would trigger the problem... the only scenario I can find is when tt is zero to begin with.

     
  • Maxim Yanchenko

    Maxim Yanchenko - 2008-12-26

    Well, if we want to make really good and safe operator=(), we should think about getting strong exception safety guarantee, which briefly states that the state of an object is not changed if an exception was thrown. This greatly simplifies user code as user doesn't need to care about saving state, making rollback after explicit exception catch etc. So it's better to try getting strong exception safety guarantee in as many cases as possible.

    Currently, the justinvallon's version leaves the object with zero tt (and deleted containee) if an exception was thrown by T's copy-ctor, which is absolutely unnecessary, because we can easily get strong exception guarantee here:

    std::auto_ptr<T> new_tt(new T(t)); T *save = tt; tt = 0; delete save; tt = new_tt.release(); return *this;

    Now, if T's copy-ctor throws, nothing is changed (which is naturally good).
    If T's dtor throws (very unlikely, of course, because throwing dtor leads to many other problems, but if you care), new_tt is not leaked due to auto_ptr.

     
  • William Fulton

    William Fulton - 2008-12-28

    Agreed strong exception safety guarantee is good to aim for if it does not impose any downside during runtime. Use of RAII has provided this and I can't see any real downside in the last proposal. The only issue is that we don't like to add extra dependencies on users and the use of the STL is avoided unless a user is already using it. With this in mind, I've redesigned SwigValueWrapper as shown below to contain an embedded smart pointer (much like auto_ptr) instead of a raw pointer. Please cast your eyes over this as it should essentially be the same code. I've cleaned up the variable names, sorry if it confuses.

    Still, though, I don't know of a use case that demonstrates the originally reported problem, but no harm done in improving the code.

    /* SwigValueWrapper is described in swig.swg */
    template<typename T> class SwigValueWrapper {
    struct Pointer {
    T *ptr;
    Pointer(T *p) : ptr(p) { }
    ~Pointer() { delete ptr; }
    Pointer& operator=(Pointer& rhs) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; }
    } pointer;
    SwigValueWrapper& operator=(const SwigValueWrapper<T>& rhs);
    public:
    SwigValueWrapper() : pointer(0) { }
    SwigValueWrapper(const SwigValueWrapper<T>& rhs) : pointer(new T(*rhs.pointer.ptr)) { }
    SwigValueWrapper(const T& t) : pointer(new T(t)) { }
    SwigValueWrapper& operator=(const T& t) { Pointer tmp(new T(t)); pointer = tmp; return *this; }
    operator T&() const { return *pointer.ptr; }
    T *operator&() { return pointer.ptr; }
    };

    Extra comments in swig.swg:

    * The class offers a strong guarantee of exception safety.
    * With regards to the implementation, the private Pointer nested class is
    * a simple smart pointer with move semantics, much like std::auto_ptr.

     

Log in to post a comment.