rayon path creates multiple aliasing &mut SkillStore through a usize-laundered pointer (UB) #13

New Issue

2026-08-04T19:16:03Z

logaritmisk commented

2026-08-04 19:16:03 +00:00

TimeSlice::sweep_color_groups under --features rayon (src/time_slice.rs:406-415):

let skills_addr: usize = (&mut self.skills as *mut SkillStore) as usize;
self.events[range].par_iter_mut().for_each(move |ev| {
    // SAFETY: see above.
    let skills: &mut SkillStore = unsafe { &mut *(skills_addr as *mut SkillStore) };
    ...
    ev.iteration_direct(skills, agents, p_draw, convergence, &mut arena);
});

Every rayon worker materialises its own &mut SkillStore pointing at the same SkillStore. That is an aliasing violation on its face: Rust's model permits at most one live &mut T to a given T, and &mut carries noalias down to LLVM. This is UB regardless of which elements the threads subsequently touch. Miri under -Zmiri-tree-borrows/Stacked Borrows will reject it.

Casting the pointer through usize also launders provenance, which is what lets the code compile without a Send wrapper — it hides the sharing from the type system rather than establishing that the sharing is sound.

What the existing SAFETY comment does and doesn't establish

The three-point argument in the comment (src/time_slice.rs:368-373, repeated at 400-405) is about element disjointness:

Events in the same color group access disjoint Index values in self.skills, so concurrent writes land on different memory locations.

That is true and it is what makes the code work in practice today — I traced the write path and iteration_direct only calls skills.get/get_mut (src/time_slice.rs:157-161), never insert, so the backing Vec cannot reallocate mid-sweep and there is no actual data race on real hardware right now.

But element disjointness is not the property the aliasing rules ask about. Two &mut SkillStore existing simultaneously is already the violation; whether they are used to touch overlapping bytes only determines whether it currently miscompiles. The invariant is also unenforced: ColorGroups::color_range (src/color_group.rs:53) derives its range from first()/last() and assumes the group's indices are contiguous — an assumption established by recompute_color_groups but never asserted. If that contiguity is ever broken, ranges from different colors overlap and the disjointness premise fails silently, turning formal UB into a real race.

Fix

Express the split so the borrow checker sees it, rather than defeating it:

Split the storage, not the reference. Give SkillStore a method that hands out disjoint &mut sub-views, or store skills as Vec<UnsafeCell<Skill>>/Vec<AtomicCell<…>> so shared access is legal by construction and the unsafe is confined to a small, auditable accessor.
Or restructure to a deferred apply: workers write per-event deltas into thread-local buffers, merged sequentially after the color completes. That removes the unsafe entirely, at the cost of the allocation profile T3 deliberately avoided.
If the raw-pointer approach is kept, at minimum: carry a *mut SkillStore in a struct SkillsPtr(*mut SkillStore); unsafe impl Send for SkillsPtr {} wrapper instead of laundering through usize, and derive &mut per element rather than per store.

Whatever the approach, add debug_assert! that each color group's indices are contiguous and that the color ranges are pairwise disjoint, so the invariant the safety argument rests on is actually checked.

Acceptance

cargo +nightly miri test --features rayon is clean (or the aliasing-sensitive tests are, if the whole suite is impractical under Miri).
The determinism test (tests/determinism.rs) still passes bit-identically at RAYON_NUM_THREADS={1,2,4,8}.
No regression on benches/history_converge.rs.

`TimeSlice::sweep_color_groups` under `--features rayon` (`src/time_slice.rs:406-415`): ```rust let skills_addr: usize = (&mut self.skills as *mut SkillStore) as usize; self.events[range].par_iter_mut().for_each(move |ev| { // SAFETY: see above. let skills: &mut SkillStore = unsafe { &mut *(skills_addr as *mut SkillStore) }; ... ev.iteration_direct(skills, agents, p_draw, convergence, &mut arena); }); ``` Every rayon worker materialises its own `&mut SkillStore` pointing at **the same** `SkillStore`. That is an aliasing violation on its face: Rust's model permits at most one live `&mut T` to a given `T`, and `&mut` carries `noalias` down to LLVM. This is UB regardless of which *elements* the threads subsequently touch. Miri under `-Zmiri-tree-borrows`/Stacked Borrows will reject it. Casting the pointer through `usize` also launders provenance, which is what lets the code compile without a `Send` wrapper — it hides the sharing from the type system rather than establishing that the sharing is sound. ## What the existing SAFETY comment does and doesn't establish The three-point argument in the comment (`src/time_slice.rs:368-373`, repeated at 400-405) is about *element* disjointness: > 2. Events in the same color group access disjoint `Index` values in `self.skills`, so concurrent writes land on different memory locations. That is true and it is what makes the code work in practice today — I traced the write path and `iteration_direct` only calls `skills.get`/`get_mut` (`src/time_slice.rs:157-161`), never `insert`, so the backing `Vec` cannot reallocate mid-sweep and there is no actual data race on real hardware right now. But element disjointness is not the property the aliasing rules ask about. Two `&mut SkillStore` existing simultaneously is already the violation; whether they are used to touch overlapping bytes only determines whether it *currently* miscompiles. The invariant is also unenforced: `ColorGroups::color_range` (`src/color_group.rs:53`) derives its range from `first()`/`last()` and assumes the group's indices are contiguous — an assumption established by `recompute_color_groups` but never asserted. If that contiguity is ever broken, ranges from different colors overlap and the disjointness premise fails silently, turning formal UB into a real race. ## Fix Express the split so the borrow checker sees it, rather than defeating it: - **Split the storage, not the reference.** Give `SkillStore` a method that hands out disjoint `&mut` sub-views, or store skills as `Vec<UnsafeCell<Skill>>`/`Vec<AtomicCell<…>>` so shared access is legal by construction and the unsafe is confined to a small, auditable accessor. - **Or restructure to a deferred apply**: workers write per-event deltas into thread-local buffers, merged sequentially after the color completes. That removes the unsafe entirely, at the cost of the allocation profile T3 deliberately avoided. - If the raw-pointer approach is kept, at minimum: carry a `*mut SkillStore` in a `struct SkillsPtr(*mut SkillStore); unsafe impl Send for SkillsPtr {}` wrapper instead of laundering through `usize`, and derive `&mut` per element rather than per store. Whatever the approach, add `debug_assert!` that each color group's indices are contiguous and that the color ranges are pairwise disjoint, so the invariant the safety argument rests on is actually checked. ## Acceptance - `cargo +nightly miri test --features rayon` is clean (or the aliasing-sensitive tests are, if the whole suite is impractical under Miri). - The determinism test (`tests/determinism.rs`) still passes bit-identically at `RAYON_NUM_THREADS={1,2,4,8}`. - No regression on `benches/history_converge.rs`. ## Related - #5 — if `rayon` ever becomes default-on, this moves from "opt-in feature has UB" to "every downstream user has UB".

