CBL-8813: Fix HeapDict::_count desync on remove()-then-set() of a sou… - #311
Merged
Conversation
…rce 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.
borrrden
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…rce 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.