From 7ad52937b2d80f1b1ddb5fea30f075c40b9593de Mon Sep 17 00:00:00 2001 From: "shoney.arickathil" Date: Sat, 29 Aug 2026 22:55:01 +0200 Subject: [PATCH] fix(db2-keys): delete on a keys-resident table was memory corruption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - wo_row_remove read the id map's value as a slot, but on a keys table that value is a LOG OFFSET (hput(t, id, wal_off + 1)). slot_row does no bounds check, so a delete indexed t->slabs[] with a byte offset and then called db_val_free on whatever it landed on — arbitrary frees, not a wrong answer - keys tables now take their own arm: no slab slot, no bitmap bit, no free-list entry to return. The index hook needs the row's values, so the row is borrowed from the log for exactly that long - wo_row_ptr carried the same trap and is public. It cannot refuse keys tables outright (insert legitimately calls it while the map still holds a slot), so it now detects the offset case — index past the slabs, or bitmap bit clear — and returns NULL. Callers all handle NULL - test_keys_resident_delete pins it; it SEGVs against the old code, verified by reverting the fix rather than assumed - found while auditing every hget() reader before narrowing the loader refusal to allow benchmarking. The refusal was justified in the docs by "updates are unimplemented" while actually standing in front of this too: a guard whose stated reason is narrower than its real one gets removed by someone who believes the stated reason Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit 76b8fd944af9ed062467bdf9ab93c2e96dd198cf) --- database/src/table.c | 36 ++++++++++++ docs/00-databasev2-chain-review.md | 18 ++++++ .../databasev2/02-table-storage-modes.md | 13 +++++ runtime/test/test_wal.c | 56 +++++++++++++++++++ 4 files changed, 123 insertions(+) diff --git a/database/src/table.c b/database/src/table.c index f66c912..813982e 100644 --- a/database/src/table.c +++ b/database/src/table.c @@ -733,6 +733,21 @@ db_row *wo_row_ptr(wo_db *db, uint32_t class_id, uint64_t id) { if (!t->row_size) return NULL; uint64_t s1 = hget(t, id); if (!s1) return NULL; + /* databasev2 2 (5d): on a keys-resident table the map value means one of + * two things — a SLOT while the row is still in its slab (between the + * insert and the post-barrier drop, which is when wo_wal_append_insert + * legitimately calls this) and a LOG OFFSET afterwards. Nothing in the + * value distinguishes them, so this function refuses to guess: an index + * past the slabs, or one whose bitmap bit is clear, is an offset and the + * row is not in RAM. Without this a caller that had not read 5d got + * slot_row() applied to a byte offset — slot_row does no bounds check — + * and a wild pointer that was then freed. Callers already handle NULL. */ + if (wo_table_is_keys_resident(db, class_id)) { + uint64_t g = s1 - 1; + uint64_t total = (uint64_t)t->slab_cnt * DB_SLAB_ROWS; + if (g >= total) return NULL; + if (!(t->bitmap[g >> 6] & (1ull << (g & 63)))) return NULL; + } return slot_row(t, (uint32_t)(s1 - 1)); } @@ -1185,6 +1200,27 @@ int wo_row_remove(wo_db *db, uint32_t class_id, uint64_t id) { if (!t->row_size) return -1; uint64_t s1 = hget(t, id); if (!s1) return -1; + + /* databasev2 2 (5d): a keys-resident row's map entry is a LOG OFFSET, not + * a slot. Falling through to the slab path below would index t->slabs[] + * with a byte offset — slot_row does no bounds check — and then free + * whatever it landed on. That is memory corruption, not a missing feature, + * which is why the loader still refuses the annotation. + * + * The row has no slab slot, no bitmap bit and no free-list entry to give + * back; only the indexes and the id map know about it. The index hook + * needs the row's column VALUES to find its bucket, and those live in the + * log, so the row is borrowed for exactly as long as that takes. */ + if (wo_table_is_keys_resident(db, class_id)) { + db_row *r = wo_row_borrow(db, class_id, id, NULL); + if (!r) return -1; + idx_remove_row(db, t, r); + wo_row_release(db, class_id, r); /* frees the materialised values */ + hdel(t, id); + t->count--; + return 0; + } + uint32_t g = (uint32_t)(s1 - 1); db_row *r = slot_row(t, g); /* the index hook's remove side: before the row's values die, while the diff --git a/docs/00-databasev2-chain-review.md b/docs/00-databasev2-chain-review.md index cdd9146..301b35f 100644 --- a/docs/00-databasev2-chain-review.md +++ b/docs/00-databasev2-chain-review.md @@ -96,6 +96,24 @@ Minor, same family: iteration 6 is "largely superseded" and to be revisited condition — yet its status is `pending` while genuinely parked iterations 8, 9 and 10 are `hold`. +## Addendum 2026-08-29 — the recommendation this review implied was wrong + +This review argued the next step was to measure `resident: keys` before +investing further, and that measuring required narrowing the loader refusal so +a benchmark could declare such a table. **Auditing the code before narrowing it +found that `delete` on a keys-resident table was memory corruption**, not a +missing feature: `wo_row_remove` read the id map's value as a slot when on such +a table it is a log offset, and `slot_row` bounds-checks nothing. + +The refusal was therefore load-bearing in a way nobody had written down. It was +justified in the docs by "updates are unimplemented" — one honest gap — while +actually standing in front of two, one of which frees arbitrary pointers. + +Both are now closed or contained (`wo_row_remove` fixed, `wo_row_ptr` returns +NULL rather than a wild pointer), but the lesson generalises: **a guard whose +stated reason is narrower than its real one will eventually be removed by +someone who believes the stated reason.** + ## What is actually blocked Nothing in databasev2 is blocked on anything else in databasev2. Iteration 2's diff --git a/docs/stories/databasev2/02-table-storage-modes.md b/docs/stories/databasev2/02-table-storage-modes.md index b847240..7c5a54c 100644 --- a/docs/stories/databasev2/02-table-storage-modes.md +++ b/docs/stories/databasev2/02-table-storage-modes.md @@ -124,6 +124,19 @@ Met: Outstanding: +- **Given** a `delete` of a row on a `resident: keys` table, **when** it runs, + **then** the row is gone and nothing else is touched. ✅ **fixed 2026-08-29, + and it was memory corruption before the fix.** `wo_row_remove` read the id + map's value as a slot, but on a keys table that value is a LOG OFFSET, and + `slot_row` does no bounds check — so a delete indexed the slab array with a + byte offset and then freed whatever it landed on. Pinned by + `test_keys_resident_delete`, which SEGVs against the old code. The same + latent trap in `wo_row_ptr` is closed too: it now returns NULL rather than a + wild pointer when the value is an offset. + + **This is why the loader refusal earns its keep.** The gap was not one + missing operation but a second one that corrupted memory silently, found + only by auditing every reader of the id map. - **Given** an `update` to a row on a `resident: keys` table, **when** it runs, **then** it is applied. ❌ **refused explicitly** by `wo_row_update_field{,_slot}`. A keys row lives in the log with no slab slot diff --git a/runtime/test/test_wal.c b/runtime/test/test_wal.c index a038161..f234cf8 100644 --- a/runtime/test/test_wal.c +++ b/runtime/test/test_wal.c @@ -578,6 +578,61 @@ static void test_keys_resident_round_trip(void) { * with its rows readable FROM THE LOG — the map rebuilt to offsets, not slabs. * This is the half the round-trip test cannot cover: it runs in a fresh db, * exactly as a restart would. */ +static void test_keys_resident_delete(void) { + /* databasev2 2 (5d): deleting a keys-resident row. Before the fix this + * read the id map's LOG OFFSET as a slot index and handed it to slot_row, + * which does no bounds check — so it indexed t->slabs[] with a byte offset + * and then freed whatever it found. ASan reports it as a wild read or a + * bad free, not as a wrong answer, which is why the annotation stays + * refused at the loader until every operation is honest. */ + char path[128]; + snprintf(path, sizeof path, "%s/keysdel.wal", g_dir); + wo_rt rt; + T_EQ(wo_rt_init(&rt, 1 << 20, KEYS_CLASSES, 1), 0); + wo_db db; + T_EQ(wo_db_init(&db, KEYS_CLASSES, 1, 0, 1), 0); + wo_wal w; + T_EQ(wo_wal_open(&w, path, 1 << 16), 0); + db.rt = &rt; rt.wal = &w; rt.db = &db; + const char *msg = ""; + + uint64_t ids[3]; + for (int i = 0; i < 3; i++) { + wo_str *sv = wo_str_new(&rt, "del", 3); + uint64_t vals[2] = {(uint64_t)(i + 500), (uint64_t)(uintptr_t)sv}; + ids[i] = wo_row_insert(&db, 0, vals, &msg, NULL); + T_CHECK(ids[i] != 0); + uint64_t off = wo_wal_next_offset(&w); + T_EQ(wo_wal_append_insert(&w, &db, 0, ids[i]), 0); + T_EQ(wo_wal_commit(&w), 0); + T_EQ(wo_row_drop_payload(&db, 0, ids[i], off), 0); + } + T_CHECK(db.tables[0].count == 3); + + /* the offset is far larger than any slot index, which is exactly what made + * the old path walk off the slab array */ + T_EQ(wo_row_remove(&db, 0, ids[1]), 0); + T_CHECK(db.tables[0].count == 2); + + /* gone, and the survivors still read correctly through their own offsets */ + T_CHECK(wo_row_borrow(&db, 0, ids[1], &msg) == NULL); + for (int i = 0; i < 3; i += 2) { + db_row *r = wo_row_borrow(&db, 0, ids[i], &msg); + T_CHECK(r != NULL); + T_CHECK(r->slots[0] == (uint64_t)(i + 500)); + wo_row_release(&db, 0, r); + } + + /* and a removed row must not come back through compaction */ + T_EQ(wo_wal_compact(&w, &db), 0); + T_CHECK(wo_row_borrow(&db, 0, ids[1], &msg) == NULL); + T_CHECK(db.tables[0].count == 2); + + wo_wal_close(&w); + wo_db_destroy(&db); + wo_rt_destroy(&rt); +} + static void test_keys_resident_survives_compaction(void) { /* databasev2 2 (5d): the obligation recorded at wo_wal_compact. Two ways * to fail it, both checked here: @@ -1119,6 +1174,7 @@ int main(void) { test_keys_resident_round_trip(); test_keys_resident_replay(); test_keys_resident_survives_compaction(); + test_keys_resident_delete(); test_stale_compact_temp_is_removed(); test_should_compact_policy(); test_compact_refuses_with_staged_records();