From add5fe4a6efef049f96fe144cf0694fa562b9ee9 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini Date: Wed, 9 Sep 2026 20:40:54 -0700 Subject: [PATCH] craft: harden free_slot() validation and close write()/populate_overlay race Independent post-#175 review findings (build-verified, 26/26 test suites pass): - free_slot() was missing the same magic/version/hdr.lsn==lsn/hdr.len validation read_slot() already has -- a corrupt record could deserialize a garbage blkid and free the wrong blocks. - write()'s post-flight block erased dlsn from missing_lsns_ and released missing_mu_ before calling populate_overlay(), letting a concurrent commit_impl (another write's/keep_alive's piggyback) apply and retire dlsn before the overlay entry existed -- leaving a permanently-orphaned (harmless but never-retired) overlay entry. Both mutations now happen under the same missing_mu_ critical section commit_impl's is_missing check also takes. --- src/lib/craft/craft_repl_dev.cpp | 34 +++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/lib/craft/craft_repl_dev.cpp b/src/lib/craft/craft_repl_dev.cpp index a2d155c..5c289ab 100644 --- a/src/lib/craft/craft_repl_dev.cpp +++ b/src/lib/craft/craft_repl_dev.cpp @@ -317,7 +317,24 @@ class HomeStoreCraftJournalBackend : public CraftJournalBackend { } CraftJournalEntry hdr{}; std::memcpy(&hdr, buf.bytes(), sizeof(CraftJournalEntry)); + // Same validate-before-trust guards as read_slot(): a corrupt/misplaced/malformed record must + // fail cleanly here too, not just when read via read_slot(). Trusting hdr.len (and deriving + // nlbas/blkid_off from it) or hdr.all_zeros from an unvalidated header risks deserializing a + // garbage blkid and freeing whatever blocks that garbage happens to decode to. + if (hdr.magic != k_journal_magic || hdr.version != k_journal_version) { + LOGE("free_slot lsn={} bad magic/version: magic={:#x} version={}", lsn, hdr.magic, hdr.version); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } + if (hdr.lsn != lsn) { + LOGE("free_slot lsn={} hdr.lsn={} mismatch -- corrupt or misplaced record", lsn, hdr.lsn); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } if (hdr.all_zeros) co_return ok(); + if (hdr.len == 0 || hdr.len % lba_size_ != 0) { + LOGE("free_slot lsn={} hdr.len={} not a positive multiple of lba_size={} -- malformed record", lsn, + hdr.len, lba_size_); + co_return std::unexpected(std::make_error_condition(std::errc::io_error)); + } // hdr.len bytes of real data means a non-empty csum array precedes the blkid in this // slot's blob -- [header][csums][blkid], not [header][blkid] -- so the blkid offset must @@ -706,6 +723,18 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 LOGW("write discarded post-flight: term changed dlsn={}", dlsn); stale_post_flight = true; } else { + // Populate the journal-tail overlay (makes this entry locally readable ahead of commit() + // applying it) BEFORE erasing dlsn from missing_lsns_, and both under the same missing_mu_ + // critical section commit_impl's is_missing check also takes: otherwise a concurrent + // commit_impl run (another write()'s or keep_alive()'s piggyback) could see dlsn already + // erased, read/apply/retire it, and advance commit_lsn past it -- all before this call gets + // around to inserting the overlay entry. That would leave a permanently-orphaned overlay + // entry below commit_lsn: harmless (clamped out at read time) but never retired until the + // same LBA happens to be overwritten by a later write. Doing both under one lock closes the + // window entirely -- commit_impl either still sees dlsn as missing (stalls) or sees it + // resolved with the overlay already populated, never the state in between. + populate_overlay(dlsn, static_cast< lba_t >(addr) / lba_size_, static_cast< uint32_t >(len / lba_size_), + all_zeros, blkid, csums); missing_lsns_.erase(dlsn); snapshot = {state_.commit_lsn, state_.last_append_lsn}; } @@ -713,11 +742,6 @@ async_result< craft::lsn_pair > CraftReplDev::write(craft::client_hdr hdr, int64 if (stale_post_flight) { co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); } - // Populate the journal-tail overlay: makes this entry locally readable ahead of commit() applying - // it to the index. - populate_overlay(dlsn, static_cast< lba_t >(addr) / lba_size_, static_cast< uint32_t >(len / lba_size_), all_zeros, - blkid, csums); - // Best-effort piggyback: advance commit_lsn toward the client's own view of it. Every outcome -- // a stall at a gap, or even a genuine commit() fault -- is ignored here; the write itself already // succeeded, and the next write/keep_alive retries the advance. keep_alive() is where a real