From 54c9314cf845c33ca6abced27cfd3480a076f39a Mon Sep 17 00:00:00 2001 From: Jianmin Zhao Date: Tue, 1 Sep 2026 13:40:59 -0700 Subject: [PATCH] CBL-8812: Fix HeapDict::_count desync on remove()-then-set() of a source key HeapDict::setting() couldn't distinguish two different reasons a slot can be empty for a key that exists in _source: a slot just created to shadow that key for the first time (already counted, no increment needed), versus a tombstone left behind by an earlier remove() or removeAll() (which already decremented _count, so re-setting it must increment). Both look identical -- empty slot, key present in _source -- so the increment was always skipped, leaving _count one too low after remove()-then-set() (or removeAll()-then-set()) on a source key. That undercount corrupts kvArray(): it sizes its cache array from the (wrong, too-low) count(), then the correct iterator overruns it, an out-of-bounds write that HeapArray::setting()'s bounds check only catches in debug builds (#if DEBUG) -- in shipping builds it silently corrupts the heap. This is the root cause of the crashes reported in CBSE-23608 (VectorRecord::setRemoteRevision performs exactly this remove-then-set pattern on load-then-save of a synced document). Fix: track whether the slot pre-existed the call (a tombstone) versus was just created (a fresh shadow of a source key), and increment count() in the former case. Adds two regression tests (remove() and removeAll() variants) that fail with the old count() and pass with the fix; verified against the full Fleece test suite with no new failures. --- Fleece/Mutable/HeapDict.cc | 10 +++++- Tests/MutableTests.cc | 71 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/Fleece/Mutable/HeapDict.cc b/Fleece/Mutable/HeapDict.cc index d9bfa7d3..cc8b609f 100644 --- a/Fleece/Mutable/HeapDict.cc +++ b/Fleece/Mutable/HeapDict.cc @@ -115,13 +115,21 @@ namespace fleece { namespace impl { namespace internal { ValueSlot& HeapDict::setting(slice stringKey) { key_t key; ValueSlot *slotp = _findValueFor(stringKey); + bool newSlot = false; if (slotp) { key = stringKey; } else { key = encodeKey(stringKey); slotp = &_makeValueFor(key); + newSlot = true; } - if (slotp->empty() && !(_source && _source->get(key))) + // An empty slot can mean two different things for a key that exists in _source, and + // they must not be conflated: a slot just created above to shadow that source key for + // the first time (already counted, don't increment), or a tombstone left behind by an + // earlier remove()/removeAll() (which already decremented _count, so re-setting it must + // increment). newSlot tells them apart: if the slot isn't new, it can only be the + // latter, so increment. (CBL-8812) + if (slotp->empty() && !(newSlot && _source && _source->get(key))) ++_count; markChanged(); return *slotp; diff --git a/Tests/MutableTests.cc b/Tests/MutableTests.cc index 905d840c..672565b1 100644 --- a/Tests/MutableTests.cc +++ b/Tests/MutableTests.cc @@ -427,6 +427,77 @@ namespace fleece { } + TEST_CASE("MutableDict remove then re-set a source key (CBL-8812)", "[Mutable]") { + // A key that exists in the immutable `_source` dict, removed and then set again, must + // bring count() back to what it was -- the empty slot left by remove() is a tombstone + // (which already decremented count), not a fresh shadow of a source key (which wouldn't + // need to increment count at all). HeapDict::setting() couldn't tell those two cases + // apart, silently leaving count() one too low forever afterward. + Retained doc = Doc::fromJSON("{\"a\":1,\"b\":2,\"c\":3}"_sl); + const Dict* source = doc->root()->asDict(); + REQUIRE(source->count() == 3); + + Retained copy = MutableDict::newDict(source); + REQUIRE(copy->count() == 3); + + copy->remove("b"_sl); + CHECK(copy->count() == 2); + + copy->set("b"_sl, 20); + CHECK(copy->count() == 3); + + // The bug doesn't just mis-report count(): kvArray() sizes its cache array from count(), + // then the (correct) iterator overruns it, corrupting the heap. So also confirm the + // iterator itself still yields all 3 keys, matching the now-correct count(). + int n = 0; + bool sawA = false, sawB = false, sawC = false; + for (MutableDict::iterator i(copy); i; ++i) { + ++n; + slice key = i.keyString(); + if (key == "a"_sl) sawA = true; + else if (key == "b"_sl) sawB = true; + else if (key == "c"_sl) sawC = true; + } + CHECK(n == copy->count()); + CHECK(n == 3); + CHECK(sawA); + CHECK(sawB); + CHECK(sawC); + CHECK(copy->get("b"_sl)->asInt() == 20); + } + + + TEST_CASE("MutableDict removeAll then re-set a source key (CBL-8812)", "[Mutable]") { + // Same underlying bug as above, reached via removeAll() instead of remove(): it shadows + // every source key with an empty tombstone slot in one call and resets count() to 0 + // directly, so re-setting any one of those keys must bring count() back up by one -- + // but setting() can't distinguish that tombstone from a fresh shadow slot either. + Retained doc = Doc::fromJSON("{\"a\":1,\"b\":2,\"c\":3}"_sl); + const Dict* source = doc->root()->asDict(); + REQUIRE(source->count() == 3); + + Retained copy = MutableDict::newDict(source); + REQUIRE(copy->count() == 3); + + copy->removeAll(); + CHECK(copy->count() == 0); + + copy->set("b"_sl, 20); + CHECK(copy->count() == 1); + + int n = 0; + for (MutableDict::iterator i(copy); i; ++i) { + ++n; + CHECK(i.keyString() == "b"_sl); + } + CHECK(n == copy->count()); + CHECK(n == 1); + CHECK(copy->get("b"_sl)->asInt() == 20); + CHECK(copy->get("a"_sl) == nullptr); + CHECK(copy->get("c"_sl) == nullptr); + } + + TEST_CASE("MutableDict as Dict", "[Mutable]") { Retained md = MutableDict::newDict(); const Dict *d = md;