andygrove opened a new pull request, #7307: URL: https://github.com/apache/datafusion-comet/pull/7307
## Which issue does this PR close? Closes #7305. ## Rationale for this change An executor JVM with Native Memory Tracking enabled aborted with `NMT corruption: ... footer canary broken (buffer overflow?)` while materializing Comet's in-memory cache with the default `zstd` codec. The same job passes with `lz4`. The cause is in Arrow 18.3.0's `ZstdCompressionCodec.doCompress`. It allocates `compressBound(n) + 8` bytes, points zstd 8 bytes in (past the uncompressed-length prefix), but still tells zstd the capacity is `compressBound(n) + 8`. zstd guarantees that it never writes past the capacity it is given, not that it never writes past `compressBound`: while it entropy-codes the literals of a block that turns out not to compress, it fills the destination up to that capacity before falling back to a stored block. The attempt can therefore write up to 8 bytes past the buffer. Two things make this rare. The allocator rounds requests up to a power of two, so the overrun lands in slack unless `compressBound(n) + 8` is within 8 bytes below a power of two (n = 432..440 for 512, and so on; the block in #7305 was 437 bytes). And without NMT the extra bytes land in the allocator's padding or in whatever sits next, with no error at all; with NMT the footer canary sits right after the buffer and the JVM aborts when the discarded output is freed, which is why the crashed block had a zstd frame but no length prefix. A standalone program that feeds Arrow's codec 437-byte inputs reproduces the exact abort in about a second under `-XX:NativeMemoryTracking=detail`; with the corrected capacity it runs clean. Arrow fixed this in apache/arrow-java#1119 (reported as apache/arrow-java#1116) for Arrow 20.0.0, which has not been released. ## What changes are included in this PR? - `CometZstdCompressionCodec`, a subclass of Arrow's codec whose `doCompress` passes `compressBound(n)` as the capacity, as the upstream fix does. Decompression is inherited unchanged. - `CachedBatchIpc.compressionCodec` builds that subclass for `zstd`. It is the only place Comet compresses with Arrow's zstd codec. The subclass can be removed once Comet moves to an Arrow release that contains the upstream fix. ## How are these changes tested? New `CometZstdCompressionCodecSuite`. It compresses through a `BufferAllocator` that hands the codec exactly the capacity it requests and keeps a sentinel-filled guard region behind every buffer, so an overrun shows up as a changed sentinel rather than as silent corruption. The inputs are the ones that provoke the bug: nearly incompressible bytes with scattered repeated values, in every length up to 8192 whose output buffer lands within 8 bytes below a power of two. The suite shows Arrow's codec overruns on those inputs, that Comet's codec does not, that both the stored-raw and the compressed outcomes round-trip, and that the guard itself detects a one-byte overrun. Also verified with the standalone reproducer under `-XX:NativeMemoryTracking=detail`: Arrow's codec aborts the JVM, the fixed codec does not. -- 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]
