fix: flate decompression logic #340
Conversation
HashimTheArab
commented
Sep 5, 2025
HashimTheArab
commented
- Proper overflow detection for limit instead of just returning truncated data
- Fixed decompression pool usage, old logic put reader back in pool without closing in cases of Reset errors
- Old logic closed the reader before reading which worked but was wrong
- Improved capacity hints, does not set to * 2 if its not needed
| defer flateDecompressPool.Put(c) | ||
| r := flateDecompressPool.Get().(io.ReadCloser) | ||
| defer func() { | ||
| _ = r.Close() |
There was a problem hiding this comment.
I am not sure if we need to Close() at all. For zlib, the Close before reading was necessary for the checksum, but I believe deflate has no such checksum. Given we don't need it, maybe Close() isn't necessary at all?
| capHint := limit | ||
| if l <= limit/2 { | ||
| capHint = l * 2 | ||
| } | ||
| decompressed.Grow(capHint) |
There was a problem hiding this comment.
I believe this can be simplified to:
decompressed := bytes.NewBuffer(make([]byte, 0, min(limit, len(compressed) * 2)))There was a problem hiding this comment.
We can simplify with bytes.newbuffer
The if checking the l <= limit/2 protects against an int overflow on 32 bit devices if a packet is 1gb, which is impossible but having the if instead of min seems "correct"
| // Handle no limit | ||
| if limit == math.MaxInt { | ||
| if _, err := io.Copy(&decompressed, r); err != nil { | ||
| return nil, fmt.Errorf("decompress flate: %w", err) | ||
| } | ||
| return decompressed.Bytes(), nil | ||
| } |
There was a problem hiding this comment.
Is there ever a sensible case to have no limit? I get that this is probably to avoid overflow, but could just do a max(limit + 1, math.MaxInt) somewhere.
| toRead := int64(limit) + 1 | ||
| n, err := io.CopyN(&decompressed, r, toRead) | ||
| if err != nil && err != io.EOF { | ||
| // CopyN returns an EOF error if the stream is shorter than the limit, we can treat that as a success. | ||
| return nil, fmt.Errorf("decompress flate: %w", err) | ||
| } | ||
| if n > int64(limit) { | ||
| return nil, fmt.Errorf("decompress flate: size %d exceeds limit %d", n, limit) | ||
| } |
There was a problem hiding this comment.
Maybe a slightly easier to understand implementation, slightly closer to what it was before, might be this?
lr := io.LimitReader(c, int64(limit) + 1)
if _, err := io.Copy(decompressed, lr); err != nil {
return nil, fmt.Errorf("decompress flate: %w", err)
} else if lr.(*io.LimitedReader).N <= 0 {
return nil, fmt.Errorf("decompress flate: size %d exceeds limit %d", n, limit)
}2 participants
