No way to correctly use `zstd.Decompress` with `tar.pipeToFileSystem`

[squeek502]

Zig Version

0.15.0-dev.1380+e98aeeb73

Steps to Reproduce and Observed Behavior

Test program (needs compressed_mingw_includes.tar.zst):

const std = @import("std");

pub fn main() !void {
    var debug_allocator: std.heap.DebugAllocator(.{}) = .{};
    defer std.debug.assert(debug_allocator.deinit() == .ok);
    const allocator = debug_allocator.allocator();

    const compressed_input = @embedFile("compressed_mingw_includes.tar.zst");
    const buffer_size = std.compress.zstd.default_window_len + std.compress.zstd.block_size_max;
    const window_buffer = try allocator.create([buffer_size]u8);
    defer allocator.destroy(window_buffer);

    var input_reader: std.Io.Reader = .fixed(compressed_input);
    var decompress: std.compress.zstd.Decompress = .init(&input_reader, window_buffer, .{});

    try std.fs.cwd().deleteTree("out");

    var out_dir = try std.fs.cwd().makeOpenPath("out", .{});
    defer out_dir.close();

    try std.tar.pipeToFileSystem(out_dir, &decompress.reader, .{ .mode_mode = .ignore });
}

This will hit error.OutputBufferUndersize, since tar.pipeToFileSystem uses a writer with a hardcoded buffer of length 1024, which is not enough for zstd.

zig/lib/std/tar.zig

Line 585 in 8843631

var file_contents_buffer: [1024]u8 = undefined;

With some debug printing added to show why the error is being returned:

8388608 + 131072 = 8519680 > 8519680
8388608 + 131072 = 8519680 > 1024
error: ReadFailed
/home/ryan/Programming/zig/zig/lib/std/compress/zstd/Decompress.zig:260:75: 0x116894a in readInFrame (std.zig)
                    if (window_len + frame_block_size_max > w.buffer.len) return error.OutputBufferUndersize;
                                                                          ^
/home/ryan/Programming/zig/zig/lib/std/compress/zstd/Decompress.zig:182:21: 0x116ab90 in stream (std.zig)
                    return error.ReadFailed;
                    ^
/home/ryan/Programming/zig/zig/lib/std/Io/Reader.zig:180:15: 0x1084db2 in stream (std.zig)
    const n = try r.vtable.stream(r, w, limit);
              ^
/home/ryan/Programming/zig/zig/lib/std/Io/Reader.zig:223:41: 0x1172792 in streamExact64 (std.zig)
    while (remaining != 0) remaining -= try r.stream(w, .limited64(remaining));
                                        ^
/home/ryan/Programming/zig/zig/lib/std/tar.zig:477:9: 0x1160c60 in streamRemaining (std.zig)
        try it.reader.streamExact64(w, file.size);
        ^
/home/ryan/Programming/zig/zig/lib/std/tar.zig:615:21: 0x1156eba in pipeToFileSystem (std.zig)
                    try it.streamRemaining(file, &file_writer.interface);
                    ^
/home/ryan/Programming/zig/tmp/tarzst/test.zig:21:5: 0x1158410 in main (test.zig)
    try std.tar.pipeToFileSystem(out_dir, &decompress.reader, .{ .mode_mode = .ignore });
    ^

Expected Behavior

Able to use zstd.Decompress with tar.pipeToFileSystem

[ianic]

I allowed setting tar writer buffer: https://github.com/ianic/zig/tree/tar_writer_buffer_options

Using this instead of last line from example

const output_buffer_size = std.compress.zstd.default_window_len + std.compress.zstd.block_size_max;
    const output_buffer = try allocator.alloc(u8, output_buffer_size);
    defer allocator.free(output_buffer);
    try std.tar.pipeToFileSystem(out_dir, &decompress.reader, .{
        .mode_mode = .ignore,
        .file_writer_buffer = output_buffer,
    });

fails with:

error: ReadFailed
/home/ianic/Code/zig/lib/std/math.zig:578:21: 0x1073b67 in sub__anon_5175 (std.zig)
    if (ov[1] != 0) return error.Overflow;
                    ^
/home/ianic/Code/zig/lib/std/compress/zstd/Decompress.zig:715:21: 0x1167ab1 in decodeSequence (std.zig)
                    return error.MalformedSequence;
                    ^
/home/ianic/Code/zig/lib/std/compress/zstd/Decompress.zig:263:42: 0x1169312 in readInFrame (std.zig)
                        bytes_written += try decode.decodeSequence(w.buffer, write_pos + bytes_written, &bit_stream);
                                         ^
/home/ianic/Code/zig/lib/std/compress/zstd/Decompress.zig:182:21: 0x116b090 in stream (std.zig)
                    return error.ReadFailed;
                    ^
/home/ianic/Code/zig/lib/std/Io/Reader.zig:178:15: 0x1084de2 in stream (std.zig)
    const n = try r.vtable.stream(r, w, limit);
              ^
/home/ianic/Code/zig/lib/std/Io/Reader.zig:221:41: 0x1172ca2 in streamExact64 (std.zig)
    while (remaining != 0) remaining -= try r.stream(w, .limited64(remaining));
                                        ^
