From 5377069bfaf29c368fe1038a053bcdc2b1e86e20 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Thu, 13 Aug 2026 18:06:01 +0200 Subject: [PATCH] fix: stop batch commands from dangling on return `Cmd.batch` holds a slice that the runtime reads after `update` has returned. `&.{ ... }` only lands in static memory when every element is comptime-known; one runtime value and the array is a stack temporary that is gone by the time the runtime walks it. Reading it back gives an invalid union tag and aborts. `examples/hello_world.zig` did exactly this: the 'c' key path built a batch containing `layout.size_cells` and friends. It now keeps the two commands in a field on the model, which outlives the call. Adds `Cmd.batchAlloc`/`Cmd.sequenceAlloc`, which copy into an allocator -- the frame allocator being the right one, since commands are processed during the tick that produced them. The lifetime rule is documented on the union fields and in the reference. Found while writing the model test harness: a batch built the obvious way came back as the wrong command entirely. --- REFERENCE.md | 35 +++++++++++ build.zig | 1 + examples/hello_world.zig | 12 +++- src/core/command.zig | 38 ++++++++++-- tests/command_tests.zig | 128 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 208 insertions(+), 6 deletions(-) create mode 100644 tests/command_tests.zig diff --git a/REFERENCE.md b/REFERENCE.md index aad604d..c0ba3b6 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -75,6 +75,41 @@ return .{ .delete_image = .{ .by_id = 1 } }; // Free cached image return .{ .delete_image = .all }; // Free all cached images ``` +#### Batch lifetimes + +`batch` and `sequence` hold a **slice**, which the runtime reads after `update` +has returned. A `&.{ ... }` literal only lives in static memory when every +element is comptime-known — one runtime value in there and the array becomes a +stack temporary that is gone by the time the runtime walks it: + +```zig +// Wrong: `row` is a runtime value, so this slice dangles on return. +return .{ .batch = &.{ + .{ .cache_image = .{ .source = .{ .file = path }, .image_id = 1 } }, + .{ .place_cached_image = .{ .image_id = 1, .row = row } }, +} }; +``` + +Either copy it into the frame allocator: + +```zig +return try zz.Cmd(Msg).batchAlloc(ctx.allocator, &.{ + .{ .cache_image = .{ .source = .{ .file = path }, .image_id = 1 } }, + .{ .place_cached_image = .{ .image_id = 1, .row = row } }, +}); +``` + +or keep the storage on the model, which outlives any single `update`: + +```zig +pending_batch: [2]zz.Cmd(Msg) = undefined, +// ... +self.pending_batch = .{ cache_cmd, place_cmd }; +return .{ .batch = &self.pending_batch }; +``` + +A batch of nothing but comptime-known commands is fine as a literal. + ### Styling Build styles by chaining properties: diff --git a/build.zig b/build.zig index 013164a..c62e353 100644 --- a/build.zig +++ b/build.zig @@ -95,6 +95,7 @@ pub fn build(b: *std.Build) void { "tests/layout_tests.zig", "tests/unicode_tests.zig", "tests/program_tests.zig", + "tests/command_tests.zig", "tests/focus_tests.zig", "tests/modal_tests.zig", "tests/tooltip_tests.zig", diff --git a/examples/hello_world.zig b/examples/hello_world.zig index f7f7c94..91a9cf5 100644 --- a/examples/hello_world.zig +++ b/examples/hello_world.zig @@ -21,6 +21,9 @@ const Model = struct { image_path: []const u8, protocol: zz.ImageProtocol, caps: zz.ImageCapabilities, + /// Backing storage for a `.batch` command. It lives on the model because + /// the runtime reads the slice after `update` has returned. + pending_batch: [2]zz.Cmd(Msg) = undefined, const image_gap_lines: u16 = 1; const cache_id: u32 = 42; @@ -75,7 +78,11 @@ const Model = struct { self.image_visible = true; self.image_cached = true; const layout = self.computeImageLayout(ctx); - return .{ .batch = &.{ + // Held on the model, not built as a `&.{ ... }` + // literal: a batch containing runtime values + // would be a stack temporary that dangles the + // moment this function returns. + self.pending_batch = .{ .{ .cache_image = .{ .source = .{ .file = self.image_path }, .image_id = cache_id, @@ -89,7 +96,8 @@ const Model = struct { .col = layout.col, .move_cursor = false, } }, - } }; + }; + return .{ .batch = &self.pending_batch }; } }, // Toggle z-index (behind text) diff --git a/src/core/command.zig b/src/core/command.zig index bb504aa..4463ec3 100644 --- a/src/core/command.zig +++ b/src/core/command.zig @@ -154,10 +154,17 @@ pub fn Cmd(comptime Msg: type) type { /// Request repeating tick at interval (nanoseconds) every: u64, - /// Execute a batch of commands + /// Execute a batch of commands. + /// + /// The slice has to outlive the command. `&.{ ... }` is only safe when + /// every element is comptime-known; one runtime value in there makes + /// the array a stack temporary that dangles as soon as `update` + /// returns. Use `batchAlloc` with the frame allocator, or point at an + /// array declared outside the function. batch: []const Cmd(Msg), - /// Execute commands in sequence (wait for each to complete) + /// Execute commands in sequence (wait for each to complete). + /// Same lifetime rule as `batch`. sequence: []const Cmd(Msg), /// Send a message to the update function @@ -231,16 +238,39 @@ pub fn Cmd(comptime Msg: type) type { return .{ .every = sec * std.time.ns_per_s }; } - /// Create a batch of commands + /// Create a batch of commands from a slice that already outlives the + /// command. See the lifetime note on `batch`. pub fn batchOf(cmds: []const Self) Self { return .{ .batch = cmds }; } - /// Create a sequence of commands + /// Create a sequence of commands from a slice that already outlives + /// the command. See the lifetime note on `batch`. pub fn sequenceOf(cmds: []const Self) Self { return .{ .sequence = cmds }; } + /// Create a batch by copying `cmds` into `allocator`. + /// + /// This is the safe way to build a batch out of runtime values: + /// + /// return try zz.Cmd(Msg).batchAlloc(ctx.allocator, &.{ + /// .{ .set_title = title }, + /// .{ .image_file = .{ .path = path, .row = row } }, + /// }); + /// + /// The frame allocator is the right choice — commands are processed + /// during the tick that produced them, and it is reset on the next one. + pub fn batchAlloc(allocator: std.mem.Allocator, cmds: []const Self) !Self { + return .{ .batch = try allocator.dupe(Self, cmds) }; + } + + /// Create a sequence by copying `cmds` into `allocator`. + /// See `batchAlloc`. + pub fn sequenceAlloc(allocator: std.mem.Allocator, cmds: []const Self) !Self { + return .{ .sequence = try allocator.dupe(Self, cmds) }; + } + /// Send a message pub fn send(message: Msg) Self { return .{ .msg = message }; diff --git a/tests/command_tests.zig b/tests/command_tests.zig new file mode 100644 index 0000000..36a1f9e --- /dev/null +++ b/tests/command_tests.zig @@ -0,0 +1,128 @@ +//! Command tests, focused on the lifetime of `batch`/`sequence` slices. +//! +//! `.{ .batch = &.{ ... } }` puts the array in static memory only when every +//! element is comptime-known. One runtime value and it becomes a stack +//! temporary, which dangles as soon as `update` returns — the runtime then +//! walks freed stack memory. These tests pin down the two ways to build a +//! batch that survives. + +const std = @import("std"); +const testing = std.testing; +const zz = @import("zigzag"); + +const Msg = union(enum) { key: zz.KeyEvent }; +const Cmd = zz.Cmd(Msg); + +/// Overwrites the stack region a returned-by-value temporary would occupy, so +/// a dangling slice shows up as garbage rather than reading intact. +fn clobberStack(seed: u64) u64 { + var scratch: [1024]u64 = undefined; + for (&scratch, 0..) |*slot, i| slot.* = seed +% i; + return scratch[seed % scratch.len]; +} + +fn buildWithAlloc(allocator: std.mem.Allocator, interval: u64) !Cmd { + return Cmd.batchAlloc(allocator, &.{ + .{ .set_title = "built at runtime" }, + .{ .every = interval }, + .{ .tick = interval * 2 }, + }); +} + +test "batchAlloc survives the frame that built it" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + // A runtime value, so the array cannot be folded into static memory. + var interval: u64 = 100; + std.mem.doNotOptimizeAway(&interval); + interval += 1; + + const cmd = try buildWithAlloc(arena.allocator(), interval); + _ = clobberStack(3); + + try testing.expect(cmd == .batch); + try testing.expectEqual(@as(usize, 3), cmd.batch.len); + try testing.expect(cmd.batch[0] == .set_title); + try testing.expectEqualStrings("built at runtime", cmd.batch[0].set_title); + try testing.expectEqual(interval, cmd.batch[1].every); + try testing.expectEqual(interval * 2, cmd.batch[2].tick); +} + +test "sequenceAlloc survives the frame that built it" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + var ns: u64 = 5; + ns += 1; + + const cmd = try Cmd.sequenceAlloc(arena.allocator(), &.{ + .{ .tick = ns }, + .quit, + }); + _ = clobberStack(11); + + try testing.expect(cmd == .sequence); + try testing.expectEqual(@as(usize, 2), cmd.sequence.len); + try testing.expectEqual(ns, cmd.sequence[0].tick); + try testing.expect(cmd.sequence[1] == .quit); +} + +/// The allocation-free alternative: storage that belongs to the model, which +/// outlives any single `update` call. +const Holder = struct { + slots: [2]Cmd = undefined, + + fn build(self: *Holder, interval: u64) Cmd { + self.slots = .{ + .{ .every = interval }, + .{ .set_title = "from model storage" }, + }; + return .{ .batch = &self.slots }; + } +}; + +test "model-owned storage survives the frame that built it" { + var holder = Holder{}; + + var interval: u64 = 42; + interval += 1; + + const cmd = holder.build(interval); + _ = clobberStack(23); + + try testing.expectEqual(@as(usize, 2), cmd.batch.len); + try testing.expectEqual(interval, cmd.batch[0].every); + try testing.expectEqualStrings("from model storage", cmd.batch[1].set_title); +} + +test "a comptime-known batch literal lives in static memory" { + const S = struct { + fn build() Cmd { + return .{ .batch = &.{ .{ .set_title = "static" }, .quit } }; + } + }; + + const cmd = S.build(); + _ = clobberStack(31); + + try testing.expectEqual(@as(usize, 2), cmd.batch.len); + try testing.expectEqualStrings("static", cmd.batch[0].set_title); + try testing.expect(cmd.batch[1] == .quit); +} + +test "batch helpers keep their contents in order" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + const nested = try Cmd.batchAlloc(arena.allocator(), &.{ + .{ .tick = 1 }, + try Cmd.batchAlloc(arena.allocator(), &.{ .{ .tick = 2 }, .{ .tick = 3 } }), + .{ .tick = 4 }, + }); + + try testing.expectEqual(@as(u64, 1), nested.batch[0].tick); + try testing.expectEqual(@as(usize, 2), nested.batch[1].batch.len); + try testing.expectEqual(@as(u64, 3), nested.batch[1].batch[1].tick); + try testing.expectEqual(@as(u64, 4), nested.batch[2].tick); +}