diff --git a/include/boost/archive/detail/basic_iarchive.hpp b/include/boost/archive/detail/basic_iarchive.hpp index 81f6b08e9..89b207187 100644 --- a/include/boost/archive/detail/basic_iarchive.hpp +++ b/include/boost/archive/detail/basic_iarchive.hpp @@ -10,6 +10,7 @@ // basic_iarchive.hpp: // (C) Copyright 2002 Robert Ramey - http://www.rrsd.com . +// Copyright 2026 Gennaro Prota. // Distributed under the Boost Software License, Version 1.0. // (See accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt) @@ -68,6 +69,14 @@ class BOOST_SYMBOL_VISIBLE basic_iarchive : virtual BOOST_ARCHIVE_DECL ~basic_iarchive(); // note: NOT part of the public API. BOOST_ARCHIVE_DECL void next_object_pointer(void *t); + // Note: *not* part of the public API. Called by load_object_ptr once the + // object has been constructed, so that a throw while loading its members + // leaves it reclaimable by delete_created_pointers. + BOOST_ARCHIVE_DECL void object_constructed(); + // Note: *not* part of the public API. Called when an owning smart pointer + // takes over the object just loaded, so that delete_created_pointers + // leaves it to that smart pointer instead of freeing it as well. + BOOST_ARCHIVE_DECL void object_adopted(); BOOST_ARCHIVE_DECL void register_basic_serializer( const basic_iserializer & bis ); diff --git a/include/boost/archive/detail/iserializer.hpp b/include/boost/archive/detail/iserializer.hpp index 6a35b9992..a048d5a87 100644 --- a/include/boost/archive/detail/iserializer.hpp +++ b/include/boost/archive/detail/iserializer.hpp @@ -350,13 +350,18 @@ BOOST_DLLEXPORT void pointer_iserializer::load_object_ptr( ); } BOOST_CATCH(...){ - // if we get here the load_construct failed. The heap_allocation - // will be automatically deleted so we don't have to do anything - // special here. + // The load_construct failed, so the object was never constructed. + // Since heap_allocation() has already released its guard, free the + // raw storage here, without running a destructor on it. + detail::heap_allocation::invoke_delete(static_cast(t)); BOOST_RETHROW; } BOOST_CATCH_END + // The object exists from here on, so let the archive reclaim it if + // loading its members throws. + ar.object_constructed(); + ar_impl >> boost::serialization::make_nvp(NULL, * static_cast(t)); } diff --git a/include/boost/archive/detail/polymorphic_iarchive_route.hpp b/include/boost/archive/detail/polymorphic_iarchive_route.hpp index d862959c0..747c4326b 100644 --- a/include/boost/archive/detail/polymorphic_iarchive_route.hpp +++ b/include/boost/archive/detail/polymorphic_iarchive_route.hpp @@ -83,6 +83,9 @@ class polymorphic_iarchive_route : void delete_created_pointers() BOOST_OVERRIDE { ArchiveImplementation::delete_created_pointers(); } + void object_adopted() BOOST_OVERRIDE { + ArchiveImplementation::object_adopted(); + } void reset_object_address( const void * new_address, const void * old_address diff --git a/include/boost/archive/polymorphic_iarchive.hpp b/include/boost/archive/polymorphic_iarchive.hpp index fadec59b1..9b5b62143 100644 --- a/include/boost/archive/polymorphic_iarchive.hpp +++ b/include/boost/archive/polymorphic_iarchive.hpp @@ -10,6 +10,7 @@ // polymorphic_iarchive.hpp // (C) Copyright 2002 Robert Ramey - http://www.rrsd.com . +// Copyright 2026 Gennaro Prota. // Distributed under the Boost Software License, Version 1.0. // (See accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt) @@ -126,6 +127,11 @@ class BOOST_SYMBOL_VISIBLE polymorphic_iarchive_impl : virtual boost::serialization::library_version_type get_library_version() const = 0; virtual unsigned int get_flags() const = 0; virtual void delete_created_pointers() = 0; + // Note: not pure, so that archives written against an earlier release + // still compile. Such an archive keeps the old behaviour, in which + // delete_created_pointers also frees objects an owning smart pointer + // has taken over. + virtual void object_adopted() {} virtual void reset_object_address( const void * new_address, const void * old_address diff --git a/include/boost/serialization/scoped_ptr.hpp b/include/boost/serialization/scoped_ptr.hpp index 066254ca1..a25ba40bf 100644 --- a/include/boost/serialization/scoped_ptr.hpp +++ b/include/boost/serialization/scoped_ptr.hpp @@ -41,6 +41,7 @@ namespace serialization { T* r; ar >> boost::serialization::make_nvp("scoped_ptr", r); t.reset(r); + ar.object_adopted(); } template diff --git a/include/boost/serialization/shared_ptr.hpp b/include/boost/serialization/shared_ptr.hpp index 7ea4e2241..4c5446193 100644 --- a/include/boost/serialization/shared_ptr.hpp +++ b/include/boost/serialization/shared_ptr.hpp @@ -140,6 +140,7 @@ inline void load( shared_ptr_helper_id ); h.reset(t,r); + ar.object_adopted(); } #else @@ -161,6 +162,7 @@ inline void load( shared_ptr_helper_id ); h.reset(t,r); + ar.object_adopted(); } #endif @@ -256,6 +258,7 @@ inline void load( shared_ptr_helper_id ); h.reset(t,r); + ar.object_adopted(); } template diff --git a/include/boost/serialization/unique_ptr.hpp b/include/boost/serialization/unique_ptr.hpp index 608ca0817..09417554c 100644 --- a/include/boost/serialization/unique_ptr.hpp +++ b/include/boost/serialization/unique_ptr.hpp @@ -48,6 +48,7 @@ inline void load( ar >> BOOST_SERIALIZATION_NVP(tx); // note that the reset automagically maintains the reference count t.reset(tx); + ar.object_adopted(); } // split non-intrusive serialization function member into separate diff --git a/src/basic_iarchive.cpp b/src/basic_iarchive.cpp index 559b5d4dd..1c6eb2047 100644 --- a/src/basic_iarchive.cpp +++ b/src/basic_iarchive.cpp @@ -2,6 +2,7 @@ // basic_archive.cpp: // (C) Copyright 2002 Robert Ramey - http://www.rrsd.com . +// Copyright 2026 Gennaro Prota. // Distributed under the Boost Software License, Version 1.0. // (See accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt) @@ -31,6 +32,8 @@ namespace std{ #define BOOST_SERIALIZATION_SOURCE #include +#include + #include #include #include @@ -173,16 +176,44 @@ class basic_iarchive_impl { void * object; const basic_iserializer * bis; version_type version; + // The object currently being loaded through a pointer: its index in + // object_id_vector, whether delete_created_pointers may reclaim it + // and whether its constructor has run yet. + std::size_t pointer_object_id; + bool pointer_reclaimable; + bool pointer_constructed; pending() : object(NULL), bis(NULL), - version(0) + version(0), + pointer_object_id(0), + pointer_reclaimable(false), + pointer_constructed(false) {} } m_pending; + // Set while a created pointer is being loaded. Only the outermost such + // load may be reclaimed by delete_created_pointers: anything created + // below it is reachable from it, so freeing it runs the destructors of + // the objects it owns. + bool m_loading_created_pointer; + + // The object which the pointer load that just finished created and + // flagged for reclamation, if any. Lets an owning smart pointer take + // that object over: see object_adopted(). + struct last_created { + std::size_t object_id; + bool reclaimable; + last_created() : + object_id(0), + reclaimable(false) + {} + } m_last_created; + basic_iarchive_impl(unsigned int flags) : m_archive_library_version(BOOST_ARCHIVE_VERSION()), - m_flags(flags) + m_flags(flags), + m_loading_created_pointer(false) {} void set_library_version(library_version_type archive_library_version){ m_archive_library_version = archive_library_version; @@ -212,6 +243,22 @@ class basic_iarchive_impl { next_object_pointer(void * t){ m_pending.object = t; } + void + object_constructed(){ + m_pending.pointer_constructed = true; + if(m_pending.pointer_reclaimable){ + object_id_vector[m_pending.pointer_object_id].loaded_as_pointer + = true; + } + } + void + object_adopted(){ + if(m_last_created.reclaimable){ + object_id_vector[m_last_created.object_id].loaded_as_pointer + = false; + m_last_created.reclaimable = false; + } + } void delete_created_pointers(); class_id_type register_type( const basic_pointer_iserializer & bpis @@ -424,6 +471,10 @@ basic_iarchive_impl::load_pointer( m_moveable_objects.is_pointer = true; serialization::state_saver w(m_moveable_objects.is_pointer); + // An adopting smart pointer may only take over an object which this very + // call creates, so forget any object the previous one left behind. + m_last_created.reclaimable = false; + class_id_type cid; load(ar, cid); @@ -480,39 +531,72 @@ basic_iarchive_impl::load_pointer( // save state serialization::state_saver w_start(m_moveable_objects.start); + // An object created by an enclosing pointer load is owned by that + // object, so only the outermost one is a candidate for reclamation. + const bool root = ! m_loading_created_pointer; + serialization::state_saver n(m_loading_created_pointer); + serialization::state_saver p_id(m_pending.pointer_object_id); + serialization::state_saver p_rec(m_pending.pointer_reclaimable); + serialization::state_saver p_con(m_pending.pointer_constructed); + m_loading_created_pointer = true; + m_pending.pointer_reclaimable = false; + m_pending.pointer_constructed = false; + // allocate space on the heap for the object - to be constructed later t = bpis_ptr->heap_allocation(); BOOST_ASSERT(NULL != t); - if(! tracking){ - bpis_ptr->load_object_ptr(ar, t, co.file_version); + BOOST_TRY{ + if(! tracking){ + bpis_ptr->load_object_ptr(ar, t, co.file_version); + } + else{ + serialization::state_saver x(m_pending.object); + serialization::state_saver y(m_pending.bis); + serialization::state_saver z(m_pending.version); + + m_pending.bis = & bpis_ptr->get_basic_serializer(); + m_pending.version = co.file_version; + + // predict next object id to be created + const size_t ui = object_id_vector.size(); + + serialization::state_saver w_end(m_moveable_objects.end); + + // add to list of serialized objects so that we can properly handle + // cyclic structures + object_id_vector.push_back(aobject(t, cid)); + m_pending.pointer_object_id = ui; + m_pending.pointer_reclaimable = root; + + // remember that that the address of these elements could change + // when we make another call so don't use the address. Once the + // object has been constructed load_object_ptr calls back through + // object_constructed(), which flags it for reclamation by + // delete_created_pointers should loading its members throw. + bpis_ptr->load_object_ptr( + ar, + t, + m_pending.version + ); + } } - else{ - serialization::state_saver x(m_pending.object); - serialization::state_saver y(m_pending.bis); - serialization::state_saver z(m_pending.version); - - m_pending.bis = & bpis_ptr->get_basic_serializer(); - m_pending.version = co.file_version; - - // predict next object id to be created - const size_t ui = object_id_vector.size(); - - serialization::state_saver w_end(m_moveable_objects.end); - - // add to list of serialized objects so that we can properly handle - // cyclic structures - object_id_vector.push_back(aobject(t, cid)); - - // remember that that the address of these elements could change - // when we make another call so don't use the address - bpis_ptr->load_object_ptr( - ar, - t, - m_pending.version - ); - object_id_vector[ui].loaded_as_pointer = true; + BOOST_CATCH(...){ + // The constructor never ran, so load_object_ptr has freed the raw + // storage. Clear the caller's pointer: otherwise the destructor of + // an enclosing object would delete storage which is already gone. + if(! m_pending.pointer_constructed){ + t = NULL; + } + BOOST_RETHROW; } + BOOST_CATCH_END + + // The load succeeded: remember what it flagged, so that a smart pointer + // adopting the object can take responsibility for freeing it. + m_last_created.object_id = m_pending.pointer_object_id; + m_last_created.reclaimable = m_pending.pointer_reclaimable + && m_pending.pointer_constructed; return bpis_ptr; } @@ -532,6 +616,16 @@ basic_iarchive::next_object_pointer(void *t){ pimpl->next_object_pointer(t); } +BOOST_ARCHIVE_DECL void +basic_iarchive::object_constructed(){ + pimpl->object_constructed(); +} + +BOOST_ARCHIVE_DECL void +basic_iarchive::object_adopted(){ + pimpl->object_adopted(); +} + BOOST_ARCHIVE_DECL basic_iarchive::basic_iarchive(unsigned int flags) : pimpl(new basic_iarchive_impl(flags)) diff --git a/test/Jamfile.v2 b/test/Jamfile.v2 index ee765893c..44e1955ed 100644 --- a/test/Jamfile.v2 +++ b/test/Jamfile.v2 @@ -81,6 +81,7 @@ test-suite "serialization" : [ test-bsl-run_files test_forward_list : A : : [ requires cxx11_hdr_forward_list ] ] # BOOST_NO_CXX11_HDR_FORWARD_LIST [ test-bsl-run_files test_forward_list_ptrs : A : : [ requires cxx11_hdr_forward_list ] ] # BOOST_NO_CXX11_HDR_FORWARD_LIST [ test-bsl-run_files test_helper_support : : : [ requires std_wstreambuf ] ] + [ test-bsl-run_files test_interrupted_pointer_reclaim ] [ test-bsl-run_files test_interrupts ] [ test-bsl-run_files test_list : A ] [ test-bsl-run_files test_list_ptrs : A ] diff --git a/test/test_interrupted_pointer_reclaim.cpp b/test/test_interrupted_pointer_reclaim.cpp new file mode 100644 index 000000000..6652a2618 --- /dev/null +++ b/test/test_interrupted_pointer_reclaim.cpp @@ -0,0 +1,165 @@ +/////////1/////////2/////////3/////////4/////////5/////////6/////////7/////////8 +// test_interrupted_pointer_reclaim.cpp + +// Copyright 2026 Gennaro Prota. +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) + +// See http://www.boost.org for updates, documentation, and revision history. + +// When loading through a pointer throws part way, delete_created_pointers() +// has to reclaim the object which was left half loaded, and leave alone the +// objects an owning smart pointer has already taken over. + +// Reported by hanzotutu in +// https://github.com/boostorg/serialization/issues/260, together with a +// careful matrix of raw pointer, shared_ptr, single object and vector +// cases. That analysis is what pinned the bug down, and it shaped the +// cases tested here. Thanks for the effort which went into it! + +#include +#include +#include +#include +#include +#include + +#include +#if defined(BOOST_NO_STDC_NAMESPACE) +namespace std{ + using ::remove; +} +#endif + +#include +#include +#include +#include +#include +#include +#include +#include + +#include "test_tools.hpp" + +// Counts live instances so that a leak shows up without a leak checker. +class base +{ + friend class boost::serialization::access; + template + void serialize(Archive & /* ar */, const unsigned int /* file_version */){ + } +public: + static int count; + base(){++count;} + virtual ~base(){--count;} +}; +int base::count = 0; + +// Which derived::load call throws, or -1 for none. +static int throw_at = -1; +static int load_calls = 0; + +class derived : public base +{ + friend class boost::serialization::access; + int x; + template + void save(Archive & ar, const unsigned int /* file_version */) const { + ar << boost::serialization::make_nvp( + "base", boost::serialization::base_object(*this)); + ar << BOOST_SERIALIZATION_NVP(x); + } + template + void load(Archive & ar, const unsigned int /* file_version */){ + ar >> boost::serialization::make_nvp( + "base", boost::serialization::base_object(*this)); + ar >> BOOST_SERIALIZATION_NVP(x); + if(load_calls++ == throw_at){ + boost::serialization::throw_exception( + std::runtime_error("interrupted load") + ); + } + } + BOOST_SERIALIZATION_SPLIT_MEMBER() +public: + derived() : x(0) {} + explicit derived(int x_) : x(x_) {} +}; + +BOOST_CLASS_EXPORT(derived) +BOOST_SERIALIZATION_SHARED_PTR(derived) + +typedef std::vector > vector_type; + +// The first element is left empty on purpose: a null pointer must not make +// the archive lose track of the objects loaded around it. +static void save(const char * testfile, std::size_t n){ + vector_type v; + v.push_back(std::shared_ptr()); + for(std::size_t i = 1; i < n; ++i){ + v.push_back(std::make_shared(static_cast(i))); + } + test_ostream os(testfile, TEST_STREAM_FLAGS); + test_oarchive oa(os, TEST_ARCHIVE_FLAGS); + oa << BOOST_SERIALIZATION_NVP(v); +} + +// A load interrupted in the middle of the last element must leave nothing +// behind: neither the half loaded object nor the complete ones before it. +static void test_interrupted(const char * testfile){ + load_calls = 0; + throw_at = 2; + { + test_istream is(testfile, TEST_STREAM_FLAGS); + test_iarchive ia(is, TEST_ARCHIVE_FLAGS); + vector_type v; + BOOST_TRY{ + ia >> BOOST_SERIALIZATION_NVP(v); + BOOST_ERROR("the interrupted load should have thrown"); + } + BOOST_CATCH(const std::runtime_error &){ + ia.delete_created_pointers(); + v.clear(); + } + BOOST_CATCH_END + } + BOOST_CHECK(0 == base::count); +} + +// A load which completes is owned by the shared_ptrs alone, so a stray +// delete_created_pointers() must not free anything under them. +static void test_completed(const char * testfile, std::size_t n){ + load_calls = 0; + throw_at = -1; + { + test_istream is(testfile, TEST_STREAM_FLAGS); + test_iarchive ia(is, TEST_ARCHIVE_FLAGS); + vector_type v; + ia >> BOOST_SERIALIZATION_NVP(v); + BOOST_CHECK(n == v.size()); + BOOST_CHECK(NULL == v[0].get()); + // Every element but the empty one holds a live object. + BOOST_CHECK(static_cast(n) - 1 == base::count); + ia.delete_created_pointers(); + } + BOOST_CHECK(0 == base::count); +} + +int +test_main(int /* argc */, char * /* argv */ []) +{ + const std::size_t n = 4; + const char * testfile = boost::archive::tmpnam(NULL); + BOOST_REQUIRE(NULL != testfile); + + save(testfile, n); + BOOST_CHECK(0 == base::count); + + test_interrupted(testfile); + test_completed(testfile, n); + + std::remove(testfile); + return EXIT_SUCCESS; +}