logaritmisk referenced this issue

2026-08-04 19:20:44 +00:00

Dead and duplicated code: a stale `iterate_to_convergence`, a copy-pasted sweep, and `#[allow(dead_code)]` used as a mute button #23

logaritmisk referenced this issue

2026-08-04 19:21:02 +00:00

No CI — nothing verifies builds, tests, feature combinations, or determinism #24

logaritmisk commented

2026-08-04 20:11:28 +00:00

Fixed in 06b6a68 — by removing the unsafe rather than tightening it.

The safety argument was about disjoint elements, which was a hint that the operation is separable rather than merely safe-in-practice. Events in a color group touch disjoint agents, so none can observe another's writes. Event::iteration_direct is now split:

compute runs inference over shared &self.skills, mutating nothing — so a whole color group runs concurrently with no aliasing question at all;
apply folds the results in afterwards, in index order.

No raw pointers, no provenance laundering, no &mut aliasing. The apply order does not depend on which worker finished first, so posteriors stay bit-identical across thread counts — tests/determinism.rs still passes at RAYON_NUM_THREADS 1/2/4/8, and CI now runs it at all four on every push.

The crate contains no unsafe at all, locked in with #![forbid(unsafe_code)]. That is stronger than the Miri acceptance criterion, which is now moot.

ColorGroups::groups_are_contiguous was added and is asserted after each rebuild and inside color_range, so the contiguity the ranges depend on is checked rather than assumed.

Cost, sequential vs parallel on this machine: the deferred apply gives back part of the win on the one workload where rayon ever helped (11.75 ms → 10.46 ms, 1.12×, against the 1.3× T3 reported). The sequential path is untouched. Splitting compute from apply also removed the duplicated sweep body flagged in #23.

Fixed in 06b6a68 — by removing the `unsafe` rather than tightening it. The safety argument was about disjoint *elements*, which was a hint that the operation is separable rather than merely safe-in-practice. Events in a color group touch disjoint agents, so none can observe another's writes. `Event::iteration_direct` is now split: - `compute` runs inference over shared `&self.skills`, mutating nothing — so a whole color group runs concurrently with no aliasing question at all; - `apply` folds the results in afterwards, in index order. No raw pointers, no provenance laundering, no `&mut` aliasing. The apply order does not depend on which worker finished first, so posteriors stay bit-identical across thread counts — `tests/determinism.rs` still passes at `RAYON_NUM_THREADS` 1/2/4/8, and CI now runs it at all four on every push. The crate contains **no `unsafe` at all**, locked in with `#![forbid(unsafe_code)]`. That is stronger than the Miri acceptance criterion, which is now moot. `ColorGroups::groups_are_contiguous` was added and is asserted after each rebuild and inside `color_range`, so the contiguity the ranges depend on is checked rather than assumed. Cost, sequential vs parallel on this machine: the deferred apply gives back part of the win on the one workload where rayon ever helped (11.75 ms → 10.46 ms, 1.12×, against the 1.3× T3 reported). The sequential path is untouched. Splitting compute from apply also removed the duplicated sweep body flagged in #23.

logaritmisk referenced this issue

2026-08-04 20:13:12 +00:00

Dead and duplicated code: a stale `iterate_to_convergence`, a copy-pasted sweep, and `#[allow(dead_code)]` used as a mute button #23

logaritmisk closed this issue

2026-08-04 20:13:35 +00:00

logaritmisk added the bug label 2026-09-07 13:53:13 +00:00

Sign in to join this conversation.