Fail closed on two corrupt node shapes the readers accepted - #2007
Conversation
Both are hostile-input hardening in the read path: the writer produces neither shape, and every other structural check passes them. An internal node pointing at an empty leaf. descendToExtremeLeaf set idx to nItems-1, which is -1 when walking right, and prollyCursorNext/Prev then marked the cursor valid over a node whose key array is a null pointer -- the next key read dereferences it. prollyCursorFirst and Last already refused it through finalizeExtremeCursor, so only the stepping paths were exposed. The check goes on the descent, so an empty root leaf, which is just an empty table, still reads as EOF. Offset arrays not starting at zero. Readers reach key data two ways, through the offset array and by striding from pKeyData for fixed-width keys, as prollyNodeIntKey does with i*8. A first offset of 8 with a stride of 8 satisfies the bounds, monotonicity, width and total-size checks, and the two readers then disagree about which bytes are the key -- silently wrong data rather than a rejected node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
SummaryCoverage focuses on safe handling of malformed and empty stored data, including traversal in both directions, cache loading, scanning, cursor cleanup, and validation of valid data alongside corruption cases. The exercised behavior is primarily defensive edge-case and adversarial data handling, with normal empty-data and valid-record behavior also confirmed. Safe to merge — all exercised behaviors passed, including corruption detection, normal end-of-data handling, valid-record access, and safe cursor cleanup, with no PR-attributable regressions or unresolved failures. Any unrelated observations do not present a merge blocker. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
The previous run's macOS and Windows builds were cancelled after several hours of queueing rather than failing, and re-running the failed jobs did not requeue them.
|
Diff SummaryCoverage focuses on safe database reading across normal and hostile data conditions: valid records and scans, empty tables, tree-boundary movement, malformed serialized data, sparse reads, and cleanup after corruption. Overall behavior is healthy, with corruption generally surfaced safely while valid data remains readable and ordinary empty-table end-of-file behavior is preserved. Safe to merge — the only failure is a medium-severity, pre-existing cursor-state safety issue explicitly marked unrelated to this PR, with no regressions or PR-attributable failures. It is appropriate as a flag for later rather than a merge blocker. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Corrupt cursor stays marked as usable
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
DoltLite source coverage
Merged 162 pooled raw profiles from the distributed Linux correctness jobs. Per-file coverage (91 files)
|
DoltLite performance vs PR base
blobpk details
compositepk details
int details
textpk details
vc details
All relative performance gates passed. |

Last two of the three prolly-core items from the deep review (after #2002). Both are hostile-input hardening in the read path — the writer produces neither shape, and every other structural check passes them.
1. An internal node pointing at an empty leaf
descendToExtremeLeafsetidx = pChild->node.nItems - 1, which is -1 when walking right, andprollyCursorNext/Prevthen marked the cursor valid over a node whose key array is a null pointer. The next key read dereferences it.prollyCursorFirstandLastalready refused this throughfinalizeExtremeCursor, so only the stepping paths were exposed. Measured on a crafted two-child tree (one populated leaf, one empty):prollyCursorNextpast the populated leafSQLITE_OK, cursor marked validSQLITE_CORRUPTprollyCursorLastSQLITE_OK, silently EOFSQLITE_CORRUPTThe check sits on the descent, so an empty root leaf — which is just an empty table, not corruption — still reads as EOF.
prolly_cursor_empty_leaf_rootpins that and still passes.2. Offset arrays not starting at zero
Readers reach key data two ways: through the offset array (
prollyNodeKey) and by striding frompKeyDatafor fixed-width keys (prollyNodeIntKey,i*8). A node withaKeyOff = [8, 16]satisfies the bounds check, monotonicity, the INTKEY stride check (16-8 == 8) and the total-size check — and the two readers then disagree about which bytes are the key. Silently wrong data rather than a rejected node.Both arrays must now start at 0.
Testing
Two cases in
test/doltlite_regression_test_c.c, beside the existingprolly_cursor_corrupt_nodeandprolly_node_corruptioncases and following their hand-built-node style.Fail-before/pass-after, same test file both runs:
prolly_node_first_offset_validation— 2/4 → 4/4prolly_cursor_empty_leaf_under_internal— 8/10 → 10/10Each includes a guard that the fix is not just rejecting everything: a well-formed node with zero-start offsets still parses, and
Firston the crafted tree still returns a valid cursor.Validation
test/doltlite_regression_test_c.sh— 310,193 tests, 0 failurestest/run_c_tests.sh— 26/26 gated c-teststest/run_doltlite_tests.sh— 99/99 suitesstoragebucket — 45,265 passing, 0 unexpected failures (the 2bigfileterminations are environmental and pre-existing, confirmed against a control build during Bound the chunk store WAL offset before trusting it #1987)core-sqlbucket — the only failures are 7 shell/loadext/zipfile files, and I diffed the exact failure sets against a control build without these changes: identical, 53 eachA note on that last point, since it nearly fooled me: my first bucket runs reported ~2300 failures with "no such table" everywhere. That was a stale
testfixture— I had rebuiltdoltliteand the library but not it. Rebuilt,affinity2went from 5 failures to 26/26 clean. Worth remembering that this binary needs an explicit rebuild.🤖 Generated with Claude Code