From c5f4f00ed3e7c43528461968b8d1611f15f2bb39 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 20 Aug 2026 13:54:51 -0700 Subject: [PATCH] fix up recent Valgrind errors - ci: valgrind runs now fail when Memcheck finds an unsuppressed error. - fix real memory issues in unit tests - add a suppression for Zig's flate compression which documents as purposely using undefined memory --- build.zig | 2 ++ src/config/formatter_file.zig | 2 +- src/terminal/Screen.zig | 8 ++++++++ src/terminal/compress/Page.zig | 14 ++++++++++++++ src/terminal/stream_terminal.zig | 28 ++++++++++++++++++++-------- valgrind.supp | 9 +++++++++ 6 files changed, 54 insertions(+), 9 deletions(-) diff --git a/build.zig b/build.zig index 5ce7e816b..41ac5f04c 100644 --- a/build.zig +++ b/build.zig @@ -326,6 +326,7 @@ pub fn build(b: *std.Build) !void { const run_cmd = b.addSystemCommand(&.{ "valgrind", "--leak-check=full", + "--error-exitcode=1", "--num-callers=50", b.fmt("--suppressions={s}", .{b.pathFromRoot("valgrind.supp")}), "--gen-suppressions=all", @@ -392,6 +393,7 @@ pub fn build(b: *std.Build) !void { const valgrind_run = b.addSystemCommand(&.{ "valgrind", "--leak-check=full", + "--error-exitcode=1", "--num-callers=50", b.fmt("--suppressions={s}", .{b.pathFromRoot("valgrind.supp")}), "--gen-suppressions=all", diff --git a/src/config/formatter_file.zig b/src/config/formatter_file.zig index 37beb7b54..71e8004e6 100644 --- a/src/config/formatter_file.zig +++ b/src/config/formatter_file.zig @@ -41,7 +41,7 @@ pub const FileFormatter = struct { if (field.name[0] == '_') continue; const value = @field(self.config, field.name); - const do_format = if (default) |d| format: { + const do_format = if (default) |*d| format: { const key = @field(Key, field.name); break :format d.changed(self.config, key); } else true; diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 29305cbc6..aa9d79bf7 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -7794,6 +7794,14 @@ test "Screen: resize errors preserve state" { try testing.expectEqual(before.pages.viewport, s.pages.viewport); try testing.expectEqual(before_viewport_pin, s.pages.viewport_pin.*); try testing.expectEqual(before_tracked_pins, s.pages.countTrackedPins()); + if (std.valgrind.runningOnValgrind() > 0) { + // This assertion deliberately compares the complete raw page, + // including semantically irrelevant struct padding. + std.valgrind.memcheck.makeMemDefined(before_page); + std.valgrind.memcheck.makeMemDefined( + s.pages.pages.first.?.page().memory, + ); + } try testing.expectEqualSlices( u8, before_page, diff --git a/src/terminal/compress/Page.zig b/src/terminal/compress/Page.zig index b7ad80256..8eb9caabc 100644 --- a/src/terminal/compress/Page.zig +++ b/src/terminal/compress/Page.zig @@ -106,6 +106,15 @@ pub fn init( if (scratch.len < required) return error.OutputTooSmall; if (required == 0) return null; + // Page memory is an opaque snapshot containing structs whose padding has + // no semantic value. Assigning those structs can leave their padding + // undefined in Memcheck even though every byte is addressable and must be + // preserved by compression. Mark the complete byte representation as + // defined at this boundary so that undefined padding does not propagate + // through the codec and obscure real Valgrind failures. + if (std.valgrind.runningOnValgrind() > 0) + std.valgrind.memcheck.makeMemDefined(source.memory); + const encoded_len = lz4.compress( source.memory, scratch[0..required], @@ -209,6 +218,11 @@ test "compressed Page retained mapping round trip" { const expected = try testing.allocator.dupe(u8, resident.memory); defer testing.allocator.free(expected); + if (std.valgrind.runningOnValgrind() > 0) { + // This snapshot deliberately includes semantically irrelevant struct + // padding so the round trip can compare the complete representation. + std.valgrind.memcheck.makeMemDefined(expected); + } const memory_ptr = resident.memory.ptr; const memory_len = resident.memory.len; diff --git a/src/terminal/stream_terminal.zig b/src/terminal/stream_terminal.zig index 5b6f08b2b..1b1ad6c65 100644 --- a/src/terminal/stream_terminal.zig +++ b/src/terminal/stream_terminal.zig @@ -2811,9 +2811,12 @@ test "kitty_keyboard_query" { defer t.deinit(testing.allocator); const S = struct { - var written: ?[:0]const u8 = null; + var written: ?[]const u8 = null; + var written_buf: [64]u8 = undefined; fn writePty(_: *Handler, data: [:0]const u8) void { - written = data; + std.debug.assert(data.len <= written_buf.len); + @memcpy(written_buf[0..data.len], data); + written = written_buf[0..data.len]; } }; S.written = null; @@ -2840,9 +2843,12 @@ test "xtversion default" { defer t.deinit(testing.allocator); const S = struct { - var written: ?[:0]const u8 = null; + var written: ?[]const u8 = null; + var written_buf: [64]u8 = undefined; fn writePty(_: *Handler, data: [:0]const u8) void { - written = data; + std.debug.assert(data.len <= written_buf.len); + @memcpy(written_buf[0..data.len], data); + written = written_buf[0..data.len]; } }; S.written = null; @@ -2863,9 +2869,12 @@ test "xtversion with effect" { defer t.deinit(testing.allocator); const S = struct { - var written: ?[:0]const u8 = null; + var written: ?[]const u8 = null; + var written_buf: [64]u8 = undefined; fn writePty(_: *Handler, data: [:0]const u8) void { - written = data; + std.debug.assert(data.len <= written_buf.len); + @memcpy(written_buf[0..data.len], data); + written = written_buf[0..data.len]; } fn xtversion(_: *Handler) []const u8 { return "ghostty 1.2.3"; @@ -2889,9 +2898,12 @@ test "xtversion with empty string effect" { defer t.deinit(testing.allocator); const S = struct { - var written: ?[:0]const u8 = null; + var written: ?[]const u8 = null; + var written_buf: [64]u8 = undefined; fn writePty(_: *Handler, data: [:0]const u8) void { - written = data; + std.debug.assert(data.len <= written_buf.len); + @memcpy(written_buf[0..data.len], data); + written = written_buf[0..data.len]; } fn xtversion(_: *Handler) []const u8 { return ""; diff --git a/valgrind.supp b/valgrind.supp index 27479fd5c..ea1d7232b 100644 --- a/valgrind.supp +++ b/valgrind.supp @@ -13,6 +13,15 @@ # You must gracefully exit Ghostty (do not SIGINT) by closing all windows # and quitting. Otherwise, we leave a number of GTK resources around. +# Zig's DEFLATE encoder intentionally increments an undefined base[0] entry +# for unused Huffman symbols. The corresponding output code is also unused; +# see the comment in std.compress.flate.Compress.huffman.buildValues. +{ + Zig std DEFLATE unused Huffman code + Memcheck:Cond + fun:compress.flate.Compress.huffman.buildValues +} + # Reproduction: # 1. Launch Ghostty (no config)