gsquire / zig-snappy

Snappy compression for Zig
MIT License
28 stars 1 forks source link

Freeing slice returned via dst[0..d] doesn't free the dst[d..] portion #2

Open ljamzin opened 7 months ago

ljamzin commented 7 months ago

Caught in following test code:


fn printHex(comptime label: []const u8, data: []u8) void {
    print("{s}: ", .{label});
    for (data) |byte| print("{x}", .{byte});
    print("\n", .{});
}

test "first snappy exec" {
    {
        print("\n\n", .{});
        var alcr = testing.allocator;

        var _buffin: [64]u8 = [_]u8{0xFF} ** 64;
        var buffin: []u8 = &_buffin;

        try testing.expectEqual(64, _buffin.len);
        printHex("Input", buffin);

        var _outexp = [_]u8{ 0x40, 0x00, 0xFF, 0xFA, 0x01, 0x00 };
        var outexp: []u8 = &_outexp;
        printHex("Expected", outexp);

        var buffout = try snappy.encode(alcr, buffin);
        defer alcr.free(buffout);
        printHex("Output", buffout);

        try testing.expectEqualSlices(u8, outexp, buffout);

        print("\n", .{});
    }
}

The specifics code probably doesn't matter though, the leak should be reported by testing.allocator with pretty much any net positive encode invocation:

[gpa] (err): Allocation size 106 bytes does not match free size 6. Allocation:
C:\dev\24-03\qrnl\src\codec\snappy\snappy.zig:427:34: 0x7ff6e29d789e in encode (test.exe.obj)
    var dst = try allocator.alloc(u8, @as(usize, @intCast(encodedLen)));
                                 ^
C:\dev\24-03\qrnl\src\codec\snappy\test.zig:34:40: 0x7ff6e29dc1f2 in test.first snappy exec (test.exe.obj)
        var buffout = try snappy.encode(alcr, buffin);
                                       ^
C:\Users\Aleksandar\AppData\Roaming\Code\User\globalStorage\ziglang.vscode-zig\zig_install\lib\test_runner.zig:100:29: 0x7ff6e29d5e41 in mainServer (test.exe.obj)
                test_fn.func() catch |err| switch (err) {
                            ^
C:\Users\Aleksandar\AppData\Roaming\Code\User\globalStorage\ziglang.vscode-zig\zig_install\lib\test_runner.zig:34:26: 0x7ff6e29d12cb in main (test.exe.obj)
        return mainServer() catch @panic("internal test runner failure");
                         ^
C:\Users\Aleksandar\AppData\Roaming\Code\User\globalStorage\ziglang.vscode-zig\zig_install\lib\std\start.zig:339:65: 0x7ff6e29d1053 in WinStartup (test.exe.obj)
    std.os.windows.kernel32.ExitProcess(initEventLoopAndCallMain());
                                                                ^
???:?:?: 0x7fffa2e4257c in ??? (KERNEL32.DLL)
???:?:?: 0x7fffa3e6aa57 in ??? (ntdll.dll)
 Free:
C:\dev\24-03\qrnl\src\codec\snappy\test.zig:35:24: 0x7ff6e29dc301 in test.first snappy exec (test.exe.obj)
        defer alcr.free(buffout);
                       ^
C:\Users\Aleksandar\AppData\Roaming\Code\User\globalStorage\ziglang.vscode-zig\zig_install\lib\test_runner.zig:100:29: 0x7ff6e29d5e41 in mainServer (test.exe.obj)
                test_fn.func() catch |err| switch (err) {
                            ^
C:\Users\Aleksandar\AppData\Roaming\Code\User\globalStorage\ziglang.vscode-zig\zig_install\lib\test_runner.zig:34:26: 0x7ff6e29d12cb in main (test.exe.obj)
        return mainServer() catch @panic("internal test runner failure");
                         ^
C:\Users\Aleksandar\AppData\Roaming\Code\User\globalStorage\ziglang.vscode-zig\zig_install\lib\std\start.zig:339:65: 0x7ff6e29d1053 in WinStartup (test.exe.obj)
    std.os.windows.kernel32.ExitProcess(initEventLoopAndCallMain());
                                                                ^
???:?:?: 0x7fffa2e4257c in ??? (KERNEL32.DLL)
???:?:?: 0x7fffa3e6aa57 in ??? (ntdll.dll)

I'm also fairly new to zig and slowly trying to write a shitty parquet file reader. This seems to work for page decompression! I intend to at some point test if/how much using this and other pure zig codec implementations affects bundle size compared to using the C versions. I can share the results if you're curious.

ljamzin commented 7 months ago
var final = try allocator.alloc(u8, d);
@memcpy(final, dst[0..d]);
allocator.free(dst);
return final;
// return dst[0..d];

crude workaround

gsquire commented 7 months ago

Thanks for finding this! I should have had a test to catch this earlier. I added one in and fixed it with your suggestion. We can leave this issue open if you'd like to share your results. It'd be interesting to see!