xiangfu0 commented on PR #19285: URL: https://github.com/apache/pinot/pull/19285#issuecomment-5377289363
## Review: Add bounded codec runtime and compression handlers I reviewed this at the source level (all 13 production files + 5 test files) and ran two focused verification passes on the highest-risk surfaces: (1) buffer ownership / use-after-free in the executor's encode/decode/ping-pong scratch loops, and (2) untrusted-frame bounds in the four compression handlers, including an empirical fuzz of the GZIP encode bound. **No correctness or security bugs found.** The design is careful and the negative-path test coverage is genuinely good. Details and a few minor nits below. ### What I verified holds **Executor buffer lifecycle (`CodecPipelineExecutor`)** - The clean-up guards `if (previous != input && previous != current)` (encode) and `if (previous != src && previous != current)` (decode) are correct across passthrough stages (handler returns its input) and 2/3-stage chains — no leak, no double-free, and the caller-owned `input`/`src` and the returned buffer are never freed. - `DecodeScratch` ping-pong: traced `scratchSlot` alternation for stageCount 2/3/4. Each stage's input slice and its output slice are always backed by different `_buffers[]` slots, and growth-triggered `cleanQuietly` on a slot only fires two stages later when that slice is already dead. No aliasing / use-after-free. - `_requiresDirectDstBuffer = stages.get(0).requiresDirectDstBuffer()` is the right stage — stage 0 is the only decode stage that writes into the caller's `dst`. The capacity-limited `view.slice()` in `DecodeScratch.buffer()` correctly prevents a corrupt inner frame from exploiting leftover capacity from a larger prior chunk. - Final `dst` position/limit mirror is correct, including `expectedDecodedSize == 0`. **Compression handlers (untrusted decode surface)** - Every declared decompressed size that drives an `allocateDirect` is routed through `checkDeclaredDecompressedSize` (1 GiB cap, rejects negatives) *before* allocation — GZIP footer, LZ4 length prefix, Snappy varint header, Zstd frame content size. No unchecked allocation path exists. - GZIP footer is anchored to `limit` (not `position`), so nonzero source positions and little-endian caller views both decode correctly — matching the two regression tests. - GZIP `inflateInto`'s completion probe genuinely hardens against truncated streams, over-expansion beyond the footer-declared size, and trailing compressed bytes. - Empty-buffer (size 0) handling is correct for LZ4/Snappy/Zstd `decodeInto`. - **GZIP `maxEncodedSize` bound is safe.** I fuzzed the exact encode logic against `java.util.zip.Deflater` (level 6, all sizes 0..300k incl. incompressible/tiny): the reserved deflate space (`capacity - 4` = zlib `compressBound`) is never exceeded; worst-case margin was +2 bytes. The 4-byte footer is added on top of the deflate budget, not carved out of it. ### Minor nits (non-blocking) 1. **`ZstdCodecDefinition.decodeInto` skips `checkDeclaredDecompressedSize`** (it only checks `> dst.capacity()`), whereas `decode()` routes through it. This is *not* a security gap — `dst` is the executor's pre-bounded scratch, so the frame size can't drive an allocation and can't overrun. But it's an asymmetry: a declared size between `dst.capacity()` and 1 GiB throws `IllegalArgumentException` here vs `IOException` in `decode()`. Routing it through the shared helper would make the error type uniform and keep the "every declared size goes through one gate" invariant literally true. 2. **`CodecPipelineExecutor.encode`/`decode` set `ByteOrder.BIG_ENDIAN` on the working view**, which is dead work for compression-only pipelines (LZ4/Snappy/Zstd/GZIP read bytes sequentially; order is irrelevant to them). It's harmless and correct/necessary for the typed transform stages that later PRs add, and the encode/decode sides are consistent — just noting it's currently a no-op for everything this PR ships. 3. **`GzipCodecDefinition` thread-local `Deflater`/`Inflater`** hold native resources released only on thread death. The comment already calls this out and it's bounded by pool size, so acceptable — flagging only because on very large elastic thread pools this is a slow native-memory accumulation. ### Tests Coverage is strong: round-trip (heap+direct), multi-stage chains, per-stage/cumulative bound caps, direct-dst enforcement, scratch reuse (`allocationCount==2`), unexpected-final-size, and a solid corrupt-input suite (garbage frames, oversized declared size at the exact 1 GiB boundary, GZIP truncated footer / under-reported size / corrupt checksum / big-endian-footer-from-LE-view / nonzero-position, inner-frame bounding). The public-surface reflection test nicely locks the "only `CodecPipelineExecutor` is public" contract. LGTM. The two nits are optional; #1 is the only one I'd consider addressing for defense-in-depth consistency. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
