fix(runtime): add acquire fence before dense subclass seqlock recheck - #8735
fix(runtime): add acquire fence before dense subclass seqlock recheck#8735jdalton wants to merge 1 commit into
Conversation
cached_dense_layout loaded slots/bounds with Relaxed ordering then rechecked sequence with Acquire, which only orders loads sequenced after it, not the payload loads that precede it in program order. On weakly ordered targets the payload loads could be reordered past the recheck, letting a reader combine slots from one publisher with bounds from a colliding publisher for a different cache key. Flagged by coderabbitai on PerryTS#8668.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesDense layout cache
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR adds an acquire fence to prevent inconsistent dense-layout cache reads. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…his-methods and proven Symbols (#8737) Lands #8735, #8729 and #8731. #8735 fixes a real data race in the dense-array subclass layout cache. `cached_dense_layout` read two payload fields `Relaxed` and then rechecked the sequence counter with an `Acquire` LOAD -- but an acquire load only constrains what follows it, so on weakly ordered hardware the preceding payload reads could sink past the recheck. A reader racing a colliding publisher could then combine one field from the old layout with one from the new and return a WRONG ELEMENT rather than faulting. A standalone `fence(Acquire)` now sits between the payload reads and the recheck, which is the canonical seqlock-reader form. #8729 lowers strict equality against a proven Symbol to raw NaN-boxed identity, keeping loose equality, reassigned locals and erased TypeScript annotation claims on the semantic runtime helpers. #8731 (fixes #8693) publishes producer-authoritative proven-`this` method capabilities through imports, aliases and re-exports, emitting guarded direct imported clone calls while retaining generic dispatch fallbacks. A changelog fragment was added for #8735, which had neither one nor a skip-changelog label. No version bump. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
|
Landed on Good catch, and the reasoning is exactly right — an I added a Validated on the merged result: all 30 lint checkers, runtime 2669/0 at |
|
@proggeramlug Fast, before I could fix the lint issue :P |
2 participants
This fixes a data race in the dense-array subclass layout cache where a reader could combine fields from two different cache publishers and return a wrong element value instead of crashing.
Root cause and fix - a relaxed payload read can be reordered past an acquire recheck on weakly-ordered hardware
cached_dense_layoutreads two payload fields with relaxed ordering and then rechecks a sequence counter with an acquire load to make sure no writer touched the cache slot in between. An acquire load only holds back operations that come after it in the code, so it does nothing to stop the two payload reads that come before it from being reordered later by the CPU on weakly-ordered hardware like ARM. If a different cache key collides into the same slot and gets published while a reader is mid-read, the reader can end up combining one field from the old layout with the other field from the new layout. This adds an explicit acquire fence between the payload reads and the recheck, which is the standard fix for this kind of seqlock reader bug.CodeRabbit originally flagged this exact issue on pull request
#8668, which closed without the fix landing. This PR carries the same fix forward against current main.cargo check -p perry-runtime --libproduces no diagnostics forarray/subclass.rs. The crate currently fails to build for an unrelated, pre-existing reason:perf_histogram.rscallsalgebraic_sub/algebraic_mulwithout the crate declaring#![feature(float_algebraic)]. I confirmed this is unrelated to this change by checking that every diagnostic file path points atperf_histogram.rs, neverarray/subclass.rs.Summary by CodeRabbit