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();