mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-04-17 21:12:39 +00:00
Fix memory leak when pruning scrollback with non-standard pages (#10251)
This _finally_ resolves #9962 (and the myriad of dupes). The core issue was that when a non-standard size page is reused as part of our scrollback pruning, it was resized to be standard size, which caused our future frees to believe it was pooled and not call `munmap` properly. The solution I chose was to never reuse non-standard sized pages. If during scrollback pruning we detect a non-standard page, we destroy it and re-alloc. This frees the old memory and reuses pool memory for the new page. As part of this, I also introduced a custom page allocator that uses macOS's mach kernel virtual memory tagging feature to specifically tag our PageList memory. I was able to use this in conjunction with Instruments and `footprint` to verify that our PageList memory was previously not freed and is now successfully freed. **No AI was used in my work here.** AI was used by others in their analysis of this issue that I used the results of to help guide me and give me other things to consider, but the ultimate understanding and fix was all done via my own meat sticks. ## Detailed Explainer Ghostty uses a memory pool of fixed-size `mmap`-ed pages to serve as the backing memory for our terminal. If the terminal requires a non-standard (larger) amount of memory due to an abundance of emoji, styles, hyperlinks, etc. then we allocate non-pooled pages directly with `mmap`. When freeing our pages, if it is `<= standard size` we just put it back into the memory pool. If it is larger, we `munmap` it. This explains and defines both a _standard_ and therefore _non-standard_ page. Ghostty also has the concept of a "scrollback limit" (exposed to the user via the `scrollback-limit` config). This caps the size of our scrollback or history. When we reach this limit, we have a trick that we do: to avoid allocation, we reuse the oldest page in the history. Unfortunately, as part of this process, we were resizing the underlying memory to be standard size again. This was causing our future frees to believe this was pooled memory, and never unmap it. This was the main source of the leak. ## Thanks Big shout out to @grishy for being the person that finally got me a reproduction so I could analyze the issue for myself. His own analysis got to the same conclusion as me but thanks to the reproduction I was able to verify both our understandings independently.
This commit is contained in:
150
src/os/mach.zig
Normal file
150
src/os/mach.zig
Normal file
@@ -0,0 +1,150 @@
|
||||
const std = @import("std");
|
||||
const assert = @import("../quirks.zig").inlineAssert;
|
||||
const mem = std.mem;
|
||||
const Allocator = std.mem.Allocator;
|
||||
|
||||
/// macOS virtual memory tags for use with mach_vm_map/mach_vm_allocate.
|
||||
/// These identify memory regions in tools like vmmap and Instruments.
|
||||
pub const VMTag = enum(u8) {
|
||||
application_specific_1 = 240,
|
||||
application_specific_2 = 241,
|
||||
application_specific_3 = 242,
|
||||
application_specific_4 = 243,
|
||||
application_specific_5 = 244,
|
||||
application_specific_6 = 245,
|
||||
application_specific_7 = 246,
|
||||
application_specific_8 = 247,
|
||||
application_specific_9 = 248,
|
||||
application_specific_10 = 249,
|
||||
application_specific_11 = 250,
|
||||
application_specific_12 = 251,
|
||||
application_specific_13 = 252,
|
||||
application_specific_14 = 253,
|
||||
application_specific_15 = 254,
|
||||
application_specific_16 = 255,
|
||||
|
||||
// We ignore the rest because we never realistic set them.
|
||||
_,
|
||||
|
||||
/// Converts the tag to the format expected by mach_vm_map/mach_vm_allocate.
|
||||
/// Equivalent to C macro: VM_MAKE_TAG(tag)
|
||||
pub fn make(self: VMTag) i32 {
|
||||
return @bitCast(@as(u32, @intFromEnum(self)) << 24);
|
||||
}
|
||||
};
|
||||
|
||||
/// Creates a page allocator that tags all allocated memory with the given
|
||||
/// VMTag.
|
||||
pub fn taggedPageAllocator(tag: VMTag) Allocator {
|
||||
return .{
|
||||
// We smuggle the tag in as the context pointer.
|
||||
.ptr = @ptrFromInt(@as(usize, @intFromEnum(tag))),
|
||||
.vtable = &TaggedPageAllocator.vtable,
|
||||
};
|
||||
}
|
||||
|
||||
/// This is based heavily on the Zig 0.15.2 PageAllocator implementation,
|
||||
/// with only the posix implementation. Zig 0.15.2 is MIT licensed.
|
||||
const TaggedPageAllocator = struct {
|
||||
pub const vtable: Allocator.VTable = .{
|
||||
.alloc = alloc,
|
||||
.resize = resize,
|
||||
.remap = remap,
|
||||
.free = free,
|
||||
};
|
||||
|
||||
fn alloc(context: *anyopaque, n: usize, alignment: mem.Alignment, ra: usize) ?[*]u8 {
|
||||
_ = ra;
|
||||
assert(n > 0);
|
||||
const tag: VMTag = @enumFromInt(@as(u8, @truncate(@intFromPtr(context))));
|
||||
return map(n, alignment, tag);
|
||||
}
|
||||
|
||||
fn resize(context: *anyopaque, memory: []u8, alignment: mem.Alignment, new_len: usize, return_address: usize) bool {
|
||||
_ = context;
|
||||
_ = alignment;
|
||||
_ = return_address;
|
||||
return realloc(memory, new_len, false) != null;
|
||||
}
|
||||
|
||||
fn remap(context: *anyopaque, memory: []u8, alignment: mem.Alignment, new_len: usize, return_address: usize) ?[*]u8 {
|
||||
_ = context;
|
||||
_ = alignment;
|
||||
_ = return_address;
|
||||
return realloc(memory, new_len, true);
|
||||
}
|
||||
|
||||
fn free(context: *anyopaque, memory: []u8, alignment: mem.Alignment, return_address: usize) void {
|
||||
_ = context;
|
||||
_ = alignment;
|
||||
_ = return_address;
|
||||
return unmap(@alignCast(memory));
|
||||
}
|
||||
|
||||
pub fn map(n: usize, alignment: mem.Alignment, tag: VMTag) ?[*]u8 {
|
||||
const page_size = std.heap.pageSize();
|
||||
if (n >= std.math.maxInt(usize) - page_size) return null;
|
||||
const alignment_bytes = alignment.toByteUnits();
|
||||
|
||||
const aligned_len = mem.alignForward(usize, n, page_size);
|
||||
const max_drop_len = alignment_bytes - @min(alignment_bytes, page_size);
|
||||
const overalloc_len = if (max_drop_len <= aligned_len - n)
|
||||
aligned_len
|
||||
else
|
||||
mem.alignForward(usize, aligned_len + max_drop_len, page_size);
|
||||
const hint = @atomicLoad(@TypeOf(std.heap.next_mmap_addr_hint), &std.heap.next_mmap_addr_hint, .unordered);
|
||||
const slice = std.posix.mmap(
|
||||
hint,
|
||||
overalloc_len,
|
||||
std.posix.PROT.READ | std.posix.PROT.WRITE,
|
||||
.{ .TYPE = .PRIVATE, .ANONYMOUS = true },
|
||||
tag.make(),
|
||||
0,
|
||||
) catch return null;
|
||||
const result_ptr = mem.alignPointer(slice.ptr, alignment_bytes) orelse return null;
|
||||
// Unmap the extra bytes that were only requested in order to guarantee
|
||||
// that the range of memory we were provided had a proper alignment in it
|
||||
// somewhere. The extra bytes could be at the beginning, or end, or both.
|
||||
const drop_len = result_ptr - slice.ptr;
|
||||
if (drop_len != 0) std.posix.munmap(slice[0..drop_len]);
|
||||
const remaining_len = overalloc_len - drop_len;
|
||||
if (remaining_len > aligned_len) std.posix.munmap(@alignCast(result_ptr[aligned_len..remaining_len]));
|
||||
const new_hint: [*]align(std.heap.page_size_min) u8 = @alignCast(result_ptr + aligned_len);
|
||||
_ = @cmpxchgStrong(@TypeOf(std.heap.next_mmap_addr_hint), &std.heap.next_mmap_addr_hint, hint, new_hint, .monotonic, .monotonic);
|
||||
return result_ptr;
|
||||
}
|
||||
|
||||
pub fn unmap(memory: []align(std.heap.page_size_min) u8) void {
|
||||
const page_aligned_len = mem.alignForward(usize, memory.len, std.heap.pageSize());
|
||||
std.posix.munmap(memory.ptr[0..page_aligned_len]);
|
||||
}
|
||||
|
||||
pub fn realloc(uncasted_memory: []u8, new_len: usize, may_move: bool) ?[*]u8 {
|
||||
const memory: []align(std.heap.page_size_min) u8 = @alignCast(uncasted_memory);
|
||||
const page_size = std.heap.pageSize();
|
||||
const new_size_aligned = mem.alignForward(usize, new_len, page_size);
|
||||
|
||||
const page_aligned_len = mem.alignForward(usize, memory.len, page_size);
|
||||
if (new_size_aligned == page_aligned_len)
|
||||
return memory.ptr;
|
||||
|
||||
if (std.posix.MREMAP != void) {
|
||||
// TODO: if the next_mmap_addr_hint is within the remapped range, update it
|
||||
const new_memory = std.posix.mremap(memory.ptr, memory.len, new_len, .{ .MAYMOVE = may_move }, null) catch return null;
|
||||
return new_memory.ptr;
|
||||
}
|
||||
|
||||
if (new_size_aligned < page_aligned_len) {
|
||||
const ptr = memory.ptr + new_size_aligned;
|
||||
// TODO: if the next_mmap_addr_hint is within the unmapped range, update it
|
||||
std.posix.munmap(@alignCast(ptr[0 .. page_aligned_len - new_size_aligned]));
|
||||
return memory.ptr;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
test "VMTag.make" {
|
||||
try std.testing.expectEqual(@as(i32, @bitCast(@as(u32, 240) << 24)), VMTag.application_specific_1.make());
|
||||
}
|
||||
@@ -23,6 +23,7 @@ pub const args = @import("args.zig");
|
||||
pub const cgroup = @import("cgroup.zig");
|
||||
pub const hostname = @import("hostname.zig");
|
||||
pub const i18n = @import("i18n.zig");
|
||||
pub const mach = @import("mach.zig");
|
||||
pub const path = @import("path.zig");
|
||||
pub const passwd = @import("passwd.zig");
|
||||
pub const xdg = @import("xdg.zig");
|
||||
@@ -73,5 +74,8 @@ test {
|
||||
|
||||
if (comptime builtin.os.tag == .linux) {
|
||||
_ = kernel_info;
|
||||
} else if (comptime builtin.os.tag.isDarwin()) {
|
||||
_ = mach;
|
||||
_ = macos;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
const PageList = @This();
|
||||
|
||||
const std = @import("std");
|
||||
const builtin = @import("builtin");
|
||||
const build_options = @import("terminal_options");
|
||||
const Allocator = std.mem.Allocator;
|
||||
const assert = @import("../quirks.zig").inlineAssert;
|
||||
@@ -255,6 +256,21 @@ fn minMaxSize(cols: size.CellCountInt, rows: size.CellCountInt) !usize {
|
||||
return PagePool.item_size * pages;
|
||||
}
|
||||
|
||||
/// This is the page allocator we'll use for all our underlying
|
||||
/// VM page allocations.
|
||||
inline fn pageAllocator() Allocator {
|
||||
// In tests we use our testing allocator so we can detect leaks.
|
||||
if (builtin.is_test) return std.testing.allocator;
|
||||
|
||||
// On non-macOS we use our standard Zig page allocator.
|
||||
if (!builtin.target.os.tag.isDarwin()) return std.heap.page_allocator;
|
||||
|
||||
// On macOS we want to tag our memory so we can assign it to our
|
||||
// core terminal usage.
|
||||
const mach = @import("../os/mach.zig");
|
||||
return mach.taggedPageAllocator(.application_specific_1);
|
||||
}
|
||||
|
||||
/// Initialize the page. The top of the first page in the list is always the
|
||||
/// top of the active area of the screen (important knowledge for quickly
|
||||
/// setting up cursors in Screen).
|
||||
@@ -280,7 +296,11 @@ pub fn init(
|
||||
// The screen starts with a single page that is the entire viewport,
|
||||
// and we'll split it thereafter if it gets too large and add more as
|
||||
// necessary.
|
||||
var pool = try MemoryPool.init(alloc, std.heap.page_allocator, page_preheat);
|
||||
var pool = try MemoryPool.init(
|
||||
alloc,
|
||||
pageAllocator(),
|
||||
page_preheat,
|
||||
);
|
||||
errdefer pool.deinit();
|
||||
var page_serial: u64 = 0;
|
||||
const page_list, const page_size = try initPages(
|
||||
@@ -669,7 +689,7 @@ pub fn clone(
|
||||
// Setup our pools
|
||||
break :alloc try .init(
|
||||
alloc,
|
||||
std.heap.page_allocator,
|
||||
pageAllocator(),
|
||||
page_count,
|
||||
);
|
||||
},
|
||||
@@ -2476,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.
|
||||
@@ -2492,18 +2516,11 @@ pub fn grow(self: *PageList) !?*List.Node {
|
||||
// satisfied then we do not prune.
|
||||
if (self.growRequiredForActive()) break :prune;
|
||||
|
||||
const layout = Page.layout(try std_capacity.adjust(.{ .cols = self.cols }));
|
||||
|
||||
// Get our first page and reset it to prepare for reuse.
|
||||
const first = self.pages.popFirst().?;
|
||||
assert(first != last);
|
||||
const buf = first.data.memory;
|
||||
@memset(buf, 0);
|
||||
|
||||
// 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: {
|
||||
@@ -2521,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
|
||||
@@ -2554,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);
|
||||
@@ -10985,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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user