From 56d3fd872e90d24c7ba75ef40a08a27852bd5570 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Wed, 27 Aug 2025 12:18:03 +0900 Subject: [PATCH 1/3] fix(terminal): improve CSI parameter parsing Make `MAX_PARAMS` public and increase CSI parameter limit from 16 to 24. Fix potential out-of-bounds read in SGR partial sequence extraction. --- src/terminal/Parser.zig | 2 +- src/terminal/sgr.zig | 2 +- src/terminal/stream.zig | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/terminal/Parser.zig b/src/terminal/Parser.zig index 0223545e5..92f405a23 100644 --- a/src/terminal/Parser.zig +++ b/src/terminal/Parser.zig @@ -206,7 +206,7 @@ const MAX_INTERMEDIATE = 4; /// 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; +pub const MAX_PARAMS = 24; /// Current state of the state machine state: State, diff --git a/src/terminal/sgr.zig b/src/terminal/sgr.zig index e4b85fbdd..d589172ad 100644 --- a/src/terminal/sgr.zig +++ b/src/terminal/sgr.zig @@ -134,7 +134,7 @@ pub const Parser = struct { self.idx += 1; return .{ .unknown = .{ .full = self.params, - .partial = slice[0 .. self.idx - start + 1], + .partial = slice[0..@min(self.idx - start + 1, slice.len)], } }; }, }; diff --git a/src/terminal/stream.zig b/src/terminal/stream.zig index ce09cbda2..c9bb50158 100644 --- a/src/terminal/stream.zig +++ b/src/terminal/stream.zig @@ -249,7 +249,7 @@ pub fn Stream(comptime Handler: type) type { // the parser state to ground. 0x18, 0x1A => self.parser.state = .ground, // A parameter digit: - '0'...'9' => if (self.parser.params_idx < 16) { + '0'...'9' => if (self.parser.params_idx < Parser.MAX_PARAMS) { self.parser.param_acc *|= 10; self.parser.param_acc +|= c - '0'; // The parser's CSI param action uses param_acc_idx @@ -259,7 +259,7 @@ pub fn Stream(comptime Handler: type) type { self.parser.param_acc_idx |= 1; }, // A parameter separator: - ':', ';' => if (self.parser.params_idx < 16) { + ':', ';' => if (self.parser.params_idx < Parser.MAX_PARAMS) { self.parser.params[self.parser.params_idx] = self.parser.param_acc; if (c == ':') self.parser.params_sep.set(self.parser.params_idx); self.parser.params_idx += 1; From a3f4997fbc75a487dd73dc57c2ba43cb2e324597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Wed, 27 Aug 2025 14:16:37 +0900 Subject: [PATCH 2/3] fix(terminal): handle CSI/SGR with many parameters Adds tests to ensure CSI and SGR sequences with 17 or more parameters are correctly parsed, fixing a bug where later parameters were previously dropped. --- src/terminal/Parser.zig | 25 +++++++++++++++++++++++++ src/terminal/stream.zig | 19 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/src/terminal/Parser.zig b/src/terminal/Parser.zig index 92f405a23..5fbf214a6 100644 --- a/src/terminal/Parser.zig +++ b/src/terminal/Parser.zig @@ -949,6 +949,31 @@ test "csi: too many params" { } } +test "csi: 17 parameters" { + // Test with exactly 17 parameters (the Kakoune case) + var p = init(); + _ = p.next(0x1B); + _ = p.next('['); + // Build 17 parameters separated by semicolons + for (0..16) |_| { + _ = p.next('1'); + _ = p.next(';'); + } + _ = p.next('2'); // 17th parameter + + { + const a = p.next('H'); + 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 csi = a[1].?.csi_dispatch; + try testing.expectEqual(@as(usize, 17), csi.params.len); + try testing.expectEqual(@as(u16, 2), csi.params[16]); + } +} + test "dcs: XTGETTCAP" { var p = init(); _ = p.next(0x1B); diff --git a/src/terminal/stream.zig b/src/terminal/stream.zig index c9bb50158..3009935ec 100644 --- a/src/terminal/stream.zig +++ b/src/terminal/stream.zig @@ -2601,3 +2601,22 @@ test "stream CSI ? W reset tab stops" { try s.nextSlice("\x1b[?1;2;3W"); try testing.expect(s.handler.reset); } + +test "stream: SGR with 17+ parameters for underline color" { + const H = struct { + attrs: ?sgr.Attribute = null, + called: bool = false, + + pub fn setAttribute(self: *@This(), attr: sgr.Attribute) !void { + self.attrs = attr; + self.called = true; + } + }; + + var s: Stream(H) = .init(.{}); + + // Kakoune-style SGR with underline color as 17th parameter + // This tests the fix where param 17 was being dropped + try s.nextSlice("\x1b[4:3;38;2;51;51;51;48;2;170;170;170;58;2;255;97;136;0m"); + try testing.expect(s.handler.called); +} From adfc93047c58102914e5be211469b7ac77514764 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 27 Aug 2025 07:15:39 -0700 Subject: [PATCH 3/3] terminal: fix up some tests to be more robust --- src/terminal/Parser.zig | 46 +++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/src/terminal/Parser.zig b/src/terminal/Parser.zig index 5fbf214a6..428274878 100644 --- a/src/terminal/Parser.zig +++ b/src/terminal/Parser.zig @@ -193,7 +193,7 @@ pub const Action = union(enum) { /// Maximum number of intermediate characters during parsing. This is /// 4 because we also use the intermediates array for UTF8 decoding which /// can be at most 4 bytes. -const MAX_INTERMEDIATE = 4; +pub const MAX_INTERMEDIATE = 4; /// Maximum number of CSI parameters. This is arbitrary. Practically, the /// only CSI command that uses more than 3 parameters is the SGR command @@ -949,28 +949,52 @@ test "csi: too many params" { } } -test "csi: 17 parameters" { - // Test with exactly 17 parameters (the Kakoune case) +test "csi: sgr with up to our max parameters" { + for (1..MAX_PARAMS + 1) |max| { + var p = init(); + _ = p.next(0x1B); + _ = p.next('['); + + for (0..max - 1) |_| { + _ = p.next('1'); + _ = p.next(';'); + } + _ = p.next('2'); + + { + const a = p.next('H'); + 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 csi = a[1].?.csi_dispatch; + try testing.expectEqual(@as(usize, max), csi.params.len); + try testing.expectEqual(@as(u16, 2), csi.params[max - 1]); + } + } +} + +test "csi: sgr beyond our max drops it" { + // Has to be +2 for the loops below + const max = MAX_PARAMS + 2; + var p = init(); _ = p.next(0x1B); _ = p.next('['); - // Build 17 parameters separated by semicolons - for (0..16) |_| { + + for (0..max - 1) |_| { _ = p.next('1'); _ = p.next(';'); } - _ = p.next('2'); // 17th parameter + _ = p.next('2'); { const a = p.next('H'); try testing.expect(p.state == .ground); try testing.expect(a[0] == null); - try testing.expect(a[1].? == .csi_dispatch); + try testing.expect(a[1] == null); try testing.expect(a[2] == null); - - const csi = a[1].?.csi_dispatch; - try testing.expectEqual(@as(usize, 17), csi.params.len); - try testing.expectEqual(@as(u16, 2), csi.params[16]); } }