Repository navigation
GH-1206: validate decompressed length in Lz4CompressionCodec - #1207
Merged
Merged
Conversation
Arawoof06
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
July 4, 2026 08:00
This comment has been minimized.
This comment has been minimized.
lidavidm
approved these changes
Jul 5, 2026
jbonofre
pushed a commit
that referenced
this pull request
Nov 2, 2026
…ter with lz4-java (#1420) ## What's Changed `Lz4CompressionCodec` uses commons-compress's pure-Java LZ4, which is slow enough that compressing an 8 MiB buffer of sequential `int64` values takes several minutes (#192). This changes the codec to use [lz4-java](https://github.com/yawkat/lz4-java), the JNI bindings to liblz4 that Spark, Kafka and others use, while keeping every frame it could read before readable. **Compression.** Each buffer is written as one LZ4 frame of independent blocks of up to 4 MiB, without checksums, compressed with lz4-java directly from the buffer's memory. Every LZ4 frame decoder reads frames with independent blocks: liblz4 (Arrow C++, pyarrow), `lz4_flex` (arrow-rs), lz4-java, and commons-compress. **Decompression.** A frame with independent blocks is decompressed with lz4-java directly into the output buffer. That covers frames from this codec, from arrow-rs, and from the previous version of this codec. The new decoder accepts any block size, block and content checksums (which it verifies), and a content size. It checks the frame's block lengths, and that they can hold the recorded decompressed length, before allocating for it. A frame whose blocks are linked, or that uses a dictionary, still goes through commons-compress. lz4-java cannot decode a block that refers to earlier blocks, and Arrow C++ writes linked blocks: liblz4's default `LZ4F_preferences_t` is `LZ4F_blockLinked`, so pyarrow 26 writes every buffer larger than 64 KiB as linked 64 KiB blocks. The LZ4 files in arrow-testing are small enough that liblz4 writes them as single, independent blocks, so the integration tests would not catch this. `TestLz4CompressionCodec` covers linked frames written by commons-compress instead. **Dependency.** This adds `at.yawk.lz4:lz4-java` 1.12.1, the maintained fork of `org.lz4:lz4-java`. The original artifact's last code release, 1.8.0, has several published advisories (for example CVE-2025-12183 and CVE-2025-66566), and its 1.8.1 is a relocation POM that points to the fork. Spark moved to the fork in 4.2. Both artifacts use the `net.jpountz` packages and the `org.lz4.java` module name, and the codec uses only APIs present in both, so an application that pins `org.lz4:lz4-java` instead still works. The codec asks for lz4-java's JNI implementation and falls back to its Java one only when the native library cannot load, because `LZ4Factory.fastestInstance()` settles for Java whenever lz4-java is not loaded by the system class loader. ### Performance `CompressionCodecBenchmarks` (new, in `performance`) on an Apple M3 Ultra, JDK 17, 8 MiB buffers: | Benchmark | LZ4_FRAME | ZSTD | | --- | ---: | ---: | | compress, sequential `int64` | 8.5 ms | 12.5 ms | | compress, random decimal text | 9.9 ms | 44.6 ms | | decompress, sequential `int64` | 2.1 ms | 4.8 ms | | decompress, random decimal text | 1.9 ms | 8.5 ms | The previous codec is too slow for the same JMH run. From a simple timing harness on the same machine: | Input | Previous compress | New compress | Previous decompress | New decompress | | --- | ---: | ---: | ---: | ---: | | 8 MiB sequential `int64` | 278 s | 7.8 ms | 93.5 ms | 2.2 ms | | 8 MiB of `i % 1000` as text | 17.9 ms | 0.3 ms | 3.3 ms | 0.3 ms | | 8 MiB random bytes | 243 ms | 0.5 ms | (stored) | (stored) | | 1 MiB random decimal text | 38.5 s | 1.4 ms | 33.3 ms | 0.3 ms | ### Compression ratio On the inputs above, the ratio is unchanged except for random decimal text, where commons-compress's more thorough match search reached 1.61 and lz4-java's fast compressor reaches 1.10. The new output matches what Arrow C++ writes by default: pyarrow's LZ4 codec also reaches 1.10 on the same kind of data, and 1.63 at compression level 9. `CommonsCompressionFactory.createCodec(LZ4_FRAME, level)` ignores the level today. Mapping it to lz4-java's high-compression mode would give users who want the better ratio a way to ask for it; I left that out to keep this change focused, and can add it here or in a follow-up. ### Testing - The existing `TestCompressionCodec` and `TestArrowReaderWriterWithCompression` pass unchanged, including the decompressed-length check from #1207. - `TestLz4CompressionCodec` (new): - round-trips buffers across block boundaries, including one whose first block is incompressible and stored, and checks that commons-compress and lz4-java's `LZ4FrameInputStream` both read the frames this codec writes; - decompresses frames written by commons-compress for every block size, with linked and independent blocks, with and without checksums, and a frame written by lz4-java with a content size; - rejects corrupt frames (descriptor, block and content checksums, a missing end mark, blocks that run past the frame or exceed the maximum block size, undecodable blocks, a wrong content size, and recorded lengths that are too long or too short), and releases everything it allocated. - I also decoded pyarrow 26 output (buffers over 64 KiB, linked blocks) by hand to confirm that lz4-java rejects linked frames and commons-compress reads them, which is why the fallback stays. ### AI usage The code, tests and benchmark in this PR were written with an AI coding assistant. The frame behavior described above (pyarrow's linked blocks, lz4-java's rejection of them) and the benchmark numbers come from running them. Closes #192.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's Changed
Lz4CompressionCodec.doDecompresssizes the output buffer to the bytes it actually decompressed, but setswriterIndexto the length taken from the untrusted 8-byte prefix. A buffer whose prefix claims more than the real output leaves the returnedArrowBufwith awriterIndexpast its capacity, and consumers then read off-heap memory beyond the allocation. This adds the actual-vs-claimed length check the ZSTD codec already does, so a mismatch throws instead of producing an over-long buffer.Closes #1206.