Skip to content

Commit 656a368

Browse files
committed
fix(hpc/blocked_grid): UB — overlapping &mut [T] in GridBlockMut (codex P1×2)
Codex review on PR #158 flagged two P1 soundness bugs: 1. `BaseBlockIterMut::next()` yields `GridBlockMut` instances carrying `data: &'a mut [T]` slices over the strided block footprint (`[start..start + (BR-1)*padded_cols + BC]`). For grids with multiple block columns, adjacent column blocks' slices overlap heavily — e.g., on a 64×128 padded grid with 64×64 blocks, block (0,0) covers `data[0..8128]` and block (0,1) covers `data[64..8192]`. Two such `&mut [T]` simultaneously live = UB. 2. `TierBlockIterMut::next()` yields `GridSuperBlockMut` instances with `data: *mut T` set to `data_ptr.add(row_origin * padded_cols)` — col_origin doesn't enter the offset, so adjacent super-blocks in the same row receive IDENTICAL raw pointers spanning the full row slab. `base_blocks_mut()` then materialized strided `&mut [T]` from these colliding super-block pointers, propagating the UB. Both bugs trace to the same root cause: `GridBlockMut::data` stored a strided `&'a mut [T]` slice that referenced cells outside the block's own column range. Adjacent column blocks fundamentally share rows that interleave in memory; no contiguous `&mut [T]` can describe a block without aliasing siblings. Fix: change `GridBlockMut::data` from `&'a mut [T]` to `*mut T` + add `data_len: usize` for bounds checking. The struct's new aliasing invariant (documented in the type-level docstring) is: `data` is NEVER converted to a wide `&mut [T]`; cell access happens exclusively through `row_mut(r)`, which materializes `&mut [T]` of length BC starting at the block's own `(row_origin + r, col_origin)`. Across blocks, these per-row materializations target disjoint cells (each block owns its own `[col_origin, col_origin + BC)` column range), so no two live `&mut [T]` ever alias. Also: - `GridBlockMut::from_raw` is now `unsafe fn` with documented caller contract (raw pointer + length + per-block column-disjoint invariant) - Added `unsafe impl Send + Sync` for `GridBlockMut<T: Send/Sync>` matching the existing pattern on `BaseBlockIterMut` / `GridSuperBlockMut` - Renamed `GridBlockMut::padded_cols` pub(crate) accessor to `padded_cols_stride` for naming consistency with `GridBlock` (resolves a PR-X3.1 housekeeping item early) - Replaced `data_mut() -> &mut [T]` pub(crate) accessor with `data_ptr() -> *mut T` + `data_len() -> usize`. The wide-slice accessor was the materialization vehicle for the UB. - Updated `iter.rs::row_mut` to materialize via `slice::from_raw_parts_mut` with a debug_assert bounds check and verbatim SAFETY comment - Updated `super_block.rs::base_blocks_mut` to pass raw pointer + length to the new unsafe `from_raw` (no intermediate strided slice) - Updated `super_block.rs::tier_mut_2_mutation_visible` test to use `row_mut(0)[0]` instead of the removed `data_mut()` accessor All 5 gates still green: - cargo check: PASS - cargo test --lib hpc::blocked_grid: 111/111 PASS - cargo test --doc hpc::blocked_grid: 79/79 PASS - cargo fmt --check: clean - cargo clippy -D warnings: clean Codex audit gap noted for PR-X3.1: future audits need a SAFETY-claim verification gate that simulates adversarial iterator usage (e.g., collect all yielded items into a Vec before consuming any) to catch this class of latent UB that passes type-checking but violates the aliasing model.
1 parent 81766e6 commit 656a368

3 files changed

Lines changed: 131 additions & 55 deletions

File tree

src/hpc/blocked_grid/base.rs

