Scope the interner's write-permit bookkeeping per mapping (memory-safety fix) - #129
Open
MesTTo wants to merge 1 commit into
Open
Scope the interner's write-permit bookkeeping per mapping (memory-safety fix)#129MesTTo wants to merge 1 commit into
MesTTo wants to merge 1 commit into
Conversation
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.
Collaborator
|
@ClarkeRemy can you look into this? |
Contributor
Author
|
Three notes for review: the last-drop path uses an |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
mork-interningcaches one permission-slot index per THREAD (MAPPING_THREAD_INDEX), unscoped by mapping and never cleared on release. Consequences, all reachable today:try_aquire_permissionon any later mapping short-circuits without registering on it: the thread reads and writespermissions[k]/to_bytes[k]of a mapping whose slot k it does not own.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: tornwrite_pos/data, i.e. garbage interned symbol bytes. Downstream this surfaces as flaky parse-timebyte_item"reserved N" panics wherever many threads and mappings mix — we hit it as a nondeterministic parse panic under a parallel test suite, backtracing throughadd_sexpr -> transformDataInto.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.WritePermitmethods 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.