GITHUB#16479: verify a CRC32C of each stored-fields and term-vectors chunk before decompressing it - #16480
Open
serhiy-bzhezytskyy wants to merge 3 commits into
Open
Conversation
…mpressing it A corrupt byte inside a stored-fields chunk currently surfaces as whatever the decompressor happens to do with it. On LUCENE-5267 in 2013, diagnosing exactly that, Adrien Grand proposed the fix and it was 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 thing 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 block format, which carries no checksum of its own. Each chunk now ends with a CRC32C of its compressed bytes, and the reader verifies it before handing anything to the decompressor. 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. 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, and the checksum is computed as the compressed bytes are written -- through a filtering DataOutput -- rather than by buffering them first. Format version 2, following the pattern used for Lucene90PointsFormat in 10.2 (GITHUB#14203): version 1 is read exactly as before, so no reindexing and no upgrade step is required, and an old reader rejects a version-2 segment with IndexFormatTooNewException rather than misreading it. Both directions were verified by writing an index with each and reading it with the other, including a mixed index holding segments of both versions. 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. Note that a segment which is never merged keeps the old layout, and that UpgradeIndexMergePolicy compares Version.LATEST rather than a format version, so it only forces this across a major version. Measured deterministically rather than by sampling byte flips, which is the method Robert Muir asked for on GITHUB#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. Space cost is 4 bytes per chunk: 0.024% at the 16 KB chunks of BEST_SPEED, 0.0065% at the 60 KB chunks of BEST_COMPRESSION.
…pressing it Term vectors share the chunked layout of stored fields -- same package, same Compressor, same fields index -- and had the same gap the previous commit closed there: a corrupt byte inside a chunk surfaced as whatever the decompressor did with it, or as a wrong term vector with no error at all. The change is the same shape. Each chunk now ends with a CRC32C of its compressed bytes, computed as they are written through a filtering DataOutput, and verified before the decompressor is called. 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. Term vectors format version 1. Version 0 is read exactly as before, verified by writing an index with the previous version and reading it with this one: 200 of 200 documents' vectors read back, CheckIndex clean. A segment acquires checksums when it is next merged, by the same mechanism as stored fields. One difference from stored fields is worth noting: a chunk with no fields at all writes no compressed payload, so there is nothing to checksum and nothing is written. testDocumentsWithoutVectors covers that, mixing documents that have vectors with documents that do not. Tests mirror the stored-fields ones and were each mutation-checked: bypassing the verification makes both corruption tests fail, and writing a wrong checksum makes the round-trip test fail.
…press
Step 4 of decompress restores "exceptions" -- bytes that did not fit the
compressible range -- by accumulating deltas read from the data and using the
result to index the output:
int i = 0;
for (int exception = 0; exception < numExceptions; ++exception) {
i += in.readByte() & 0xFF;
out[i] = in.readByte();
}
Nothing bounds i, so corrupt input fails with ArrayIndexOutOfBoundsException
rather than a checked IOException. This is the same shape as the LZ4 match offset
in GITHUB#16478: an offset taken from the data and used to index an array without
validation.
Found by measuring corruption in a .tim file rather than by reading the code.
Flipping one byte at each of 320 sampled positions across a 67 KB .tim of 20,000
long shared-prefix terms, two of them landed here:
ArrayIndexOutOfBoundsException @ ByteArrayDataInput.readBytes 8 2.5%
ArrayIndexOutOfBoundsException @ LowercaseAsciiCompression:158 2 0.6%
EOFException 4 1.3%
IllegalArgumentException 5 1.6%
wrong term count, no error 2 0.6%
no effect 299 93.4%
The ByteArrayDataInput cases are deliberate -- it carries the comment "NOTE:
AIOOBE not EOF if you read too much" -- so they are left alone.
The compressor cannot produce an out-of-range offset: it only records deltas
between positions it has already visited within len, and inserts artificial
exceptions when a delta would exceed 0xFF. So this only affects input that was not
produced by compress().
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.
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
ArrayIndexOutOfBoundsExceptionfrom 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 fromcheckIntegrity, which runs at merge and inCheckIndex, not when a document is fetched.This is the fix proposed on #6331 in 2013 and never built:
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
Compressorand fields index. Format version 1. One difference: a chunk with no fields writes no compressed payload, so there is nothing to checksum — covered bytestDocumentsWithoutVectors.3.
LowercaseAsciiCompression. Not a checksum, but the same class of defect, and found by measuring rather than by reading: step 4 ofdecompressaccumulates exception offsets read from the data and uses them to index the output without a bound check, so corrupt input threwArrayIndexOutOfBoundsException. 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 ofBEST_COMPRESSION. The checksum is computed as the compressed bytes are written, through a filteringDataOutput, 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:
CheckIndex clean=trueCheckIndex clean=trueIndexFormatTooNewException: 2 (needs to be between 1 and 1)A segment acquires checksums when it is next merged:
getMergeStrategydeclines the bulk-copy path for a reader whose version is notVERSION_CURRENTand 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, andUpgradeIndexMergePolicycomparesVersion.LATESTrather than a format version, so within one major version it will not force the rewrite.Format version bumps have shipped in minor releases before —
Lucene90PointsFormatwent 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
.fdtof a 500-document index:maintestEveryChunkIsCoveredasserts the last row is zero and fails onmainwithexpected:<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
LowercaseAsciiCompressiontest 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:testand:lucene:memory:testpass (10,818 tests), as do:lucene:core:checkandtidy. The blocktree and terms suites were also run with-Ptests.nightly=true -Ptests.iters=3for the third commit.One thing deliberately left out
.timwas measured for comparison, since blocktree also runs LZ4 over stored bytes. Corrupting one byte at each of 320 positions across a 67 KB.timof 20,000 long shared-prefix terms gave 0.6% silently wrong, against 48.5% for.fdt—.timis mostly metadata (its size is 0.05 of the raw term bytes) and a fullTermsEnumscan exposes discrepancies. So no chunk checksum is proposed there. That measurement is what turned up theLowercaseAsciiCompressiondefect.