Lines changed: 86 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -448,19 +448,54 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlock<'a, T, BR, BC> {
448448
/// assert_eq!(blk.block_row(), 0);
449449
/// assert_eq!(blk.block_col(), 0);
450450
/// ```
451+
/// Mutable view of a `BR × BC` base block.
452+
///
453+
/// # Aliasing invariant
454+
///
455+
/// `GridBlockMut` stores a raw `*mut T` pointer + `data_len` rather than a
456+
/// `&'a mut [T]` slice. The strided layout of base blocks means adjacent
457+
/// column blocks share rows that interleave in memory (block (r, c) and
458+
/// block (r, c+1) on the same block-row overlap by `(BR-1)*padded_cols + BC -
459+
/// BC = (BR-1)*padded_cols` cells if represented as a single strided slice).
460+
/// A strided `&mut [T]` would be unsound when two such blocks coexist (as
461+
/// `BaseBlockIterMut` allows when callers `.next()` multiple times before
462+
/// dropping prior items).
463+
///
464+
/// Soundness is preserved by **never materializing a `&mut [T]` over the
465+
/// strided footprint**. Cell access goes through [`row_mut`] which produces
466+
/// `&mut [T]` of length BC only — exactly one logical row of the block.
467+
/// Across blocks, row materializations target disjoint cell ranges (each
468+
/// block owns its own `[col_origin, col_origin + BC)` column range on the
469+
/// shared physical rows), so no two simultaneous `&mut [T]` ever alias.
470+
///
471+
/// [`row_mut`]: GridBlockMut::row_mut
451472
pub struct GridBlockMut<'a, T, const BR: usize, const BC: usize> {
452473
block_row: usize,
453474
block_col: usize,
454475
row_origin: usize,
455476
col_origin: usize,
456477
padded_cols: usize,
457-
/// Points to the element at `(row_origin, col_origin)` in the parent grid's
458-
/// flat storage. Length is `BR * padded_cols` minus the trailing gap
459-
/// (covers the last block row up to the final cell).
460-
data: &'a mut [T],
478+
/// Raw pointer to cell `(row_origin, col_origin)` in the parent grid's
479+
/// flat storage. See the type-level aliasing invariant — `data` is
480+
/// **never** converted to a `&mut [T]` that covers the strided footprint.
481+
data: *mut T,
482+
/// Number of elements addressable from `data`. Equals
483+
/// `(BR - 1) * padded_cols + BC` for non-zero BR (or 0 if BR is 0),
484+
/// clamped to the parent grid's remaining storage. Used for bounds
485+
/// checking; the slice is never materialized at this full length.
486+
data_len: usize,
461487
_marker: PhantomData<&'a mut T>,
462488
}
463489

490+
// SAFETY: `GridBlockMut` holds a raw pointer that the issuing iterator
491+
// (`BaseBlockIterMut` or `GridSuperBlockMut::base_blocks_mut`) creates from a
492+
// single `&'a mut BlockedGrid` borrow. The aliasing invariant documented on
493+
// the struct guarantees that simultaneously-live `GridBlockMut`s never
494+
// materialize overlapping `&mut [T]` slices, so cross-thread Send/Sync are
495+
// safe under T: Send / T: Sync.
496+
unsafe impl<'a, T: Send, const BR: usize, const BC: usize> Send for GridBlockMut<'a, T, BR, BC> {}
497+
unsafe impl<'a, T: Sync, const BR: usize, const BC: usize> Sync for GridBlockMut<'a, T, BR, BC> {}
498+
464499
impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
465500
/// Construct a `GridBlockMut` from a mutable grid reference and block coordinates.
466501
///
@@ -475,21 +510,26 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
475510
pub fn from_grid(grid: &'a mut BlockedGrid<T, BR, BC>, block_row: usize, block_col: usize) -> Self {
476511
let row_origin = block_row * BR;
477512
let col_origin = block_col * BC;
478-
let start = row_origin * grid.padded_cols + col_origin;
479-
let end = if BR == 0 {
480-
start
481-
} else {
482-
start + (BR - 1) * grid.padded_cols + BC
483-
};
484-
let end = end.min(grid.data.len());
485513
let padded_cols = grid.padded_cols;
514+
let start = row_origin * padded_cols + col_origin;
515+
let raw_len = if BR == 0 { 0 } else { (BR - 1) * padded_cols + BC };
516+
let data_len = raw_len.min(grid.data.len().saturating_sub(start));
517+
// SAFETY: `start` is within `grid.data.len()` because the iterator
518+
// (or direct caller) gates `(block_row, block_col)` to valid block
519+
// coordinates. `data_len` is clamped to the remaining storage. The
520+
// raw pointer is offset from a unique `&'a mut BlockedGrid` borrow;
521+
// although adjacent column-block raw pointers cover overlapping
522+
// strided regions, the struct's aliasing invariant (see GridBlockMut
523+
// doc) ensures no `&mut [T]` is ever materialized over the overlap.
524+
let data = unsafe { grid.data.as_mut_ptr().add(start) };
486525
Self {
487526
block_row,
488527
block_col,
489528
row_origin,
490529
col_origin,
491530
padded_cols,
492-
data: &mut grid.data[start..end],
531+
data,
532+
data_len,
493533
_marker: PhantomData,
494534
}
495535
}
@@ -542,30 +582,47 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
542582
self.col_origin
543583
}
544584

