From 8307349ec5ff1502fc033869643b36f5173bf0a2 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 7 Jul 2026 19:52:39 -0700 Subject: [PATCH 1/3] terminal: fix increaseCapacity growth from zero-capacity dimensions PageList.increaseCapacity grows a capacity dimension by doubling it. If the dimension is zero, doubling "succeeds" without growing: the page is reallocated and recloned with an identical capacity, violating the documented guarantee that we always increase by at least one unit. Every unbounded retry site (startHyperlink, cursorSetHyperlink, the reflow probes, insertLines/deleteLines) then loops forever reallocating a page per iteration, and the single-retry sites (styles, graphemes) fail their retry and silently drop data. No production page has a zero dimension today, which is why this has never fired: standard capacities are nonzero and doubling keeps them nonzero. But exactRowCapacity legitimately returns zero for dimensions with no content (a compacted plain text page has zero styles, grapheme, string, and hyperlink capacity), so any compaction work makes this reachable. Growth from zero now jumps straight to the standard default for the dimension rather than doubling. The default is what every standard page starts with, so single-retry callers are guaranteed enough room for their pending allocation, whereas doubling from a minimum unit could still come up short (a single grapheme can need multiple chunks, and a style set below capacity 3 cannot store anything). --- src/terminal/PageList.zig | 79 +++++++++++++++++++++++++++++++-------- src/terminal/Screen.zig | 48 ++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 928abddac..61b85b7d8 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3346,21 +3346,36 @@ pub fn increaseCapacity( const Int = @FieldType(Capacity, field_name); const old = @field(cap, field_name); - // We use checked math to prevent overflow. If there is an - // overflow it means we're out of space in this dimension, - // since pages can take up to their maxInt capacity in any - // category. - const new = std.math.mul( - Int, - old, - 2, - ) catch |err| overflow: { - comptime assert(@TypeOf(err) == error{Overflow}); - // Our final doubling would overflow since maxInt is - // 2^N - 1 for an unsignged int of N bits. So, if we overflow - // and we haven't used all the bits, use all the bits. - if (old < std.math.maxInt(Int)) break :overflow std.math.maxInt(Int); - return error.OutOfSpace; + const new: Int = new: { + // A dimension can be zero for pages with exact + // capacities (see compact). Doubling zero stays zero, + // which would break our guarantee that we always + // increase by at least one unit and turn caller retry + // loops into infinite loops. Jump straight to the + // standard default instead: it is what all standard + // pages start with, so retrying callers are guaranteed + // enough room for their pending allocation. + if (old == 0) { + const default: Capacity = .{ .cols = 0, .rows = 0 }; + break :new @field(default, field_name); + } + + // We use checked math to prevent overflow. If there is + // an overflow it means we're out of space in this + // dimension, since pages can take up to their maxInt + // capacity in any category. + break :new std.math.mul( + Int, + old, + 2, + ) catch |err| overflow: { + comptime assert(@TypeOf(err) == error{Overflow}); + // Our final doubling would overflow since maxInt is + // 2^N - 1 for an unsignged int of N bits. So, if we overflow + // and we haven't used all the bits, use all the bits. + if (old < std.math.maxInt(Int)) break :overflow std.math.maxInt(Int); + return error.OutOfSpace; + }; }; @field(cap, field_name) = new; @@ -14322,6 +14337,40 @@ test "PageList compact oversized page" { } } +test "PageList increaseCapacity from zero-capacity dimensions" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 0); + defer s.deinit(); + + // Compact the only page. A plain page has no styled, grapheme, + // or hyperlink content so the exact capacity is zero in every + // managed dimension. + var node = (try s.compact(s.pages.first.?)).?; + try testing.expectEqual(0, node.data.capacity.styles); + try testing.expectEqual(0, node.data.capacity.grapheme_bytes); + try testing.expectEqual(0, node.data.capacity.string_bytes); + try testing.expectEqual(0, node.data.capacity.hyperlink_bytes); + + // Increasing each dimension from zero must actually grow it. + // Regression: 0 * 2 == 0 used to "succeed" without growing, + // which turned caller retry loops into infinite loops. + node = try s.increaseCapacity(node, .styles); + try testing.expect(node.data.capacity.styles > 0); + node = try s.increaseCapacity(node, .grapheme_bytes); + try testing.expect(node.data.capacity.grapheme_bytes > 0); + node = try s.increaseCapacity(node, .string_bytes); + try testing.expect(node.data.capacity.string_bytes > 0); + node = try s.increaseCapacity(node, .hyperlink_bytes); + try testing.expect(node.data.capacity.hyperlink_bytes > 0); + + // Increasing a non-zero dimension still doubles. + const styles = node.data.capacity.styles; + node = try s.increaseCapacity(node, .styles); + try testing.expectEqual(styles * 2, node.data.capacity.styles); +} + test "PageList compact after increaseCapacity" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 6eb741af8..b9d421982 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -3775,6 +3775,54 @@ test "Screen cursorCopy hyperlink deref" { try testing.expect(s2.cursor.hyperlink_id == 0); } +test "Screen write regrows compacted page capacity" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try Screen.init(alloc, .{ + .cols = 80, + .rows = 24, + .max_scrollback = 0, + }); + defer s.deinit(); + + // Compact the active page so every managed capacity dimension is + // zero, then reload the cursor since its cached row/cell pointers + // point into the replaced page. + { + const node = (try s.pages.compact(s.cursor.page_pin.node)).?; + try testing.expectEqual(0, node.data.capacity.styles); + try testing.expectEqual(0, node.data.capacity.grapheme_bytes); + try testing.expectEqual(0, node.data.capacity.string_bytes); + try testing.expectEqual(0, node.data.capacity.hyperlink_bytes); + s.cursorReload(); + } + + // Styled write: exercises the manualStyleUpdate single-retry + // path. Prior to increaseCapacity handling zero dimensions, the + // retry would fail and the style would be dropped. + try s.setAttribute(.{ .bold = {} }); + try s.testWriteString("A"); + + // Grapheme write: exercises the appendGrapheme single-retry path. + // We can't use testWriteString here because it appends graphemes + // directly on the page without the capacity retry. + try s.testWriteString("a"); + try s.appendGrapheme(s.cursorCellLeft(1), 0x0301); + + // Hyperlink: exercises the startHyperlink retry loop, which used + // to loop forever when capacity growth from zero didn't grow. + try s.startHyperlink("https://example.com/", null); + try s.testWriteString("B"); + s.endHyperlink(); + + // Verify the content landed on the page. + const page = &s.cursor.page_pin.node.data; + try testing.expect(page.styles.count() >= 1); + try testing.expect(page.hyperlink_set.count() >= 1); + try testing.expect(page.graphemeCount() >= 1); +} + test "Screen cursorCopy hyperlink deref new page" { const testing = std.testing; const alloc = testing.allocator; From e44f5cb0fa1aa02158e29b295833e1c3c024570e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 7 Jul 2026 19:54:41 -0700 Subject: [PATCH 2/3] terminal: guard RefCountedSet lookups against zero-capacity sets Layout.init(0) is an explicitly supported special case that produces a valid zero-capacity set with a zero-size table. But lookupContext had no guard for it: probing computes `table[hash & 0]` and reads whatever memory follows the set in its backing buffer, treating those bytes as an item ID which is then used to index the (also zero-size) items array, an out-of-bounds read reaching arbitrarily far past the set. This has never fired in practice because no production page carries a zero-capacity set today, and where one could occur the adjacent bytes happen to be zero (which reads as an empty bucket and returns null). Page.exactRowCapacity legitimately produces zero capacities for pages without styled or hyperlinked cells though, so any page compaction work makes this reachable with nonzero adjacent memory: in a Page layout the styles set can be followed by the grapheme bitmap, which is initialized to all ones. Lookups on a zero-capacity set now return null without touching the table. This also covers add, which looks up before inserting and already handles the zero capacity correctly after that point by returning OutOfMemory. All other entry points assert on valid IDs, which a zero-capacity set cannot have. --- src/terminal/ref_counted_set.zig | 9 +++++++++ src/terminal/style.zig | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 8040039ae..dd76079f4 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -497,6 +497,15 @@ pub fn RefCountedSet( return self.lookupContext(base, value, self.context); } pub fn lookupContext(self: *const Self, base: anytype, value: T, ctx: Context) ?Id { + // A zero-capacity set (a valid special case of Layout.init) + // contains nothing and has a zero-size table, so we can't + // probe it: table[0] would read whatever memory follows the + // set in the backing buffer. + if (self.layout.table_cap == 0) { + @branchHint(.cold); + return null; + } + const table = self.table.ptr(base); const items = self.items.ptr(base); diff --git a/src/terminal/style.zig b/src/terminal/style.zig index ae9488af8..720789bb2 100644 --- a/src/terminal/style.zig +++ b/src/terminal/style.zig @@ -966,6 +966,36 @@ test "Set capacities" { _ = Set.Layout.init(16384); } +test "Set zero capacity" { + const testing = std.testing; + const alloc = testing.allocator; + + // A zero-capacity set is a valid special case (see Layout.init). + // It occurs in practice for pages with exact capacities where no + // cell is styled (see Page.exactRowCapacity). + const layout: Set.Layout = .init(0); + try testing.expectEqual(0, layout.total_size); + + // We allocate a nonzero buffer filled with 0xFF to simulate the + // set being embedded in a larger structure (e.g. a Page) where + // other data follows it. Lookups must not probe the zero-size + // table: table[0] would read this adjacent memory and treat it + // as an item ID, leading to far out-of-bounds item reads. + const buf = try alloc.alignedAlloc(u8, Set.base_align, 64); + defer alloc.free(buf); + @memset(buf, 0xFF); + + var set = Set.init(.init(buf), layout, .{}); + + const style: Style = .{ .flags = .{ .bold = true } }; + try testing.expectEqual(null, set.lookup(buf, style)); + try testing.expectError(error.OutOfMemory, set.add(buf, style)); + try testing.expectEqual(0, set.count()); + + // The adjacent memory must be untouched. + for (buf) |b| try testing.expectEqual(0xFF, b); +} + test "Style HTML formatting basic bold" { const testing = std.testing; const alloc = testing.allocator; From c442eb4b308381aeabfbdbedc092e42eba806b66 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 7 Jul 2026 20:00:30 -0700 Subject: [PATCH 3/3] terminal: document the pool reuse zeroing invariant PageList skips zeroing pooled page buffers in release builds, relying on the OS page allocator handing out zeroed pages and destroyNodeExt zeroing buffers before returning them to the pool. There is a hidden exception: std.heap.MemoryPool writes its free list node into the first pointer-size bytes of a free-listed buffer, so a reused buffer is not fully zero. This is only safe because the page rows array is laid out at offset 0, a page always has at least one row, and initBuf fully rewrites every row, overwriting the stale free list pointer. None of that was written down or checked anywhere, so a future layout reorder (or a zero-row page) would corrupt pages in release builds only, in a way that depends on pool reuse patterns. This adds a comptime assert that a Row covers at least a pointer, a runtime assert that pages always have at least one row, and comments tying the invariant together at the layout, initBuf, and pool reuse sites. Also fixes stale doc comments: deinit referenced a clonePool function that no longer exists, and Screen tests referenced increaseCapacity by its old adjustCapacity name. --- src/terminal/PageList.zig | 11 +++++++++-- src/terminal/Screen.zig | 8 ++++---- src/terminal/page.zig | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 61b85b7d8..f26748c35 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -642,8 +642,7 @@ fn verifyIntegrity(self: *const PageList) IntegrityError!void { } } -/// Deinit the pagelist. If you own the memory pool (used clonePool) then -/// this will reset the pool and retain capacity. +/// Deinit the pagelist, freeing all page memory and the memory pool. pub fn deinit(self: *PageList) void { // Verify integrity before cleanup self.assertIntegrity(); @@ -3499,6 +3498,14 @@ inline fn createPageExt( // In runtime safety modes, allocators fill with 0xAA. On freestanding // (WASM), the WasmAllocator reuses freed slots without zeroing. + // + // Otherwise, we rely on pool item buffers being zeroed: fresh items + // come from the OS page allocator (zeroed pages) and destroyNodeExt + // zeroes buffers before returning them to the pool. The one + // exception is the first pointer-size bytes, which hold the pool's + // free list node while a buffer is in the free list; initBuf below + // always overwrites those since the page rows start at offset 0 + // (comptime-asserted in Page). if (comptime std.debug.runtime_safety or builtin.os.tag == .freestanding) @memset(page_buf, 0); diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index b9d421982..5ecfe7933 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -10181,9 +10181,9 @@ test "Screen: cursorDown to page with insufficient capacity" { // cursor movement functions). The bug pattern: // // 1. cursorDown creates a by-value copy of the pin via page_pin.down(n) - // 2. cursorChangePin is called, which may trigger adjustCapacity + // 2. cursorChangePin is called, which may trigger increaseCapacity // if the target page's style map is full - // 3. adjustCapacity frees the old page and creates a new one + // 3. increaseCapacity frees the old page and creates a new one // 4. The local pin copy still points to the freed page // 5. rowAndCell() on the stale pin accesses freed memory @@ -10206,7 +10206,7 @@ test "Screen: cursorDown to page with insufficient capacity" { try testing.expect(start_page != new_page); // Fill new_page's style map to capacity. When we move INTO this page - // with a style set, adjustCapacity will be triggered. + // with a style set, increaseCapacity will be triggered. { new_page.pauseIntegrityChecks(true); defer new_page.pauseIntegrityChecks(false); @@ -10235,7 +10235,7 @@ test "Screen: cursorDown to page with insufficient capacity" { try testing.expect(&next_pin.node.data == new_page); // This cursorDown triggers the bug: the local page_pin copy - // becomes stale after adjustCapacity, causing rowAndCell() + // becomes stale after increaseCapacity, causing rowAndCell() // to access freed memory. s.cursorDown(1); diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 803e67c8f..c2759d2a2 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -145,6 +145,15 @@ pub const Page = struct { @alignOf(Cell), StyleSet.base_align.toByteUnits(), ) == 0); + + // The PageList memory pool requires that initBuf overwrites at + // least the first pointer-size bytes of the backing buffer: + // std.heap.MemoryPool stores its free list node there when a + // page buffer is returned to it, and pool reuse skips zeroing + // in release builds. This holds because the rows array is at + // offset 0 (see layout), a page always has at least one row, + // and initBuf fully rewrites every row. + assert(@sizeOf(Row) >= @sizeOf(usize)); } /// The backing memory for the page. A page is always made up of a @@ -238,6 +247,13 @@ pub const Page = struct { /// It is up to the caller to not call deinit on these pages. pub inline fn initBuf(buf: OffsetBuf, l: Layout) Page { const cap = l.capacity; + + // A page must always have at least one row. Aside from being + // useless otherwise, the row initialization below must always + // overwrite the start of the buffer for pool reuse. See the + // comptime assert at the top of Page. + assert(cap.rows > 0); + const rows = buf.member(Row, l.rows_start); const cells = buf.member(Cell, l.cells_start); @@ -1703,6 +1719,11 @@ pub const Page = struct { /// and rows size. pub inline fn layout(cap: Capacity) Layout { const rows_count: usize = @intCast(cap.rows); + + // The rows array must stay at offset 0: the PageList memory + // pool relies on initBuf overwriting the first bytes of a + // reused page buffer, which hold the pool's free list node. + // See the comptime assert at the top of Page. const rows_start = 0; const rows_end: usize = rows_start + (rows_count * @sizeOf(Row));