unlink rolled-back keys from hash buckets on insert rollback

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());
 }
 
 //----------------------------------------------------------

test/object.cpp

@@ -14,6 +14,7 @@
 #include <boost/json/monotonic_resource.hpp>
 #include <boost/json/parse.hpp>
 #include <boost/json/serialize.hpp>
+#include <boost/json/static_resource.hpp>
 
 #include <cmath>
 #include <forward_list>
@@ -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<throws_on_convert, 3> 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();
     }
 };