mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-07-10 19:29:37 +00:00
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.
This commit is contained in:
@@ -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);
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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));
|
||||
|
||||
|
||||
Reference in New Issue
Block a user