Fix the record boundaries after all, and retract a claim built on the bug
I reverted the record-boundary fix last commit on the grounds that "fixed-length
records plus inter-record padding" and "variable-length records" produce the same
offset table. That reasoning was wrong, and cheaply so.
stride is the FIRST offset delta. On the 92, 88, 84, 92, 92, 92, 84, 88 chunk,
if every record were stride bytes long then records 1, 2, 6 and 7 would each run
past the START of their own successor. Records are addressed by an explicit
offset table; they cannot overlap. The padding model is not merely less likely
than variable-length, it is incoherent -- I should have checked that before
reverting a correct fix on the strength of a plausible-sounding alternative.
So: resolve now measures each record against its successor again, with the
overlap argument written into the documentation so it cannot be re-litigated
without someone noticing it is arithmetic.
THE RETRACTION, which is the substantive part:
Three tests broke. Two were straightforward, but the third was not.
* constant_positions indexed records as if they were all the same length. Now
that they are not, it compared past the short ones. It takes the minimum
length instead.
* "1,509 of 1,509 chunks have uniform record length" -- which was true only
because the parser sliced every record at one stride, so the test was
confirming the parser's behaviour back to itself. It now asserts 257 uniform
and 1,252 ragged, and the sum is checked against 1,509 so neither number can
drift alone.
* moby_class_ids_sit_in_one_slot_per_chunk_type asserted >= 20 chunk types
carry a slot of moby class IDs at 38x-289x enrichment. It now finds 2, of
which 1 clears 100x. RETRACTED. That test scored records that carried 4 to 8
bytes of the NEXT record, which read as extra fields and lifted every slot's
apparent hit rate. The general claim -- one class-ID slot per chunk type
across the container -- does not hold. The test asserts 2, asserts 1 strong,
and asserts the count has NOT returned to 20, so the retraction cannot quietly
lapse back into the claim.
That is a committed finding withdrawn on the strength of a parser bug. It is also
the second time in this area that a clean number -- >= 20 types, 1509 uniform --
was really the parser describing itself.
243 tests, clippy clean.
This commit is contained in:
@@ -103,12 +103,12 @@ pub const CHUNK_REGION_MARKER: [u8; 2] = [0xf6, 0x7c];
|
||||
/// A chunk header plus the records its offsets address.
|
||||
///
|
||||
/// `stride` is the distance between the first two resolved offsets, and nothing
|
||||
/// more. The parser takes each offset literally rather than assuming a uniform
|
||||
/// stride, because the deltas are not constant. On level 0 some chunks step
|
||||
/// 340, 340, 336, 340; on all 27 levels signature `[75 42 ef f2 71 b5]` steps
|
||||
/// 92, 88, 84, 92, 92, 92, 84, 88, which is a real variation in record length
|
||||
/// and not a rounding artifact. Record *lengths* come from the deltas, in
|
||||
/// [`resolve`].
|
||||
/// more. The deltas within a chunk are **not** constant, and the variation is
|
||||
/// real: on level 0 some chunks step 340, 340, 336, 340, and on all 27 levels
|
||||
/// signature `[75 42 ef f2 71 b5]` steps 92, 88, 84, 92, 92, 92, 84, 88. Taking
|
||||
/// every record at `stride` would overlap four of those eight with their own
|
||||
/// successors, which an explicit offset table cannot express. Record *lengths*
|
||||
/// come from the deltas; see [`resolve`].
|
||||
pub struct Chunk {
|
||||
/// The 16 opaque header bytes.
|
||||
pub header: [u8; CHUNK_HEADER_BYTES],
|
||||
@@ -391,18 +391,26 @@ pub fn parse_chunk(region: &[u8], offset: usize) -> Result<Option<Chunk>> {
|
||||
|
||||
/// Resolve every record a chunk addresses, bounds-checked against `region`.
|
||||
///
|
||||
/// Each record is `region[chunk.offset + low .. + stride]`, and `stride` is the
|
||||
/// chunk's first offset delta.
|
||||
/// A record runs from its own offset to the **next** record's offset, not to
|
||||
/// `chunk.offset + stride`.
|
||||
///
|
||||
/// **This may slice past a record's real end.** 1,252 of the 1,509 chunks have
|
||||
/// offsets that are not uniformly spaced -- the deviations are word-aligned
|
||||
/// multiples of 4, mostly +/-4 -- so for those the last few bytes of a slice can
|
||||
/// be the head of the next record. Whether that means the records are genuinely
|
||||
/// variable-length, or that the offset table has inter-record padding, **cannot
|
||||
/// be told from the offsets alone**: both models produce exactly this table.
|
||||
/// Deciding between them needs a field whose end is independently observable, so
|
||||
/// until that exists this function takes the uniform reading and the ambiguity
|
||||
/// is recorded rather than guessed at. See `docs/formats/core-data-regions.md`.
|
||||
/// This is not a stylistic choice. `stride` is the *first* offset delta, and on
|
||||
/// 1,252 of the 1,509 measured chunks later deltas are smaller -- the deviations
|
||||
/// are word-aligned multiples of 4, most exactly +/-4. Slicing every record at
|
||||
/// the first delta would therefore make some records run past the start of their
|
||||
/// own successor. On the measured `92, 88, 84, 92, 92, 92, 84, 88` chunk, four
|
||||
/// of the eight records overlap the next one that way, and records are addressed
|
||||
/// by an explicit offset table, so they cannot overlap. The only coherent
|
||||
/// reading is that a record ends where the next begins.
|
||||
///
|
||||
/// Slicing at `stride` was not wrong in a way that would error; it handed four
|
||||
/// records 4 to 8 bytes of the following record, which is the kind of defect
|
||||
/// that surfaces much later as fields that look meaningful and are quietly off
|
||||
/// by four bytes.
|
||||
///
|
||||
/// The last record has no successor to measure against and falls back to
|
||||
/// `stride`; on the measured levels the final delta is itself uniform, so this
|
||||
/// is a fallback rather than a silent guess.
|
||||
///
|
||||
/// A record that would run past the end of the region is dropped rather than
|
||||
/// clamped, so the returned length may be shorter than `chunk.offsets` -- on the
|
||||
@@ -410,7 +418,7 @@ pub fn parse_chunk(region: &[u8], offset: usize) -> Result<Option<Chunk>> {
|
||||
/// malformed chunk rather than report it.
|
||||
pub fn resolve<'r>(chunk: &Chunk, region: &'r [u8]) -> Result<ResolvedChunk<'r>> {
|
||||
let mut records = Vec::with_capacity(chunk.offsets.len());
|
||||
for &low in &chunk.offsets {
|
||||
for (i, &low) in chunk.offsets.iter().enumerate() {
|
||||
let start = chunk
|
||||
.offset
|
||||
.checked_add(low as usize)
|
||||
@@ -419,10 +427,14 @@ pub fn resolve<'r>(chunk: &Chunk, region: &'r [u8]) -> Result<ResolvedChunk<'r>>
|
||||
context: "chunk record",
|
||||
kind: ErrorKind::Invalid("chunk offset overflows usize"),
|
||||
})?;
|
||||
if start + chunk.stride > region.len() {
|
||||
let len = match chunk.offsets.get(i + 1) {
|
||||
Some(&next) => (next - low) as usize,
|
||||
None => chunk.stride,
|
||||
};
|
||||
if start + len > region.len() {
|
||||
continue;
|
||||
}
|
||||
records.push(®ion[start..start + chunk.stride]);
|
||||
records.push(®ion[start..start + len]);
|
||||
}
|
||||
Ok(ResolvedChunk {
|
||||
chunk: Chunk {
|
||||
|
||||
Reference in New Issue
Block a user