Hi @SpringMT and @Watson1978,
While reviewing the skippable-frame functions I found a mismatch between what Zstd.write_skippable_frame returns and what its name (and the README) suggest.
The README shows:
compressed_data_with_skippable_frame = Zstd.write_skippable_frame(compressed_data, "sample data")
Zstd.read_skippable_frame(compressed_data_with_skippable_frame) # => "sample data"
The name suggests the result holds the compressed data plus the frame, but it actually holds only the skippable frame — the compressed data is discarded.
Consequence: Zstd.decompress(compressed_data_with_skippable_frame) raises RuntimeError: not a zstd frame (magic not found). Anyone storing that result as their blob loses the payload; read_skippable_frame still returns the metadata correctly, so the loss isn't obvious until you try to decompress.
Concrete example: for a 185-byte compressed input and an 11-byte metadata string, the result is 19 bytes — just the 8-byte header plus the metadata.
Why: the function allocates room for input + header + metadata, but ZSTD_writeSkippableFrame writes the frame starting at the beginning of the buffer, and the string is then resized down to the frame's size, dropping everything after it.
It's behaved this way since the feature was added in 8187f8e; #137 later fixed an out-of-bounds read in the same function but kept the same return value. The existing specs only check that the metadata reads back, so they pass either way.
I've put together a proposed fix in a separate PR: return the skippable frame followed by the input. That layout works with both readers — read_skippable_frame reads a frame at the start, and Zstd.decompress skips skippable frames to reach the data.
Worth flagging this is a behaviour change, and keeping today's behaviour and fixing the README instead would be a reasonable alternative too — happy to go either way, it's your call.
Hi @SpringMT and @Watson1978,
While reviewing the skippable-frame functions I found a mismatch between what
Zstd.write_skippable_framereturns and what its name (and the README) suggest.The README shows:
The name suggests the result holds the compressed data plus the frame, but it actually holds only the skippable frame — the compressed data is discarded.
Consequence:
Zstd.decompress(compressed_data_with_skippable_frame)raisesRuntimeError: not a zstd frame (magic not found). Anyone storing that result as their blob loses the payload;read_skippable_framestill returns the metadata correctly, so the loss isn't obvious until you try to decompress.Concrete example: for a 185-byte compressed input and an 11-byte metadata string, the result is 19 bytes — just the 8-byte header plus the metadata.
Why: the function allocates room for input + header + metadata, but
ZSTD_writeSkippableFramewrites the frame starting at the beginning of the buffer, and the string is then resized down to the frame's size, dropping everything after it.It's behaved this way since the feature was added in 8187f8e; #137 later fixed an out-of-bounds read in the same function but kept the same return value. The existing specs only check that the metadata reads back, so they pass either way.
I've put together a proposed fix in a separate PR: return the skippable frame followed by the input. That layout works with both readers —
read_skippable_framereads a frame at the start, andZstd.decompressskips skippable frames to reach the data.Worth flagging this is a behaviour change, and keeping today's behaviour and fixing the README instead would be a reasonable alternative too — happy to go either way, it's your call.