diff --git a/include/boost/json/impl/object.ipp b/include/boost/json/impl/object.ipp index 8169693ab..06b30155f 100644 --- a/include/boost/json/impl/object.ipp +++ b/include/boost/json/impl/object.ipp @@ -186,9 +186,24 @@ object:: revert_insert:: destroy() noexcept { - obj_->destroy( - &(*obj_->t_)[size_], - obj_->end()); + // 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 ); + } + } + if(! obj_->sp_.is_not_shared_and_deallocate_is_trivial()) + obj_->destroy( + &(*obj_->t_)[size_], + obj_->end()); } //---------------------------------------------------------- diff --git a/test/object.cpp b/test/object.cpp index 17b2a752f..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); } }; @@ -1730,6 +1733,49 @@ 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 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 run() { @@ -1746,6 +1792,7 @@ class object_test testAllocation(); testHash(); testStrongGurantee(); + testInsertRollback(); } };