The example in the post is using the “direct mode” of std.compress.zstd.Decompress (i.e. giving it a zero-length buffer) which actually has much more unsafety than detailed in the post:
(the issue is about flate but it applies to zstd as well)
Any usage of Decompress in “direct mode” that calls into vtable.rebase/discard/readVec is guaranteed to trigger illegal behavior.
See this comment for context. As I commented here, I think there is room for improvement on the documentation surrounding this, but there’s also some inherent problems with this use case that will hopefully get some attention soon.
I am confused at the point of that blog post.
This isn’t an issue with the new interfaces.
This is an issue with implementations having potentially complex buffer requirements.
It’s an issue that exists regardless of the new or old API, even if you avoid reader/writer entirely.
You can see in the old implementation, there were multiple buffers, the new interfaces having buffers + being able to have requirements for those buffers seems to simplify the implementation.
The difference is now it’s exposed to the caller, as the caller has control over the buffer, and is also able to take advantage of its existence for its own purposes.
I don’t think that’s right. If you take the original snippet:
fn output(r: *std.Io.Reader) !void {
const stdout = std.fs.File.stdout();
var buffer: [???]u8 = undefined;
var writer = stdout.writer(&.{});
_ = try r.stream(&writer.interface, .unlimited);
try writer.interface.flush();
}
What’s the correct buffer size to use? I believe the answer is: it depends. But the issue, and the point of the post, is that there are cases where you might not be able to know.
Possibly this was an issue with the old API as well (got an example?). But that doesn’t change the fact that it’s an issue with the new one as well.
Do you think something like this doc comment on zstd.Decompress.init would help?
/// `buffer` fulfilling the requirements supersedes/obsoletes the requirements on
/// the `Writer.buffer`, but may introduce redundant memcpy calls. This can be useful
/// when you do not know/control the buffer capacity of the `Writer`.
In the old API, this “direct” mode wasn’t supported (but, as mentioned in my last comment, there’s still a lot of limitations around it currently that are unresolved/unresolvable).
The file reader/writer implementation has almost no requirements, some fall back operations (like when the os doesn’t have a send file syscall) need a non-zero sized buffer.
The rest of the requirements are from your use case and the data you’re working with, which is something only you could know, the implementations can’t. This applies to pretty much all implementations.
With the old API, the implementations had their own buffer(s) if they needed them, and BufferedReader/Writer was usually used with a default size of 4096 due to the convenience functions.
an example
new zstd decompresser state
input: *Reader,
reader: Reader,
state: State,
verify_checksum: bool,
window_len: u32,
err: ?Error = null,
old state
source: std.io.CountingReader(ReaderType),
state: enum { NewFrame, InFrame, LastBlock },
decode_state: decompress.block.DecodeState,
frame_context: decompress.FrameContext,
buffer: WindowBuffer,
literal_fse_buffer: [table_size_max.literal]types.compressed_block.Table.Fse,
match_fse_buffer: [table_size_max.match]types.compressed_block.Table.Fse,
offset_fse_buffer: [table_size_max.offset]types.compressed_block.Table.Fse,
literals_buffer: [types.block_size_max]u8,
sequence_buffer: [types.block_size_max]u8,
verify_checksum: bool,
checksum: ?u32,
current_frame_decompressed_size: usize,
some of those types are also a lot of data. most of which is replaced by the buffer in the new interfaces.
The correct buffer size is almost entirely a matter of performance, most implementations both old and new can work with no buffer perfectly fine.
I feel like I tried passing a buffer, but then it did the same thing in reverse - it placed a buffering requirement on the input source. So the buffer size of the original reader (say a file) now becomes dependent on an unknown. However, I had/have a lot of problems doing this - I keep getting SIGILL (0.16.0-dev.253+4314c9653 and 0.15.1), so I maybe I’m wrong and it would solve it.
The copy case would be additive, right? If you have a chain of transforms, and they all opt for this, then it’s potential extra copy(ies) at each step.
Also, it isn’t a global solution. It’s specific to flate/zstd. Other readers that impose a buffer requirement on writers wouldn’t necessary offer the same tradeoff. So the original snippet of trying to output a Reader, the correct size of the buffer is still impossible to know - because you don’t know if Reader has its own “you don’t need to supply your own buffer” mode.
I don’t think this is always true.
I meant something only you could know, the implementation has no idea how you’re using it and with what data so it can’t document or assert requirements specific to your use case.
It shouldn’t. IMO a lot of combinations are undertested currently though, and in my experience it can be hard to distinguish between “holding it wrong” and a bug in some part of the implementation.
Yep, and those copies were unavoidable with the old API.
The intention is for the implementation to document this:
The implementation chooses:
- What the minimum buffer capacity is asserted to be (perhaps zero)
- Under what conditions an undersized buffer capacity can cause runtime failures (as opposed to assertions)
The implementation documents these things carefully, and asserts as early as possible.
The API user chooses:
- How to obtain that buffer (stack, static, heap), if any
- How big to make it
Note that buffers are edges not nodes in a stream pipeline. That means when an API user is choosing a buffer, it has to manage the expectations of two stream implementations - one on either side of the buffer.
It certainly doesn’t help that I’m having to link to random issue comments for all this stuff 