/home/ianic/Code/zig/lib/std/tar.zig:479:9: 0x1161360 in streamRemaining (std.zig)
        try it.reader.streamExact64(w, file.size);
        ^
/home/ianic/Code/zig/lib/std/tar.zig:616:21: 0x11571f7 in pipeToFileSystem (std.zig)
                    try it.streamRemaining(file, &file_writer.interface);
                    ^
/home/ianic/Code/issues/24735/main.zig:25:5: 0x11589a7 in main (main.zig)
    try std.tar.pipeToFileSystem(out_dir, &decompress.reader, .{
    ^

But this seems to be zstd related like in #24608

[squeek502]

file_writer_buffer should probably have length std.compress.zstd.default_window_length + std.compress.zstd.block_size_max, but the same error occurs with that buffer size so that's not relevant for this particular file.

Note also that there are also likely to be real zstd correctness regressions. I only dipped my toes into fuzzing the new implementation for correctness, and it's a bit harder to disentangle real regressions from false positives since the Zig implementation intentionally differs from the reference implementation in a few places (the Zig implementation is more strict), but at some point I'll make an issue like #24741 for zstd.

[squeek502]

Yeah, this particular .tar.zst is not a good test for this particular issue, since it exposes a regression in zstd unrelated to pipeToFileSystem. Using an allocating writer with the (I believe) proper amount of unused buffer, it hits a similar infinite loop as #24741 (comment) (same loop in writableSliceGreedyPreserve, but called from here)

Writer.Allocating test program

const std = @import("std");

pub fn main() !void {
    var debug_allocator: std.heap.DebugAllocator(.{}) = .{};
    defer std.debug.assert(debug_allocator.deinit() == .ok);
    const allocator = debug_allocator.allocator();

    const compressed_input = @embedFile("compressed_mingw_includes.tar.zst");
    const buffer_size = std.compress.zstd.default_window_len + std.compress.zstd.block_size_max;
    const window_buffer = try allocator.create([buffer_size]u8);
    defer allocator.destroy(window_buffer);

    var input_reader: std.Io.Reader = .fixed(compressed_input);
    // The buffer given to Decompress doesn't matter, giving it a 0-byte buffer still triggers the infinite loop
    var decompress: std.compress.zstd.Decompress = .init(&input_reader, window_buffer, .{});

    var out: std.io.Writer.Allocating = .init(allocator);
    defer out.deinit();
    try out.ensureUnusedCapacity(buffer_size);

    _ = try decompress.reader.streamRemaining(&out.writer);
}

EDIT: The previous version of this comment said that a fixed writer also hit the infinite loop, but that was partially wrong. Using a fixed writer with enough space for the full uncompressed data does work:

Writer.Fixed test program

const std = @import("std");

pub fn main() !void {
    var debug_allocator: std.heap.DebugAllocator(.{}) = .{};
    defer std.debug.assert(debug_allocator.deinit() == .ok);
    const allocator = debug_allocator.allocator();

    const compressed_input = @embedFile("compressed_mingw_includes.tar.zst");
    const buffer_size = std.compress.zstd.default_window_len + std.compress.zstd.block_size_max;
    const window_buffer = try allocator.create([buffer_size]u8);
    defer allocator.destroy(window_buffer);

    var input_reader: std.Io.Reader = .fixed(compressed_input);
    // The buffer given to Decompress doesn't matter, giving it a 0-byte buffer still triggers the infinite loop
    var decompress: std.compress.zstd.Decompress = .init(&input_reader, window_buffer, .{});

    // The uncompressed data is ~70 MiB
    const uncompressed_buffer = try allocator.create([100 * 1024 * 1024]u8);
    defer allocator.destroy(uncompressed_buffer);
    var out: std.Io.Writer = .fixed(uncompressed_buffer);

    _ = try decompress.reader.streamRemaining(&out);
}

Using a fixed buffer with less than the uncompressed data size hits the infinite loop, though (change the 100 * 1024 * 1024 in the uncompressed_buffer allocation to 10 * 1024 * 1024, for example)

Will prioritize making a "zstd regressions" issue. EDIT: #24817

[andrewrk]

This was solved by #24853. Since the test program initializes Decompress with a buffer, it chooses "indirect mode", in which all bytes are first written to that buffer before they are consumed, with no changes needed to tar functionality.

If an empty buffer was chosen instead, then the problem would arise because the same Writer is not used consistently with every call into Decompress, so the window is not maintained.

I suppose a way you can think of it is that initializing Decompress with a buffer is a "safe" API that avoids a bunch of problems, but may be doing an unnecessary memcpy. And then in some cases you can choose for a more optimal pipeline by passing an empty buffer, but there you risk runtime bugs if you violate certain constraints.

Does it make sense?

[squeek502]

Makes sense, also seems worth trying to include that info in the documentation.

Just for completeness, I'll mention that the window size in Fetch.zig might want to be updated to std.compress.zstd.default_window_len + std.compress.zstd.block_size_max:

zig/src/Package/Fetch.zig

Line 1215 in d835a6b

const window_size = std.compress.zstd.default_window_len;

(or the zstd.Decompress.Options.window_len documentation should be updated)