From: Stefan Z. <ste...@am...> - 2010-12-13 20:19:46
|
I might be insane, but it really looks to me like SwigValueWrapper is doing something heinous. From swig.swg: template<typename T> class SwigValueWrapper { struct SwigMovePointer { T *ptr; SwigMovePointer(T *p) : ptr(p) { } ~SwigMovePointer() { delete ptr; } SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } } pointer; SwigValueWrapper& operator=(const SwigValueWrapper<T>& rhs); SwigValueWrapper(const SwigValueWrapper<T>& rhs); public: SwigValueWrapper() : pointer(0) { } SwigValueWrapper& operator=(const T& t) { SwigMovePointer tmp(new T(t)); pointer = tmp; return *this; } operator T&() const { return *pointer.ptr; } T *operator&() { return pointer.ptr; } }; The weird part is: SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } ... and the nature of the weirdness is that oldptr isn't checked for NULL prior to calling 'delete'. It seems to me that oldptr *will*, in fact, be NULL a great deal of the time. To test this hypothesis, I tried modifying the code: SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; printf("oldptr=0x%x\n", oldptr); ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } Then, I ran the iadd test in python: $ cd Examples/test-suite/python $ make iadd.cpptest ... oldptr=0x0 It may seem strange that the process did not segfault, but in fact, because the deleted object (of type test::A, defined in iadd.i) doesn't have a destructor, it's actually OK to destruct the NULL pointer. So, I tried modifying iadd.i like this: namespace test { struct A { private: char *c; public: int x; A(const int x): x(x), c((char*) malloc(8)) {} ~A () { free(c); } ... }; Presto! This gives me the segfault I was looking for. Note that there's another potential pitfall, although I don't know if it ever happens in practice -- if rhs is the same object as 'this'. So, my proposed patch is: SwigMovePointer& operator=(SwigMovePointer& rhs) { if (ptr) delete ptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } Thanks, Stefan Zager |
From: Matthias <ni...@dr...> - 2010-12-13 20:36:15
|
Am 13.12.2010, 21:19 Uhr, schrieb Stefan Zager <ste...@am...>: > I might be insane, but it really looks to me like SwigValueWrapper is > doing > something heinous. From swig.swg: > > template<typename T> class SwigValueWrapper { > struct SwigMovePointer { > T *ptr; > SwigMovePointer(T *p) : ptr(p) { } > ~SwigMovePointer() { delete ptr; } > SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; > ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } > } pointer; > SwigValueWrapper& operator=(const SwigValueWrapper<T>& rhs); > SwigValueWrapper(const SwigValueWrapper<T>& rhs); > public: > SwigValueWrapper() : pointer(0) { } > SwigValueWrapper& operator=(const T& t) { SwigMovePointer tmp(new > T(t)); pointer = tmp; return *this; } > operator T&() const { return *pointer.ptr; } > T *operator&() { return pointer.ptr; } > }; > > > The weird part is: > > SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; > ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } > > ... and the nature of the weirdness is that oldptr isn't checked for > NULL prior > to calling 'delete'. It seems to me that oldptr *will*, in fact, be > NULL a > great deal of the time. Calling delete on a null pointer is actually valid and should be treated as a noop by the compiler/runtime library. E.g. see http://en.wikipedia.org/wiki/Delete_(C%2B%2B) . > To test this hypothesis, I tried modifying the code: > > SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; > printf("oldptr=0x%x\n", oldptr); ptr = 0; delete oldptr; ptr = rhs.ptr; > rhs.ptr = 0; return *this; } > > Then, I ran the iadd test in python: > > $ cd Examples/test-suite/python > $ make iadd.cpptest > ... > oldptr=0x0 > > > It may seem strange that the process did not segfault, but in fact, > because the > deleted object (of type test::A, defined in iadd.i) doesn't have a > destructor, > it's actually OK to destruct the NULL pointer. It should always be ok to call delete on a NULL pointer. No destruction of any kind takes place. > So, I tried modifying iadd.i like this: > > namespace test { > > struct A { > private: > char *c; > public: > int x; > A(const int x): x(x), c((char*) malloc(8)) {} > ~A () { free(c); } > ... > }; > > > Presto! This gives me the segfault I was looking for. I don't know why you get a segfault here, maybe something todo with the malloc/free, maybe something else. > Note that there's another potential pitfall, although I don't know if it > ever > happens in practice -- if rhs is the same object as 'this'. So, my > proposed > patch is: > > SwigMovePointer& operator=(SwigMovePointer& rhs) { if (ptr) delete > ptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } The original function looks a bit "weird" to ensure thread safety iirc. There was discussion about it on the mailing list here before. I am not sure if your new version still satisfies the problems addressed by the original function. You seem to be right about the function not handling self-assignment properly though. -Matthias |
From: William S F. <ws...@fu...> - 2010-12-13 22:41:11
|
Matthias wrote: > Am 13.12.2010, 21:19 Uhr, schrieb Stefan Zager <ste...@am...>: > > > It should always be ok to call delete on a NULL pointer. No destruction of > any kind takes place. > Indeed and I'd consider poor style to check for NULL before calling delete or free on a pointer. >> So, I tried modifying iadd.i like this: >> >> namespace test { >> >> struct A { >> private: >> char *c; >> public: >> int x; >> A(const int x): x(x), c((char*) malloc(8)) {} >> ~A () { free(c); } >> ... >> }; >> >> >> Presto! This gives me the segfault I was looking for. > > I don't know why you get a segfault here, maybe something todo with the > malloc/free, maybe something else. > I don't know how you are using A, but it is a class waiting for a disaster to occur as it has no copy constructor or assignment operator, yet it contains memory management in constructor/destructor. Any copies of A will use the compiler's default copying resulting in a bit wise copy of all the members including 'c' and as free(c) is called in the destructor, it will be called on the same pointer value for each copy made... kerboomb! >> Note that there's another potential pitfall, although I don't know if it >> ever >> happens in practice -- if rhs is the same object as 'this'. So, my >> proposed >> patch is: >> >> SwigMovePointer& operator=(SwigMovePointer& rhs) { if (ptr) delete >> ptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; } > > The original function looks a bit "weird" to ensure thread safety iirc. > There was discussion about it on the mailing list here before. I am not > sure if your new version still satisfies the problems addressed by the > original function. The operator= was designed to be exception safe. Read the notes in swig.swg for more information on this class. > You seem to be right about the function not handling > self-assignment properly though. The code generation should not ever make handling self assignment necessary. Also note that SwigMovePointer is a private nested class within SwigValueWrapper and as SwigValueWrapper does not allow normal copying or assignment, this prevents any normal copying of the SwigValueWrapper and hence SwigMovePointer instance. If you look at what values SwigMovePointer can take (usually NULL) or the result of a copy constructor call, ie a unique new pointer, I think you'll see that you'll have to go to quite some length to get self assignment and isn't something that SWIG generates. I'm not sure it is worth putting this in, but I'll have a think about it. William |