From b758f2978d77d8b4dc6d3c4ceb5295d257092707 Mon Sep 17 00:00:00 2001 From: "shoney.arickathil" Date: Sun, 30 Aug 2026 07:53:17 +0200 Subject: [PATCH] fix(db2-delta): fold's cycle guard checks direction, not step count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - wal.c: wo_wal_fold_row_at now refuses any delta back-pointer that does not point strictly earlier than the record naming it (back_off >= cur), instead of capping total hops at off/13+1 - this is the real invariant, not a proxy for it: a step-count bound lets a forward-pointing back-pointer through in one hop whenever it happens to land on a genuine record, returning a plausible-but-wrong row instead of refusing it - removes the 13-byte-record magic number entirely; no arithmetic tied to record framing remains in the guard - wal.h: docblock updated to describe the direction invariant - test_wal.c: two new tests — self-pointing back-pointer (boundary case, back_off == cur) and forward-pointing back-pointer to a real future record for the same row (the actual gap: verified failing against the old step-count guard, passing after the fix) Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit 173dbf28a42d45a8c7f9fe430355f61f70c81935) --- database/src/wal.c | 35 ++++++++------- database/src/wal.h | 9 ++-- runtime/test/test_wal.c | 97 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 19 deletions(-) diff --git a/database/src/wal.c b/database/src/wal.c index e4c07b2..6abaa07 100644 --- a/database/src/wal.c +++ b/database/src/wal.c @@ -877,15 +877,6 @@ int wo_wal_read_row_at(wo_wal *w, wo_db *db, wo_rt *rt, uint64_t off, * and compaction all call this one function — never a second copy. */ int wo_wal_fold_row_at(wo_wal *w, wo_db *db, uint64_t off, uint32_t *class_out, uint64_t *id_out, uint64_t *out_vals, const char **msg) { - /* Cycle guard: every legitimate back-pointer names a record already on - * disk before [off], so the chain cannot be longer than the number of - * minimal-sized records the bytes up to [off] could hold. 13 = the - * smallest an on-disk record can ever be (scan_record refuses len == 0, - * so 8-byte header + 1-byte payload + 4-byte mark). A malicious or - * corrupt back-pointer — even a record pointing at itself — still - * terminates here, loudly, instead of spinning forever. */ - uint64_t max_steps = off / 13u + 1u; - uint32_t cid = 0; uint64_t id = 0; uint32_t field_cnt = 0; @@ -893,13 +884,9 @@ int wo_wal_fold_row_at(wo_wal *w, wo_db *db, uint64_t off, uint32_t *class_out, uint64_t *resolved_val = NULL; /* field idx -> its remembered engine value */ uint64_t cur = off; int rc = 0; + int first = 1; - for (uint64_t step = 0;; step++) { - if (step >= max_steps) { - *msg = "delta chain exceeds what the log could hold (cycle?)"; - rc = -1; - break; - } + for (;;) { uint32_t len; uint8_t *payload; if (scan_record(w->fd, cur, &len, &payload) != 0) { @@ -917,7 +904,8 @@ int wo_wal_fold_row_at(wo_wal *w, wo_db *db, uint64_t off, uint32_t *class_out, rc = -1; break; } - if (step == 0) { + if (first) { + first = 0; cid = rec_cid; id = rec_id; field_cnt = db->classes[cid].field_cnt; @@ -945,6 +933,21 @@ int wo_wal_fold_row_at(wo_wal *w, wo_db *db, uint64_t off, uint32_t *class_out, rc = -1; break; } + /* THE cycle/forgery guard: a back-pointer names the row's + * PREVIOUS record, which by construction is earlier in the + * (append-only) log than the delta naming it. Anything else — + * a self-pointer, a forward pointer, a pointer that only forms + * a cycle several hops later — is corruption or forgery, and + * this is what actually rules all of those out: not a bound on + * how many records could exist, which a forward pointer to a + * genuine record satisfies trivially and a bound would then let + * straight through. */ + if (back_off >= cur) { + free(payload); + *msg = "delta back-pointer does not point earlier in the log"; + rc = -1; + break; + } const wo_classdesc *c = &db->classes[cid]; uint64_t v; if (dec_val(&r, db, c->kinds[field_idx], &v) != 0 || diff --git a/database/src/wal.h b/database/src/wal.h index 523abd3..0d7e694 100644 --- a/database/src/wal.h +++ b/database/src/wal.h @@ -294,9 +294,12 @@ int wo_wal_read_row_at(wo_wal *w, wo_db *db, wo_rt *rt, uint64_t off, * against EVERY record touched — a chain that disagrees about whose row it * is is corruption, not a new row. * - * A cycle in the back-pointers is corruption, possibly malicious: the walk - * refuses to look at more records than the log at [off] could possibly - * hold, and fails loudly instead of spinning. + * A back-pointer must name something STRICTLY EARLIER in the log than the + * record holding it — the row's PREVIOUS record, by construction, always + * is. Anything else (a self-pointer, a forward pointer, corruption or + * forgery of any shape) is refused on the very hop that violates it, which + * also rules out a cycle: a walk that only ever moves to a lower offset + * cannot revisit one. * * 0 ok, -1 no intact/malformed/corrupt record anywhere in the chain (or a * REMOVE tombstone reached mid-chain), -2 out of memory (*msg set). */ diff --git a/runtime/test/test_wal.c b/runtime/test/test_wal.c index 5a1f0c8..0d44584 100644 --- a/runtime/test/test_wal.c +++ b/runtime/test/test_wal.c @@ -761,6 +761,101 @@ static void test_delta_fold_same_field_newest_wins(void) { wo_rt_destroy(&rt); } +/* keys-resident delta updates, Task 2 (review follow-up): a back-pointer + * naming ITS OWN offset is the boundary case of the fold's invariant — + * every hop must land on a STRICTLY earlier offset than the record naming + * it. back_off == cur violates that on the very first hop and must be + * refused immediately, not walked. */ +static void test_fold_refuses_self_pointing_delta(void) { + char path[128]; + snprintf(path, sizeof path, "%s/foldself.wal", g_dir); + wo_rt rt; + T_EQ(wo_rt_init(&rt, 1 << 20, DELTA_CLASSES, 1), 0); + wo_db db; + T_EQ(wo_db_init(&db, DELTA_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 vals[3] = {10, 20, 30}; + uint64_t id = wo_row_insert(&db, 0, vals, &msg, NULL); + T_CHECK(id != 0); + + /* a delta whose back-pointer names ITS OWN offset */ + uint64_t delta_off = wo_wal_next_offset(&w); + T_EQ(wo_wal_append_delta(&w, &db, 0, id, 0, delta_off, 999), 0); + T_EQ(wo_wal_commit(&w), 0); + T_EQ(wo_row_drop_payload(&db, 0, id, delta_off), 0); + + T_CHECK(wo_row_borrow(&db, 0, id, &msg) == NULL); + + wo_wal_close(&w); + wo_db_destroy(&db); + wo_rt_destroy(&rt); +} + +/* The case a mere chain-length bound cannot rule out: a back-pointer that + * points FORWARD to a real, valid record for the SAME row. Nothing about + * this loops, so a cap on chain length would let it straight through in + * one hop and return a plausible-but-wrong answer. Only checking that + * every hop moves to a STRICTLY earlier offset catches it, immediately. */ +static void test_fold_refuses_forward_pointing_delta(void) { + char path[128]; + snprintf(path, sizeof path, "%s/foldfwd.wal", g_dir); + wo_rt rt; + T_EQ(wo_rt_init(&rt, 1 << 20, DELTA_CLASSES, 1), 0); + wo_db db; + T_EQ(wo_db_init(&db, DELTA_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 = ""; + + /* filler row/record, same reason as test_delta_record's: a fresh WAL's + first record sits at offset 0, which would make the forged delta's + own offset indistinguishable from a zeroed field either way. */ + uint64_t filler_vals[3] = {1, 2, 3}; + uint64_t filler_id = wo_row_insert(&db, 0, filler_vals, &msg, NULL); + T_CHECK(filler_id != 0); + T_EQ(wo_wal_append_insert(&w, &db, 0, filler_id), 0); + T_EQ(wo_wal_commit(&w), 0); + + uint64_t vals[3] = {10, 20, 30}; + uint64_t id = wo_row_insert(&db, 0, vals, &msg, NULL); + T_CHECK(id != 0); + + /* forge a delta BEFORE the row's real base record exists, naming the + offset the base record WILL occupy right after it. A scalar-field + delta payload is kind|class|id|field_idx|back_off|value(u64) = 33 + bytes (established by test_delta_record); the frame is 8+33+4 = 45. */ + uint64_t delta_off = wo_wal_next_offset(&w); + uint64_t insert_off = delta_off + 45u; + T_EQ(wo_wal_append_delta(&w, &db, 0, id, 0, insert_off, 999), 0); + T_EQ(wo_wal_commit(&w), 0); + T_EQ(wo_wal_next_offset(&w), insert_off); /* the hand-computed frame size held */ + + /* the row's TRUE base record, landing exactly where the forged delta + claimed — the row is still in RAM, so this is an ordinary insert-log */ + T_EQ(wo_wal_append_insert(&w, &db, 0, id), 0); + T_EQ(wo_wal_commit(&w), 0); + + /* point the row at the forged, forward-pointing delta */ + T_EQ(wo_row_drop_payload(&db, 0, id, delta_off), 0); + + /* the fold must refuse — not silently return {999, 20, 30} by walking + forward into the base record the forged back-pointer named */ + T_CHECK(wo_row_borrow(&db, 0, id, &msg) == NULL); + + wo_wal_close(&w); + wo_db_destroy(&db); + wo_rt_destroy(&rt); +} + /* databasev2 2 (5c): boot. A keys-resident store must come back from replay * 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, @@ -1407,6 +1502,8 @@ int main(void) { test_delta_record(); test_delta_fold_two_fields(); test_delta_fold_same_field_newest_wins(); + test_fold_refuses_self_pointing_delta(); + test_fold_refuses_forward_pointing_delta(); test_keys_resident_replay(); test_keys_resident_survives_compaction(); test_keys_resident_delete();