From 9521bbcb95b59fe21f729abdba4852b88424af0c Mon Sep 17 00:00:00 2001 From: zclawz Date: Sat, 9 May 2026 09:05:03 +0000 Subject: [PATCH 1/2] snappy: fix uvarint integer overflow on >9-byte continuation runs A malformed snappy header that consists of 10 or more continuation bytes (byte >= 0x80) walks the loop in uvarint() past the u6 shift counter. After 9 iterations s == 63, and on the 10th iteration s += 7 panics with `integer overflow` in safe builds. Triggered in zeam by the Hive `gossip: ignores malformed ssz` test (1024 bytes of 0xef on a valid gossip topic). Reject malformed varints as soon as we see continuation byte 10 by returning the corrupt sentinel (`bytesRead == 0`) instead of pushing the shift counter further. Also tighten decodedLen() to compare against 0 explicitly (the previous `<= 0` on a usize was a no-op for the wrap- around sentinel that the old code tried to use). --- snappy.zig | 72 +++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/snappy.zig b/snappy.zig index d307413..6673276 100644 --- a/snappy.zig +++ b/snappy.zig @@ -53,22 +53,31 @@ const Varint = struct { // https://golang.org/pkg/encoding/binary/#Uvarint fn uvarint(buf: []const u8) Varint { + // A 64-bit varint occupies at most 10 bytes; a well-formed terminator on the + // 10th byte must encode at most 1 bit of data. Anything else is malformed and + // would overflow a u64. Returning `bytesRead == 0` signals "corrupt or + // incomplete" to callers (matches the existing `decodedLen` check). var x: u64 = 0; var s: u6 = 0; // We can shift a maximum of 2^6 (64) times. for (buf, 0..) |b, i| { if (b < 0x80) { - if (i > 9 or i == 9 and b > 1) { - return Varint{ - .value = 0, - .bytesRead = -%i + 1, - }; + if (i > 9 or (i == 9 and b > 1)) { + // 10th byte sets bits beyond u64 — overflow. + return Varint{ .value = 0, .bytesRead = 0 }; } return Varint{ .value = x | (@as(u64, b) << s), .bytesRead = i + 1, }; } + // After 10 continuation bytes a varint can no longer be valid (would + // exceed 64 bits). Reject before `s += 7` overflows the u6 shift count. + // Without this guard, 10+ bytes of 0x80+ (e.g. 1024 bytes of 0xef from + // a malformed gossip payload) panic with integer overflow. + if (i >= 9) { + return Varint{ .value = 0, .bytesRead = 0 }; + } x |= (@as(u64, b & 0x7f) << s); s += 7; } @@ -103,7 +112,9 @@ const SnappyBlock = struct { // Return the length of the decoded block and the number of bytes that the header occupied. fn decodedLen(src: []const u8) !SnappyBlock { const varint = uvarint(src); - if (varint.bytesRead <= 0 or varint.value > 0xffffffff) { + // `bytesRead == 0` means uvarint either ran out of input without a terminator + // or detected a u64 overflow; both are corrupt headers. + if (varint.bytesRead == 0 or varint.value > 0xffffffff) { return SnappyError.Corrupt; } @@ -508,6 +519,55 @@ test "decoding variable integers" { try testing.expect(case2.bytesRead == 3); } +test "uvarint rejects long continuation runs without overflow" { + // Regression: 1024 bytes of 0xef previously panicked with `integer overflow` + // because `s += 7` walked past the u6 shift count. Must now return the + // corrupt sentinel (`bytesRead == 0`) instead of panicking. + const garbage = [_]u8{0xef} ** 1024; + const v = uvarint(&garbage); + try testing.expectEqual(@as(usize, 0), v.bytesRead); + try testing.expectEqual(@as(u64, 0), v.value); + + // Exactly 10 continuation bytes is also malformed (no terminator at all). + const ten_continuations = [_]u8{0xff} ** 10; + const v2 = uvarint(&ten_continuations); + try testing.expectEqual(@as(usize, 0), v2.bytesRead); + + // 11 continuation bytes followed by a valid terminator: still malformed, + // because the varint exceeded the 10-byte u64 budget before terminating. + var eleven: [12]u8 = undefined; + @memset(eleven[0..11], 0xff); + eleven[11] = 0x01; + const v3 = uvarint(&eleven); + try testing.expectEqual(@as(usize, 0), v3.bytesRead); + + // Boundary: 10th byte with high bit clear and value > 1 is overflow. + var ten_overflow = [_]u8{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x02 }; + const v4 = uvarint(&ten_overflow); + try testing.expectEqual(@as(usize, 0), v4.bytesRead); + + // Boundary: 10th byte with value == 1 is the largest valid u64. + var max_u64 = [_]u8{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01 }; + const v5 = uvarint(&max_u64); + try testing.expectEqual(@as(usize, 10), v5.bytesRead); + try testing.expectEqual(@as(u64, std.math.maxInt(u64)), v5.value); + + // Empty buffer is incomplete, not corrupt-by-overflow, but still signals + // bytesRead == 0 (no data consumed). + const empty = [_]u8{}; + const v6 = uvarint(&empty); + try testing.expectEqual(@as(usize, 0), v6.bytesRead); +} + +test "decode rejects garbage with long continuation run" { + // Regression for blockblaz/zeam Hive `gossip: ignores malformed ssz`: + // 1024 bytes of 0xef on a valid gossip topic must not panic the client. + const allocator = testing.allocator; + const garbage = [_]u8{0xef} ** 1024; + try testing.expectError(SnappyError.Corrupt, decode(allocator, &garbage)); + try testing.expectError(SnappyError.Corrupt, decodeWithMax(allocator, &garbage, 16 * 1024 * 1024)); +} + test "simple encode" { const allocator = testing.allocator; From a01e5034780e2f4ddebd9f69757a71fbd5ee0c9a Mon Sep 17 00:00:00 2001 From: zclawz Date: Sat, 9 May 2026 10:04:17 +0000 Subject: [PATCH 2/2] address review feedback: tighten loop bound, doc Varint sentinel, expand tests Changes from review on PR #9: uvarint: - Restructure loop to iterate buf[0..@min(len,10)] so the 10-byte budget is encoded in the loop bound itself; `s += 7` can no longer push s:u6 past 63 by construction. - Replace `i >= 9` continuation-bit-still-set guard with `i == 9` (loop now cannot reach i == 10). - Move struct-level rationale onto Varint with explicit doc that `bytesRead == 0` is overloaded across truncated and malformed cases, and what to do if uvarint is ever made pub (return ?Varint or isize byte count, matching Go's encoding/binary.Uvarint convention). - Strip the gossip-payload reference from the library function comment; downstream context lives in the test comment. - Multi-line all early-return Varint{} literals to match the existing multi-line style. decodedLen: - Revert the `<= 0` -> `== 0` clarification. For usize the two are identical and the original PR description framed it as a tightening, which is misleading. Tests: - Symmetric `expectEqual(@as(u64, 0), vN.value)` on every malformed sub-case for parity with the bytesRead asserts. - New: 9 continuation bytes with no 10th byte (truncated input falls through to the post-loop return, distinct from the i == 9 guard). - New: non-canonical `[0x80, 0x00]` encoding of zero. Pins current behaviour (varint format does not require minimal encoding) so a future canonicalisation tightening is a deliberate breaking change. --- snappy.zig | 130 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 42 deletions(-) diff --git a/snappy.zig b/snappy.zig index 6673276..a29f050 100644 --- a/snappy.zig +++ b/snappy.zig @@ -46,42 +46,64 @@ pub fn crc(b: []const u8) u32 { // Represents a variable length integer that we read from a byte stream along with how many bytes // were read to decode it. +// +// `bytesRead == 0` is overloaded: it signals both "buffer ran out before a +// terminator" (caller may need more input) and "varint overflows u64" +// (definitely malformed). The Go reference (`encoding/binary.Uvarint`) +// distinguishes these via a negative byte count, but `bytesRead: usize` +// can't carry that signal. The current sole caller (`decodedLen`) treats +// both as `SnappyError.Corrupt`, which is correct for a block-format +// header. If `uvarint` is ever made `pub`, switch to `?Varint` or an +// `isize` byte count so callers can tell the two apart. const Varint = struct { value: u64, bytesRead: usize, }; // https://golang.org/pkg/encoding/binary/#Uvarint +// +// A 64-bit varint occupies at most 10 bytes (9 * 7 = 63 data bits plus 1 +// bit on the 10th byte). The loop bound encodes that budget directly so +// `s += 7` can never push `s: u6` past 63. Anything beyond 10 bytes, or a +// 10th byte whose value sets bits above bit 63, is malformed. fn uvarint(buf: []const u8) Varint { - // A 64-bit varint occupies at most 10 bytes; a well-formed terminator on the - // 10th byte must encode at most 1 bit of data. Anything else is malformed and - // would overflow a u64. Returning `bytesRead == 0` signals "corrupt or - // incomplete" to callers (matches the existing `decodedLen` check). var x: u64 = 0; var s: u6 = 0; // We can shift a maximum of 2^6 (64) times. - for (buf, 0..) |b, i| { + const max_varint_bytes = 10; + const window = buf[0..@min(buf.len, max_varint_bytes)]; + for (window, 0..) |b, i| { if (b < 0x80) { - if (i > 9 or (i == 9 and b > 1)) { - // 10th byte sets bits beyond u64 — overflow. - return Varint{ .value = 0, .bytesRead = 0 }; + // On the 10th byte (i == 9) with continuation bit clear, the + // value must be 0 or 1; anything larger sets bits above 63 and + // overflows u64. + if (i == 9 and b > 1) { + return Varint{ + .value = 0, + .bytesRead = 0, + }; } return Varint{ .value = x | (@as(u64, b) << s), .bytesRead = i + 1, }; } - // After 10 continuation bytes a varint can no longer be valid (would - // exceed 64 bits). Reject before `s += 7` overflows the u6 shift count. - // Without this guard, 10+ bytes of 0x80+ (e.g. 1024 bytes of 0xef from - // a malformed gossip payload) panic with integer overflow. - if (i >= 9) { - return Varint{ .value = 0, .bytesRead = 0 }; + // 10th byte (i == 9) with continuation bit still set: varint exceeds + // the 64-bit budget. Bail before the next `s += 7` would overflow + // the u6 shift counter. + if (i == 9) { + return Varint{ + .value = 0, + .bytesRead = 0, + }; } x |= (@as(u64, b & 0x7f) << s); s += 7; } + // Either `buf` was empty or every byte in `window` had the continuation + // bit set without the loop hitting the 10-byte budget guard above + // (i.e. truncated input). Either way: nothing consumed. return Varint{ .value = 0, .bytesRead = 0, @@ -112,9 +134,7 @@ const SnappyBlock = struct { // Return the length of the decoded block and the number of bytes that the header occupied. fn decodedLen(src: []const u8) !SnappyBlock { const varint = uvarint(src); - // `bytesRead == 0` means uvarint either ran out of input without a terminator - // or detected a u64 overflow; both are corrupt headers. - if (varint.bytesRead == 0 or varint.value > 0xffffffff) { + if (varint.bytesRead <= 0 or varint.value > 0xffffffff) { return SnappyError.Corrupt; } @@ -520,43 +540,69 @@ test "decoding variable integers" { } test "uvarint rejects long continuation runs without overflow" { - // Regression: 1024 bytes of 0xef previously panicked with `integer overflow` - // because `s += 7` walked past the u6 shift count. Must now return the - // corrupt sentinel (`bytesRead == 0`) instead of panicking. + // Regression for blockblaz/zeam Hive `gossip: ignores malformed ssz`: + // 1024 bytes of 0xef previously panicked with `integer overflow` because + // `s += 7` walked past the u6 shift count. Must now return the corrupt + // sentinel (`bytesRead == 0`, `value == 0`) instead of panicking. const garbage = [_]u8{0xef} ** 1024; - const v = uvarint(&garbage); - try testing.expectEqual(@as(usize, 0), v.bytesRead); - try testing.expectEqual(@as(u64, 0), v.value); + const v1 = uvarint(&garbage); + try testing.expectEqual(@as(usize, 0), v1.bytesRead); + try testing.expectEqual(@as(u64, 0), v1.value); - // Exactly 10 continuation bytes is also malformed (no terminator at all). + // Exactly 10 continuation bytes, no terminator: hits the i == 9 + // continuation-bit-still-set guard. const ten_continuations = [_]u8{0xff} ** 10; const v2 = uvarint(&ten_continuations); try testing.expectEqual(@as(usize, 0), v2.bytesRead); + try testing.expectEqual(@as(u64, 0), v2.value); + + // Exactly 9 continuation bytes, no 10th byte: input is truncated, falls + // through to the post-loop "nothing consumed" return. Distinct codepath + // from the i == 9 guard above. + const nine_continuations = [_]u8{0xff} ** 9; + const v3 = uvarint(&nine_continuations); + try testing.expectEqual(@as(usize, 0), v3.bytesRead); + try testing.expectEqual(@as(u64, 0), v3.value); - // 11 continuation bytes followed by a valid terminator: still malformed, + // 11 continuation bytes followed by a valid terminator: malformed, // because the varint exceeded the 10-byte u64 budget before terminating. + // Loop window slices `buf` to 10 bytes so the terminator is never seen. var eleven: [12]u8 = undefined; @memset(eleven[0..11], 0xff); eleven[11] = 0x01; - const v3 = uvarint(&eleven); - try testing.expectEqual(@as(usize, 0), v3.bytesRead); - - // Boundary: 10th byte with high bit clear and value > 1 is overflow. - var ten_overflow = [_]u8{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x02 }; - const v4 = uvarint(&ten_overflow); + const v4 = uvarint(&eleven); try testing.expectEqual(@as(usize, 0), v4.bytesRead); - - // Boundary: 10th byte with value == 1 is the largest valid u64. - var max_u64 = [_]u8{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01 }; - const v5 = uvarint(&max_u64); - try testing.expectEqual(@as(usize, 10), v5.bytesRead); - try testing.expectEqual(@as(u64, std.math.maxInt(u64)), v5.value); - - // Empty buffer is incomplete, not corrupt-by-overflow, but still signals - // bytesRead == 0 (no data consumed). + try testing.expectEqual(@as(u64, 0), v4.value); + + // Boundary: 10th byte with continuation bit clear and value > 1 sets + // bits above bit 63 and overflows u64. + const ten_overflow = [_]u8{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x02 }; + const v5 = uvarint(&ten_overflow); + try testing.expectEqual(@as(usize, 0), v5.bytesRead); + try testing.expectEqual(@as(u64, 0), v5.value); + + // Boundary: 10th byte == 1 encodes the largest valid u64 + // (0xffff_ffff_ffff_ffff). 10 bytes consumed, full u64 returned. + const max_u64 = [_]u8{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01 }; + const v6 = uvarint(&max_u64); + try testing.expectEqual(@as(usize, 10), v6.bytesRead); + try testing.expectEqual(@as(u64, std.math.maxInt(u64)), v6.value); + + // Empty buffer: nothing to read, no consumption. const empty = [_]u8{}; - const v6 = uvarint(&empty); - try testing.expectEqual(@as(usize, 0), v6.bytesRead); + const v7 = uvarint(&empty); + try testing.expectEqual(@as(usize, 0), v7.bytesRead); + try testing.expectEqual(@as(u64, 0), v7.value); + + // Non-canonical-but-valid encoding: 0x00 encoded as two bytes. + // The varint format does not require minimal encoding, so this is + // accepted and decodes to value 0 with bytesRead == 2. Pinning current + // behaviour so a future canonicalisation tightening is a deliberate + // breaking change rather than a silent drift. + const non_canonical_zero = [_]u8{ 0x80, 0x00 }; + const v8 = uvarint(&non_canonical_zero); + try testing.expectEqual(@as(usize, 2), v8.bytesRead); + try testing.expectEqual(@as(u64, 0), v8.value); } test "decode rejects garbage with long continuation run" {