Make the decode window size limit configurable - #111
Conversation
zleyyij
left a comment
zleyyij
left a comment
There was a problem hiding this comment.
Other than semantic discussion around the signature of FrameDecoderState::new(), this PR seems structurally sound and a reasonable enough addition.
I'm just contributing a review, and don't have final say.
Good work!
|
|
||
| impl FrameDecoderState { | ||
| pub fn new(source: impl Read) -> Result<FrameDecoderState, FrameDecoderError> { | ||
| pub fn new( |
There was a problem hiding this comment.
To avoid a breaking change, we could keep the old function signature and use DEFAULT_MAX_WINDOW_SIZE in new(), and expect people to call set_max_window_size() if they want to update it.
This would keep the new() interface streamlined and sufficient for most usecases.
There was a problem hiding this comment.
The public FrameDecoder::new() already works that way. The signature in the diff is the private FrameDecoderState, now a plain fn to make that obvious.
|
Thanks for the work! @zleyyij maybe I am missing something but I think the signature with a breaking change is on the non-public FrameDecoderState and not the public FrameDecoder itself. If that's the case this PR looks good to me but since you brought it up I want to make sure I did not miss a breaking change there :) The change in the error type is fine for me, I have no issue bumping to a 0.9 version instead of the next 0.8 |
The parameter is only on the private |
|
will fix |
|
Thanks again :) |
| ) -> Result<FrameDecoderState, FrameDecoderError> { | ||
| let (frame, header_size) = frame::read_frame_header(source)?; | ||
| let window_size = frame.window_size()?; | ||
| Self::check_window_size(window_size, max_window_size)?; |
There was a problem hiding this comment.
Thank you for adding this check. I think before malicious input could have triggered OOM.
4 participants
ruzstdcaps the decode window at a hardcoded 100 MB with no override, so it cannot decode frames declaring a larger window such as anything fromzstd --long. This adds an opt-in to raise the cap while keeping the 100 MB default for everyone else, mirroringZSTD_d_windowLogMaxinlibzstd. My use case is streaming a 2 GiB-window corpus from pure Rust without pulling in Clibzstd.It also makes the cap apply to the first frame of a stream, which
newskipped whileresetenforced it, and reshapesFrameDecoderError::WindowSizeTooBigto report the effective limit. That last piece is a small breaking change on the unreleased 0.8.4 that I can drop if you prefer strictly non-breaking.