From a2ef0ca75108c4be9ba495419823345cab5b442a Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Mon, 19 Aug 2024 00:15:36 -0500 Subject: [PATCH] Address review comments. - Cap the total number of requests at twice the maximum number of keys (currently 263, so 526 requests). Basically you can set and then query every key in one message. This is an absurdly high number but should prevent serious DOS attacks. - Clarify meaning of new hex color codes. - Better handle sending messages to the renderer in a way that should prevent deadlocks. - Handle 0-255 palette color requests by creatively using non-exhautive enums. - Fix an error in the query reply. --- src/terminal/color.zig | 6 +- src/terminal/osc.zig | 74 +++++++++++++++++---- src/termio/stream_handler.zig | 119 ++++++++++++++++++++++++++-------- 3 files changed, 155 insertions(+), 44 deletions(-) diff --git a/src/terminal/color.zig b/src/terminal/color.zig index c8929b060..46aa2aaae 100644 --- a/src/terminal/color.zig +++ b/src/terminal/color.zig @@ -208,9 +208,11 @@ pub const RGB = struct { /// where , , and are floating point values between /// 0.0 and 1.0 (inclusive). /// - /// 3. #hhh, #hhhhhh, #hhhhhhhhh #hhhhhhhhhhhh + /// 3. #rgb, #rrggbb, #rrrgggbbb #rrrrggggbbbb /// - /// where `h` is a single hexadecimal digit. + /// where `r`, `g`, and `b` are a single hexadecimal digit. + /// These specifiy a color with 4, 8, 12, and 16 bits of precision + /// per color channel. pub fn parse(value: []const u8) !RGB { if (value.len == 0) { return error.InvalidFormat; diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index 89bd8e976..fd3917d60 100644 --- a/src/terminal/osc.zig +++ b/src/terminal/osc.zig @@ -174,16 +174,24 @@ pub const Command = union(enum) { }; pub const KittyColorProtocol = struct { - const Kind = enum { - foreground, - background, - selection_foreground, - selection_background, - cursor, - cursor_text, - visual_bell, - second_transparent_background, + const Kind = enum(u9) { + // These _must_ start at 256 since enum values 0-255 are reserved + // for the palette. + foreground = 256, + background = 257, + selection_foreground = 258, + selection_background = 259, + cursor = 260, + cursor_text = 261, + visual_bell = 262, + second_transparent_background = 263, + _, + + // Make sure that this stays in sync with the higest numbered enum + // value. + const max: u9 = 263; }; + const Request = union(enum) { query: Kind, set: struct { @@ -1024,9 +1032,18 @@ pub const Parser = struct { return; } - const key = std.meta.stringToEnum(Command.KittyColorProtocol.Kind, self.temp_state.key) orelse { - log.warn("unknown key in kitty color protocol: {s}", .{self.temp_state.key}); - return; + const key = key: { + break :key std.meta.stringToEnum(Command.KittyColorProtocol.Kind, self.temp_state.key) orelse { + const v = std.fmt.parseUnsigned(u9, self.temp_state.key, 10) catch { + log.warn("unknown key in kitty color protocol: {s}", .{self.temp_state.key}); + return; + }; + if (v > 255) { + log.warn("unknown key in kitty color protocol: {s}", .{self.temp_state.key}); + return; + } + break :key @as(Command.KittyColorProtocol.Kind, @enumFromInt(v)); + }; }; const value = value: { @@ -1037,6 +1054,11 @@ pub const Parser = struct { switch (self.command) { .kitty_color_protocol => |*v| { + if (v.list.items.len >= @as(usize, Command.KittyColorProtocol.Kind.max) * 2) { + self.state = .invalid; + log.warn("exceeded limit for number of keys in kitty color protocol, ignoring", .{}); + return; + } if (kind == .key_only) { v.list.append(.{ .reset = key }) catch |err| { log.warn("unable to append kitty color protocol option: {}", .{err}); @@ -1651,12 +1673,12 @@ test "OSC: kitty color protocol" { var p: Parser = .{ .alloc = testing.allocator }; defer p.deinit(); - const input = "21;foreground=?;background=rgb:f0/f8/ff;cursor=aliceblue;cursor_text;visual_bell=;selection_foreground=#xxxyyzz;selection_background=?;selection_background=#aabbcc"; + const input = "21;foreground=?;background=rgb:f0/f8/ff;cursor=aliceblue;cursor_text;visual_bell=;selection_foreground=#xxxyyzz;selection_background=?;selection_background=#aabbcc;2=?;3=rgbi:1.0/1.0/1.0"; for (input) |ch| p.next(ch); const cmd = p.end('\x1b').?; try testing.expect(cmd == .kitty_color_protocol); - try testing.expectEqual(@as(usize, 7), cmd.kitty_color_protocol.list.items.len); + try testing.expectEqual(@as(usize, 9), cmd.kitty_color_protocol.list.items.len); try testing.expect(cmd.kitty_color_protocol.list.items[0] == .query); try testing.expectEqual(@as(Command.KittyColorProtocol.Kind, .foreground), cmd.kitty_color_protocol.list.items[0].query); try testing.expect(cmd.kitty_color_protocol.list.items[1] == .set); @@ -1680,4 +1702,28 @@ test "OSC: kitty color protocol" { try testing.expectEqual(@as(u8, 0xaa), cmd.kitty_color_protocol.list.items[6].set.color.r); try testing.expectEqual(@as(u8, 0xbb), cmd.kitty_color_protocol.list.items[6].set.color.g); try testing.expectEqual(@as(u8, 0xcc), cmd.kitty_color_protocol.list.items[6].set.color.b); + try testing.expect(cmd.kitty_color_protocol.list.items[7] == .query); + try testing.expectEqual(@as(Command.KittyColorProtocol.Kind, @enumFromInt(2)), cmd.kitty_color_protocol.list.items[7].query); + try testing.expect(cmd.kitty_color_protocol.list.items[8] == .set); + try testing.expectEqual(@as(Command.KittyColorProtocol.Kind, @enumFromInt(3)), cmd.kitty_color_protocol.list.items[8].set.key); + try testing.expectEqual(@as(u8, 0xff), cmd.kitty_color_protocol.list.items[8].set.color.r); + try testing.expectEqual(@as(u8, 0xff), cmd.kitty_color_protocol.list.items[8].set.color.g); + try testing.expectEqual(@as(u8, 0xff), cmd.kitty_color_protocol.list.items[8].set.color.b); +} + +test "OSC: kitty color protocol kind" { + const info = @typeInfo(Command.KittyColorProtocol.Kind); + + try std.testing.expectEqual(false, info.Enum.is_exhaustive); + + var min: usize = std.math.maxInt(info.Enum.tag_type); + var max: usize = 0; + + inline for (info.Enum.fields) |field| { + if (field.value > max) max = field.value; + if (field.value < min) min = field.value; + } + + try std.testing.expect(min >= 256); + try std.testing.expect(max == Command.KittyColorProtocol.Kind.max); } diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index b3f31e4b0..2299c6e47 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -1279,73 +1279,136 @@ pub const StreamHandler = struct { for (request.list.items) |item| { switch (item) { .query => |key| { + const i = @intFromEnum(key); const color = switch (key) { .foreground => self.foreground_color, .background => self.background_color, .cursor => self.cursor_color, - else => { - log.warn("ignoring unsupported kitty color protocol key: {s}", .{@tagName(key)}); - continue; + else => color: { + if (i > 255) { + log.warn("ignoring unsupported kitty color protocol key: {s}", .{@tagName(key)}); + continue; + } + break :color self.terminal.color_palette.colors[i]; }, } orelse { log.warn("no color configured for: {s}", .{@tagName(key)}); continue; }; - try writer.print( - ";rgb:{x:0>2}/{x:0>2}/{x:0>2}", - .{ - @as(u16, color.r), - @as(u16, color.g), - @as(u16, color.b), - }, - ); + if (i <= 255) + try writer.print( + ";{d}=rgb:{x:0>2}/{x:0>2}/{x:0>2}", + .{ i, color.r, color.g, color.b }, + ) + else + try writer.print( + ";{s}=rgb:{x:0>2}/{x:0>2}/{x:0>2}", + .{ @tagName(key), color.r, color.g, color.b }, + ); }, .set => |v| switch (v.key) { .foreground => { self.foreground_color = v.color; - _ = self.renderer_mailbox.push(.{ + // See messageWriter which has similar logic and + // explains why we may have to do this. + const msg: renderer.Message = .{ .foreground_color = v.color, - }, .{ .forever = {} }); + }; + if (self.renderer_mailbox.push(msg, .{ .instant = {} }) == 0) { + self.renderer_state.mutex.unlock(); + defer self.renderer_state.mutex.lock(); + _ = self.renderer_mailbox.push(msg, .{ .forever = {} }); + } }, .background => { self.background_color = v.color; + // See messageWriter which has similar logic and + // explains why we may have to do this. + const msg: renderer.Message = .{ + .background_color = v.color, + }; + if (self.renderer_mailbox.push(msg, .{ .instant = {} }) == 0) { + self.renderer_state.mutex.unlock(); + defer self.renderer_state.mutex.lock(); + _ = self.renderer_mailbox.push(msg, .{ .forever = {} }); + } _ = self.renderer_mailbox.push(.{ .background_color = v.color, }, .{ .forever = {} }); }, .cursor => { self.cursor_color = v.color; - _ = self.renderer_mailbox.push(.{ + // See messageWriter which has similar logic and + // explains why we may have to do this. + const msg: renderer.Message = .{ .cursor_color = v.color, - }, .{ .forever = {} }); + }; + if (self.renderer_mailbox.push(msg, .{ .instant = {} }) == 0) { + self.renderer_state.mutex.unlock(); + defer self.renderer_state.mutex.lock(); + _ = self.renderer_mailbox.push(msg, .{ .forever = {} }); + } }, else => { - log.warn("ignoring unsupported kitty color protocol key: {s}", .{@tagName(v.key)}); - continue; + const i = @intFromEnum(v.key); + if (i > 255) { + log.warn("ignoring unsupported kitty color protocol key: {s}", .{@tagName(v.key)}); + continue; + } + self.terminal.flags.dirty.palette = true; + self.terminal.color_palette.colors[i] = v.color; + self.terminal.color_palette.mask.unset(i); }, }, .reset => |key| switch (key) { .foreground => { self.foreground_color = self.default_foreground_color; - _ = self.renderer_mailbox.push(.{ - .foreground_color = self.foreground_color, - }, .{ .forever = {} }); + // See messageWriter which has similar logic and + // explains why we may have to do this. + const msg: renderer.Message = .{ + .foreground_color = self.default_foreground_color, + }; + if (self.renderer_mailbox.push(msg, .{ .instant = {} }) == 0) { + self.renderer_state.mutex.unlock(); + defer self.renderer_state.mutex.lock(); + _ = self.renderer_mailbox.push(msg, .{ .forever = {} }); + } }, .background => { self.background_color = self.default_background_color; - _ = self.renderer_mailbox.push(.{ - .background_color = self.background_color, - }, .{ .forever = {} }); + // See messageWriter which has similar logic and + // explains why we may have to do this. + const msg: renderer.Message = .{ + .background_color = self.default_background_color, + }; + if (self.renderer_mailbox.push(msg, .{ .instant = {} }) == 0) { + self.renderer_state.mutex.unlock(); + defer self.renderer_state.mutex.lock(); + _ = self.renderer_mailbox.push(msg, .{ .forever = {} }); + } }, .cursor => { self.cursor_color = self.default_cursor_color; - _ = self.renderer_mailbox.push(.{ - .cursor_color = self.cursor_color, - }, .{ .forever = {} }); + // See messageWriter which has similar logic and + // explains why we may have to do this. + const msg: renderer.Message = .{ + .cursor_color = self.default_cursor_color, + }; + if (self.renderer_mailbox.push(msg, .{ .instant = {} }) == 0) { + self.renderer_state.mutex.unlock(); + defer self.renderer_state.mutex.lock(); + _ = self.renderer_mailbox.push(msg, .{ .forever = {} }); + } }, else => { - log.warn("ignoring unsupported kitty color protocol key: {s}", .{@tagName(key)}); - continue; + const i = @intFromEnum(key); + if (i > 255) { + log.warn("ignoring unsupported kitty color protocol key: {s}", .{@tagName(key)}); + continue; + } + self.terminal.flags.dirty.palette = true; + self.terminal.color_palette.colors[i] = self.terminal.default_palette[i]; + self.terminal.color_palette.mask.unset(i); }, }, }