From 691fbccbfc111392a578696fb711ff66cad9fa4c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Feb 2025 20:40:06 -0800 Subject: [PATCH] terminal: increase CSI max params to 24 to accept Kakoune sequence See #5930 Kakoune sends a real SGR sequence with 17 parameters. Our previous max was 16 so we through away the entire sequence. This commit increases the max rather than fundamentally addressing limitations. Practically, it took us this long to witness a real world sequence that exceeded our previous limit. We may need to revisit this in the future, but this is an easy fix for now. In the future, as the comment states in this diff, we should probably look into a rare slow path where we heap allocate to accept up to some larger size (but still would need a cap to avoid DoS). For now, increasing to 24 slightly increases our memory usage but shouldn't result in any real world issues. --- src/terminal/Parser.zig | 76 +++++++++++++++++++++++++++++++++++++++-- src/terminal/sgr.zig | 62 +++++++++++++++++++++++++++++---- src/terminal/stream.zig | 2 +- 3 files changed, 131 insertions(+), 9 deletions(-) diff --git a/src/terminal/Parser.zig b/src/terminal/Parser.zig index a779c3350..bc5859ede 100644 --- a/src/terminal/Parser.zig +++ b/src/terminal/Parser.zig @@ -86,7 +86,9 @@ pub const Action = union(enum) { final: u8, /// The list of separators used for CSI params. The value of the - /// bit can be mapped to Sep. + /// bit can be mapped to Sep. The index of this bit set specifies + /// the separator AFTER that param. For example: 0;4:3 would have + /// index 1 set. pub const SepList = std.StaticBitSet(MAX_PARAMS); /// The separator used for CSI params. @@ -192,7 +194,19 @@ pub const Action = union(enum) { /// 4 because we also use the intermediates array for UTF8 decoding which /// can be at most 4 bytes. const MAX_INTERMEDIATE = 4; -const MAX_PARAMS = 16; + +/// Maximum number of CSI parameters. This is arbitrary. Practically, the +/// only CSI command that uses more than 3 parameters is the SGR command +/// which can be infinitely long. 24 is a reasonable limit based on empirical +/// data. This used to be 16 but Kakoune has a SGR command that uses 17 +/// parameters. +/// +/// We could in the future make this the static limit and then allocate after +/// but that's a lot more work and practically its so rare to exceed this +/// number. I implore TUI authors to not use more than this number of CSI +/// params, but I suspect we'll introduce a slow path with heap allocation +/// one day. +const MAX_PARAMS = 24; /// Current state of the state machine state: State = .ground, @@ -689,6 +703,64 @@ test "csi: SGR mixed colon and semicolon with blank" { } } +// This is from a Kakoune actual SGR sequence also. +test "csi: SGR mixed colon and semicolon setting underline, bg, fg" { + var p = init(); + _ = p.next(0x1B); + for ("[4:3;38;2;51;51;51;48;2;170;170;170;58;2;255;97;136") |c| { + const a = p.next(c); + try testing.expect(a[0] == null); + try testing.expect(a[1] == null); + try testing.expect(a[2] == null); + } + + { + const a = p.next('m'); + try testing.expect(p.state == .ground); + try testing.expect(a[0] == null); + try testing.expect(a[1].? == .csi_dispatch); + try testing.expect(a[2] == null); + + const d = a[1].?.csi_dispatch; + try testing.expect(d.final == 'm'); + try testing.expectEqual(17, d.params.len); + try testing.expectEqual(@as(u16, 4), d.params[0]); + try testing.expect(d.params_sep.isSet(0)); + try testing.expectEqual(@as(u16, 3), d.params[1]); + try testing.expect(!d.params_sep.isSet(1)); + try testing.expectEqual(@as(u16, 38), d.params[2]); + try testing.expect(!d.params_sep.isSet(2)); + try testing.expectEqual(@as(u16, 2), d.params[3]); + try testing.expect(!d.params_sep.isSet(3)); + try testing.expectEqual(@as(u16, 51), d.params[4]); + try testing.expect(!d.params_sep.isSet(4)); + try testing.expectEqual(@as(u16, 51), d.params[5]); + try testing.expect(!d.params_sep.isSet(5)); + try testing.expectEqual(@as(u16, 51), d.params[6]); + try testing.expect(!d.params_sep.isSet(6)); + try testing.expectEqual(@as(u16, 48), d.params[7]); + try testing.expect(!d.params_sep.isSet(7)); + try testing.expectEqual(@as(u16, 2), d.params[8]); + try testing.expect(!d.params_sep.isSet(8)); + try testing.expectEqual(@as(u16, 170), d.params[9]); + try testing.expect(!d.params_sep.isSet(9)); + try testing.expectEqual(@as(u16, 170), d.params[10]); + try testing.expect(!d.params_sep.isSet(10)); + try testing.expectEqual(@as(u16, 170), d.params[11]); + try testing.expect(!d.params_sep.isSet(11)); + try testing.expectEqual(@as(u16, 58), d.params[12]); + try testing.expect(!d.params_sep.isSet(12)); + try testing.expectEqual(@as(u16, 2), d.params[13]); + try testing.expect(!d.params_sep.isSet(13)); + try testing.expectEqual(@as(u16, 255), d.params[14]); + try testing.expect(!d.params_sep.isSet(14)); + try testing.expectEqual(@as(u16, 97), d.params[15]); + try testing.expect(!d.params_sep.isSet(15)); + try testing.expectEqual(@as(u16, 136), d.params[16]); + try testing.expect(!d.params_sep.isSet(16)); + } +} + test "csi: colon for non-m final" { var p = init(); _ = p.next(0x1B); diff --git a/src/terminal/sgr.zig b/src/terminal/sgr.zig index 52bfb2c31..2bc32c5f9 100644 --- a/src/terminal/sgr.zig +++ b/src/terminal/sgr.zig @@ -103,12 +103,16 @@ pub const Parser = struct { /// Next returns the next attribute or null if there are no more attributes. pub fn next(self: *Parser) ?Attribute { - if (self.idx > self.params.len) return null; + if (self.idx >= self.params.len) { + // If we're at index zero it means we must have an empty + // list and an empty list implicitly means unset. + if (self.idx == 0) { + // Add one to ensure we don't loop on unset + self.idx += 1; + return .unset; + } - // Implicitly means unset - if (self.params.len == 0) { - self.idx += 1; - return .unset; + return null; } const slice = self.params[self.idx..self.params.len]; @@ -788,7 +792,6 @@ test "sgr: direct fg colon with colorspace and extra param" { { const v = p.next().?; - std.log.warn("WHAT={}", .{v}); try testing.expect(v == .direct_color_fg); try testing.expectEqual(@as(u8, 1), v.direct_color_fg.r); try testing.expectEqual(@as(u8, 2), v.direct_color_fg.g); @@ -864,3 +867,50 @@ test "sgr: kakoune input" { //try testing.expect(p.next() == null); } + +// Discussion #5930, another input sent by kakoune +test "sgr: kakoune input issue underline, fg, and bg" { + // echo -e "\033[4:3;38;2;51;51;51;48;2;170;170;170;58;2;255;97;136mset everything in one sequence, broken\033[m" + + // This used to crash + var p: Parser = .{ + .params = &[_]u16{ 4, 3, 38, 2, 51, 51, 51, 48, 2, 170, 170, 170, 58, 2, 255, 97, 136 }, + .params_sep = sep: { + var list = SepList.initEmpty(); + list.set(0); + break :sep list; + }, + }; + + { + const v = p.next().?; + try testing.expect(v == .underline); + try testing.expectEqual(Attribute.Underline.curly, v.underline); + } + + { + const v = p.next().?; + try testing.expect(v == .direct_color_fg); + try testing.expectEqual(@as(u8, 51), v.direct_color_fg.r); + try testing.expectEqual(@as(u8, 51), v.direct_color_fg.g); + try testing.expectEqual(@as(u8, 51), v.direct_color_fg.b); + } + + { + const v = p.next().?; + try testing.expect(v == .direct_color_bg); + try testing.expectEqual(@as(u8, 170), v.direct_color_bg.r); + try testing.expectEqual(@as(u8, 170), v.direct_color_bg.g); + try testing.expectEqual(@as(u8, 170), v.direct_color_bg.b); + } + + { + const v = p.next().?; + try testing.expect(v == .underline_color); + try testing.expectEqual(@as(u8, 255), v.underline_color.r); + try testing.expectEqual(@as(u8, 97), v.underline_color.g); + try testing.expectEqual(@as(u8, 136), v.underline_color.b); + } + + try testing.expect(p.next() == null); +} diff --git a/src/terminal/stream.zig b/src/terminal/stream.zig index eb5ab2c65..3d9ed72fb 100644 --- a/src/terminal/stream.zig +++ b/src/terminal/stream.zig @@ -932,7 +932,7 @@ pub fn Stream(comptime Handler: type) type { // SGR - Select Graphic Rendition 'm' => switch (input.intermediates.len) { 0 => if (@hasDecl(T, "setAttribute")) { - // log.info("parse SGR params={any}", .{action.params}); + // log.info("parse SGR params={any}", .{input.params}); var p: sgr.Parser = .{ .params = input.params, .params_sep = input.params_sep,