From 81f7ae63b067c981ec9d1e85b7f44fc7ebc723ce Mon Sep 17 00:00:00 2001 From: Nameless Date: Thu, 19 Oct 2023 14:28:23 -0500 Subject: [PATCH 1/7] fuzz: src/terminal/stream.zig osc.zig: undefined pointer was dereferenced when warning was issued for handler missing Parser.zig: too many parameters was not handled in the final case Parser.zig: parameters being too long (>255 digits) was not handled --- src/terminal/Parser.zig | 6 ++++++ src/terminal/osc.zig | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/terminal/Parser.zig b/src/terminal/Parser.zig index 154dfee22..186b99608 100644 --- a/src/terminal/Parser.zig +++ b/src/terminal/Parser.zig @@ -373,6 +373,9 @@ fn doAction(self: *Parser, action: TransitionAction, c: u8) ?Action { break :param null; } + // Ignore parameters that are too long + if (self.param_acc_idx == std.math.maxInt(u8)) break :param null; + // A numeric value. Add it to our accumulator. if (self.param_acc_idx > 0) { self.param_acc *|= 10; @@ -388,6 +391,9 @@ fn doAction(self: *Parser, action: TransitionAction, c: u8) ?Action { break :osc_put null; }, .csi_dispatch => csi_dispatch: { + // Ignore too many parameters + if (self.params_idx >= MAX_PARAMS) break :csi_dispatch null; + // Finalize parameters if we have one if (self.param_acc_idx > 0) { self.params[self.params_idx] = self.param_acc; diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index 823f9ff67..fe5a72f2f 100644 --- a/src/terminal/osc.zig +++ b/src/terminal/osc.zig @@ -284,7 +284,7 @@ pub const Parser = struct { .@"0" => switch (c) { ';' => { - self.command = .{ .change_window_title = undefined }; + self.command = .{ .change_window_title = &.{} }; self.state = .string; self.temp_state = .{ .str = &self.command.change_window_title }; @@ -328,7 +328,7 @@ pub const Parser = struct { .@"2" => switch (c) { '2' => self.state = .@"22", ';' => { - self.command = .{ .change_window_title = undefined }; + self.command = .{ .change_window_title = &.{} }; self.state = .string; self.temp_state = .{ .str = &self.command.change_window_title }; @@ -339,7 +339,7 @@ pub const Parser = struct { .@"22" => switch (c) { ';' => { - self.command = .{ .mouse_shape = undefined }; + self.command = .{ .mouse_shape = .{ .value = &.{} } }; self.state = .string; self.temp_state = .{ .str = &self.command.mouse_shape.value }; @@ -366,7 +366,7 @@ pub const Parser = struct { .@"52" => switch (c) { ';' => { - self.command = .{ .clipboard_contents = undefined }; + self.command = .{ .clipboard_contents = .{ .kind = undefined, .data = &.{} } }; self.state = .clipboard_kind; }, else => self.state = .invalid, From 49f1866f2820a3f1325a1e2d0c97da7c17f87d5c Mon Sep 17 00:00:00 2001 From: Nameless Date: Thu, 19 Oct 2023 16:38:52 -0500 Subject: [PATCH 2/7] add tests for fuzzed results, clean up unimplemented osc warning --- src/Command.zig | 3 ++ src/terminal/osc.zig | 8 ++--- src/terminal/stream.zig | 72 ++++++++++++++++++++++++++++++++--------- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/src/Command.zig b/src/Command.zig index ce606f770..1f0e81bec 100644 --- a/src/Command.zig +++ b/src/Command.zig @@ -255,6 +255,9 @@ pub fn expandPath(alloc: Allocator, cmd: []const u8) !?[]u8 { path_buf[path_len] = 0; const full_path = path_buf[0..path_len :0]; + // Skip if this isn't an absolute path + if (!std.fs.path.isAbsolute(full_path)) continue; + // Stat it const f = std.fs.openFileAbsolute(full_path, .{}) catch |err| switch (err) { error.FileNotFound => continue, diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index fe5a72f2f..823f9ff67 100644 --- a/src/terminal/osc.zig +++ b/src/terminal/osc.zig @@ -284,7 +284,7 @@ pub const Parser = struct { .@"0" => switch (c) { ';' => { - self.command = .{ .change_window_title = &.{} }; + self.command = .{ .change_window_title = undefined }; self.state = .string; self.temp_state = .{ .str = &self.command.change_window_title }; @@ -328,7 +328,7 @@ pub const Parser = struct { .@"2" => switch (c) { '2' => self.state = .@"22", ';' => { - self.command = .{ .change_window_title = &.{} }; + self.command = .{ .change_window_title = undefined }; self.state = .string; self.temp_state = .{ .str = &self.command.change_window_title }; @@ -339,7 +339,7 @@ pub const Parser = struct { .@"22" => switch (c) { ';' => { - self.command = .{ .mouse_shape = .{ .value = &.{} } }; + self.command = .{ .mouse_shape = undefined }; self.state = .string; self.temp_state = .{ .str = &self.command.mouse_shape.value }; @@ -366,7 +366,7 @@ pub const Parser = struct { .@"52" => switch (c) { ';' => { - self.command = .{ .clipboard_contents = .{ .kind = undefined, .data = &.{} } }; + self.command = .{ .clipboard_contents = undefined }; self.state = .clipboard_kind; }, else => self.state = .invalid, diff --git a/src/terminal/stream.zig b/src/terminal/stream.zig index 1fda8c514..b2855797f 100644 --- a/src/terminal/stream.zig +++ b/src/terminal/stream.zig @@ -969,44 +969,53 @@ pub fn Stream(comptime Handler: type) type { .change_window_title => |title| { if (@hasDecl(T, "changeWindowTitle")) { try self.handler.changeWindowTitle(title); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .clipboard_contents => |clip| { if (@hasDecl(T, "clipboardContents")) { try self.handler.clipboardContents(clip.kind, clip.data); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .prompt_start => |v| { - if (@hasDecl(T, "promptStart")) switch (v.kind) { - .primary, .right => try self.handler.promptStart(v.aid, v.redraw), - .continuation => try self.handler.promptContinuation(v.aid), - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + if (@hasDecl(T, "promptStart")) { + switch (v.kind) { + .primary, .right => try self.handler.promptStart(v.aid, v.redraw), + .continuation => try self.handler.promptContinuation(v.aid), + } + return; + } }, .prompt_end => { if (@hasDecl(T, "promptEnd")) { try self.handler.promptEnd(); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .end_of_input => { if (@hasDecl(T, "endOfInput")) { try self.handler.endOfInput(); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .end_of_command => |end| { if (@hasDecl(T, "endOfCommand")) { try self.handler.endOfCommand(end.exit_code); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .report_pwd => |v| { if (@hasDecl(T, "reportPwd")) { try self.handler.reportPwd(v.value); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .mouse_shape => |v| { @@ -1017,19 +1026,25 @@ pub fn Stream(comptime Handler: type) type { }; try self.handler.setMouseShape(shape); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, .report_default_color => |v| { if (@hasDecl(T, "reportDefaultColor")) { try self.handler.reportDefaultColor(v.kind, v.terminator); - } else log.warn("unimplemented OSC callback: {}", .{cmd}); + return; + } }, - else => if (@hasDecl(T, "oscUnimplemented")) - try self.handler.oscUnimplemented(cmd) - else - log.warn("unimplemented OSC command: {}", .{cmd}), + else => {}, + } + + // Fall through for when we don't have a handler. + if (@hasDecl(T, "oscUnimplemented")) { + try self.handler.oscUnimplemented(cmd); + } else { + log.warn("unimplemented OSC command: {s}", .{@tagName(cmd)}); } } @@ -1598,3 +1613,28 @@ test "stream: insert characters" { for ("\x1B[?42@") |c| try s.next(c); try testing.expect(!s.handler.called); } + +test "stream: too many csi params" { + const H = struct { + pub fn setCursorRight(self: *@This(), v: u16) !void { + _ = v; + _ = self; + unreachable; + } + }; + + var s: Stream(H) = .{ .handler = .{} }; + try s.nextSlice("\x1B[1;1;1;1;1;1;1;1;1;1;1;1;1;1;1;1;1C"); +} + +test "stream: csi param too long" { + const H = struct { + pub fn setCursorRight(self: *@This(), v: u16) !void { + _ = v; + _ = self; + } + }; + + var s: Stream(H) = .{ .handler = .{} }; + try s.nextSlice("\x1B[1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111C"); +} From 41de71ae9e16d97f93fefa00802e9d167bb297aa Mon Sep 17 00:00:00 2001 From: Nameless Date: Wed, 25 Oct 2023 11:36:21 -0500 Subject: [PATCH 3/7] fuzz: termio.MessageData small_size is max of 255, not 256 --- src/apprt/surface.zig | 2 +- src/termio/message.zig | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/apprt/surface.zig b/src/apprt/surface.zig index c7ebc6ac8..58e3cea9b 100644 --- a/src/apprt/surface.zig +++ b/src/apprt/surface.zig @@ -9,7 +9,7 @@ const Config = @import("../config.zig").Config; pub const Message = union(enum) { /// Represents a write request. Magic number comes from the max size /// we want this union to be. - pub const WriteReq = termio.MessageData(u8, 256); + pub const WriteReq = termio.MessageData(u8, 255); /// Set the title of the surface. /// TODO: we should change this to a "WriteReq" style structure in diff --git a/src/termio/message.zig b/src/termio/message.zig index c7fe19976..d0212950e 100644 --- a/src/termio/message.zig +++ b/src/termio/message.zig @@ -84,6 +84,8 @@ pub const Message = union(enum) { /// are a stable pointer, or require deallocation. This is helpful for thread /// messaging utilities. pub fn MessageData(comptime Elem: type, comptime small_size: comptime_int) type { + assert(small_size <= std.math.maxInt(u8)); + return union(enum) { pub const Self = @This(); From b049cb7d21c48a5f18cae325208e71096f44d2d5 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 26 Oct 2023 09:30:11 -0700 Subject: [PATCH 4/7] command: allow relative paths in PATH --- src/Command.zig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Command.zig b/src/Command.zig index 1f0e81bec..f963a1857 100644 --- a/src/Command.zig +++ b/src/Command.zig @@ -255,11 +255,11 @@ pub fn expandPath(alloc: Allocator, cmd: []const u8) !?[]u8 { path_buf[path_len] = 0; const full_path = path_buf[0..path_len :0]; - // Skip if this isn't an absolute path - if (!std.fs.path.isAbsolute(full_path)) continue; - // Stat it - const f = std.fs.openFileAbsolute(full_path, .{}) catch |err| switch (err) { + const f = std.fs.cwd().openFile( + full_path, + .{}, + ) catch |err| switch (err) { error.FileNotFound => continue, error.AccessDenied => { // Accumulate this and return it later so we can try other From 2fd269369333e30895ce608c6e4f77b72db4a2fd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 26 Oct 2023 09:36:56 -0700 Subject: [PATCH 5/7] termio: MessageData should pick appropriately sized int for len --- src/termio/message.zig | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/termio/message.zig b/src/termio/message.zig index d0212950e..e2b98c4aa 100644 --- a/src/termio/message.zig +++ b/src/termio/message.zig @@ -84,16 +84,15 @@ pub const Message = union(enum) { /// are a stable pointer, or require deallocation. This is helpful for thread /// messaging utilities. pub fn MessageData(comptime Elem: type, comptime small_size: comptime_int) type { - assert(small_size <= std.math.maxInt(u8)); - return union(enum) { pub const Self = @This(); pub const Small = struct { pub const Max = small_size; pub const Array = [Max]Elem; + pub const Len = std.math.IntFittingRange(0, small_size); data: Array = undefined, - len: u8 = 0, + len: Len = 0, }; pub const Alloc = struct { @@ -184,3 +183,14 @@ test "MessageData init alloc" { try testing.expect(io == .alloc); io.alloc.alloc.free(io.alloc.data); } + +test "MessageData small fits non-u8 sized data" { + const testing = std.testing; + const alloc = testing.allocator; + + const len = 500; + const Data = MessageData(u8, len); + const input: []const u8 = "X" ** len; + const io = try Data.init(alloc, input); + try testing.expect(io == .small); +} From 99591f280b3ca68946d894a9380871e9eb65c6d4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 26 Oct 2023 09:50:29 -0700 Subject: [PATCH 6/7] terminal: addWithOverflow to detect max int --- src/terminal/Parser.zig | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/terminal/Parser.zig b/src/terminal/Parser.zig index 186b99608..785fd938e 100644 --- a/src/terminal/Parser.zig +++ b/src/terminal/Parser.zig @@ -373,15 +373,16 @@ fn doAction(self: *Parser, action: TransitionAction, c: u8) ?Action { break :param null; } - // Ignore parameters that are too long - if (self.param_acc_idx == std.math.maxInt(u8)) break :param null; - // A numeric value. Add it to our accumulator. if (self.param_acc_idx > 0) { self.param_acc *|= 10; } self.param_acc +|= c - '0'; - self.param_acc_idx += 1; + + // Increment our accumulator index. If we overflow then + // we're out of bounds and we exit immediately. + self.param_acc_idx, const overflow = @addWithOverflow(self.param_acc_idx, 1); + if (overflow > 0) break :param null; // The client is expected to perform no action. break :param null; @@ -910,3 +911,22 @@ test "csi followed by utf8" { try testing.expect(a[2] == null); } } + +test "csi: too many params" { + var p = init(); + _ = p.next(0x1B); + _ = p.next('['); + for (0..100) |_| { + _ = p.next('1'); + _ = p.next(';'); + } + _ = p.next('1'); + + { + const a = p.next('C'); + try testing.expect(p.state == .ground); + try testing.expect(a[0] == null); + try testing.expect(a[1] == null); + try testing.expect(a[2] == null); + } +} From 0baf3522b4651aeb2c10096a2468604c3af17036 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 26 Oct 2023 09:53:57 -0700 Subject: [PATCH 7/7] terminal: bring back unimplemented logs --- src/terminal/stream.zig | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/terminal/stream.zig b/src/terminal/stream.zig index b2855797f..4309db4f9 100644 --- a/src/terminal/stream.zig +++ b/src/terminal/stream.zig @@ -970,14 +970,14 @@ pub fn Stream(comptime Handler: type) type { if (@hasDecl(T, "changeWindowTitle")) { try self.handler.changeWindowTitle(title); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .clipboard_contents => |clip| { if (@hasDecl(T, "clipboardContents")) { try self.handler.clipboardContents(clip.kind, clip.data); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .prompt_start => |v| { @@ -987,35 +987,35 @@ pub fn Stream(comptime Handler: type) type { .continuation => try self.handler.promptContinuation(v.aid), } return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .prompt_end => { if (@hasDecl(T, "promptEnd")) { try self.handler.promptEnd(); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .end_of_input => { if (@hasDecl(T, "endOfInput")) { try self.handler.endOfInput(); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .end_of_command => |end| { if (@hasDecl(T, "endOfCommand")) { try self.handler.endOfCommand(end.exit_code); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .report_pwd => |v| { if (@hasDecl(T, "reportPwd")) { try self.handler.reportPwd(v.value); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .mouse_shape => |v| { @@ -1027,17 +1027,20 @@ pub fn Stream(comptime Handler: type) type { try self.handler.setMouseShape(shape); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, .report_default_color => |v| { if (@hasDecl(T, "reportDefaultColor")) { try self.handler.reportDefaultColor(v.kind, v.terminator); return; - } + } else log.warn("unimplemented OSC callback: {}", .{cmd}); }, - else => {}, + else => if (@hasDecl(T, "oscUnimplemented")) + try self.handler.oscUnimplemented(cmd) + else + log.warn("unimplemented OSC command: {}", .{cmd}), } // Fall through for when we don't have a handler.