From 861531a1fd510081195a64ca213b9ca26ddfdb01 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Wed, 25 Sep 2024 09:01:39 -0700 Subject: [PATCH] terminal/kitty: increase value buffer, make 'H' and 'V' i32 This commit increases the value buffer to 11 characters - this is technically what it should be to allow for large negative integers (i.e., 2.4 billion plus the sign character). Additionally corrected the types for 'H' (horizontal offset) and 'V' (vertical offset) - these are supposed to be i32 versus u32. Added some extra tests to test all of this stuff as well. I've also added a few more tests to help track these cases. --- src/terminal/apc.zig | 20 ++++ src/terminal/kitty/graphics_command.zig | 125 ++++++++++++++++++++---- src/terminal/kitty/graphics_exec.zig | 34 +++++++ 3 files changed, 161 insertions(+), 18 deletions(-) diff --git a/src/terminal/apc.zig b/src/terminal/apc.zig index 6a6b8cc36..26c59729a 100644 --- a/src/terminal/apc.zig +++ b/src/terminal/apc.zig @@ -122,6 +122,26 @@ test "garbage Kitty command" { try testing.expect(h.end() == null); } +test "Kitty command with overflow u32" { + const testing = std.testing; + const alloc = testing.allocator; + + var h: Handler = .{}; + h.start(); + for ("Ga=p,i=10000000000") |c| h.feed(alloc, c); + try testing.expect(h.end() == null); +} + +test "Kitty command with overflow i32" { + const testing = std.testing; + const alloc = testing.allocator; + + var h: Handler = .{}; + h.start(); + for ("Ga=p,i=1,z=-9999999999") |c| h.feed(alloc, c); + try testing.expect(h.end() == null); +} + test "valid Kitty command" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/kitty/graphics_command.zig b/src/terminal/kitty/graphics_command.zig index 0ef054293..d199711d3 100644 --- a/src/terminal/kitty/graphics_command.zig +++ b/src/terminal/kitty/graphics_command.zig @@ -23,10 +23,10 @@ pub const Parser = struct { /// This is the list of KV pairs that we're building up. kv: KV = .{}, - /// This is used as a buffer to store the key/value of a KV pair. - /// The value of a KV pair is at most a 32-bit integer which at most - /// is 10 characters (4294967295). - kv_temp: [10]u8 = undefined, + /// This is used as a buffer to store the key/value of a KV pair. The value + /// of a KV pair is at most a 32-bit integer which at most is 10 characters + /// (4294967295), plus one character for the sign bit on signed ints. + kv_temp: [11]u8 = undefined, kv_temp_len: u4 = 0, kv_current: u8 = 0, // Current kv key @@ -237,16 +237,14 @@ pub const Parser = struct { } } - // Only "z" is currently signed. This is a bit of a kloodge; if more - // fields become signed we can rethink this but for now we parse - // "z" as i32 then bitcast it to u32 then bitcast it back later. - if (self.kv_current == 'z') { - const v = try std.fmt.parseInt(i32, self.kv_temp[0..self.kv_temp_len], 10); - try self.kv.put(alloc, self.kv_current, @bitCast(v)); - } else { - const v = try std.fmt.parseInt(u32, self.kv_temp[0..self.kv_temp_len], 10); - try self.kv.put(alloc, self.kv_current, v); - } + // Handle integer fields, parsing signed fields accordingly. We still + // store the fields as u32 as they can be bitcast back later during + // building of the higher-level command tree. + const v: u32 = switch (self.kv_current) { + 'z', 'H', 'V' => @bitCast(try std.fmt.parseInt(i32, self.kv_temp[0..self.kv_temp_len], 10)), + else => try std.fmt.parseInt(u32, self.kv_temp[0..self.kv_temp_len], 10), + }; + try self.kv.put(alloc, self.kv_current, v); // Clear our temp buffer self.kv_temp_len = 0; @@ -505,8 +503,8 @@ pub const Display = struct { virtual_placement: bool = false, // U parent_id: u32 = 0, // P parent_placement_id: u32 = 0, // Q - horizontal_offset: u32 = 0, // H - vertical_offset: u32 = 0, // V + horizontal_offset: i32 = 0, // H + vertical_offset: i32 = 0, // V z: i32 = 0, // z pub const CursorMovement = enum { @@ -591,11 +589,13 @@ pub const Display = struct { } if (kv.get('H')) |v| { - result.horizontal_offset = v; + // We can bitcast here because of how we parse it earlier. + result.horizontal_offset = @bitCast(v); } if (kv.get('V')) |v| { - result.vertical_offset = v; + // We can bitcast here because of how we parse it earlier. + result.vertical_offset = @bitCast(v); } return result; @@ -1069,6 +1069,95 @@ test "ignore very long values" { try testing.expectEqual(@as(u32, 0), v.height); } +test "ensure very large negative values don't get skipped" { + const testing = std.testing; + const alloc = testing.allocator; + var p = Parser.init(alloc); + defer p.deinit(); + + const input = "a=p,i=1,z=-2000000000"; + for (input) |c| try p.feed(c); + const command = try p.complete(); + defer command.deinit(alloc); + + try testing.expect(command.control == .display); + const v = command.control.display; + try testing.expectEqual(1, v.image_id); + try testing.expectEqual(-2000000000, v.z); +} + +test "ensure proper overflow error for u32" { + const testing = std.testing; + const alloc = testing.allocator; + var p = Parser.init(alloc); + defer p.deinit(); + + const input = "a=p,i=10000000000"; + for (input) |c| try p.feed(c); + try testing.expectError(error.Overflow, p.complete()); +} + +test "ensure proper overflow error for i32" { + const testing = std.testing; + const alloc = testing.allocator; + var p = Parser.init(alloc); + defer p.deinit(); + + const input = "a=p,i=1,z=-9999999999"; + for (input) |c| try p.feed(c); + try testing.expectError(error.Overflow, p.complete()); +} + +test "all i32 values" { + const testing = std.testing; + const alloc = testing.allocator; + + { + // 'z' (usually z-axis values) + var p = Parser.init(alloc); + defer p.deinit(); + const input = "a=p,i=1,z=-1"; + for (input) |c| try p.feed(c); + const command = try p.complete(); + defer command.deinit(alloc); + + try testing.expect(command.control == .display); + const v = command.control.display; + try testing.expectEqual(1, v.image_id); + try testing.expectEqual(-1, v.z); + } + + { + // 'H' (relative placement, horizontal offset) + var p = Parser.init(alloc); + defer p.deinit(); + const input = "a=p,i=1,H=-1"; + for (input) |c| try p.feed(c); + const command = try p.complete(); + defer command.deinit(alloc); + + try testing.expect(command.control == .display); + const v = command.control.display; + try testing.expectEqual(1, v.image_id); + try testing.expectEqual(-1, v.horizontal_offset); + } + + { + // 'V' (relative placement, vertical offset) + var p = Parser.init(alloc); + defer p.deinit(); + const input = "a=p,i=1,V=-1"; + for (input) |c| try p.feed(c); + const command = try p.complete(); + defer command.deinit(alloc); + + try testing.expect(command.control == .display); + const v = command.control.display; + try testing.expectEqual(1, v.image_id); + try testing.expectEqual(-1, v.vertical_offset); + } +} + test "response: encode nothing without ID or image number" { const testing = std.testing; var buf: [1024]u8 = undefined; diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index c43bbbb9f..42f12ea07 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -495,3 +495,37 @@ test "kittygfx default format is rgba" { const img = storage.imageById(1).?; try testing.expectEqual(command.Transmission.Format.rgba, img.format); } + +test "kittygfx test valid u32 (expect invalid image ID)" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + const cmd = try command.Parser.parseString( + alloc, + "a=p,i=4294967295", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd).?; + try testing.expect(!resp.ok()); + try testing.expectEqual(resp.message, "ENOENT: image not found"); +} + +test "kittygfx test valid i32 (expect invalid image ID)" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + const cmd = try command.Parser.parseString( + alloc, + "a=p,i=1,z=-2147483648", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd).?; + try testing.expect(!resp.ok()); + try testing.expectEqual(resp.message, "ENOENT: image not found"); +}