545-
/// Access internal mutable data slice (intra-crate seam used by `iter.rs`
546-
/// / `compute.rs`). `pub(crate)` — not exposed downstream since callers
547-
/// should go through `GridBlockMut::row_mut`, which enforces the BR
548-
/// bound.
549-
pub(crate) fn data_mut(&mut self) -> &mut [T] {
585+
/// Raw data pointer (intra-crate seam used by `iter.rs::row_mut`). Returns
586+
/// a `*mut T` that callers MUST NOT convert to a wide `&mut [T]` covering
587+
/// the block's strided footprint — see the type-level aliasing invariant.
588+
/// The only sound use is `unsafe { slice::from_raw_parts_mut(ptr.add(r *
589+
/// padded_cols_stride()), BC) }` for `r < BR`, which `row_mut` performs.
590+
pub(crate) fn data_ptr(&mut self) -> *mut T {
550591
self.data
551592
}
552593

553-
/// Access padded_cols stride (intra-crate seam — see `data_mut` note).
554-
/// NOTE: Named `padded_cols` (not `padded_cols_stride` as on `GridBlock`)
555-
/// for back-compat with `iter.rs`'s `row_mut` impl. Naming consistency is
556-
/// queued as PR-X3.1 housekeeping.
557-
pub(crate) fn padded_cols(&self) -> usize {
594+
/// Number of elements addressable from `data_ptr` (intra-crate seam used
595+
/// for debug-build bounds checks in `row_mut`).
596+
pub(crate) fn data_len(&self) -> usize {
597+
self.data_len
598+
}
599+
600+
/// Access padded_cols stride (intra-crate seam — see `data_ptr` note).
601+
/// NOTE: Named `padded_cols_stride` to match `GridBlock`. The
602+
/// previous `padded_cols` name is gone (Q-X3.1 housekeeping resolved
603+
/// in this commit).
604+
pub(crate) fn padded_cols_stride(&self) -> usize {
558605
self.padded_cols
559606
}
560607

561608
/// Construct a `GridBlockMut` directly from raw components.
562609
///
563-
/// Used by super_block.rs (worker A3) to construct mutable base-block views
564-
/// inside a super-block without needing `T: Copy`. The caller is responsible
565-
/// for ensuring that `data` is a valid exclusive sub-slice of the parent
566-
/// grid's flat storage with the correct `padded_cols` stride.
567-
pub(super) fn from_raw(
568-
data: &'a mut [T], block_row: usize, block_col: usize, row_origin: usize, col_origin: usize, padded_cols: usize,
610+
/// Used by super_block.rs (worker A3) to construct mutable base-block
611+
/// views inside a super-block without needing `T: Copy`.
612+
///
613+
/// # Safety
614+
///
615+
/// The caller must guarantee that:
616+
/// - `data` points to a valid element in the parent grid's flat storage
617+
/// - `data_len` is the number of elements addressable from `data` (equal to
618+
/// `(BR - 1) * padded_cols + BC` for non-zero BR, clamped to the remaining
619+
/// storage)
620+
/// - `data` and `data_len` together cover the strided footprint of one
621+
/// `BR × BC` block; no other live `GridBlockMut` will materialize an
622+
/// overlapping `&mut [T]` over the same physical cells
623+
pub(super) unsafe fn from_raw(
624+
data: *mut T, data_len: usize, block_row: usize, block_col: usize, row_origin: usize, col_origin: usize,
625+
padded_cols: usize,
569626
) -> Self {
570627
Self {
571628
block_row,
@@ -574,6 +631,7 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
574631
col_origin,
575632
padded_cols,
576633
data,
634+
data_len,
577635
_marker: PhantomData,
578636
}
579637
}

src/hpc/blocked_grid/iter.rs

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -305,9 +305,29 @@ impl<'a, T, const BR: usize, const BC: usize> GridBlockMut<'a, T, BR, BC> {
305305
/// ```
306306
pub fn row_mut(&mut self, r: usize) -> &mut [T] {
307307
debug_assert!(r < BR, "row {} out of block range {}", r, BR);
308-
let stride = self.padded_cols();
308+
let stride = self.padded_cols_stride();
309309
let start = r * stride;
310-
&mut self.data_mut()[start..start + BC]
310+
debug_assert!(
311+
start + BC <= self.data_len(),
312+
"row materialization {} extends past data_len {}",
313+
start + BC,
314+
self.data_len()
315+
);
316+
let ptr = self.data_ptr();
317+
// SAFETY: This is the SOLE materialization site for a `&mut [T]` over
318+
// a `GridBlockMut`'s storage. The slice has length BC and starts at
319+
// `(row_origin + r, col_origin)`, covering exactly one logical row of
320+
// the block (cells `[col_origin, col_origin + BC)`). This range is
321+
// **disjoint** from any other live `GridBlockMut`'s row materialization
322+
// because:
323+
// - Adjacent column-block rows (same physical grid row, different
324+
// block_col) have non-overlapping `[col_origin, col_origin + BC)`
325+
// intervals.
326+
// - Adjacent row-block rows (different block_row) target different
327+
// physical grid rows entirely.
328+
// The bounds check above confirms `start + BC <= data_len`, ensuring
329+
// the pointer arithmetic stays within the parent grid's allocation.
330+
unsafe { std::slice::from_raw_parts_mut(ptr.add(start), BC) }
311331
}
312332
}
313333

src/hpc/blocked_grid/super_block.rs

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -295,26 +295,26 @@ impl<'a, T, const BR: usize, const BC: usize, const N: usize> GridSuperBlockMut<
295295
let abs_col_origin = col_origin_abs + local_bc * BC;
296296

297297
let start = local_br * BR * padded_cols + abs_col_origin;
298-
let end = if BR == 0 {
299-
start
300-
} else {
301-
start + (BR - 1) * padded_cols + BC
302-
};
303-
let end = end.min(data_len);
304-
305-
// SAFETY: Each (local_br, local_bc) pair accesses a
306-
// non-overlapping sub-region of the super-block's data buffer.
307-
// Base blocks within the N×N super-block occupy disjoint
308-
// row ranges (local_br selects which BR-row group) and disjoint
309-
// column positions within each row (local_bc selects which BC
310-
// column group). The iterator yields them one at a time, so
311-
// the caller cannot hold two simultaneous mutable borrows to
312-
// the same data. `data_ptr` is valid for `data_len` elements
313-
// — guaranteed by `TierBlockIterMut` which derives it from the
314-
// grid's `Vec<T>`.
315-
let slice = unsafe { std::slice::from_raw_parts_mut(data_ptr.add(start), end - start) };
316-
317-
GridBlockMut::from_raw(slice, abs_block_row, abs_block_col, abs_row_origin, abs_col_origin, padded_cols)
298+
let raw_len = if BR == 0 { 0 } else { (BR - 1) * padded_cols + BC };
299+
let block_data_len = raw_len.min(data_len.saturating_sub(start));
300+
301+
// SAFETY: data_ptr was issued by TierBlockIterMut from a
302+
// unique `&'a mut BlockedGrid` borrow; data_len elements from
303+
// data_ptr are within the grid's allocation. (start,
304+
// block_data_len) stays within those bounds. We pass the raw
305+
// pointer (not a materialized slice) to GridBlockMut::from_raw
306+
// because the strided footprint of adjacent column-blocks
307+
// overlaps in memory — GridBlockMut's aliasing invariant
308+
// ensures `&mut [T]` is only ever materialized per-row via
309+
// `row_mut`, where each block's column range [col_origin,
310+
// col_origin+BC) is disjoint from siblings.
311+
let block_data = unsafe { data_ptr.add(start) };
312+
unsafe {
313+
GridBlockMut::from_raw(
314+
block_data, block_data_len, abs_block_row, abs_block_col, abs_row_origin, abs_col_origin,
315+
padded_cols,
316+
)
317+
}
318318
})
319319
})
320320
}
@@ -804,11 +804,9 @@ mod tests {
804804
// Write via base_blocks_mut: set first cell of first base block.
805805
for mut blk in sb.base_blocks_mut() {
806806
if blk.block_row() == sr * 2 && blk.block_col() == sc * 2 {
807-
// Use base_blocks_mut raw data — access via data_mut()
808-
let d = blk.data_mut();
809-
if !d.is_empty() {
810-
d[0] = sentinel;
811-
}
807+
// Materialize one row slice (sound — see GridBlockMut
808+
// aliasing invariant); write cell 0.
809+
blk.row_mut(0)[0] = sentinel;
812810
break;
813811
}
814812
}

0 commit comments

Comments
 (0)