unlink rolled-back keys from hash buckets on insert rollback - #1188
unlink rolled-back keys from hash buckets on insert rollback#1188Ramya-9353 wants to merge 3 commits into
Conversation
|
An automated preview of the documentation is available at https://1188.json.prtest2.cppalliance.org/libs/json/doc/html/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-08-26 16:54:04 UTC |
|
GCOVR code coverage report https://1188.json.prtest2.cppalliance.org/gcovr/index.html Build time: 2026-08-26 17:05:50 UTC |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1188 +/- ##
===========================================
+ Coverage 93.71% 93.76% +0.05%
===========================================
Files 85 85
Lines 8971 8986 +15
===========================================
+ Hits 8407 8426 +19
+ Misses 564 560 -4
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
I have managed to reliably reproduce this without needing an ASAN or adding elements in a loop. It goes like this: // static_resource is important because it doesn't deallocate before its destructor
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; // array of things convertible to key_value_pair
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); // check that we indeed throw
// stale buckets from insert have info for jo["2"]
// which points to jo.begin() + 2 , which is currently jo.end() + 1
BOOST_TEST( jo.find("3") != jo.end() );Please change the test in the PR to something like this. This also reveals another bug, which will require a separate commit. |
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.
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.
|
Done. I've replaced the test with your repro (gave throws_on_convert a key member so the rolled-back keys are distinct), asserting find("2")/find("3") == end() after the throw. Without the unlink it fails exactly as you describe, with find("3") returning an iterator past the end. The other bug it surfaces is revert_insert::destroy calling object::destroy unconditionally, which trips the BOOST_ASSERT for non-shared trivial-deallocate storage like static_resource. Guarded it the same way clear() does, in a separate commit. |
|
|
3 participants
Repro: bulk-insert a range or initializer_list into a hash-mode object that already has spare capacity, where a later element throws (e.g. an allocation failure) after an earlier one has been inserted; a subsequent lookup reads a freed key (ASAN heap-use-after-free in
find_in_object).Cause: on the no-reallocation path
revert_insert::destroyrolls back the partially inserted elements and the size, butinsert_implhas already linked each of them into a bucket chain, and those bucket heads are left pointing at the slots that are then destroyed.Fix: unlink the rolled-back elements from their buckets in
revert_insert::destroy, on the no-reallocation path only, before they are destroyed. Both bulk-insert overloads share this path. Regression test intest/object.cppfails under ASAN before the change and passes after.