serhiy-bzhezytskyy opened a new pull request, #16480: URL: https://github.com/apache/lucene/pull/16480
Closes #16479. ### Description A corrupt byte inside a compressed chunk of stored fields or term vectors is not detected on the read path. It surfaces as whatever the decompressor does with it — an `ArrayIndexOutOfBoundsException` from LZ4, or no error at all and a different document than the one that was stored. The per-file CRC32 footer does not help: it is verified from `checkIntegrity`, which runs at merge and in `CheckIndex`, not when a document is fetched. This is the fix proposed on #6331 in 2013 and never built: > maybe we should add 4 bytes of checksum per chunk in order to be able to distinguish index corruptions from bugs in the compression layer The LZ4 frame format specifies the same check over the same bytes — an optional 4-byte xxHash-32 per compressed block, whose *"intention is to detect data corruption (storage or transmission errors) immediately, before decoding"* — but Lucene implements the LZ4 block format, which carries no checksum of its own. ### Three commits **1. Stored fields.** Each chunk now ends with a CRC32C of its compressed bytes, verified before the decompressor is called. Format version 2. **2. Term vectors.** The same, in the same package with the same `Compressor` and fields index. Format version 1. One difference: a chunk with no fields writes no compressed payload, so there is nothing to checksum — covered by `testDocumentsWithoutVectors`. **3. `LowercaseAsciiCompression`.** Not a checksum, but the same class of defect, and found by measuring rather than by reading: step 4 of `decompress` accumulates exception offsets read from the data and uses them to index the output without a bound check, so corrupt input threw `ArrayIndexOutOfBoundsException`. This is the shape of the LZ4 match-offset check in #16478. CRC32C rather than xxHash-32 because it is in the JDK and hardware-accelerated — measured at 8,900 MB/s here, against 1–3 GB/s for the LZ4 decompression it guards — and it is already the primitive behind `CodecUtil`'s footers. ### Cost 4 bytes per chunk: 0.024% at the 16 KB chunks of `BEST_SPEED`, 0.0065% at the 60 KB chunks of `BEST_COMPRESSION`. The checksum is computed as the compressed bytes are written, through a filtering `DataOutput`, so there is no extra buffering pass. The chunk's length comes from the fields index rather than from the file, so a corrupt length cannot be used to read outside the chunk. ### Back-compat Version 1 (and version 0 for term vectors) is read exactly as before, so no reindexing and no upgrade step. Verified by writing an index with each version and reading it with the other: | | | |---|---| | old writer → new reader | 500/500 documents, `CheckIndex clean=true` | | new writer → new reader | 500/500 documents, `CheckIndex clean=true` | | new writer → old reader | `IndexFormatTooNewException: 2 (needs to be between 1 and 1)` | | mixed index, segments of both versions | read correctly, each segment per its own version | A segment acquires checksums when it is next merged: `getMergeStrategy` declines the bulk-copy path for a reader whose version is not `VERSION_CURRENT` and re-encodes instead. This happens under the default merge policy, which is version-blind and needs no change. Two notes on the edges: a segment that is never merged keeps the old layout, and `UpgradeIndexMergePolicy` compares `Version.LATEST` rather than a format version, so within one major version it will not force the rewrite. Format version bumps have shipped in minor releases before — `Lucene90PointsFormat` went to version 1 in 10.2 (#14203) — so this is not necessarily 11.0-only, though that is the project's call. ### Verification Measured deterministically rather than by sampling byte flips, which is the method @rmuir asked for on #10396 after a byte-flipping test there was written and then disabled. Corrupting one byte at each of 102 positions across the `.fdt` of a 500-document index: | outcome | `main` | with this change | |---|---|---| | detected as chunk checksum mismatch | 0 | 87 | | some other error | 82 | 15 | | **wrong document returned, no error** | **16** | **0** | `testEveryChunkIsCovered` asserts the last row is zero and fails on `main` with `expected:<0> but was:<16>`. The residual "some other error" cases are corruption of the chunk header or of the fields index, which a payload checksum does not cover and should not. 13 tests across the three commits, each mutation-checked: bypassing the verification makes the corruption tests fail, writing a wrong checksum makes the round-trip tests fail, and removing the offset check makes the `LowercaseAsciiCompression` test fail. Also covered: `BEST_COMPRESSION`, sliced chunks larger than the chunk size, documents without term vectors, and a merge across format versions. `:lucene:core:test`, `:lucene:codecs:test`, `:lucene:backward-codecs:test` and `:lucene:memory:test` pass (10,818 tests), as do `:lucene:core:check` and `tidy`. The blocktree and terms suites were also run with `-Ptests.nightly=true -Ptests.iters=3` for the third commit. ### One thing deliberately left out `.tim` was measured for comparison, since blocktree also runs LZ4 over stored bytes. Corrupting one byte at each of 320 positions across a 67 KB `.tim` of 20,000 long shared-prefix terms gave 0.6% silently wrong, against 48.5% for `.fdt` — `.tim` is mostly metadata (its size is 0.05 of the raw term bytes) and a full `TermsEnum` scan exposes discrepancies. So no chunk checksum is proposed there. That measurement is what turned up the `LowercaseAsciiCompression` defect. -- 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]
