Skip to content

Fix Reader stale data, Writer buffer leak, and zero-length write#240

Merged
pierrec merged 2 commits into
pierrec:v4from
lkinley-rythmos:v4
Mar 5, 2026
Merged

Fix Reader stale data, Writer buffer leak, and zero-length write#240
pierrec merged 2 commits into
pierrec:v4from
lkinley-rythmos:v4

Conversation

@lkinley-rythmos

Copy link
Copy Markdown
Contributor
  • Reader.read: clear r.data in direct mode so that a subsequent zero-length block does not copy stale pool data
  • Writer.Reset: return data buffer to pool before resetting to prevent a memory leak on reuse
  • Writer.ReadFrom: skip write and handler call when read returns zero bytes

- Reader.read: clear r.data in direct mode so that a subsequent
  zero-length block does not copy stale pool data
- Writer.Reset: return data buffer to pool before resetting to
  prevent a memory leak on reuse
- Writer.ReadFrom: skip write and handler call when read returns
  zero bytes

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lkinley-rythmos

Copy link
Copy Markdown
Contributor Author

Fixes some bugs I have personally run into.

Problem: When the source data is an exact multiple of the block size (e.g., 128MB with 4MB blocks), io.ReadFull returns (0, io.EOF) on the final iteration. ReadFrom still calls w.write(data[:0], true) with empty data. This flows into FrameDataBlock.Compress, which produces a block with Size = 0x80000000 (uncompressed flag set, size=0). This degenerate block is written to the output stream before the proper end-of-stream marker 0x00000000.

Problem: When reading in non-concurrent mode with a user buffer >= block size, the reader uses "direct mode" — decompressing directly into the user's buffer. But r.data is never updated in the direct path. If a block decompresses to 0 bytes, r.read(buf) returns (0, nil). Back in Reader.Read, bn == 0 causes the fallback to copy stale pool data from r.data.

This is the reader-side companion to the first bug. Even with the writer fix, the reader should be robust against degenerate blocks from any encoder.

Problem: Reset() doesn't free w.data back to the pool.

@pierrec

pierrec commented Feb 8, 2026

Copy link
Copy Markdown
Owner

Nice catch. Can you add tests for each fix please?

…ero-length write

- TestReader_DirectModeStaleData: synthetic LZ4 frame with a zero-length
  uncompressed block verifies direct-mode r.data clearing
- TestReader_BufferIssue: end-to-end test with bundle testdata
- TestWriter_ResetWithoutClose: Reset without Close returns buffer to pool
- TestWriter_ReadFromZeroLengthRead: zero-byte reads are skipped in ReadFrom

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lkinley-rythmos

Copy link
Copy Markdown
Contributor Author

Tests added

@pierrec
pierrec merged commit feb996b into pierrec:v4 Mar 5, 2026
0 of 9 checks passed
@pierrec

pierrec commented Mar 5, 2026

Copy link
Copy Markdown
Owner

@lkinley-rythmos merged your patches but the "testdata/bundle.00001.part00000.lz4" file is missing, so the TestReader_BufferIssue fails. Can you provide it please? Thanks!

@lkinley-rythmos

Copy link
Copy Markdown
Contributor Author

I meant to remove that test -- the file is way too big to include.

@serprex serprex mentioned this pull request Jun 1, 2026
debebantur pushed a commit to wal-g/wal-g that referenced this pull request Jun 1, 2026
4.1.27 contains both
pierrec/lz4#240
& pierrec/lz4#245

which are relevant when we're often compressing files with sizes that are a power of 2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants