Misc APC improvements (#12349)

* Fix a memory leak when invalid Kitty graphics data is sent via APC
(this is the only commit for backporting to 1.3.2)
* Add `max_bytes` to limit size of buffered APC data by protocol to
prevent DoS, default to reasonable values
* libghostty: expose max bytes APC options

include/ghostty/vt/terminal.h

@@ -573,6 +573,25 @@ typedef enum GHOSTTY_ENUM_TYPED {
    * Input type: bool*
    */
   GHOSTTY_TERMINAL_OPT_KITTY_IMAGE_MEDIUM_SHARED_MEM = 18,
+
+  /**
+   * Set the maximum bytes the APC handler will buffer for all protocols.
+   * This prevents malicious input from causing unbounded memory allocation.
+   * A NULL value pointer removes all overrides, reverting to the built-in
+   * defaults.
+   *
+   * Input type: size_t*
+   */
+  GHOSTTY_TERMINAL_OPT_APC_MAX_BYTES = 19,
+
+  /**
+   * Set the maximum bytes the APC handler will buffer for Kitty graphics
+   * protocol data. A NULL value pointer removes the override, reverting
+   * to the built-in default.
+   *
+   * Input type: size_t*
+   */
+  GHOSTTY_TERMINAL_OPT_APC_MAX_BYTES_KITTY = 20,
   GHOSTTY_TERMINAL_OPT_MAX_VALUE = GHOSTTY_ENUM_MAX_VALUE,
 } GhosttyTerminalOption;
 

src/terminal/apc.zig

@@ -12,6 +12,14 @@ const log = std.log.scoped(.terminal_apc);
 pub const Handler = struct {
     state: State = .inactive,
 
+    /// Maximum bytes each APC protocol can buffer. This is to prevent
+    /// malicious input from causing us to allocate too much memory.
+    /// If you want to be lazy and set a single value for all protocols,
+    /// use `.initFull`.
+    max_bytes: std.EnumMap(Protocol, usize) = .initFullWith(.{
+        .kitty = Protocol.defaultMaxBytes(.kitty),
+    }),
+
     pub fn deinit(self: *Handler) void {
         self.state.deinit();
     }
@@ -34,7 +42,11 @@ pub const Handler = struct {
                 switch (byte) {
                     // Kitty graphics protocol
                     'G' => self.state = if (comptime build_options.kitty_graphics)
-                        .{ .kitty = kitty_gfx.CommandParser.init(alloc) }
+                        .{ .kitty = .init(
+                            alloc,
+                            self.max_bytes.get(.kitty) orelse
+                                Protocol.defaultMaxBytes(.kitty),
+                        ) }
                     else
                         .ignore,
 
@@ -46,6 +58,7 @@ pub const Handler = struct {
             .kitty => |*p| if (comptime build_options.kitty_graphics) {
                 p.feed(byte) catch |err| {
                     log.warn("kitty graphics protocol error: {}", .{err});
+                    p.deinit();
                     self.state = .ignore;
                 };
             } else unreachable,
@@ -106,8 +119,22 @@ pub const State = union(enum) {
     }
 };
 
+/// Possible APC command types.
+pub const Protocol = enum {
+    kitty,
+
+    /// Returns the default maximum bytes for the given protocol.
+    pub fn defaultMaxBytes(self: Protocol) usize {
+        return switch (self) {
+            // Kitty graphics payloads can be very large (e.g. full images
+            // encoded as base64), so the default is set to 65 MiB.
+            .kitty => 65 * 1024 * 1024,
+        };
+    }
+};
+
 /// Possible APC commands.
-pub const Command = union(enum) {
+pub const Command = union(Protocol) {
     kitty: if (build_options.kitty_graphics)
         kitty_gfx.Command
     else
@@ -169,6 +196,41 @@ test "Kitty command with overflow i32" {
     try testing.expect(h.end() == null);
 }
 
+test "kitty feed error deinits parser" {
+    if (comptime !build_options.kitty_graphics) return error.SkipZigTest;
+
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    // Feed a valid kitty command start to allocate parser state, then
+    // trigger an error during feed via an integer overflow. The testing
+    // allocator will detect leaks if deinit is not called.
+    var h: Handler = .{};
+    defer h.deinit();
+    h.start();
+    for ("Ga=p,i=10000000000;") |c| h.feed(alloc, c);
+    try testing.expect(h.state == .ignore);
+}
+
+test "kitty max bytes exceeded" {
+    if (comptime !build_options.kitty_graphics) return error.SkipZigTest;
+
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var h: Handler = .{ .max_bytes = .init(.{ .kitty = 4 }) };
+    defer h.deinit();
+    h.start();
+    // 'G' identifies kitty, 'a=t;' moves to data state, then feed exceeds max_bytes.
+    for ("Ga=t;") |c| h.feed(alloc, c);
+    try testing.expect(h.state != .ignore);
+    for ("abcd") |c| h.feed(alloc, c);
+    try testing.expect(h.state != .ignore);
+    // The 5th data byte exceeds the 4-byte limit.
+    h.feed(alloc, 'e');
+    try testing.expect(h.state == .ignore);
+}
+
 test "valid Kitty command" {
     if (comptime !build_options.kitty_graphics) return error.SkipZigTest;
 

src/terminal/c/terminal.zig

@@ -7,6 +7,7 @@ const ZigTerminal = @import("../Terminal.zig");
 const Stream = @import("../stream_terminal.zig").Stream;
 const ScreenSet = @import("../ScreenSet.zig");
 const PageList = @import("../PageList.zig");
+const apc = @import("../apc.zig");
 const kitty = @import("../kitty/key.zig");
 const kitty_gfx_c = @import("kitty_graphics.zig");
 const modes = @import("../modes.zig");
@@ -310,6 +311,8 @@ pub const Option = enum(c_int) {
     kitty_image_medium_file = 16,
     kitty_image_medium_temp_file = 17,
     kitty_image_medium_shared_mem = 18,
+    apc_max_bytes = 19,
+    apc_max_bytes_kitty = 20,
 
     /// Input type expected for setting the option.
     pub fn InType(comptime self: Option) type {
@@ -331,6 +334,7 @@ pub const Option = enum(c_int) {
             .kitty_image_medium_temp_file,
             .kitty_image_medium_shared_mem,
             => ?*const bool,
+            .apc_max_bytes, .apc_max_bytes_kitty => ?*const usize,
         };
     }
 };
@@ -425,6 +429,19 @@ fn setTyped(
                 }
             }
         },
+        .apc_max_bytes => {
+            wrapper.stream.handler.apc_handler.max_bytes = if (value) |ptr|
+                .initFull(ptr.*)
+            else
+                .{};
+        },
+        .apc_max_bytes_kitty => {
+            if (value) |ptr| {
+                wrapper.stream.handler.apc_handler.max_bytes.put(.kitty, ptr.*);
+            } else {
+                wrapper.stream.handler.apc_handler.max_bytes.remove(.kitty);
+            }
+        },
     }
     return .success;
 }

src/terminal/kitty/graphics_command.zig

@@ -38,6 +38,10 @@ pub const Parser = struct {
     /// here instead of a fixed buffer so that we can too.
     data: std.ArrayList(u8),
 
+    /// Maximum bytes the data payload can take. This is to prevent
+    /// malicious input from causing us to allocate too much memory.
+    max_bytes: usize,
+
     /// Internal state for parsing.
     state: State,
 
@@ -55,7 +59,7 @@ pub const Parser = struct {
 
     /// Initialize the parser. The allocator given will be used for both
     /// temporary data and long-lived values such as the final image blob.
-    pub fn init(alloc: Allocator) Parser {
+    pub fn init(alloc: Allocator, max_bytes: usize) Parser {
         var arena = ArenaAllocator.init(alloc);
         errdefer arena.deinit();
         var result: Parser = .{
@@ -64,6 +68,7 @@ pub const Parser = struct {
             .kv = .{},
             .kv_temp_len = 0,
             .kv_current = 0,
+            .max_bytes = max_bytes,
             .state = .control_key,
 
             .kv_temp = undefined,
@@ -84,7 +89,7 @@ pub const Parser = struct {
 
     /// Parse a complete command string.
     pub fn parseString(alloc: Allocator, data: []const u8) !Command {
-        var parser = init(alloc);
+        var parser = init(alloc, 1024 * 1024);
         defer parser.deinit();
         for (data) |c| try parser.feed(c);
         return try parser.complete(alloc);
@@ -137,7 +142,10 @@ pub const Parser = struct {
                 else => {},
             },
 
-            .data => try self.data.append(self.arena.child_allocator, c),
+            .data => {
+                if (self.data.items.len >= self.max_bytes) return error.OutOfMemory;
+                try self.data.append(self.arena.child_allocator, c);
+            },
         }
     }
 
@@ -964,7 +972,7 @@ pub const CompositionMode = enum {
 test "transmission command" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "f=24,s=10,v=20";
@@ -982,7 +990,7 @@ test "transmission command" {
 test "transmission ignores 'm' if medium is not direct" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=t,t=t,m=1";
@@ -999,7 +1007,7 @@ test "transmission ignores 'm' if medium is not direct" {
 test "transmission respects 'm' if medium is direct" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=t,t=d,m=1";
@@ -1016,7 +1024,7 @@ test "transmission respects 'm' if medium is direct" {
 test "query command" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "i=31,s=1,v=1,a=q,t=d,f=24;QUFBQQ";
@@ -1036,7 +1044,7 @@ test "query command" {
 test "display command" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=p,U=1,i=31,c=80,r=120";
@@ -1054,7 +1062,7 @@ test "display command" {
 test "delete command" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=d,d=p,x=3,y=4";
@@ -1074,7 +1082,7 @@ test "delete command" {
 test "no control data" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = ";QUFBQQ";
@@ -1089,7 +1097,7 @@ test "no control data" {
 test "ignore unknown keys (long)" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "f=24,s=10,v=20,hello=world";
@@ -1107,7 +1115,7 @@ test "ignore unknown keys (long)" {
 test "ignore very long values" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "f=24,s=10,v=2000000000000000000000000000000000000000";
@@ -1125,7 +1133,7 @@ test "ignore very long values" {
 test "ensure very large negative values don't get skipped" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=p,i=1,z=-2000000000";
@@ -1142,7 +1150,7 @@ test "ensure very large negative values don't get skipped" {
 test "ensure proper overflow error for u32" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=p,i=10000000000";
@@ -1153,7 +1161,7 @@ test "ensure proper overflow error for u32" {
 test "ensure proper overflow error for i32" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=p,i=1,z=-9999999999";
@@ -1167,7 +1175,7 @@ test "all i32 values" {
 
     {
         // 'z' (usually z-axis values)
-        var p = Parser.init(alloc);
+        var p = Parser.init(alloc, 1024 * 1024);
         defer p.deinit();
         const input = "a=p,i=1,z=-1";
         for (input) |c| try p.feed(c);
@@ -1182,7 +1190,7 @@ test "all i32 values" {
 
     {
         // 'H' (relative placement, horizontal offset)
-        var p = Parser.init(alloc);
+        var p = Parser.init(alloc, 1024 * 1024);
         defer p.deinit();
         const input = "a=p,i=1,H=-1";
         for (input) |c| try p.feed(c);
@@ -1197,7 +1205,7 @@ test "all i32 values" {
 
     {
         // 'V' (relative placement, vertical offset)
-        var p = Parser.init(alloc);
+        var p = Parser.init(alloc, 1024 * 1024);
         defer p.deinit();
         const input = "a=p,i=1,V=-1";
         for (input) |c| try p.feed(c);
@@ -1254,7 +1262,7 @@ test "response: encode with image ID and number" {
 test "delete range command 1" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=d,d=r,x=3,y=4";
@@ -1274,7 +1282,7 @@ test "delete range command 1" {
 test "delete range command 2" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=d,d=R,x=5,y=11";
@@ -1294,7 +1302,7 @@ test "delete range command 2" {
 test "delete range command 3" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=d,d=R,x=5,y=4";
@@ -1305,7 +1313,7 @@ test "delete range command 3" {
 test "delete range command 4" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=d,d=R,x=5";
@@ -1316,7 +1324,7 @@ test "delete range command 4" {
 test "delete range command 5" {
     const testing = std.testing;
     const alloc = testing.allocator;
-    var p = Parser.init(alloc);
+    var p = Parser.init(alloc, 1024 * 1024);
     defer p.deinit();
 
     const input = "a=d,d=R,y=5";