From 509f0733667d7631266f787ea800ba05d3672cec Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 10 Jan 2026 06:58:19 -0800 Subject: [PATCH] terminal: fix up our total row and pin accounting during reuse --- src/terminal/PageList.zig | 111 ++++++++++++++++++++++++++++++-------- 1 file changed, 88 insertions(+), 23 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 2acdfc9c3..1cf4b376c 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2496,6 +2496,10 @@ pub fn grow(self: *PageList) !?*List.Node { // Slower path: we have no space, we need to allocate a new page. + // Get the layout first so our failable work is done early. + // We'll need this for both paths. + const cap = try std_capacity.adjust(.{ .cols = self.cols }); + // If allocation would exceed our max size, we prune the first page. // We don't need to reallocate because we can simply reuse that first // page. @@ -2515,28 +2519,8 @@ pub fn grow(self: *PageList) !?*List.Node { const first = self.pages.popFirst().?; assert(first != last); - // If our first node has non-standard memory size, we can't reuse - // it. This is because our initBuf below would change the underlying - // memory length which would break our memory free outside the pool. - // It is easiest in this case to prune the node. - if (first.data.memory.len > std_size) { - // Node is already removed so we can just destroy it. - self.destroyNode(first); - break :prune; - } - - // Get the layout first so our failable work is done early. - const layout = Page.layout(try std_capacity.adjust(.{ .cols = self.cols })); - - // Reset our memory - const buf = first.data.memory; - @memset(buf, 0); - assert(buf.len <= std_size); - - // Decrease our total row count from the pruned page and then - // add one for our new row. + // Decrease our total row count from the pruned page self.total_rows -= first.data.size.rows; - self.total_rows += 1; // If we have a pin viewport cache then we need to update it. if (self.viewport == .pin) viewport: { @@ -2554,10 +2538,26 @@ pub fn grow(self: *PageList) !?*List.Node { } } + // If our first node has non-standard memory size, we can't reuse + // it. This is because our initBuf below would change the underlying + // memory length which would break our memory free outside the pool. + // It is easiest in this case to prune the node. + if (first.data.memory.len > std_size) { + // Node is already removed so we can just destroy it. + self.destroyNode(first); + break :prune; + } + + // Reset our memory + const buf = first.data.memory; + @memset(buf, 0); + assert(buf.len <= std_size); + // Initialize our new page and reinsert it as the last - first.data = .initBuf(.init(buf), layout); + first.data = .initBuf(.init(buf), Page.layout(cap)); first.data.size.rows = 1; self.pages.insertAfter(last, first); + self.total_rows += 1; // We also need to reset the serial number. Since this is the only // place we ever reuse a serial number, we also can safely set @@ -2587,7 +2587,7 @@ pub fn grow(self: *PageList) !?*List.Node { } // We need to allocate a new memory buffer. - const next_node = try self.createPage(try std_capacity.adjust(.{ .cols = self.cols })); + const next_node = try self.createPage(cap); // we don't errdefer this because we've added it to the linked // list and its fine to have dangling unused pages. self.pages.append(next_node); @@ -11018,3 +11018,68 @@ test "PageList resize grow cols with unwrap fixes viewport pin" { const br_after = s.getBottomRight(.viewport); try testing.expect(br_after != null); } + +test "PageList grow reuses non-standard page without leak" { + const testing = std.testing; + const alloc = testing.allocator; + + // Create a PageList with 3 * std_size max so we can fit multiple pages + // but will still trigger reuse. + var s = try init(alloc, 80, 24, 3 * std_size); + defer s.deinit(); + + // Save the first page node before adjustment + const first_before = s.pages.first.?; + + // Adjust the first page to have non-standard capacity. We use a small + // increase that makes it just slightly larger than std_size. + _ = try s.adjustCapacity(first_before, .{ .grapheme_bytes = std_size + 1 }); + + // The first page should now have non-standard memory size. + try testing.expect(s.pages.first.?.data.memory.len > std_size); + + // First, fill up the first page's capacity + const first_page = s.pages.first.?; + while (first_page.data.size.rows < first_page.data.capacity.rows) { + _ = try s.grow(); + } + + // Now grow to create a second page + _ = try s.grow(); + try testing.expect(s.pages.first != s.pages.last); + + // Continue growing until we exceed max_size AND the last page is full + while (s.page_size + PagePool.item_size <= s.maxSize() or + s.pages.last.?.data.size.rows < s.pages.last.?.data.capacity.rows) + { + _ = try s.grow(); + } + + // The first page should still be non-standard + try testing.expect(s.pages.first.?.data.memory.len > std_size); + + // Verify we have enough rows for active area (so prune path isn't skipped) + try testing.expect(!s.growRequiredForActive()); + + // Verify last page is full (so grow will need to allocate/reuse) + try testing.expect(s.pages.last.?.data.size.rows == s.pages.last.?.data.capacity.rows); + + // Remember the first page memory pointer before the reuse attempt + const first_page_ptr = s.pages.first.?; + const first_page_mem_ptr = s.pages.first.?.data.memory.ptr; + + // Now grow one more time to trigger the reuse path. Since the first page + // is non-standard, it should be destroyed (not reused). The testing + // allocator will detect a leak if destroyNode doesn't properly free + // the non-standard memory. + _ = try s.grow(); + + // After grow, check if the first page is a different one + // (meaning the non-standard page was pruned, not reused at the end) + // The original first page should no longer be the first page + try testing.expect(s.pages.first.? != first_page_ptr); + + // If the non-standard page was properly destroyed and not reused, + // the last page should not have the same memory pointer + try testing.expect(s.pages.last.?.data.memory.ptr != first_page_mem_ptr); +}