[Serialization] [1.36.0] extended_type_info_typeid::destroy should use boost::serialization::access
Somewhere between Boost 1.35.0 and 1.36.0, Boost.Serialization lost the ability to (de-)serialize a pointer to a class with a protected or private destructor. The problem is in extended_type_info_typeid::destroy, which tries to delete a pointer directly rather than using boost::serialization::access::destroy. This is unfortunate, because it's breaking a use of the serialization library in VTK. The patch below fixes the problem and passes all regression tests. Okay to commit to trunk and the 1.36.0 branch? - Doug Index: extended_type_info_typeid.hpp =================================================================== --- extended_type_info_typeid.hpp (revision 47849) +++ extended_type_info_typeid.hpp (working copy) @@ -28,6 +28,7 @@ #include <boost/type_traits/is_polymorphic.hpp> #include <boost/type_traits/remove_const.hpp> +#include <boost/serialization/access.hpp> #include <boost/serialization/singleton.hpp> #include <boost/serialization/extended_type_info.hpp> #include <boost/serialization/factory.hpp> @@ -113,7 +114,7 @@ public: } } void destroy(void const * const p) const{ - delete static_cast<T const *>(p) ; + boost::serialization::access::destroy(static_cast<T const *>(p)); } };
I've been aware of this for sometime. There is a Trak item open on it. I wasn't aware that it broke anything in any other library. I don't know what VTK is. I did take a look at it when it was reported and concluded that the obvious fix might have more subtle implications than first met the eye, so I left it pending on purpose. For example, the proposed fix creates a circular dependency between extended_type_info and serialization whereas before serialization depended on extended_type_info and not the other way around. So, though I'm interested in seeing the situation addressed, I'm not convinced that this is a good way to do it. Also, it is too late to checkin changes to release branch which is already late and should be released ASAP. And I would never recommend that some change with possible unintended consequences be checked into the release branch without having gone through all the testing on the trunk. Robert Ramey Doug Gregor wrote:
Somewhere between Boost 1.35.0 and 1.36.0, Boost.Serialization lost the ability to (de-)serialize a pointer to a class with a protected or private destructor. The problem is in extended_type_info_typeid::destroy, which tries to delete a pointer directly rather than using boost::serialization::access::destroy. This is unfortunate, because it's breaking a use of the serialization library in VTK.
The patch below fixes the problem and passes all regression tests. Okay to commit to trunk and the 1.36.0 branch?
- Doug
Index: extended_type_info_typeid.hpp =================================================================== --- extended_type_info_typeid.hpp (revision 47849) +++ extended_type_info_typeid.hpp (working copy) @@ -28,6 +28,7 @@ #include <boost/type_traits/is_polymorphic.hpp> #include <boost/type_traits/remove_const.hpp>
+#include <boost/serialization/access.hpp> #include <boost/serialization/singleton.hpp> #include <boost/serialization/extended_type_info.hpp> #include <boost/serialization/factory.hpp> @@ -113,7 +114,7 @@ public: } } void destroy(void const * const p) const{ - delete static_cast<T const *>(p) ; + boost::serialization::access::destroy(static_cast<T const *>(p)); } };
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Jul 28, 2008, at 4:36 PM, Robert Ramey wrote:
I've been aware of this for sometime. There is a Trak item open on it.
Oh, sorry; I didn't see it before. For anyone else interested in this, it's ticket #1708: http://svn.boost.org/trac/boost/ticket/1708
I wasn't aware that it broke anything in any other library. I don't know what VTK is.
The Visualization ToolKit (http://www.vtk.org/). It's an open-source library for scientific visualization that's quite popular. The part that's actually relying on Boost.Serialization isn't widely used yet, but fixing the problem means taking a much more widely-used class and making its destructor public... they're not going to be too happy with me if I do that, especially because this code used to work with 1.35.0.
I did take a look at it when it was reported and concluded that the obvious fix might have more subtle implications than first met the eye, so I left it pending on purpose. For example, the proposed fix creates a circular dependency between extended_type_info and serialization whereas before serialization depended on extended_type_info and not the other way around.
Well, "access" is a pretty basic facility, and all of this code is part of the serialization library. If extended_type_info were part of a completely separate library, I'd be more concerned about the circular dependency.
So, though I'm interested in seeing the situation addressed, I'm not convinced that this is a good way to do it. Also, it is too late to checkin changes to release branch which is already late and should be released ASAP. And I would never recommend that some change with possible unintended consequences
This is a regression in a *very* popular library. Such things are exactly why we have beta releases, and are cause for delaying a release further.
be checked into the release branch without having gone through all the testing on the trunk.
Shall I commit it to the trunk, then? - Doug
participants (2)
-
Doug Gregor -
Robert Ramey