diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 928abddac..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(); @@ -3346,21 +3345,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; @@ -3484,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); @@ -14322,6 +14344,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..5ecfe7933 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; @@ -10133,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 @@ -10158,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); @@ -10187,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)); 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;