Executor field order is load-bearing for teardown safety and undocumented #14

New Issue

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

hugo commented

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

Symptom

The field declaration order of struct Executor is load-bearing for memory safety: it determines the order in which GPU buffers, the Metal context, and the mmap'd model are torn down. Reordering those three fields — a change that looks purely cosmetic and that no reviewer or lint would flag — introduces a use-after-free. There is no comment saying so.

This is not currently a bug. It is a correct arrangement with no documentation protecting it, in a file that was recently split and reorganised (4292e0d Split Rust code into domain modules).

Evidence

src/engine/metal.rs:561-571:

pub(super) struct Executor {
    weights: Weights,
    session: Session,        // owns every Buffer -> ds4_gpu_tensor_free
    logits: Vec<f32>,
    tokens: Vec<i32>,
    quality: bool,
    checkpoint_tag: [u8; 32],
    model_modified: (u64, u32),
    _context: Context,       // Drop -> ds4_gpu_cleanup()
    model: Model,            // owns the Mmap -> munmap
}

Rust drops fields in declaration order, so the current sequence is: buffers freed → ds4_gpu_cleanup() → mmap unmapped. Both edges matter:

session before _context: Session/LayerState/CompressionState (src/engine/metal.rs:463-484) own Buffers whose Drop calls ds4_gpu_tensor_free (src/engine/metal/gpu.rs:922). Those must run before the device teardown in ds4_gpu_cleanup().
_context before model: this is the sharp one. Context::open passes the raw mmap pointer to the C side (src/engine/metal/gpu.rs:783-789, ds4_gpu_set_model_map_range(model.main.map_ptr().cast(), ...)), and the Objective-C side wraps that memory in Metal buffers without copying it:
- native/metal/ds4_metal.m:10329 and :1791 — [g_device newBufferWithBytesNoCopy:(void *)(base + page_offset) ...]
So live MTLBuffers alias the mapping directly. The mapping must outlive them, which is exactly what the current order guarantees.

How ds4 handles this — and why DS4Server deliberately differs

The C reference tears down in the opposite order:

ds4.c:56282-56289:

weights_free(&e->weights);      /* :6564 — this is just a memset, it releases nothing */
vocab_free(&e->vocab);
ds4_threads_shutdown();
if (e->mtp_model.map) model_close(&e->mtp_model);
model_close(&e->model);          /* :2306 — munmap() happens HERE */
ds4_gpu_cleanup();               /* ...and the GPU model views are released AFTER */

model_close() at ds4.c:2310 calls munmap((void *)m->map, m->size).
weights_free() at ds4.c:6564 is memset(w, 0, sizeof(*w)) — it does not call ds4_gpu_model_views_clear(). The only callers of that helper are inside ds4_metal.m itself (:9283, :10138, and the re-map paths).

So ds4 unmaps the model and only then releases the no-copy Metal buffers that alias it. In practice this is likely benign — an MTLBuffer created with newBufferWithBytesNoCopy and no deallocator block does not dereference the pages on release — but it is the weaker ordering, and DS4Server's is the defensible one.

This is the actionable part: because the two codebases disagree, someone comparing them later could "fix" DS4Server to match the C reference and introduce a real use-after-free. The divergence needs to be recorded as intentional.

Suggested fix

No code change. Add a comment above the last three fields of Executor stating:

fields drop in declaration order, and this order is required;
session's Buffers must be freed before ds4_gpu_cleanup();
model's mmap must outlive ds4_gpu_cleanup() because ds4_metal.m wraps it with newBufferWithBytesNoCopy (cite native/metal/ds4_metal.m:10329);
this intentionally differs from ../ds4/ds4.c:56287-56288, which unmaps first — do not "align" it.

Optional belt-and-braces: a Drop for Executor that explicitly documents/enforces the sequence, so the invariant survives even a field reorder. Weigh that against the extra code; the comment may well be enough.

Acceptance criteria

The comment exists and names both constraints plus the ObjC line it depends on.
If a Drop impl is added instead, make bundle and a real generation run still succeed with no Metal validation errors (MTL_DEBUG_LAYER=1 is a useful check here).

