commands: drop does not unlock the collection it just freed

A use-after-free. Dispatch held `drop`'s collection lock across the handler,
`drop_collection` freed the Collection the lock lives in, and dispatch then
ran `unlock_collection` on freed memory -- an atomic read-modify-write
inside `Io.RwLock.unlock`. One insert and one drop was enough.

Two things kept it hidden for this long. `drop` had no unit test at all:
before this commit every `parse_fake_msg("drop", ...)` in the tree was in a
test written to hunt it. And over the wire it does not fault -- 25
insert/drop cycles against a live server pass -- because the general
allocator leaves the freed page mapped and the atomic write lands somewhere
harmless. That was luck, not safety: the same undefined behaviour either
way, and testing.allocator is what makes it visible, which is why the
regression test is a unit test rather than an e2e script.

Fixed by giving `drop` no collection lock at all. The catalog lock is what
actually excludes here: every collection lock in this engine -- dispatch,
the TTL sweep, `compact`'s rebuild, `write_catalog`, `slab_stats`,
reclamation -- is taken while holding the catalog at least shared, so
holding it exclusively already keeps every one of them out. The collection
lock was buying exclusion that was already there and paying for it by
locking an object about to cease existing.

That exposed a second bug rather than creating one. The dispatch epilogue --
commit, then maybe checkpoint, then maybe compact -- fired on
`locks.coll == .exclusive` as a stand-in for "this was a write". It is now
keyed on `kind == .write`, because the two agreed only by accident:
`dropDatabase` is the one write that never held a collection lock, so it has
never reached that epilogue, and a dropped database waited for some later
write to trigger the checkpoint that records it.

Mutation-checked: restoring `.coll = .exclusive` on the drop row reproduces
the original SIGSEGV in the new test.

208/208 unit (2 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, crud
scorecard unchanged at 204/87, aggregation corpus 70/0, full e2e matrix,
crash-fuzz and the 25-cycle wire drop probe green.

Left open and recorded in PLAN §6: `drop_collection` still writes no log
record, so a dropped collection resurrects on reopen unless a checkpoint ran
(pre-existing, with its own test at db.zig:5542); and `apply_pending_write`
drops under `engine.rwlock` rather than the catalog lock, so the two drop
paths disagree about which lock protects a namespace.
This commit was merged in pull request #5.
This commit is contained in:
A.Shakhmatov
2026-08-10 18:41:55 +03:00
parent 8136ffe8d4
commit 3c2ac38fd9
2 changed files with 120 additions and 15 deletions

50
PLAN.md
View File

@@ -1045,19 +1045,43 @@ has to be its own commit with its own re-recorded scorecard.
`$out`/`$merge` durability semantics, whether the expression evaluator is
shared with M3's pipeline updates, and whether `allowDiskUse` has to stop
being a lie.
- **`drop` segfaults when dispatched in-process** — found while writing the
positional-refusal tests, unrelated to them, and **reproduces at `3c5eee2`
with the change stashed**, so it is not caused by that work. A single
`insert` followed by a single `drop` against a `TestDb` engine terminates
with SIGSEGV and no stack trace; dropping a collection that never existed is
fine. The wire path is clean — 25 insert / refused-update / distinct / drop
cycles against a live server on `:27020` all passed — so what differs is the
in-process caller, not the command. That matters more than it looks: D1's
whole architecture is "in-process server now, library + C API later", and
this is the embedding path. **`drop` has no unit test at all** — the only
`parse_fake_msg("drop", ...)` calls in the tree were the scratch ones written
to find this — which is why it went unseen. Needs its own commit: reproduce
it in a committed test first, then fix.
- **`drop` unlocked the collection it had just freed** — *fixed.* Found while
writing the positional-refusal tests and not caused by them; it reproduced
at `3c5eee2` with that work stashed.
A use-after-free, not an allocator quirk. Dispatch held `drop`'s collection
lock across the handler, the handler freed the `Collection` the lock lives
in, and dispatch then ran `unlock_collection` on freed memory — an atomic
read-modify-write inside `Io.RwLock.unlock`. One insert and one drop was
enough.
The wire path not faulting was luck, not safety: 25 insert/drop cycles
against a live server pass because the general allocator leaves the freed
page mapped, so the atomic write lands somewhere harmless. It was the same
undefined behaviour either way, and `testing.allocator` is what made it
visible. **`drop` had no unit test at all**, which is why it went unseen.
Fixed by giving `drop` no collection lock. The catalog lock is what actually
excludes: every collection lock in the engine — dispatch, the TTL sweep,
`compact`'s rebuild, `write_catalog`, `slab_stats`, reclamation — is taken
while holding the catalog at least shared, so `drop` holding it exclusively
already keeps all of them out. The collection lock bought exclusion that was
already there and paid for it by locking an object about to cease existing.
That change also made the dispatch epilogue's predicate wrong, and it turned
out to have been wrong already: it fired on `locks.coll == .exclusive` as a
stand-in for "this was a write", and **`dropDatabase` is the one write that
never held a collection lock**, so it had never reached the commit and
checkpoint epilogue at all. Now keyed on `kind == .write`.
Still open, and deliberately not fixed here: `drop_collection` writes no log
record, so a dropped collection resurrects on reopen unless a checkpoint
happened to run — a pre-existing limitation with its own test at
`db.zig:5542`. Separately, `apply_pending_write` (`$out`/`$merge`) calls
`drop_collection` under `engine.rwlock` rather than the catalog lock, so it
is not excluded by the reasoning above; it holds no collection lock, so it
is not this crash, but the two drops disagree about which lock protects the
namespace and that wants one answer.
- **M3 update operators** — open. `distinct` landed first because it was a
whole missing command with no dependencies, and measuring it turned up three
things worth keeping, none of which are `distinct`'s to fix: