From 2296754529ca2864d89f9b352f1fd3bfc9d3d0ee Mon Sep 17 00:00:00 2001 From: Ramya Eliger Date: Tue, 18 Aug 2026 18:55:09 +0530 Subject: [PATCH 1/3] unlink rolled-back keys from hash buckets on insert rollback --- include/boost/json/impl/object.ipp | 14 ++++++++ test/object.cpp | 57 ++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/include/boost/json/impl/object.ipp b/include/boost/json/impl/object.ipp index 8169693ab..3386d238b 100644 --- a/include/boost/json/impl/object.ipp +++ b/include/boost/json/impl/object.ipp @@ -186,6 +186,20 @@ object:: revert_insert:: destroy() noexcept { + // when no reallocation happened, insert_impl linked each rolled-back + // element into a bucket of the live table; unlink them while their keys + // are still valid, otherwise a bucket head is left pointing at a slot + // that is about to be destroyed + if( !t_ && !obj_->t_->is_small() ) + { + key_value_pair* const first = &(*obj_->t_)[size_]; + key_value_pair* last = obj_->end(); + while( last != first ) + { + --last; + obj_->remove( obj_->t_->bucket( last->key() ), *last ); + } + } obj_->destroy( &(*obj_->t_)[size_], obj_->end()); diff --git a/test/object.cpp b/test/object.cpp index 17b2a752f..5dd89a0dd 100644 --- a/test/object.cpp +++ b/test/object.cpp @@ -1730,6 +1730,62 @@ class object_test BOOST_TEST( capacity == o.capacity() ); } + void + testInsertRollback() + { + // A bulk insert that needs no new storage still links each new element + // into a hash bucket as it goes. If a later element throws, the + // rollback in ~revert_insert has to unlink those entries, not merely + // restore the size; otherwise a bucket head is left pointing at a slot + // that has just been destroyed, and the next lookup reads a freed key. + for( std::size_t k = 1; k < 40; ++k ) + { + fail_resource fr; + fr.fail_max = 100000; // don't fail while the object is built + { + object o( &fr ); + o.reserve( detail::small_object_size_ + 8 ); // hash mode + spare + for( std::size_t i = 0; i <= detail::small_object_size_; ++i ) + o.emplace( std::to_string(i), i ); + std::size_t const n0 = o.size(); + std::size_t const cap = o.capacity(); + BOOST_TEST( cap > detail::small_object_size_ ); + + std::vector< std::pair > in{ + { "insert_a", value( std::string(300, 'a') ) }, + { "insert_b", value( std::string(301, 'b') ) }, + { "insert_c", value( std::string(302, 'c') ) } }; + + fr.fail_max = fr.fail + k; // fail on the k-th insert allocation + bool threw = false; + try + { + o.insert( in.begin(), in.end() ); + } + catch( test_failure const& ) + { + threw = true; + } + fr.fail_max = 100000; + + // looking up the rolled-back keys must not touch freed memory, + // and every surviving element must stay reachable through its + // bucket + (void)o.find( "insert_a" ); + (void)o.find( "insert_b" ); + (void)o.find( "insert_c" ); + for( auto const& kv : o ) + BOOST_TEST( o.find( kv.key() ) != o.end() ); + + if( threw ) + { + BOOST_TEST( o.size() == n0 ); + BOOST_TEST( o.capacity() == cap ); // no reallocation + } + } + } + } + void run() { @@ -1746,6 +1802,7 @@ class object_test testAllocation(); testHash(); testStrongGurantee(); + testInsertRollback(); } }; From ffdfb560ccdab713d57ea8c92d4f3809808945b0 Mon Sep 17 00:00:00 2001 From: Ramya Eliger Date: Wed, 26 Aug 2026 22:14:51 +0530 Subject: [PATCH 2/3] skip element destruction on insert rollback when deallocate is trivial revert_insert::destroy called object::destroy unconditionally, which asserts when the storage is not shared and deallocate is trivial (e.g. static_resource); element destruction is elided everywhere else for such storages. Guard the call the same way clear() does. --- include/boost/json/impl/object.ipp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/boost/json/impl/object.ipp b/include/boost/json/impl/object.ipp index 3386d238b..06b30155f 100644 --- a/include/boost/json/impl/object.ipp +++ b/include/boost/json/impl/object.ipp @@ -200,9 +200,10 @@ destroy() noexcept obj_->remove( obj_->t_->bucket( last->key() ), *last ); } } - obj_->destroy( - &(*obj_->t_)[size_], - obj_->end()); + if(! obj_->sp_.is_not_shared_and_deallocate_is_trivial()) + obj_->destroy( + &(*obj_->t_)[size_], + obj_->end()); } //---------------------------------------------------------- From 0d78eab9995c75a33898b1963631375e764890c3 Mon Sep 17 00:00:00 2001 From: Ramya Eliger Date: Wed, 26 Aug 2026 22:14:55 +0530 Subject: [PATCH 3/3] make insert rollback test deterministic with static_resource Replace the allocation-failure sweep with the reviewer's repro: a static_resource keeps the rolled-back key bytes readable, so a stale bucket entry makes find() return an iterator past the end of the object without needing ASAN. Fails before the bucket unlink fix, passes after. --- test/object.cpp | 88 ++++++++++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/test/object.cpp b/test/object.cpp index 5dd89a0dd..7bfe2d6a3 100644 --- a/test/object.cpp +++ b/test/object.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -39,13 +40,15 @@ struct throws_on_convert // the line is reachable in other instantiations bool should_throw = true; + string_view k; + throws_on_convert() = default; operator key_value_pair() { if( should_throw ) throw std::invalid_argument(""); - return key_value_pair( "", nullptr); + return key_value_pair( k, nullptr); } }; @@ -1736,54 +1739,41 @@ class object_test // A bulk insert that needs no new storage still links each new element // into a hash bucket as it goes. If a later element throws, the // rollback in ~revert_insert has to unlink those entries, not merely - // restore the size; otherwise a bucket head is left pointing at a slot - // that has just been destroyed, and the next lookup reads a freed key. - for( std::size_t k = 1; k < 40; ++k ) - { - fail_resource fr; - fr.fail_max = 100000; // don't fail while the object is built - { - object o( &fr ); - o.reserve( detail::small_object_size_ + 8 ); // hash mode + spare - for( std::size_t i = 0; i <= detail::small_object_size_; ++i ) - o.emplace( std::to_string(i), i ); - std::size_t const n0 = o.size(); - std::size_t const cap = o.capacity(); - BOOST_TEST( cap > detail::small_object_size_ ); - - std::vector< std::pair > in{ - { "insert_a", value( std::string(300, 'a') ) }, - { "insert_b", value( std::string(301, 'b') ) }, - { "insert_c", value( std::string(302, 'c') ) } }; - - fr.fail_max = fr.fail + k; // fail on the k-th insert allocation - bool threw = false; - try - { - o.insert( in.begin(), in.end() ); - } - catch( test_failure const& ) - { - threw = true; - } - fr.fail_max = 100000; - - // looking up the rolled-back keys must not touch freed memory, - // and every surviving element must stay reachable through its - // bucket - (void)o.find( "insert_a" ); - (void)o.find( "insert_b" ); - (void)o.find( "insert_c" ); - for( auto const& kv : o ) - BOOST_TEST( o.find( kv.key() ) != o.end() ); - - if( threw ) - { - BOOST_TEST( o.size() == n0 ); - BOOST_TEST( o.capacity() == cap ); // no reallocation - } - } - } + // restore the size; otherwise a bucket head is left pointing past the + // end of the object and the next lookup reads a destroyed slot. + + // static_resource is important because it doesn't deallocate before + // its destructor, so the stale bucket entry finds the old key bytes + // still in place instead of tripping a sanitizer + unsigned char buf[1024]; + static_resource mr( buf, sizeof(buf) ); + + object jo( &mr ); + jo.reserve(20); // reserve more than "small object" + + jo["1"] = 1; // add one element so that we remain not empty + + std::array input; + input[0].k = "2"; + input[0].should_throw = false; + + input[1].k = "3"; + input[1].should_throw = false; + + input[2].k = "4"; + input[2].should_throw = true; // third element throws on conversion + + BOOST_TEST_THROWS( + jo.insert( input.begin(), input.end() ), + std::invalid_argument ); + + // the rolled-back elements must not stay reachable through stale + // bucket entries pointing past the end of the object + BOOST_TEST( jo.find("2") == jo.end() ); + BOOST_TEST( jo.find("3") == jo.end() ); + BOOST_TEST( jo.find("4") == jo.end() ); + BOOST_TEST( jo.size() == 1 ); + BOOST_TEST( jo.find("1") != jo.end() ); } void