Skip to content

sizeOfPackedArray/sizeOfPackedMap under-report size for non-byte elements #23

Description

@acoustid-bot

sizeOfPackedArray and sizeOfPackedMap add the element count to the header size, as if every element encoded to exactly one byte:

// src/array.zig:28
pub fn sizeOfPackedArray(len: usize) !usize {
    return try sizeOfPackedArrayHeader(len) + len;
}

// src/map.zig:29
pub fn sizeOfPackedMap(len: usize) !usize {
    return try sizeOfPackedMapHeader(len) + len;
}

That identity holds for sizeOfPackedString, where len really is a byte count, but not for arrays or maps, where the payload size depends on the element values and types. Both functions are exported from src/msgpack.zig, and sizeOfPackedArray is additionally reachable from sizeOfPackedAny for any non-u8 slice (src/any.zig:77).

Since the result is an under-estimate, using it to size a buffer produces a buffer that is too small.

Reproducer

test "sizeOfPackedAny vs real encoded length" {
    const values = [_]u16{ 1000, 2000, 3000 };
    const predicted = try sizeOfPackedAny([]const u16, &values);   // => 4

    var buf: [128]u8 = undefined;
    var w = std.Io.Writer.fixed(&buf);
    try msgpack.encode(@as([]const u16, &values), &w);
    const actual = w.buffered().len;                                // => 10

    try std.testing.expectEqual(actual, predicted);                 // fails
}
value sizeOfPackedAny actual encoded
[]const u16{1000, 2000, 3000} 4 10
[]const []const u8{"hello", "world"} 3 13

The existing test hides this

// src/any.zig:392
test "sizeOfPackedAny: array slice" {
    const values = [_]u16{ 1, 2, 3 };
    try std.testing.expectEqual(4, try sizeOfPackedAny([]const u16, &values));
}

1, 2 and 3 all encode as single-byte fixints, so header + count coincidentally equals the true length here. Any element above 127 breaks it.

Options

  1. Make it correct. Have sizeOfPackedAny sum sizeOfPackedAny over the elements, and reduce sizeOfPackedArray/sizeOfPackedMap to their header-only forms. Exact, but O(n) — and not expressible for maps through the current (len: usize) signature, which would have to take the container.
  2. Make it an explicit upper bound. Rename to maxSizeOfPacked* and compute header + len * getMaxIntSize(Item)-style worst cases, matching the existing getMaxIntSize/getMaxFloatSize/getMaxEnumSize convention. Over-estimates are safe for buffer sizing; under-estimates are not.
  3. Remove them. Nothing inside the library depends on sizeOfPackedArray/sizeOfPackedMap except sizeOfPackedAny, which is not itself exported.

Option 2 looks like the best fit for the existing API vocabulary, but this is a public-API decision so I did not change anything.

Either way src/any.zig:392 should be re-pointed at values that actually exercise a multi-byte encoding.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions