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

@@ -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);

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
+            return null;
-        if (self.params.len == 0) {
-            self.idx += 1;
-            return .unset;
         }
 
         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);
+}

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,