Buffer::view has no lifetime tie to its base buffer #13

New Issue

hugo · 2026-07-25T09:41:13Z

hugo commented

2026-07-25 09:41:13 +00:00

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:

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 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):

: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:

Tie the lifetime. Introduce a View<'a> newtype that borrows its base:
```
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.
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

hugo commented

2026-07-25 11:02:33 +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.

hugo closed this issue

2026-07-25 11:02:40 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: hugo/DS4Server#13