## Symptom The field declaration order of `struct Executor` is load-bearing for memory safety: it determines the order in which GPU buffers, the Metal context, and the mmap'd model are torn down. Reordering those three fields — a change that looks purely cosmetic and that no reviewer or lint would flag — introduces a use-after-free. There is no comment saying so. This is not currently a bug. It is a correct arrangement with no documentation protecting it, in a file that was recently split and reorganised (`4292e0d Split Rust code into domain modules`). ## Evidence `src/engine/metal.rs:561-571`: ```rust pub(super) struct Executor { weights: Weights, session: Session, // owns every Buffer -> ds4_gpu_tensor_free logits: Vec<f32>, tokens: Vec<i32>, quality: bool, checkpoint_tag: [u8; 32], model_modified: (u64, u32), _context: Context, // Drop -> ds4_gpu_cleanup() model: Model, // owns the Mmap -> munmap } ``` Rust drops fields in declaration order, so the current sequence is: **buffers freed → `ds4_gpu_cleanup()` → mmap unmapped.** Both edges matter: - `session` before `_context`: `Session`/`LayerState`/`CompressionState` (`src/engine/metal.rs:463-484`) own `Buffer`s whose `Drop` calls `ds4_gpu_tensor_free` (`src/engine/metal/gpu.rs:922`). Those must run before the device teardown in `ds4_gpu_cleanup()`. - `_context` before `model`: this is the sharp one. `Context::open` passes the raw mmap pointer to the C side (`src/engine/metal/gpu.rs:783-789`, `ds4_gpu_set_model_map_range(model.main.map_ptr().cast(), ...)`), and the Objective-C side wraps that memory in Metal buffers **without copying it**: - `native/metal/ds4_metal.m:10329` and `:1791` — `[g_device newBufferWithBytesNoCopy:(void *)(base + page_offset) ...]` So live `MTLBuffer`s alias the mapping directly. The mapping must outlive them, which is exactly what the current order guarantees. ## How ds4 handles this — and why DS4Server deliberately differs The C reference tears down in the **opposite** order: - `ds4.c:56282-56289`: ```c weights_free(&e->weights); /* :6564 — this is just a memset, it releases nothing */ vocab_free(&e->vocab); ds4_threads_shutdown(); if (e->mtp_model.map) model_close(&e->mtp_model); model_close(&e->model); /* :2306 — munmap() happens HERE */ ds4_gpu_cleanup(); /* ...and the GPU model views are released AFTER */ ``` - `model_close()` at `ds4.c:2310` calls `munmap((void *)m->map, m->size)`. - `weights_free()` at `ds4.c:6564` is `memset(w, 0, sizeof(*w))` — it does **not** call `ds4_gpu_model_views_clear()`. The only callers of that helper are inside `ds4_metal.m` itself (`:9283`, `:10138`, and the re-map paths). So ds4 unmaps the model and only then releases the no-copy Metal buffers that alias it. In practice this is likely benign — an `MTLBuffer` created with `newBufferWithBytesNoCopy` and no deallocator block does not dereference the pages on release — but it is the weaker ordering, and DS4Server's is the defensible one. **This is the actionable part:** because the two codebases disagree, someone comparing them later could "fix" DS4Server to match the C reference and introduce a real use-after-free. The divergence needs to be recorded as intentional. ## Suggested fix No code change. Add a comment above the last three fields of `Executor` stating: - fields drop in declaration order, and this order is required; - `session`'s `Buffer`s must be freed before `ds4_gpu_cleanup()`; - `model`'s mmap must outlive `ds4_gpu_cleanup()` because `ds4_metal.m` wraps it with `newBufferWithBytesNoCopy` (cite `native/metal/ds4_metal.m:10329`); - this intentionally differs from `../ds4/ds4.c:56287-56288`, which unmaps first — do not "align" it. Optional belt-and-braces: a `Drop for Executor` that explicitly documents/enforces the sequence, so the invariant survives even a field reorder. Weigh that against the extra code; the comment may well be enough. ## Acceptance criteria - The comment exists and names both constraints plus the ObjC line it depends on. - If a `Drop` impl is added instead, `make bundle` and a real generation run still succeed with no Metal validation errors (`MTL_DEBUG_LAYER=1` is a useful check here).

hugo added the bug label 2026-07-25 09:41:14 +00:00

hugo commented

2026-07-25 11:04:23 +00:00

Fixed in 3708afb. Executor now documents its safety-critical declaration/drop order: session buffers must precede Metal context teardown, and the model mmap must outlive context cleanup because native/metal/ds4_metal.m:10329 wraps it with newBufferWithBytesNoCopy. The comment also records the intentional divergence from ../ds4/ds4.c:56287-56288. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.

Fixed in 3708afb. Executor now documents its safety-critical declaration/drop order: session buffers must precede Metal context teardown, and the model mmap must outlive context cleanup because native/metal/ds4_metal.m:10329 wraps it with newBufferWithBytesNoCopy. The comment also records the intentional divergence from ../ds4/ds4.c:56287-56288. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.

hugo closed this issue

2026-07-25 11:04:27 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: hugo/DS4Server#14