Buffer::view() returns an owned Buffer with no borrow relationship to the buffer it points into. Nothing in the type system prevents a view from outliving its base. The code is sound today only because of a property of the Objective-C side that the Rust signature does not state — so a future change on either side of the boundary can break it silently.
Evidence
src/engine/metal/gpu.rs:848-852:
pub(super)fnview(&self,offset: u64,bytes: u64)-> Result<Self,String>{NonNull::new(unsafe{ds4_gpu_tensor_view(self.raw(),offset,bytes)}).map(Self).ok_or_else(||"Metal could not create a tensor view".to_owned())}
The returned Self is a full Buffer: it is Dropped through the same ds4_gpu_tensor_free path at src/engine/metal/gpu.rs:922-926, and it can be stored, moved, and returned independently of self.
Why it is currently fine, from the C side (native/metal/ds4_metal.m):
:7916ds4_gpu_tensor_view() creates a fresh DS4MetalTensor with view.buffer = base_obj.buffer — ARC retains the underlying id<MTLBuffer>, so the allocation outlives the base wrapper.
:7929 sets view.owner = 0, and :7942ds4_gpu_tensor_free() only performs the owning teardown when owner is set, so freeing a view does not free the base and double-free is not possible.
Bounds are checked at :7919-7922 — offset > base_obj.bytes, bytes > base_obj.bytes - offset, and overflow of base_obj.offset + offset all return NULL, which the Rust side turns into an error via NonNull::new.
That is a genuinely careful C implementation. The problem is that none of it is visible from the Rust signature, and the file it lives in is a vendored copy (see the build.rs issue) that gets re-synced from ../ds4/ds4_metal.m by hand.
How ds4 handles this
The C engine has the same aliasing pattern and manages it by convention plus explicit commentary rather than by type:
ds4.c:15564-15625 — the teardown block frees a long list of tensors, with comments spelling out the ownership rule where it is non-obvious, e.g. ds4.c:15579"Unallocated slots are NULL and ds4_gpu_tensor_free(NULL) is a no-op."
ds4.c:56270-56277 — tensor-parallel view arrays (e->tp.batch_out_views, in_views, out_views) are freed element-by-element before the slab they view into (ds4_gpu_tensor_free(e->tp.slab) at :56277). The ordering is deliberate and manual.
ds4.c:188 — void ds4_gpu_tensor_free_in_place(ds4_gpu_tensor *t) { (void)t; }, a deliberate no-op, exists so callers can express "this one is not mine to free".
In C there is no alternative to convention. In Rust there is, and DS4Server is currently paying the C convention's cost without taking the Rust benefit.
Suggested fix
Pick one — either is acceptable, the first is stronger:
Tie the lifetime. Introduce a View<'a> newtype that borrows its base:
with the same Drop and a raw() accessor, and change view() to fn view<'a>(&'a self, ...) -> Result<View<'a>, String>. Check the call sites in src/engine/metal.rs first — if views are stored in longer-lived structs, this may cascade, in which case go with option 2 and record why.
Document the invariant at minimum. Add a // SAFETY: comment on view() recording that (a) the base allocation is kept alive by ARC on the Objective-C side, not by Rust, (b) views are owner = 0 so freeing one never frees the base, and (c) both properties live in native/metal/ds4_metal.m:7916-7940 and must be re-checked whenever that vendored file is re-synced from ../ds4/ds4_metal.m.
Acceptance criteria
Either view() returns a lifetime-bound type, or it carries a SAFETY comment naming the exact ObjC lines the soundness argument depends on.
No behaviour change; make bundle and cargo test --all-features pass.
## Symptom
`Buffer::view()` returns an owned `Buffer` with no borrow relationship to the buffer it points into. Nothing in the type system prevents a view from outliving its base. The code is sound today only because of a property of the Objective-C side that the Rust signature does not state — so a future change on either side of the boundary can break it silently.
## Evidence
`src/engine/metal/gpu.rs:848-852`:
```rust
pub(super) fn view(&self, offset: u64, bytes: u64) -> Result<Self, String> {
NonNull::new(unsafe { ds4_gpu_tensor_view(self.raw(), offset, bytes) })
.map(Self)
.ok_or_else(|| "Metal could not create a tensor view".to_owned())
}
```
The returned `Self` is a full `Buffer`: it is `Drop`ped through the same `ds4_gpu_tensor_free` path at `src/engine/metal/gpu.rs:922-926`, and it can be stored, moved, and returned independently of `self`.
Why it is currently fine, from the C side (`native/metal/ds4_metal.m`):
- `:7916` `ds4_gpu_tensor_view()` creates a fresh `DS4MetalTensor` with `view.buffer = base_obj.buffer` — ARC retains the underlying `id<MTLBuffer>`, so the allocation outlives the base wrapper.
- `:7929` sets `view.owner = 0`, and `:7942` `ds4_gpu_tensor_free()` only performs the owning teardown when `owner` is set, so freeing a view does not free the base and double-free is not possible.
- Bounds are checked at `:7919-7922` — `offset > base_obj.bytes`, `bytes > base_obj.bytes - offset`, and overflow of `base_obj.offset + offset` all return `NULL`, which the Rust side turns into an error via `NonNull::new`.
That is a genuinely careful C implementation. The problem is that **none of it is visible from the Rust signature**, and the file it lives in is a vendored copy (see the `build.rs` issue) that gets re-synced from `../ds4/ds4_metal.m` by hand.
## How ds4 handles this
The C engine has the same aliasing pattern and manages it by convention plus explicit commentary rather than by type:
- `ds4.c:15564-15625` — the teardown block frees a long list of tensors, with comments spelling out the ownership rule where it is non-obvious, e.g. `ds4.c:15579` *"Unallocated slots are NULL and ds4_gpu_tensor_free(NULL) is a no-op."*
- `ds4.c:56270-56277` — tensor-parallel view arrays (`e->tp.batch_out_views`, `in_views`, `out_views`) are freed element-by-element *before* the slab they view into (`ds4_gpu_tensor_free(e->tp.slab)` at `:56277`). The ordering is deliberate and manual.
- `ds4.c:188` — `void ds4_gpu_tensor_free_in_place(ds4_gpu_tensor *t) { (void)t; }`, a deliberate no-op, exists so callers can express "this one is not mine to free".
In C there is no alternative to convention. In Rust there is, and DS4Server is currently paying the C convention's cost without taking the Rust benefit.
## Suggested fix
Pick one — either is acceptable, the first is stronger:
1. **Tie the lifetime.** Introduce a `View<'a>` newtype that borrows its base:
```rust
pub(super) struct View<'a> { handle: NonNull<GpuTensor>, _base: PhantomData<&'a Buffer> }
```
with the same `Drop` and a `raw()` accessor, and change `view()` to `fn view<'a>(&'a self, ...) -> Result<View<'a>, String>`. Check the call sites in `src/engine/metal.rs` first — if views are stored in longer-lived structs, this may cascade, in which case go with option 2 and record why.
2. **Document the invariant at minimum.** Add a `// SAFETY:` comment on `view()` recording that (a) the base allocation is kept alive by ARC on the Objective-C side, not by Rust, (b) views are `owner = 0` so freeing one never frees the base, and (c) both properties live in `native/metal/ds4_metal.m:7916-7940` and must be re-checked whenever that vendored file is re-synced from `../ds4/ds4_metal.m`.
## Acceptance criteria
- Either `view()` returns a lifetime-bound type, or it carries a `SAFETY` comment naming the exact ObjC lines the soundness argument depends on.
- No behaviour change; `make bundle` and `cargo test --all-features` pass.
hugo
added the bug label 2026-07-25 09:41:13 +00:00
Fixed in 44fce65 using the issue's documented-invariant option. Buffer::view now carries a SAFETY contract naming native/metal/ds4_metal.m:7916-7940, including bounds checks, ARC retention of the base Metal buffer, non-owning view teardown, and the required re-check on vendor re-sync. No behavior changed. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.
Fixed in 44fce65 using the issue's documented-invariant option. Buffer::view now carries a SAFETY contract naming native/metal/ds4_metal.m:7916-7940, including bounds checks, ARC retention of the base Metal buffer, non-owning view teardown, and the required re-check on vendor re-sync. No behavior changed. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Symptom
Buffer::view()returns an ownedBufferwith no borrow relationship to the buffer it points into. Nothing in the type system prevents a view from outliving its base. The code is sound today only because of a property of the Objective-C side that the Rust signature does not state — so a future change on either side of the boundary can break it silently.Evidence
src/engine/metal/gpu.rs:848-852:The returned
Selfis a fullBuffer: it isDropped through the sameds4_gpu_tensor_freepath atsrc/engine/metal/gpu.rs:922-926, and it can be stored, moved, and returned independently ofself.Why it is currently fine, from the C side (
native/metal/ds4_metal.m)::7916ds4_gpu_tensor_view()creates a freshDS4MetalTensorwithview.buffer = base_obj.buffer— ARC retains the underlyingid<MTLBuffer>, so the allocation outlives the base wrapper.:7929setsview.owner = 0, and:7942ds4_gpu_tensor_free()only performs the owning teardown whenowneris set, so freeing a view does not free the base and double-free is not possible.:7919-7922—offset > base_obj.bytes,bytes > base_obj.bytes - offset, and overflow ofbase_obj.offset + offsetall returnNULL, which the Rust side turns into an error viaNonNull::new.That is a genuinely careful C implementation. The problem is that none of it is visible from the Rust signature, and the file it lives in is a vendored copy (see the
build.rsissue) that gets re-synced from../ds4/ds4_metal.mby hand.How ds4 handles this
The C engine has the same aliasing pattern and manages it by convention plus explicit commentary rather than by type:
ds4.c:15564-15625— the teardown block frees a long list of tensors, with comments spelling out the ownership rule where it is non-obvious, e.g.ds4.c:15579"Unallocated slots are NULL and ds4_gpu_tensor_free(NULL) is a no-op."ds4.c:56270-56277— tensor-parallel view arrays (e->tp.batch_out_views,in_views,out_views) are freed element-by-element before the slab they view into (ds4_gpu_tensor_free(e->tp.slab)at:56277). The ordering is deliberate and manual.ds4.c:188—void ds4_gpu_tensor_free_in_place(ds4_gpu_tensor *t) { (void)t; }, a deliberate no-op, exists so callers can express "this one is not mine to free".In C there is no alternative to convention. In Rust there is, and DS4Server is currently paying the C convention's cost without taking the Rust benefit.
Suggested fix
Pick one — either is acceptable, the first is stronger:
Tie the lifetime. Introduce a
View<'a>newtype that borrows its base:with the same
Dropand araw()accessor, and changeview()tofn view<'a>(&'a self, ...) -> Result<View<'a>, String>. Check the call sites insrc/engine/metal.rsfirst — if views are stored in longer-lived structs, this may cascade, in which case go with option 2 and record why.Document the invariant at minimum. Add a
// SAFETY:comment onview()recording that (a) the base allocation is kept alive by ARC on the Objective-C side, not by Rust, (b) views areowner = 0so freeing one never frees the base, and (c) both properties live innative/metal/ds4_metal.m:7916-7940and must be re-checked whenever that vendored file is re-synced from../ds4/ds4_metal.m.Acceptance criteria
view()returns a lifetime-bound type, or it carries aSAFETYcomment naming the exact ObjC lines the soundness argument depends on.make bundleandcargo test --all-featurespass.Fixed in
44fce65using the issue's documented-invariant option. Buffer::view now carries a SAFETY contract naming native/metal/ds4_metal.m:7916-7940, including bounds checks, ARC retention of the base Metal buffer, non-owning view teardown, and the required re-check on vendor re-sync. No behavior changed. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.