From d866fa4553850050ec54eefa9daf2c63a9f4f4e5 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 07:26:40 -0700 Subject: [PATCH 1/8] terminal/kitty: fix graphics range deletion Use inclusive image ID bounds for the Kitty graphics protocol range delete operation. Range deletion previously joined the lower and upper bound checks with or, which matched every placement for any valid range. A targeted delete could therefore remove every graphics placement. Join the bounds with and and update the lowercase and uppercase range tests to keep placements below and above the selected interval. --- src/terminal/kitty/graphics_storage.zig | 38 ++++++++++++++++++------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index d59de54d4..3db1cdde1 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -567,7 +567,7 @@ pub const ImageStorage = struct { var it = self.placements.iterator(); while (it.next()) |entry| { - if (entry.key_ptr.image_id >= v.first or entry.key_ptr.image_id <= v.last) { + if (entry.key_ptr.image_id >= v.first and entry.key_ptr.image_id <= v.last) { const image_id = entry.key_ptr.image_id; entry.value_ptr.deinit(t.screens.active); self.placements.removeByPtr(entry.key_ptr); @@ -1485,15 +1485,24 @@ test "storage: delete images by range 3" { try s.addImage(io, alloc, .{ .id = 3 }); try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, 3, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try testing.expectEqual(@as(usize, 3), s.images.count()); - try testing.expectEqual(@as(usize, 2), s.placements.count()); + try testing.expectEqual(@as(usize, 3), s.placements.count()); s.dirty = false; - s.delete(io, alloc, &t, .{ .range = .{ .delete = false, .first = 1, .last = 1 } }); + s.delete(io, alloc, &t, .{ .range = .{ .delete = false, .first = 2, .last = 2 } }); try testing.expect(s.dirty); try testing.expectEqual(@as(usize, 3), s.images.count()); - try testing.expectEqual(@as(usize, 0), s.placements.count()); - try testing.expectEqual(tracked, t.screens.active.pages.countTrackedPins()); + try testing.expectEqual(@as(usize, 2), s.placements.count()); + try testing.expect(s.placements.get(.{ + .image_id = 1, + .placement_id = .{ .tag = .external, .id = 1 }, + }) != null); + try testing.expect(s.placements.get(.{ + .image_id = 3, + .placement_id = .{ .tag = .external, .id = 1 }, + }) != null); + try testing.expectEqual(tracked + 2, t.screens.active.pages.countTrackedPins()); } test "storage: delete images by range 4" { @@ -1511,15 +1520,24 @@ test "storage: delete images by range 4" { try s.addImage(io, alloc, .{ .id = 3 }); try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, 3, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try testing.expectEqual(@as(usize, 3), s.images.count()); - try testing.expectEqual(@as(usize, 2), s.placements.count()); + try testing.expectEqual(@as(usize, 3), s.placements.count()); s.dirty = false; - s.delete(io, alloc, &t, .{ .range = .{ .delete = true, .first = 1, .last = 1 } }); + s.delete(io, alloc, &t, .{ .range = .{ .delete = true, .first = 2, .last = 2 } }); try testing.expect(s.dirty); - try testing.expectEqual(@as(usize, 1), s.images.count()); - try testing.expectEqual(@as(usize, 0), s.placements.count()); - try testing.expectEqual(tracked, t.screens.active.pages.countTrackedPins()); + try testing.expectEqual(@as(usize, 2), s.images.count()); + try testing.expectEqual(@as(usize, 2), s.placements.count()); + try testing.expect(s.placements.get(.{ + .image_id = 1, + .placement_id = .{ .tag = .external, .id = 1 }, + }) != null); + try testing.expect(s.placements.get(.{ + .image_id = 3, + .placement_id = .{ .tag = .external, .id = 1 }, + }) != null); + try testing.expectEqual(tracked + 2, t.screens.active.pages.countTrackedPins()); } test "storage: aspect ratio calculation when only columns or rows specified" { From af2faa311a5a16b40afff8410a28da077b8ced57 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 07:28:54 -0700 Subject: [PATCH 2/8] terminal/kitty: restrict temporary image file paths Require temporary image file paths to match complete directory components when checking /tmp, /dev/shm, the configured temporary directory, and its resolved path. The previous byte-prefix checks accepted similarly named sibling directories such as /tmpX. A temporary-file transmission could read and unlink a file outside the permitted temporary directories. Add a component-boundary helper and regression coverage for built-in and configured directory prefixes. An integration test also verifies that a rejected file remains on disk. --- src/terminal/kitty/graphics_image.zig | 77 +++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-) diff --git a/src/terminal/kitty/graphics_image.zig b/src/terminal/kitty/graphics_image.zig index bfb8e6b44..3068a6a81 100644 --- a/src/terminal/kitty/graphics_image.zig +++ b/src/terminal/kitty/graphics_image.zig @@ -357,9 +357,9 @@ pub const LoadingImage = struct { /// Returns true if path appears to be in a temporary directory. /// Copies logic from Kitty. fn isPathInTempDir(io: std.Io, dir: []const u8, path: []const u8) bool { - if (std.mem.startsWith(u8, path, "/tmp")) return true; - if (std.mem.startsWith(u8, path, "/dev/shm")) return true; - if (std.mem.startsWith(u8, path, dir)) return true; + if (isPathInDir("/tmp", path)) return true; + if (isPathInDir("/dev/shm", path)) return true; + if (isPathInDir(dir, path)) return true; // The temporary dir is sometimes a symlink. On macOS for // example /tmp is /private/var/... @@ -369,7 +369,7 @@ pub const LoadingImage = struct { dir, &buf, ) catch return false]; - if (std.mem.startsWith(u8, path, real_dir)) return true; + if (isPathInDir(real_dir, path)) return true; return false; } @@ -626,6 +626,27 @@ pub const Rect = struct { bottom_right: PageList.Pin, }; +/// Returns true if `path` is `dir` or is contained within it, requiring a +/// path-separator boundary so similarly prefixed directories do not match. +fn isPathInDir(dir: []const u8, path: []const u8) bool { + if (dir.len == 0 or !std.mem.startsWith(u8, path, dir)) return false; + if (path.len == dir.len or std.fs.path.isSep(dir[dir.len - 1])) return true; + return std.fs.path.isSep(path[dir.len]); +} + +test "temporary file path must be inside directory" { + const testing = std.testing; + + try testing.expect(isPathInDir("/tmp", "/tmp/tty-graphics-protocol-image.data")); + try testing.expect(isPathInDir("/tmp/", "/tmp/tty-graphics-protocol-image.data")); + try testing.expect(isPathInDir("/tmp", "/tmp")); + + try testing.expect(!isPathInDir("", "/tmp/tty-graphics-protocol-image.data")); + try testing.expect(!isPathInDir("/tmp", "/tmpX/tty-graphics-protocol-image.data")); + try testing.expect(!isPathInDir("/dev/shm", "/dev/shm-evil/tty-graphics-protocol-image.data")); + try testing.expect(!isPathInDir("/custom/tmp", "/custom/tmp-suffix/tty-graphics-protocol-image.data")); +} + // This specifically tests we ALLOW invalid RGB data because Kitty // documents that this should work. test "image load with invalid RGB data" { @@ -861,6 +882,54 @@ test "image load: temporary file without correct path" { try tmp_dir.dir.access(testing.io, path, .{}); } +test "image load: temporary file outside directory prefix is rejected" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + try tmp_dir.dir.createDir(io, "temp", .default_dir); + try tmp_dir.dir.createDir(io, "temp-suffix", .default_dir); + + var trusted_dir = try tmp_dir.dir.openDir(io, "temp", .{}); + defer trusted_dir.close(io); + var outside_dir = try tmp_dir.dir.openDir(io, "temp-suffix", .{}); + defer outside_dir.close(io); + + const filename = "tty-graphics-protocol-image.data"; + const data = @embedFile("testdata/image-rgb-none-20x15-2147483647-raw.data"); + try outside_dir.writeFile(io, .{ + .sub_path = filename, + .data = data, + }); + + var trusted_path_buf: [std.fs.max_path_bytes]u8 = undefined; + const trusted_path = trusted_path_buf[0..try trusted_dir.realPath(io, &trusted_path_buf)]; + var outside_path_buf: [std.fs.max_path_bytes]u8 = undefined; + const outside_path = outside_path_buf[0..try outside_dir.realPathFile(io, filename, &outside_path_buf)]; + + var cmd: command.Command = .{ + .control = .{ .transmit = .{ + .format = .rgb, + .medium = .temporary_file, + .compression = .none, + .width = 20, + .height = 15, + .image_id = 31, + } }, + .data = try alloc.dupe(u8, outside_path), + }; + defer cmd.deinit(alloc); + try testing.expectError( + error.TemporaryFileNotInTempDir, + LoadingImage.init(io, alloc, &cmd, .allWithTempDir(trusted_path)), + ); + + // Rejection must happen before temporary-file cleanup is armed. + try outside_dir.access(io, filename, .{}); +} + test "image load: rgb, not compressed, temporary file" { const testing = std.testing; const alloc = testing.allocator; From e5840bb9bacdaae78c42baa4b276f52eaa91fdbe Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 07:33:41 -0700 Subject: [PATCH 3/8] terminal/kitty: harden placement geometry Treat Kitty placement dimensions and offsets as untrusted values when calculating pixel, grid, and rectangle geometry. Saturate results that do not fit and return no rectangle when missing pixel metrics produces an empty grid. Unchecked u32 arithmetic previously panicked in safe builds and wrapped in fast builds. A zero row count could underflow into a maximum-size page traversal, while maximum dimensions could spin cursor movement or overflow render visibility calculations. Use checked integer scaling instead of floating-point casts, saturating arithmetic for extents and cursor columns, and bound off-screen cursor work to the terminal row count. Compute C API visibility in i64 and cover maximum protocol values in storage, execution, and render-info tests. --- src/terminal/c/kitty_graphics.zig | 38 +++++- src/terminal/kitty/graphics_exec.zig | 30 ++++- src/terminal/kitty/graphics_storage.zig | 153 ++++++++++++++++++++---- 3 files changed, 193 insertions(+), 28 deletions(-) diff --git a/src/terminal/c/kitty_graphics.zig b/src/terminal/c/kitty_graphics.zig index e47e6894f..c1c738a0b 100644 --- a/src/terminal/c/kitty_graphics.zig +++ b/src/terminal/c/kitty_graphics.zig @@ -631,9 +631,8 @@ fn computeViewportPos( // is above the viewport, or its top edge is at or below the // viewport's last row. const grid_size = p.gridSize(image.*, t); - const rows_i32: i32 = @intCast(grid_size.rows); - const term_rows: i32 = @intCast(t.rows); - const visible = vp_row + rows_i32 > 0 and vp_row < term_rows; + const bottom_row = @as(i64, vp_row) + @as(i64, grid_size.rows); + const visible = bottom_row > 0 and vp_row < @as(i32, t.rows); return .{ .col = vp_col, .row = vp_row, .visible = visible }; } @@ -1624,6 +1623,39 @@ test "placement_render_info returns all fields" { try testing.expectEqual(2, ri.source_height); } +test "placement_render_info handles maximum grid dimensions" { + if (comptime !build_options.kitty_graphics) return error.SkipZigTest; + + var t: terminal_c.Terminal = null; + try testing.expectEqual(Result.success, terminal_c.new( + &lib.alloc.test_allocator, + &t, + 80, + 24, + )); + defer terminal_c.free(t); + try testing.expectEqual(Result.success, terminal_c.resize(t, 80, 24, 10, 20)); + + const cmd = "\x1b_Ga=T,t=d,f=24,i=1,p=1,s=1,v=2,c=1,r=4294967295,C=1;////////\x1b\\"; + terminal_c.vt_write(t, cmd.ptr, cmd.len); + + var graphics: KittyGraphics = undefined; + try testing.expectEqual(Result.success, terminal_c.get(t, .kitty_graphics, @ptrCast(&graphics))); + const img = image_get_handle(graphics, 1); + try testing.expect(img != null); + + var iter: PlacementIterator = null; + try testing.expectEqual(Result.success, placement_iterator_new(&lib.alloc.test_allocator, &iter)); + defer placement_iterator_free(iter); + try testing.expectEqual(Result.success, get(graphics, .placement_iterator, @ptrCast(&iter))); + try testing.expect(placement_iterator_next(iter)); + + var ri: PlacementRenderInfo = .{}; + try testing.expectEqual(Result.success, placement_render_info(iter, img, t, &ri)); + try testing.expect(ri.viewport_visible); + try testing.expectEqual(std.math.maxInt(u32), ri.grid_rows); +} + test "placement_render_info off-screen sets viewport_visible false" { if (comptime !build_options.kitty_graphics) return error.SkipZigTest; diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index e0756ccc7..449e859f6 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -273,14 +273,21 @@ fn display( .after => { // We use terminal.index to properly handle scroll regions. const size = p.gridSize(img, terminal); - for (0..size.rows) |_| terminal.index() catch |err| { + // Once the requested movement leaves the screen, its exact + // position is undefined by the Kitty graphics protocol. Bound + // the work so an untrusted row count can't make us spin. + const rows_to_move: usize = @min( + @as(usize, size.rows), + @as(usize, terminal.rows), + ); + for (0..rows_to_move) |_| terminal.index() catch |err| { log.warn("failed to move cursor: {}", .{err}); break; }; terminal.setCursorPos( terminal.screens.active.cursor.y, - pin.x + size.cols + 1, + @as(usize, pin.x) +| @as(usize, size.cols) +| 1, ); }, }, @@ -673,3 +680,22 @@ test "kittygfx delete then retransmit same id gets fresh generation" { try testing.expect(gen2 > gen1); try testing.expect(gen2 > gen_delete); } + +test "kittygfx placement bounds cursor movement for untrusted dimensions" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + var t = try Terminal.init(io, alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + const cmd = try command.Parser.parseString( + alloc, + "a=T,t=d,f=24,i=1,s=1,v=1,c=4294967295,r=4294967295;////", + ); + defer cmd.deinit(alloc); + + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + try testing.expectEqual(@as(usize, 1), t.screens.active.kitty_images.placements.count()); +} diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index 3db1cdde1..eb476c33a 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -849,6 +849,24 @@ pub const ImageStorage = struct { } } + /// Multiply two protocol-controlled values without allowing them to + /// wrap. Placement geometry is exposed as u32, so values larger than + /// that are represented by the largest possible value. + fn saturatingMul(lhs: u32, rhs: u32) u32 { + return std.math.mul(u32, lhs, rhs) catch std.math.maxInt(u32); + } + + /// Scale a dimension by an aspect ratio and round to the nearest + /// integer. The u64 intermediate can hold the product of two u32s as + /// well as the rounding adjustment. + fn scaleDimension(value: u32, numerator: u32, denominator: u32) u32 { + if (denominator == 0) return 0; + + const rounded = (@as(u64, value) * @as(u64, numerator) + + @as(u64, denominator) / 2) / @as(u64, denominator); + return std.math.cast(u32, rounded) orelse std.math.maxInt(u32); + } + /// Returns the size of this placement's image in pixels, /// taking into account the source rectangle, specified /// rows/columns, and aspect ratio. @@ -880,15 +898,12 @@ pub const ImageStorage = struct { const cell_width: u32 = t.width_px / t.cols; const cell_height: u32 = t.height_px / t.rows; - const width_f64: f64 = @floatFromInt(width); - const height_f64: f64 = @floatFromInt(height); - // If we have a specified cols AND rows then we calculate // the width and height from them directly, we don't need // to adjust for aspect ratio. if (self.columns > 0 and self.rows > 0) { - const calc_width = cell_width * self.columns; - const calc_height = cell_height * self.rows; + const calc_width = saturatingMul(cell_width, self.columns); + const calc_height = saturatingMul(cell_height, self.rows); return .{ .width = calc_width, @@ -902,11 +917,8 @@ pub const ImageStorage = struct { // If only the columns were specified, we determine // the height of the image based on the aspect ratio. if (self.columns > 0) { - const aspect = height_f64 / width_f64; - const calc_width: u32 = cell_width * self.columns; - const calc_height: u32 = @intFromFloat(@round( - @as(f64, @floatFromInt(calc_width)) * aspect, - )); + const calc_width = saturatingMul(cell_width, self.columns); + const calc_height = scaleDimension(calc_width, height, width); return .{ .width = calc_width, @@ -917,11 +929,8 @@ pub const ImageStorage = struct { // Otherwise, only the rows were specified, so we // determine the width based on the aspect ratio. { - const aspect = width_f64 / height_f64; - const calc_height: u32 = cell_height * self.rows; - const calc_width: u32 = @intFromFloat(@round( - @as(f64, @floatFromInt(calc_height)) * aspect, - )); + const calc_height = saturatingMul(cell_height, self.rows); + const calc_width = scaleDimension(calc_height, width, height); return .{ .width = calc_width, @@ -951,12 +960,12 @@ pub const ImageStorage = struct { return .{ .cols = std.math.divCeil( u32, - calc_size.width + self.x_offset, + calc_size.width +| self.x_offset, t.width_px / t.cols, ) catch 0, .rows = std.math.divCeil( u32, - calc_size.height + self.y_offset, + calc_size.height +| self.y_offset, t.height_px / t.rows, ) catch 0, }; @@ -965,8 +974,8 @@ pub const ImageStorage = struct { } /// Returns a selection of the entire rectangle this placement - /// occupies within the screen. This can return null if the placement - /// doesn't have an associated rect (i.e. a virtual placement). + /// occupies within the screen. This can return null for a virtual + /// placement or when unavailable pixel geometry makes it empty. pub fn rect( self: Placement, image: Image, @@ -978,17 +987,21 @@ pub const ImageStorage = struct { .virtual => return null, }; + // A zero pixel-sized placement can produce a zero grid size when + // pixel geometry is unavailable. It occupies no rectangle. + if (grid_size.cols == 0 or grid_size.rows == 0) return null; + var br = switch (pin.downOverflow(grid_size.rows - 1)) { .offset => |v| v, .overflow => |v| v.end, }; - br.x = @min( + br.x = @intCast(@min( // We need to sub one here because the x value is // one width already. So if the image is width "1" // then we add zero to X because X itself is width 1. - pin.x + (grid_size.cols - 1), - t.cols - 1, - ); + @as(u32, pin.x) +| (grid_size.cols - 1), + @as(u32, t.cols) - 1, + )); return .{ .top_left = pin.*, @@ -1587,6 +1600,100 @@ test "storage: aspect ratio calculation when only columns or rows specified" { } } +test "storage: placement geometry handles untrusted dimensions" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + const max = std.math.maxInt(u32); + + var t = try terminal.Terminal.init(io, alloc, .{ .cols = 2, .rows = 2 }); + defer t.deinit(alloc); + t.width_px = max; + t.height_px = max; + + // Cell dimensions multiplied by protocol-controlled row and column + // counts saturate instead of panicking or wrapping. + { + const placement: ImageStorage.Placement = .{ + .location = .{ .virtual = {} }, + .columns = 3, + .rows = 3, + }; + const actual = placement.pixelSize(.{ .width = 1, .height = 1 }, &t); + try testing.expectEqual(max, actual.width); + try testing.expectEqual(max, actual.height); + } + + // Aspect-ratio scaling also saturates when the derived dimension does + // not fit in the public u32 geometry type. + { + const placement: ImageStorage.Placement = .{ + .location = .{ .virtual = {} }, + .columns = 3, + .source_height = max, + }; + const actual = placement.pixelSize(.{ .width = 1, .height = 1 }, &t); + try testing.expectEqual(max, actual.width); + try testing.expectEqual(max, actual.height); + } + { + const placement: ImageStorage.Placement = .{ + .location = .{ .virtual = {} }, + .rows = 3, + .source_width = max, + }; + const actual = placement.pixelSize(.{ .width = 1, .height = 1 }, &t); + try testing.expectEqual(max, actual.width); + try testing.expectEqual(max, actual.height); + } + + // Pixel offsets are protocol-controlled too. Include them without + // allowing the grid-size numerator to wrap. + t.width_px = 2; + t.height_px = 2; + { + const placement: ImageStorage.Placement = .{ + .location = .{ .virtual = {} }, + .x_offset = max, + .y_offset = max, + }; + const actual = placement.gridSize(.{ .width = 1, .height = 1 }, &t); + try testing.expectEqual(max, actual.cols); + try testing.expectEqual(max, actual.rows); + } + + const pin = try trackPin(&t, .{ .x = 0, .y = 0 }); + defer t.screens.active.pages.untrackPin(pin); + + // Explicit maximum dimensions must clamp the rectangle to the terminal + // without overflowing its horizontal extent. + { + const placement: ImageStorage.Placement = .{ + .location = .{ .pin = pin }, + .columns = max, + .rows = 1, + }; + const rect = placement.rect(.{ .width = 1, .height = 1 }, &t).?; + try testing.expectEqual(@as(size.CellCountInt, 1), rect.bottom_right.x); + } + + // Terminals can temporarily have no pixel geometry. A placement whose + // computed grid is empty has no rectangle, so rect must not subtract one + // from a zero row count. + t.width_px = 0; + t.height_px = 0; + { + const placement: ImageStorage.Placement = .{ + .location = .{ .pin = pin }, + .columns = 1, + }; + const actual = placement.gridSize(.{ .width = 1, .height = 1 }, &t); + try testing.expectEqual(@as(u32, 0), actual.cols); + try testing.expectEqual(@as(u32, 0), actual.rows); + try testing.expect(placement.rect(.{ .width = 1, .height = 1 }, &t) == null); + } +} + test "storage: generation stamps on image add and replace" { const testing = std.testing; const io = testing.io; From ec04900ab957c7637584e2002aeb6f24c985bd70 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 08:11:16 -0700 Subject: [PATCH 4/8] terminal/kitty: validate opened image file paths Validate Kitty file transmissions against a canonical path derived from the open file handle. Keep temporary file policy and cleanup keyed to that handle path. Path validation previously ran before opening, so a local cooperating process could replace a symlink or directory entry and make Ghostty read a blocklisted file. Open the submitted path once, derive its canonical path from the handle, and use the same handle for stat and reads. Add a regression test that replaces a blocked symlink after open and verifies the pinned target is still rejected. --- src/terminal/kitty/graphics_image.zig | 144 ++++++++++++++++++-------- 1 file changed, 99 insertions(+), 45 deletions(-) diff --git a/src/terminal/kitty/graphics_image.zig b/src/terminal/kitty/graphics_image.zig index 3068a6a81..3c36a37d0 100644 --- a/src/terminal/kitty/graphics_image.zig +++ b/src/terminal/kitty/graphics_image.zig @@ -139,36 +139,18 @@ pub const LoadingImage = struct { if (comptime builtin.os.tag != .windows) { if (std.mem.indexOfScalar(u8, cmd.data, 0) != null) { - // posix.realpath *asserts* that the path does not have - // internal nulls instead of erroring. - log.warn("failed to get absolute path: BadPathName", .{}); + // POSIX paths cannot contain internal nulls. + log.warn("invalid image path: BadPathName", .{}); return error.InvalidData; } } - var abs_buf: [std.fs.max_path_bytes]u8 = undefined; - const path = switch (t.medium) { - .direct => unreachable, // handled above - .file, .temporary_file => path: { - const len = std.Io.Dir.cwd().realPathFile( - io, - cmd.data, - &abs_buf, - ) catch |err| { - log.warn("failed to get absolute path: {}", .{err}); - return error.InvalidData; - }; - break :path abs_buf[0..len]; - }, - .shared_memory => cmd.data, - }; - // Depending on the medium, load the data from the path. switch (t.medium) { .direct => unreachable, // handled above - .file => try result.readFile(.file, io, alloc, t, path), - .temporary_file => try result.readFile(.temporary_file, io, alloc, t, path), - .shared_memory => try result.readSharedMemory(io, alloc, t, path), + .file => try result.readFile(.file, io, alloc, t, cmd.data), + .temporary_file => try result.readFile(.temporary_file, io, alloc, t, cmd.data), + .shared_memory => try result.readSharedMemory(io, alloc, t, cmd.data), } return result; @@ -289,35 +271,56 @@ pub const LoadingImage = struct { else => @compileError("readFile only supports file and temporary_file"), } - // Verify file seems "safe". This is logic copied directly from Kitty, - // mostly. This is really rough but it will catch obvious bad actors. - if (std.mem.startsWith(u8, path, "/proc/") or - std.mem.startsWith(u8, path, "/sys/") or - (std.mem.startsWith(u8, path, "/dev/") and - !std.mem.startsWith(u8, path, "/dev/shm/"))) - { + // Open our file right away before we do validation. This avoids + // TOCTOU issues. + var file = std.Io.Dir.cwd().openFile( + io, + path, + .{}, + ) catch |err| { + log.warn("failed to open image file: {}", .{err}); return error.InvalidData; + }; + + // We'll populate a delete path if this is a temporary file. + var delete_path: ?[]const u8 = null; + defer { + file.close(io); + if (delete_path) |p| { + std.Io.Dir.cwd().deleteFile(io, p) catch |err| { + log.warn("failed to delete temporary file: {}", .{err}); + }; + } } + // Derive the path from the open handle so the file we validate is the + // exact file we read. Resolving a path before opening it would allow a + // cooperating process to swap a symlink or directory entry in between. + var abs_buf: [std.fs.max_path_bytes]u8 = undefined; + const abs_path = validatedFilePath( + io, + file, + &abs_buf, + ) catch |err| { + log.warn("failed to validate image file path: {}", .{err}); + return error.InvalidData; + }; + // Temporary file logic if (medium == .temporary_file) { assert(self.temporary_directory != null); - if (!isPathInTempDir(io, self.temporary_directory.?, path)) return error.TemporaryFileNotInTempDir; - if (std.mem.indexOf(u8, path, "tty-graphics-protocol") == null) { - return error.TemporaryFileNotNamedCorrectly; - } + if (!isPathInTempDir( + io, + self.temporary_directory.?, + abs_path, + )) return error.TemporaryFileNotInTempDir; + if (std.mem.indexOf( + u8, + abs_path, + "tty-graphics-protocol", + ) == null) return error.TemporaryFileNotNamedCorrectly; + delete_path = abs_path; } - defer if (medium == .temporary_file) { - std.Io.Dir.cwd().deleteFile(io, path) catch |err| { - log.warn("failed to delete temporary file: {}", .{err}); - }; - }; - - var file = std.Io.Dir.cwd().openFile(io, path, .{}) catch |err| { - log.warn("failed to open temporary file: {}", .{err}); - return error.InvalidData; - }; - defer file.close(io); // File must be a regular file if (file.stat(io)) |stat| { @@ -354,6 +357,24 @@ pub const LoadingImage = struct { self.data = .{ .items = managed.items, .capacity = managed.capacity }; } + /// Returns the canonical path of an open file after applying the file + /// transmission blocklist. + fn validatedFilePath(io: std.Io, file: std.Io.File, buf: []u8) ![]const u8 { + const path = buf[0..try file.realPath(io, buf)]; + + // This is logic copied directly from Kitty, mostly. This is really + // rough but it will catch obvious bad actors. + if (std.mem.startsWith(u8, path, "/proc/") or + std.mem.startsWith(u8, path, "/sys/") or + (std.mem.startsWith(u8, path, "/dev/") and + !std.mem.startsWith(u8, path, "/dev/shm/"))) + { + return error.InvalidData; + } + + return path; + } + /// Returns true if path appears to be in a temporary directory. /// Copies logic from Kitty. fn isPathInTempDir(io: std.Io, dir: []const u8, path: []const u8) bool { @@ -1056,6 +1077,39 @@ test "image load: rgb, not compressed, relative regular file" { try testing.expect(img.compression == .none); } +test "image load: blocklist applies to opened file after symlink swap" { + if (builtin.os.tag == .windows) return error.SkipZigTest; + + const testing = std.testing; + const io = testing.io; + + var tmp_dir = testing.tmpDir(.{}); + defer tmp_dir.cleanup(); + try tmp_dir.dir.writeFile(io, .{ + .sub_path = "safe.data", + .data = "safe", + }); + try tmp_dir.dir.symLink(io, "/dev/null", "image.data", .{}); + + // Pin the blocked file, then simulate the cooperating process replacing + // the path with a safe target before validation. + const blocked_file = try tmp_dir.dir.openFile(io, "image.data", .{}); + defer blocked_file.close(io); + try tmp_dir.dir.symLinkAtomic(io, "safe.data", "image.data", .{}); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + try testing.expectError( + error.InvalidData, + LoadingImage.validatedFilePath(io, blocked_file, &path_buf), + ); + + // The pathname now resolves to the safe replacement, demonstrating that + // the rejection above came from the already-open file handle. + const safe_file = try tmp_dir.dir.openFile(io, "image.data", .{}); + defer safe_file.close(io); + _ = try LoadingImage.validatedFilePath(io, safe_file, &path_buf); +} + test "image load: png, not compressed, regular file" { if (sys.decode_png == null) return error.SkipZigTest; From f766f303a7d39b9f41fdda1b6d71b329f410d45f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 08:17:43 -0700 Subject: [PATCH 5/8] terminal/kitty: validate shared memory ranges Validate Kitty shared memory byte ranges before mapping and copying image data. Interpret S as a byte count from O and preserve default raw-image sizing. Shared memory transmissions previously multiplied untrusted u32 dimensions before the limit check and sliced mappings with an unchecked offset. Malformed commands could panic in safe builds or request a wrapped allocation in fast builds. Reject oversized dimensions before widening size arithmetic, derive bounded ranges from the stat size, and enforce max_size before constructing a slice. Add regression tests for explicit and implicit offsets, out-of-bounds offsets, and maximum dimensions. --- src/terminal/kitty/graphics_image.zig | 153 ++++++++++++++++++++------ 1 file changed, 122 insertions(+), 31 deletions(-) diff --git a/src/terminal/kitty/graphics_image.zig b/src/terminal/kitty/graphics_image.zig index 3c36a37d0..91fd34508 100644 --- a/src/terminal/kitty/graphics_image.zig +++ b/src/terminal/kitty/graphics_image.zig @@ -204,30 +204,13 @@ pub const LoadingImage = struct { return error.InvalidData; }; if (stat.size <= 0) return error.InvalidData; - break :stat @intCast(stat.size); + break :stat std.math.cast(usize, stat.size) orelse + return error.InvalidData; }; - const expected_size: usize = switch (self.image.format) { - // Png we decode the full data size because later decoding will - // get the proper dimensions and assert validity. - .png => stat_size, - - // For these formats we have a size we must have. - .gray, .gray_alpha, .rgb, .rgba => size: { - const bpp = command.Transmission.formatBpp(self.image.format); - break :size self.image.width * self.image.height * bpp; - }, - }; - - // Our stat size must be at least the expected size otherwise - // the shared memory data is invalid. - if (stat_size < expected_size) { - log.warn( - "shared memory size too small expected={} actual={}", - .{ expected_size, stat_size }, - ); - return error.InvalidData; - } + // Get the memory range we'll read. Validate it to make sure + // it doesn't overflow. + const range = try self.sharedMemoryRange(t, stat_size); const map = std.posix.mmap( null, @@ -242,16 +225,63 @@ pub const LoadingImage = struct { }; defer std.posix.munmap(map); - // Our end size always uses the expected size so we cut off the - // padding for mmap alignment. - const start: usize = @intCast(t.offset); - const end: usize = if (t.size > 0) @min( - @as(usize, @intCast(t.offset)) + @as(usize, @intCast(t.size)), - expected_size, - ) else expected_size; - assert(self.data.items.len == 0); - try self.data.appendSlice(alloc, map[start..end]); + try self.data.appendSlice(alloc, map[range.start..range.end]); + } + + const SharedMemoryRange = struct { + start: usize, + end: usize, + }; + + /// Returns the byte range to copy from a shared memory object. + fn sharedMemoryRange( + self: *const LoadingImage, + t: command.Transmission, + stat_size: usize, + ) error{ + InvalidData, + DimensionsTooLarge, + }!SharedMemoryRange { + const expected_size: ?usize = switch (self.image.format) { + // PNG dimensions come from the decoded data. + .png => null, + + // Validate before multiplying because protocol dimensions are + // u32 values and may otherwise overflow in safe builds. + .gray, .gray_alpha, .rgb, .rgba => size: { + if (self.image.width > max_dimension or + self.image.height > max_dimension) + { + return error.DimensionsTooLarge; + } + + const bpp: usize = command.Transmission.formatBpp(self.image.format); + break :size @as(usize, self.image.width) * + @as(usize, self.image.height) * bpp; + }, + }; + + // Get our start offset and validate its within the range of + // the statted data. + const start = std.math.cast(usize, t.offset) orelse + return error.InvalidData; + if (start > stat_size) return error.InvalidData; + + // Validate that our length is within the stat range too. + const available = stat_size - start; + const data_size: usize = if (t.size > 0) + std.math.cast(usize, t.size) orelse return error.InvalidData + else if (self.image.compression == .none and expected_size != null) + expected_size.? + else + available; + if (data_size > max_size or data_size > available) { + return error.InvalidData; + } + + // data_size <= available guarantees this addition cannot overflow. + return .{ .start = start, .end = start + data_size }; } /// Reads the data from a temporary file and returns it. This allocates @@ -668,6 +698,67 @@ test "temporary file path must be inside directory" { try testing.expect(!isPathInDir("/custom/tmp", "/custom/tmp-suffix/tty-graphics-protocol-image.data")); } +test "shared memory range with offset and size" { + const testing = std.testing; + + const loading: LoadingImage = .{ + .image = .{ + .width = 1, + .height = 1, + .format = .rgb, + }, + .quiet = .no, + .temporary_directory = null, + }; + + const explicit = try loading.sharedMemoryRange(.{ + .offset = 2, + .size = 3, + }, 5); + try testing.expectEqual(@as(usize, 2), explicit.start); + try testing.expectEqual(@as(usize, 5), explicit.end); + + const implicit = try loading.sharedMemoryRange(.{ + .offset = 2, + }, 5); + try testing.expectEqual(@as(usize, 2), implicit.start); + try testing.expectEqual(@as(usize, 5), implicit.end); +} + +test "shared memory range rejects out of bounds offset" { + const loading: LoadingImage = .{ + .image = .{ + .width = 1, + .height = 1, + .format = .rgb, + }, + .quiet = .no, + .temporary_directory = null, + }; + + try std.testing.expectError( + error.InvalidData, + loading.sharedMemoryRange(.{ .offset = 4 }, 3), + ); +} + +test "shared memory range validates dimensions before multiplication" { + const loading: LoadingImage = .{ + .image = .{ + .width = std.math.maxInt(u32), + .height = std.math.maxInt(u32), + .format = .rgba, + }, + .quiet = .no, + .temporary_directory = null, + }; + + try std.testing.expectError( + error.DimensionsTooLarge, + loading.sharedMemoryRange(.{}, 1), + ); +} + // This specifically tests we ALLOW invalid RGB data because Kitty // documents that this should work. test "image load with invalid RGB data" { From 590d669c4a72eb9cb990bf0162071c2b9eb0f7ad Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 08:24:42 -0700 Subject: [PATCH 6/8] terminal/kitty: limit png decoder allocations Limit individual allocator requests made by PNG decoders to the Kitty graphics protocol's 400 MiB image ceiling. Add a reusable allocator wrapper for callers that need per-request bounds. PNG decoding previously used Wuffs' 4 GiB package limit and checked the result only after allocation. A tiny PNG with oversized dimensions could cause a multi-gigabyte RSS spike before being rejected. Wrap decoder allocators with LimitedAllocator and translate limit rejections to invalid image data while preserving genuine out-of-memory errors. Add allocator boundary tests and regression coverage for a crafted PNG below Wuffs' limit. --- src/datastruct/limited_allocator.zig | 124 ++++++++++++++++++++++++++ src/datastruct/main.zig | 1 + src/terminal/kitty/graphics_image.zig | 79 +++++++++++++++- 3 files changed, 201 insertions(+), 3 deletions(-) create mode 100644 src/datastruct/limited_allocator.zig diff --git a/src/datastruct/limited_allocator.zig b/src/datastruct/limited_allocator.zig new file mode 100644 index 000000000..11c4fbeaf --- /dev/null +++ b/src/datastruct/limited_allocator.zig @@ -0,0 +1,124 @@ +const std = @import("std"); +const Allocator = std.mem.Allocator; + +/// An allocator that rejects any single allocation or resize larger than a +/// configured byte limit. The limit applies to each request independently, +/// not to the total amount of memory currently allocated. +pub const LimitedAllocator = struct { + child: Allocator, + limit: usize, + + /// Set after an allocation, resize, or remap is rejected for exceeding + /// the limit. This remains set until the caller clears it. + limit_exceeded: bool = false, + + pub fn init(child: Allocator, limit: usize) LimitedAllocator { + return .{ + .child = child, + .limit = limit, + }; + } + + pub fn allocator(self: *LimitedAllocator) Allocator { + return .{ + .ptr = self, + .vtable = &.{ + .alloc = alloc, + .resize = resize, + .remap = remap, + .free = free, + }, + }; + } + + fn check(self: *LimitedAllocator, size: usize) bool { + if (size <= self.limit) return true; + self.limit_exceeded = true; + return false; + } + + fn alloc( + ctx: *anyopaque, + len: usize, + alignment: std.mem.Alignment, + ret_addr: usize, + ) ?[*]u8 { + const self: *LimitedAllocator = @ptrCast(@alignCast(ctx)); + if (!self.check(len)) return null; + return self.child.rawAlloc(len, alignment, ret_addr); + } + + fn resize( + ctx: *anyopaque, + memory: []u8, + alignment: std.mem.Alignment, + new_len: usize, + ret_addr: usize, + ) bool { + const self: *LimitedAllocator = @ptrCast(@alignCast(ctx)); + if (!self.check(new_len)) return false; + return self.child.rawResize(memory, alignment, new_len, ret_addr); + } + + fn remap( + ctx: *anyopaque, + memory: []u8, + alignment: std.mem.Alignment, + new_len: usize, + ret_addr: usize, + ) ?[*]u8 { + const self: *LimitedAllocator = @ptrCast(@alignCast(ctx)); + if (!self.check(new_len)) return null; + return self.child.rawRemap(memory, alignment, new_len, ret_addr); + } + + fn free( + ctx: *anyopaque, + memory: []u8, + alignment: std.mem.Alignment, + ret_addr: usize, + ) void { + const self: *LimitedAllocator = @ptrCast(@alignCast(ctx)); + self.child.rawFree(memory, alignment, ret_addr); + } +}; + +test "LimitedAllocator allows allocations through the limit" { + const testing = std.testing; + + var limited: LimitedAllocator = .init(testing.allocator, 8); + const alloc = limited.allocator(); + const data = try alloc.alloc(u8, 8); + defer alloc.free(data); + + try testing.expect(!limited.limit_exceeded); +} + +test "LimitedAllocator rejects allocations before the child" { + const testing = std.testing; + + var failing = testing.FailingAllocator.init(testing.allocator, .{ + .fail_index = 0, + }); + var limited: LimitedAllocator = .init(failing.allocator(), 8); + + try testing.expectError(error.OutOfMemory, limited.allocator().alloc(u8, 9)); + try testing.expect(limited.limit_exceeded); + try testing.expect(!failing.has_induced_failure); +} + +test "LimitedAllocator rejects oversized resize and remap" { + const testing = std.testing; + + var limited: LimitedAllocator = .init(testing.allocator, 8); + const alloc = limited.allocator(); + const data = try alloc.alloc(u8, 8); + defer alloc.free(data); + + try testing.expect(!alloc.resize(data, 9)); + try testing.expect(limited.limit_exceeded); + + limited.limit_exceeded = false; + try testing.expect(alloc.remap(data, 9) == null); + try testing.expect(limited.limit_exceeded); +} diff --git a/src/datastruct/main.zig b/src/datastruct/main.zig index 16a61ae5a..b3bdc6a99 100644 --- a/src/datastruct/main.zig +++ b/src/datastruct/main.zig @@ -12,6 +12,7 @@ pub const BlockingQueue = blocking_queue.BlockingQueue; pub const CacheTable = cache_table.CacheTable; pub const CircBuf = circ_buf.CircBuf; pub const IntrusiveDoublyLinkedList = intrusive_linked_list.DoublyLinkedList; +pub const LimitedAllocator = @import("limited_allocator.zig").LimitedAllocator; pub const MessageData = @import("message_data.zig").MessageData; pub const SegmentedPool = segmented_pool.SegmentedPool; pub const SplitTree = split_tree.SplitTree; diff --git a/src/terminal/kitty/graphics_image.zig b/src/terminal/kitty/graphics_image.zig index 91fd34508..5078ac4c6 100644 --- a/src/terminal/kitty/graphics_image.zig +++ b/src/terminal/kitty/graphics_image.zig @@ -9,6 +9,7 @@ const fastmem = @import("../../fastmem.zig"); const command = @import("graphics_command.zig"); const PageList = @import("../PageList.zig"); const sys = @import("../sys.zig"); +const LimitedAllocator = @import("../../datastruct/main.zig").LimitedAllocator; const log = std.log.scoped(.kitty_gfx); @@ -551,14 +552,20 @@ pub const LoadingImage = struct { const decode_png_fn = sys.decode_png orelse return error.UnsupportedFormat; + + var limited: LimitedAllocator = .init(alloc, max_size); + const decode_alloc = limited.allocator(); const result = decode_png_fn( - alloc, + decode_alloc, self.data.items, ) catch |err| switch (err) { error.InvalidData => return error.InvalidData, - error.OutOfMemory => return error.OutOfMemory, + error.OutOfMemory => if (limited.limit_exceeded) + return error.InvalidData + else + return error.OutOfMemory, }; - defer alloc.free(result.data); + defer decode_alloc.free(result.data); if (result.data.len > max_size) { log.warn("png image too large size={} max_size={}", .{ result.data.len, max_size }); @@ -1246,6 +1253,72 @@ test "image load: png, not compressed, regular file" { try tmp_dir.dir.access(testing.io, path, .{}); } +test "image load: png rejects oversized decoder allocation" { + const testing = std.testing; + + const oversized_decoder = struct { + fn decode( + alloc: Allocator, + _: []const u8, + ) sys.DecodeError!sys.Image { + const data = try alloc.alloc(u8, max_size + 1); + return .{ + .width = 1, + .height = 1, + .data = data, + }; + } + }.decode; + + const original_decode_png = sys.decode_png; + defer sys.decode_png = original_decode_png; + sys.decode_png = &oversized_decoder; + + // Fail any allocation which reaches the underlying allocator. The size + // limiter should reject the decoder's request before it gets that far. + var failing = testing.FailingAllocator.init(testing.allocator, .{ + .fail_index = 0, + }); + const alloc = failing.allocator(); + + var loading: LoadingImage = .{ + .image = .{ .format = .png }, + .quiet = .no, + .temporary_directory = null, + }; + defer loading.deinit(alloc); + + try testing.expectError(error.InvalidData, loading.complete(alloc)); + try testing.expect(!failing.has_induced_failure); +} + +test "image load: png rejects oversized Wuffs image before allocation" { + if (sys.decode_png == null) return error.SkipZigTest; + + const testing = std.testing; + const alloc = testing.allocator; + + // Turn the small test PNG into a 32768x32767 image. Its decoded RGBA + // size is just under Wuffs' 4 GiB package limit but over Kitty's 400 MiB + // limit, which previously allowed the large allocation to happen first. + var data = @embedFile("testdata/image-png-none-50x76-2147483647-raw.data").*; + std.mem.writeInt(u32, data[16..20], 32768, .big); + std.mem.writeInt(u32, data[20..24], 32767, .big); + std.mem.writeInt(u32, data[29..33], std.hash.Crc32.hash(data[12..29]), .big); + + const cmd: command.Command = .{ + .control = .{ .transmit = .{ + .format = .png, + .medium = .direct, + } }, + .data = &data, + }; + var loading = try LoadingImage.init(testing.io, alloc, &cmd, .direct); + defer loading.deinit(alloc); + + try testing.expectError(error.InvalidData, loading.complete(alloc)); +} + test "limits: direct medium always allowed" { const testing = std.testing; const alloc = testing.allocator; From d0c516f8f384ee4cc3304c4eef3dabc07c432de4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 08:28:13 -0700 Subject: [PATCH 7/8] terminal/kitty: release replaced placement pins Release a Kitty graphics placement's tracked pin before replacement. Repeated updates to an external placement previously leaked tracked pins. Pass the owning screen to storage and deinitialize the old placement. --- src/renderer/image.zig | 2 +- src/terminal/kitty/graphics_exec.zig | 1 + src/terminal/kitty/graphics_storage.zig | 137 ++++++++++++++++-------- src/terminal/kitty/graphics_unicode.zig | 6 +- src/terminal/snapshot/snapshot.zig | 1 + 5 files changed, 96 insertions(+), 51 deletions(-) diff --git a/src/renderer/image.zig b/src/renderer/image.zig index 3d5416209..927c7371e 100644 --- a/src/renderer/image.zig +++ b/src/renderer/image.zig @@ -1000,7 +1000,7 @@ test "kitty renderer ignores pending payloads and retains native placements" { const pin = try t.screens.active.pages.trackPin( t.screens.active.cursor.page_pin.*, ); - try storage.addPlacement(io, alloc, 1, 1, .{ + try storage.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = pin }, .columns = 1, .rows = 1, diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index 449e859f6..f499ca5bd 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -256,6 +256,7 @@ fn display( storage.addPlacement( io, alloc, + terminal.screens.active, img.id, result.placement_id, p, diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index eb476c33a..6472d0efb 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -300,11 +300,14 @@ pub const ImageStorage = struct { } /// Add a placement for a given image. The caller must verify in advance - /// the image exists to prevent memory corruption. + /// the image exists to prevent memory corruption. On success, storage + /// owns `p`; on error, the caller retains ownership. The screen must own + /// any tracked pin in `p` and in the placement it replaces. pub fn addPlacement( self: *ImageStorage, io: std.Io, alloc: Allocator, + s: *terminal.Screen, image_id: u32, placement_id: u32, p: Placement, @@ -336,6 +339,7 @@ pub const ImageStorage = struct { }; const gop = try self.placements.getOrPut(alloc, key); + if (gop.found_existing) gop.value_ptr.deinit(s); gop.value_ptr.* = p; self.markMutated(io); @@ -1034,8 +1038,8 @@ test "storage: add placement with zero placement id" { defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 50, .height = 50 }); try s.addImage(io, alloc, .{ .id = 2, .width = 25, .height = 25 }); - try s.addPlacement(io, alloc, 1, 0, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); - try s.addPlacement(io, alloc, 1, 0, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); try testing.expectEqual(@as(usize, 2), s.placements.count()); try testing.expectEqual(@as(usize, 2), s.images.count()); @@ -1051,6 +1055,44 @@ test "storage: add placement with zero placement id" { }) != null); } +test "storage: replacing placement releases tracked pin" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + var t = try terminal.Terminal.init(io, alloc, .{ .cols = 3, .rows = 3 }); + defer t.deinit(alloc); + + var s: ImageStorage = .{}; + defer s.deinit(alloc, t.screens.active); + try s.addImage(io, alloc, .{ .id = 1 }); + + const tracked = t.screens.active.pages.countTrackedPins(); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ + .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) }, + }); + try testing.expectEqual( + tracked + 1, + t.screens.active.pages.countTrackedPins(), + ); + + const replacement_pin = try trackPin(&t, .{ .x = 2, .y = 2 }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ + .location = .{ .pin = replacement_pin }, + }); + try testing.expectEqual(@as(usize, 1), s.placements.count()); + try testing.expectEqual( + tracked + 1, + t.screens.active.pages.countTrackedPins(), + ); + try testing.expectEqual( + replacement_pin, + s.placements.get(.{ + .image_id = 1, + .placement_id = .{ .tag = .external, .id = 1 }, + }).?.location.pin, + ); +} + test "storage: delete all placements and images" { const testing = std.testing; const alloc = testing.allocator; @@ -1064,8 +1106,8 @@ test "storage: delete all placements and images" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .all = true }); @@ -1089,8 +1131,8 @@ test "storage: delete all placements and images preserves limit" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .all = true }); @@ -1114,8 +1156,8 @@ test "storage: delete all placements" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .all = false }); @@ -1138,8 +1180,8 @@ test "storage: delete all placements by image id" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .id = .{ .image_id = 2 } }); @@ -1162,8 +1204,8 @@ test "storage: delete all placements by image id and unused images" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .id = .{ .delete = true, .image_id = 2 } }); @@ -1186,9 +1228,9 @@ test "storage: delete placement by specific id" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .id = .{ @@ -1216,8 +1258,8 @@ test "storage: delete intersecting cursor" { defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 50, .height = 50 }); try s.addImage(io, alloc, .{ .id = 2, .width = 25, .height = 25 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); t.screens.active.cursorAbsolute(12, 12); @@ -1249,8 +1291,8 @@ test "storage: delete intersecting cursor plus unused" { defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 50, .height = 50 }); try s.addImage(io, alloc, .{ .id = 2, .width = 25, .height = 25 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); t.screens.active.cursorAbsolute(12, 12); @@ -1282,8 +1324,8 @@ test "storage: delete intersecting cursor hits multiple" { defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 50, .height = 50 }); try s.addImage(io, alloc, .{ .id = 2, .width = 25, .height = 25 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); t.screens.active.cursorAbsolute(26, 26); @@ -1309,8 +1351,8 @@ test "storage: delete by column" { defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 50, .height = 50 }); try s.addImage(io, alloc, .{ .id = 2, .width = 25, .height = 25 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .column = .{ @@ -1341,9 +1383,9 @@ test "storage: delete by column 1x1" { var s: ImageStorage = .{}; defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 1, .height = 1 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 3, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 2, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 3, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 2, .y = 0 }) } }); s.delete(io, alloc, &t, .{ .column = .{ .delete = false, @@ -1377,8 +1419,8 @@ test "storage: delete by row" { defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 50, .height = 50 }); try s.addImage(io, alloc, .{ .id = 2, .width = 25, .height = 25 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 0, .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 25, .y = 25 }) } }); s.dirty = false; s.delete(io, alloc, &t, .{ .row = .{ @@ -1409,9 +1451,9 @@ test "storage: delete by row 1x1" { var s: ImageStorage = .{}; defer s.deinit(alloc, t.screens.active); try s.addImage(io, alloc, .{ .id = 1, .width = 1, .height = 1 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .y = 0 }) } }); - try s.addPlacement(io, alloc, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .y = 1 }) } }); - try s.addPlacement(io, alloc, 1, 3, .{ .location = .{ .pin = try trackPin(&t, .{ .y = 2 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .y = 0 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 2, .{ .location = .{ .pin = try trackPin(&t, .{ .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 3, .{ .location = .{ .pin = try trackPin(&t, .{ .y = 2 }) } }); s.delete(io, alloc, &t, .{ .row = .{ .delete = false, @@ -1444,8 +1486,8 @@ test "storage: delete images by range 1" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try testing.expectEqual(@as(usize, 3), s.images.count()); try testing.expectEqual(@as(usize, 2), s.placements.count()); @@ -1470,8 +1512,8 @@ test "storage: delete images by range 2" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try testing.expectEqual(@as(usize, 3), s.images.count()); try testing.expectEqual(@as(usize, 2), s.placements.count()); @@ -1496,9 +1538,9 @@ test "storage: delete images by range 3" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 3, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 3, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try testing.expectEqual(@as(usize, 3), s.images.count()); try testing.expectEqual(@as(usize, 3), s.placements.count()); @@ -1531,9 +1573,9 @@ test "storage: delete images by range 4" { try s.addImage(io, alloc, .{ .id = 1 }); try s.addImage(io, alloc, .{ .id = 2 }); try s.addImage(io, alloc, .{ .id = 3 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); - try s.addPlacement(io, alloc, 3, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 3, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); try testing.expectEqual(@as(usize, 3), s.images.count()); try testing.expectEqual(@as(usize, 3), s.placements.count()); @@ -1744,7 +1786,7 @@ test "storage: generation bumps on placement and delete" { try s.addImage(io, alloc, .{ .id = 1 }); const gen_add = s.generation; - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); const gen_place = s.generation; try testing.expect(gen_place > gen_add); @@ -1838,7 +1880,7 @@ test "storage: no-op delete does not mark a mutation" { // Same for a delete that matches nothing. try s.addImage(io, alloc, .{ .id = 1 }); - try s.addPlacement(io, alloc, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }); const gen = s.generation; s.dirty = false; s.delete(io, alloc, &t, .{ .id = .{ .image_id = 42 } }); @@ -1879,6 +1921,7 @@ test "storage: evict unused transient image" { try s.addPlacement( io, alloc, + t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) } }, @@ -1917,7 +1960,7 @@ test "storage: pending image completes once and preserves age" { try testing.expectEqual(@as(u32, 1), s.imageByNumber(7).?.id); try testing.expect(s.imageById(1).?.data.isPending()); - try s.addPlacement(io, alloc, 1, 1, .{ + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) }, }); try testing.expectEqual(@as(usize, 1), s.placements.count()); @@ -2009,7 +2052,7 @@ test "storage: replacement reuses pending reservation and preserves placements" .format = .rgba, .data = .{ .pending = 8 }, }); - try s.addPlacement(io, alloc, 1, 1, .{ + try s.addPlacement(io, alloc, t.screens.active, 1, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) }, }); try s.addImage(io, alloc, .{ @@ -2071,7 +2114,7 @@ test "storage: pending images share exact eviction ordering" { .data = .{ .complete = try alloc.dupe(u8, "*" ** 64) }, .usage = .{ .transient = true }, }); - try s.addPlacement(io, alloc, 2, 1, .{ + try s.addPlacement(io, alloc, t.screens.active, 2, 1, .{ .location = .{ .pin = try trackPin(&t, .{ .x = 1, .y = 1 }) }, }); diff --git a/src/terminal/kitty/graphics_unicode.zig b/src/terminal/kitty/graphics_unicode.zig index dd1a6ce44..21fa0b3a5 100644 --- a/src/terminal/kitty/graphics_unicode.zig +++ b/src/terminal/kitty/graphics_unicode.zig @@ -1196,7 +1196,7 @@ test "unicode render placement: dog 4x2" { const image: Image = .{ .id = 1, .width = 500, .height = 306 }; try s.addImage(io, alloc, image); - try s.addPlacement(io, alloc, 1, 0, .{ + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ .location = .{ .virtual = {} }, .columns = 4, .rows = 2, @@ -1264,7 +1264,7 @@ test "unicode render placement: dog 2x2 with blank cells" { const image: Image = .{ .id = 1, .width = 500, .height = 306 }; try s.addImage(io, alloc, image); - try s.addPlacement(io, alloc, 1, 0, .{ + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ .location = .{ .virtual = {} }, .columns = 2, .rows = 2, @@ -1331,7 +1331,7 @@ test "unicode render placement: dog 1x1" { const image: Image = .{ .id = 1, .width = 500, .height = 306 }; try s.addImage(io, alloc, image); - try s.addPlacement(io, alloc, 1, 0, .{ + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ .location = .{ .virtual = {} }, .columns = 1, .rows = 1, diff --git a/src/terminal/snapshot/snapshot.zig b/src/terminal/snapshot/snapshot.zig index f3adda8cb..dc8d7524d 100644 --- a/src/terminal/snapshot/snapshot.zig +++ b/src/terminal/snapshot/snapshot.zig @@ -1231,6 +1231,7 @@ test "complete snapshot preserves Kitty virtual placeholders" { try t.screens.active.kitty_images.addPlacement( testing.io, testing.allocator, + t.screens.active, 1, 0, .{ From 402b9227de1efab24de981ce0d71b4d378417490 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 5 Aug 2026 08:37:45 -0700 Subject: [PATCH 8/8] terminal/kitty: reclaim pruned placements Reclaim pin-backed Kitty graphics placements after their tracked screen content is pruned. Treat garbage pins as non-renderable until the next placement command sweeps them. Placements that scrolled beyond retained history previously remained in the placement map and tracked-pin set. Long-running graphics output could accumulate stale state, and remapped garbage pins could appear at an unrelated fallback location. Sweep garbage placements before growing the placement map, releasing each tracked pin while preserving virtual placements. Return no geometry or visible render position for garbage pins and cover both storage and C API behavior with regression tests. --- src/terminal/c/kitty_graphics.zig | 21 ++++++- src/terminal/kitty/graphics_storage.zig | 82 +++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/src/terminal/c/kitty_graphics.zig b/src/terminal/c/kitty_graphics.zig index c1c738a0b..4e36b88a8 100644 --- a/src/terminal/c/kitty_graphics.zig +++ b/src/terminal/c/kitty_graphics.zig @@ -595,9 +595,9 @@ pub fn placement_render_info( /// the placement's origin has scrolled above the top of the viewport. /// /// A placement is considered not visible if it is a virtual (unicode -/// placeholder) placement, or if it is fully off-screen (its bottom -/// edge is above the viewport or its top edge is at or below the -/// viewport's last row). +/// placeholder) placement, its tracked content has been pruned, or it is +/// fully off-screen (its bottom edge is above the viewport or its top edge is +/// at or below the viewport's last row). fn computeViewportPos( p: *const kitty_storage.ImageStorage.Placement, image: *const Image, @@ -609,6 +609,7 @@ fn computeViewportPos( .pin => |pin| pin, .virtual => return .{ .col = 0, .row = 0, .visible = false }, }; + if (pin.garbage) return .{ .col = 0, .row = 0, .visible = false }; // Convert both the placement's pin and the viewport's top-left // corner to screen-absolute coordinates so we can subtract them @@ -1621,6 +1622,20 @@ test "placement_render_info returns all fields" { try testing.expectEqual(0, ri.source_y); try testing.expectEqual(1, ri.source_width); try testing.expectEqual(2, ri.source_height); + + const entry = iter.?.entry.?; + const pin = switch (entry.value_ptr.location) { + .pin => |pin| pin, + .virtual => unreachable, + }; + pin.garbage = true; + + ri = .{}; + try testing.expectEqual(Result.success, placement_render_info(iter, img, t, &ri)); + try testing.expect(!ri.viewport_visible); + + var rect: selection_c.CSelection = undefined; + try testing.expectEqual(Result.no_value, placement_rect(iter, img, t, &rect)); } test "placement_render_info handles maximum grid dimensions" { diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index 6472d0efb..83bd5f9cb 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -319,6 +319,14 @@ pub const ImageStorage = struct { p, }); + // Tracked pins are marked garbage when their underlying history is + // pruned. Kitty removes placements once they scroll out of retained + // history, so reclaim those placements before growing the map for a + // new one. If allocation below fails, the sweep is still a content + // mutation and must be visible to consumers. + const removed_garbage = self.removeGarbagePlacements(s); + errdefer if (removed_garbage) self.markMutated(io); + // The important piece here is that the placement ID needs to // be marked internal if it is zero. This allows multiple placements // to be added for the same image. If it is non-zero, then it is @@ -345,6 +353,29 @@ pub const ImageStorage = struct { self.markMutated(io); } + /// Remove pin-backed placements whose tracked content has been pruned. + /// Virtual placements have no tracked screen location and are retained. + fn removeGarbagePlacements( + self: *ImageStorage, + s: *terminal.Screen, + ) bool { + var removed = false; + var it = self.placements.iterator(); + while (it.next()) |entry| { + const pin = switch (entry.value_ptr.location) { + .pin => |pin| pin, + .virtual => continue, + }; + if (!pin.garbage) continue; + + entry.value_ptr.deinit(s); + self.placements.removeByPtr(entry.key_ptr); + removed = true; + } + + return removed; + } + fn clearPlacements(self: *ImageStorage, s: *terminal.Screen) void { var it = self.placements.iterator(); while (it.next()) |entry| entry.value_ptr.deinit(s); @@ -990,6 +1021,7 @@ pub const ImageStorage = struct { .pin => |p| p, .virtual => return null, }; + if (pin.garbage) return null; // A zero pixel-sized placement can produce a zero grid size when // pixel geometry is unavailable. It occupies no rectangle. @@ -1093,6 +1125,43 @@ test "storage: replacing placement releases tracked pin" { ); } +test "storage: adding placement reclaims garbage placements" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + var t = try terminal.Terminal.init(io, alloc, .{ .cols = 3, .rows = 3 }); + defer t.deinit(alloc); + + var s: ImageStorage = .{}; + defer s.deinit(alloc, t.screens.active); + try s.addImage(io, alloc, .{ .id = 1 }); + + const tracked = t.screens.active.pages.countTrackedPins(); + const old_pin = try trackPin(&t, .{ .x = 0, .y = 0 }); + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ + .location = .{ .pin = old_pin }, + }); + old_pin.garbage = true; + + const new_pin = try trackPin(&t, .{ .x = 1, .y = 1 }); + try s.addPlacement(io, alloc, t.screens.active, 1, 0, .{ + .location = .{ .pin = new_pin }, + }); + + try testing.expectEqual(@as(usize, 1), s.placements.count()); + try testing.expectEqual( + tracked + 1, + t.screens.active.pages.countTrackedPins(), + ); + try testing.expectEqual( + new_pin, + s.placements.get(.{ + .image_id = 1, + .placement_id = .{ .tag = .internal, .id = 1 }, + }).?.location.pin, + ); +} + test "storage: delete all placements and images" { const testing = std.testing; const alloc = testing.allocator; @@ -1719,6 +1788,19 @@ test "storage: placement geometry handles untrusted dimensions" { try testing.expectEqual(@as(size.CellCountInt, 1), rect.bottom_right.x); } + // A garbage pin represents content that has been pruned from retained + // history. Its fallback location must not make the placement visible. + pin.garbage = true; + { + const placement: ImageStorage.Placement = .{ + .location = .{ .pin = pin }, + .columns = 1, + .rows = 1, + }; + try testing.expect(placement.rect(.{ .width = 1, .height = 1 }, &t) == null); + } + pin.garbage = false; + // Terminals can temporarily have no pixel geometry. A placement whose // computed grid is empty has no rectangle, so rect must not subtract one // from a zero row count.