M3: refuse positional paths instead of destroying arrays; fix the drop use-after-free #5

dev · 2026-08-10T15:48:30Z

dev commented

2026-08-10 15:48:30 +00:00

Four commits: the design review that found the bug, the refusal, the record of
a second bug found while testing it, and that bug's fix.

The design review found data loss, not a missing feature

PLAN sized arrayFilters as "14 corpus cases". Measured against mongod 8.3.7:

// { _id: 1, y: [ {b: 3}, {b: 1} ] }
updateOne({}, {$set: {'y.$[i].b': 2}}, {arrayFilters: [{'i.b': 3}]})

mongod answers y: [{b: 2}, {b: 1}]. This server answered y: {"$[i]": {"b": 2}}
-- the array replaced by a document keyed by the path segment's literal text,
every element discarded, ok: 1, modifiedCount: 1. Remotely reachable by any
client issuing an ordinary update.

arrayFilters was not implicated (the string appears nowhere in src/). The
destruction was in set_path's "treat as non-array: replace with a doc"
branch, so all three spellings -- $, $[], $[<ident>] -- destroyed under
every operator. $inc through $[i] stored its operand instead of
incrementing. 16 of 17 probe cases diverged.

The gate finding. M3's gate is "remaining crud coverage; e2e3/e2e4 green".
e2e3/e2e4 contain zero positional paths. The pinned corpus covers
$[<ident>] only -- no $[] case, no bare $ case anywhere. A fix scoped to
the gate would have gone green on 14 new passes with two of the three ways to
destroy an array still live: the gate would have certified the bug fixed.

The refusal

Chosen over implementing (review §5, option D): it stops the data loss now
rather than waiting on a path-model redesign, and it lets the eventual corpus
be recorded from mongod while this server refuses cleanly.

positional segments: refused up front, before anything is applied, BadValue (2)
plain non-numeric segment under an array (y.nope.b): PathNotViable (28), measured

Numeric segments untouched, including null padding past the end, now pinned.

Re-running the probe: 0 destructive cases remain, and 5 agree with mongod's
code exactly -- every case where mongod also refuses.

The drop use-after-free

Found while writing those tests, and not caused by them -- it reproduces at
3c5eee2 with the work stashed.

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. One insert and one drop was enough. Over the wire it did not fault
-- 25 cycles pass -- because the general allocator leaves the page mapped, so
the atomic write lands somewhere harmless. Luck, not safety.

Fixed by giving drop no collection lock: every collection lock in the engine is
taken while holding the catalog at least shared, so drop holding it exclusively
already excludes them all.

That exposed a second bug rather than creating one. The commit/checkpoint
epilogue 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 that epilogue at all. Now keyed on kind == .write.

Verification

