Skip to content

Scope the interner's write-permit bookkeeping per mapping (memory-safety fix) - #129

Open
MesTTo wants to merge 1 commit into
trueagi-io:mainfrom
MesTTo:interner-permit-scoping
Open

Scope the interner's write-permit bookkeeping per mapping (memory-safety fix)#129
MesTTo wants to merge 1 commit into
trueagi-io:mainfrom
MesTTo:interner-permit-scoping

Conversation

@MesTTo

@MesTTo MesTTo commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

The defect

mork-interning caches one permission-slot index per THREAD (MAPPING_THREAD_INDEX), unscoped by mapping and never cleared on release. Consequences, all reachable today:

  • After a thread's first permit ever, try_aquire_permission on any later mapping short-circuits without registering on it: the thread reads and writes permissions[k] / to_bytes[k] of a mapping whose slot k it does not own.
  • Another thread can then legitimately CAS that same slot, breaking the per-slot exclusivity that get_sym_or_insert's lock-free eager slab write relies on (its comment: "once a thread has a permission it is the only one allowed to change those atomic fields"). Two writers interleave on one slab: torn write_pos/data, i.e. garbage interned symbol bytes. Downstream this surfaces as flaky parse-time byte_item "reserved N" panics wherever many threads and mappings mix — we hit it as a nondeterministic parse panic under a parallel test suite, backtracing through add_sexpr -> transformDataInto.
  • Nested permits across two mappings leak the second mapping's slot (never released), and the LAST drop zeroes a slot on whichever mapping that permit references — freeing a foreign thread's slot out from under it.

The fix

Per-thread permit entries keyed by mapping identity: (mapping, slot, live). Acquisition refcounts the entry for the same mapping or CAS-registers a fresh slot on the requested mapping; release decrements its own mapping's entry and frees the slot exactly when that count reaches zero. WritePermit methods resolve their slot through their own mapping's entry. The single-slot-per-thread-per-mapping design is preserved; only the bookkeeping is scoped correctly.

Tests

  • permits_are_scoped_per_mapping: nested permits across two mappings hold one slot each; releases never cross mappings; re-acquisition after full release registers afresh.
  • interning_is_exclusive_under_thread_and_mapping_churn: 8 threads, each alternating a private mapping and a shared one per round, read-back-verifying every interned symbol — the torn-write class, guarded.

Also makes serialization.rs's test module compile under current nightly (explicit reborrow instead of an implicit autoref of a raw-pointer dereference). The two serialization tests fail before and after this change on my environment and are otherwise untouched.

The interning crate's suite: 8 passed / the 2 pre-existing serialization failures. The full kernel suite over this change (together with in-flight work in my fork) has run clean repeatedly under heavy test parallelism that previously tripped the corruption several times per run.

The interner cached ONE permission-slot index per thread, unscoped by mapping,
and never cleared it: after a thread's first permit ever, a permit on any LATER
mapping short-circuited without registering there at all. The thread then used
an unowned slot index on that mapping -- a slot another thread could
legitimately acquire, breaking the per-slot exclusivity the lock-free eager
slab write relies on (two writers tear one slab: garbage interned symbol
bytes, surfacing as flaky parse-time byte_item 'reserved' panics wherever many
threads and mappings mix, e.g. a parallel test suite). Nested permits across
two mappings additionally leaked the second mapping's slot, and the last drop
zeroed a slot on whichever mapping that permit referenced, freeing a foreign
thread's slot out from under it.

Replace the cached index and the global live count with per-thread permit
ENTRIES keyed by mapping identity: (mapping, slot, live). Acquisition
refcounts an existing entry for the same mapping or CAS-registers a fresh
slot; release decrements ITS OWN mapping's entry and frees the slot exactly
when that count reaches zero. WritePermit methods resolve their slot through
their own mapping's entry.

Regression tests: nested permits across two mappings hold one slot EACH with
releases that never cross mappings, and an 8-thread churn hammer (a private
mapping then a shared one, per round) that read-back-verifies every interned
symbol against the torn-write class.

Also make serialization.rs's test compile under current nightly (explicit
reborrow instead of an implicit autoref of a raw-pointer dereference). The two
serialization tests fail before AND after this change on this environment;
they are untouched otherwise.
@Adam-Vandervorst

Copy link
Copy Markdown
Collaborator

@ClarkeRemy can you look into this?

@MesTTo

MesTTo commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Three notes for review: the last-drop path uses an .expect() inside Drop (a poisoned state would abort rather than surface); there is a duplicated impl<'a> WritePermit<'a> header block worth merging; and one body imprecision — in the nested-permit case it is the FIRST mapping's slot that leaks (the second was never registered), not the second as written. The fix itself reproduces the corruption on stock main deterministically (the churn test reads back another thread's bytes) and resolves it; the two remaining failures are the pre-existing env-dependent serialization tests, failing identically on both sides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants