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)structExecutor{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
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.
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
The field declaration order of
struct Executoris 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 drops fields in declaration order, so the current sequence is: buffers freed →
ds4_gpu_cleanup()→ mmap unmapped. Both edges matter:sessionbefore_context:Session/LayerState/CompressionState(src/engine/metal.rs:463-484) ownBuffers whoseDropcallsds4_gpu_tensor_free(src/engine/metal/gpu.rs:922). Those must run before the device teardown inds4_gpu_cleanup()._contextbeforemodel: this is the sharp one.Context::openpasses 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:10329and: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:model_close()atds4.c:2310callsmunmap((void *)m->map, m->size).weights_free()atds4.c:6564ismemset(w, 0, sizeof(*w))— it does not callds4_gpu_model_views_clear(). The only callers of that helper are insideds4_metal.mitself (: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
MTLBuffercreated withnewBufferWithBytesNoCopyand 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
Executorstating:session'sBuffers must be freed beforeds4_gpu_cleanup();model's mmap must outliveds4_gpu_cleanup()becauseds4_metal.mwraps it withnewBufferWithBytesNoCopy(citenative/metal/ds4_metal.m:10329);../ds4/ds4.c:56287-56288, which unmaps first — do not "align" it.Optional belt-and-braces: a
Drop for Executorthat 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
Dropimpl is added instead,make bundleand a real generation run still succeed with no Metal validation errors (MTL_DEBUG_LAYER=1is a useful check here).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.