208/208 unit (10 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation
corpus 70/0, full e2e matrix, crash-fuzz, and a 25-cycle wire drop probe.

Scorecard unchanged at 204/87 by design: the 14 arrayFilters cases still fail,
now reporting the refusal rather than a corrupted document.

Three mutation checks: dropping the up-front scan reddens four tests on the
array's contents rather than on the error; restoring the destructive branch
reddens the PathNotViable test; restoring .coll = .exclusive on the drop row
reproduces the original SIGSEGV.

Left open, recorded in PLAN §6

drop_collection writes no log record, so a dropped collection resurrects on
reopen unless a checkpoint ran (pre-existing, own test at db.zig:5542)
apply_pending_write drops under engine.rwlock rather than the catalog
lock, so the two drop paths disagree about which lock protects a namespace
the unknown-query-operator gap from the distinct work

Four commits: the design review that found the bug, the refusal, the record of a second bug found while testing it, and that bug's fix. ## The design review found data loss, not a missing feature PLAN sized `arrayFilters` as "14 corpus cases". Measured against mongod 8.3.7: ```js // { _id: 1, y: [ {b: 3}, {b: 1} ] } updateOne({}, {$set: {'y.$[i].b': 2}}, {arrayFilters: [{'i.b': 3}]}) ``` mongod answers `y: [{b: 2}, {b: 1}]`. This server answered **`y: {"$[i]": {"b": 2}}`** -- the array replaced by a document keyed by the path segment's literal text, every element discarded, `ok: 1, modifiedCount: 1`. Remotely reachable by any client issuing an ordinary update. `arrayFilters` was not implicated (the string appears nowhere in `src/`). The destruction was in `set_path`'s "treat as non-array: replace with a doc" branch, so all three spellings -- `$`, `$[]`, `$[<ident>]` -- destroyed under every operator. `$inc` through `$[i]` stored its operand instead of incrementing. 16 of 17 probe cases diverged. **The gate finding.** M3's gate is "remaining crud coverage; e2e3/e2e4 green". `e2e3`/`e2e4` contain zero positional paths. The pinned corpus covers `$[<ident>]` only -- no `$[]` case, no bare `$` case anywhere. A fix scoped to the gate would have gone green on 14 new passes with two of the three ways to destroy an array still live: **the gate would have certified the bug fixed.** ## The refusal Chosen over implementing (review §5, option D): it stops the data loss now rather than waiting on a path-model redesign, and it lets the eventual corpus be recorded from mongod while this server refuses cleanly. - positional segments: refused up front, before anything is applied, `BadValue` (2) - plain non-numeric segment under an array (`y.nope.b`): `PathNotViable` (28), measured Numeric segments untouched, including null padding past the end, now pinned. Re-running the probe: **0 destructive cases remain**, and 5 agree with mongod's code exactly -- every case where mongod also refuses. ## The drop use-after-free Found while writing those tests, and **not caused by them** -- it reproduces at `3c5eee2` with the work stashed. 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. One insert and one drop was enough. Over the wire it did not fault -- 25 cycles pass -- because the general allocator leaves the page mapped, so the atomic write lands somewhere harmless. Luck, not safety. Fixed by giving `drop` no collection lock: every collection lock in the engine is taken while holding the catalog at least shared, so `drop` holding it exclusively already excludes them all. That exposed a second bug rather than creating one. The commit/checkpoint epilogue 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 that epilogue at all. Now keyed on `kind == .write`. ## Verification 208/208 unit (10 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation corpus 70/0, full e2e matrix, crash-fuzz, and a 25-cycle wire drop probe. Scorecard unchanged at 204/87 by design: the 14 arrayFilters cases still fail, now reporting the refusal rather than a corrupted document. Three mutation checks: dropping the up-front scan reddens four tests on the array's contents rather than on the error; restoring the destructive branch reddens the PathNotViable test; restoring `.coll = .exclusive` on the drop row reproduces the original SIGSEGV. ## Left open, recorded in PLAN §6 - `drop_collection` writes no log record, so a dropped collection resurrects on reopen unless a checkpoint ran (pre-existing, own test at `db.zig:5542`) - `apply_pending_write` drops under `engine.rwlock` rather than the catalog lock, so the two drop paths disagree about which lock protects a namespace - the unknown-query-operator gap from the `distinct` work

dev added 4 commits 2026-08-10 15:48:30 +00:00

docs: M3 design review -- arrayFilters, and the positional operators 3c5eee2171

The plan names a missing feature sized at 14 corpus cases. The measurement
found a data-loss bug.

`{$set: {'y.$[i].b': 2}}` does not fail to update the array -- it replaces
the array with `{"$[i]": {"b": 2}}` and answers ok: 1, modifiedCount: 1.
Every element is discarded. `src/update.zig:283` reaches a non-numeric
segment under an array and takes the "treat as non-array: replace with a
doc" branch, so `$`, `$[]` and `$[<ident>]` all destroy what they were meant
to descend into, under every operator -- `$inc` through `$[i]` stores the
operand rather than incrementing. 16 of 17 probe cases diverge from mongod.

`arrayFilters` is not implicated: the string appears nowhere in `src/`. The
option is accepted off the wire and dropped.

The gate finding, which is the reason to write this before any code: M3's
gate is "remaining crud coverage; e2e3/e2e4 green". e2e3 and e2e4 contain
zero positional paths and would stay green through every version of this
bug. The pinned corpus covers `$[<ident>]` only -- it has no `$[]` case and
no bare `$` case anywhere. A fix scoped to what the gate measures would go
green on 14 new passes with two of the three ways to destroy an array still
live. The gate would certify the bug as fixed.

Also measured, none of it guessable and two of them inverting how set_path
behaves today: one path can name many targets (`y.$[].c.$[].d` is a genuine
cross-product); a positional segment never creates anything, so a missing
or non-array path is an error where `$set: {'a.b': 1}` would construct, and
upsert gets no special case; ten distinct refusals across codes 2, 9 and 14,
recorded with their messages, including one row that breaks the otherwise
tidy 2/9 split and is left untidy on purpose.

Recommends refusing first as its own commit -- the M2 doctrine, and it stops
the data loss without waiting on the redesign -- then a recorded positional
corpus built like tests/spec/aggregate/, with `$[]` and `$[<ident>]`
together and `$` after, since only `$` needs the matched index carried out
of query evaluation.

update: refuse a positional path instead of destroying the array f04e7125c9

`{$set: {"y.$[i].b": 2}}` did not fail to update the array. It replaced
`y: [{b: 3}, {b: 1}]` with `y: {"$[i]": {"b": 2}}` -- every element
discarded -- and answered ok: 1, modifiedCount: 1. Remotely reachable by any
client issuing an ordinary MongoDB update.

`arrayFilters` was not implicated: the string appears nowhere in src/, the
option is accepted off the wire and dropped. The destruction was in the
path, at `set_path`'s "treat as non-array: replace with a doc" branch, so it
fired for all three spellings of "descend into this array" -- `$`, `$[]` and
`$[<ident>]` -- under every operator. `$inc` through `$[i]` stored its
operand rather than incrementing.

Two refusals, because the branch held two different mistakes:

  - a positional segment is refused up front, before anything is applied, so
    an update naming a good path and a positional one lands neither. Its
    code is BadValue (2), which is what mongod answers for every positional
    path failure.
  - a plain non-numeric segment under an array -- `y.nope.b`, `y.$x.b` -- is
    PathNotViable (28), measured. It is never a field to create, which is
    the opposite of what `set_path` does for a missing *document* field and
    the reason this branch existed at all.

Numeric segments are untouched, including the null padding past the end,
which a test now pins.

Messages are this server's own words. mongod's PathNotViable text embeds a
shell-syntax rendering of the offending element (`Cannot create field 'nope'
in element {y: [ { b: 3 }, { b: 1 } ]}`) and no BSON formatter here produces
it. The code is what the corpus asserts and the code is exact; a half-copy
of the text would be worse than a clear sentence that does not pretend.

Re-running the 17-case probe against both servers: 16 of 17 diverged before,
0 are destructive now, and 5 agree with mongod's code exactly -- every case
where mongod also refuses. The rest refuse where mongod succeeds, which is
the honest not-implemented state and is what the design review chose.

Scorecard unchanged at 204 pass / 87 fail: the 14 arrayFilters cases still
fail, now reporting the refusal rather than a corrupted document. That was
the gate this review picked -- `docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md` §5,
option D -- because the corpus has no `$[]` case and no bare `$` case at
all, so passing it would have certified two live ways to destroy an array.

206/206 unit (8 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation
corpus 70/0, full e2e matrix and crash-fuzz green. Both refusals are
mutation-checked: dropping the up-front scan reddens four tests on the
array's contents rather than on the error, and restoring the destructive
branch reddens the PathNotViable test.

plan: drop segfaults when dispatched in-process 8136ffe8d4

Found while writing the positional-refusal tests and not caused by them --
it reproduces at 3c5eee2 with that work stashed. One insert followed by one
drop against a TestDb engine terminates with SIGSEGV and no stack trace,
while 25 insert/update/distinct/drop cycles over the wire are clean, so what
differs is the in-process caller rather than the command.

Worth more than it looks: D1's architecture is "in-process server now,
library + C API later", and this is that path. `drop` has no unit test in
the tree at all, which is why it went unseen.

commands: drop does not unlock the collection it just freed 3c2ac38fd9

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.

dev merged commit 3c2ac38fd9 into main

2026-08-10 15:48:40 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dev/MultiforaDB#5