Fix structure checks: table placement, bias, bit width, indices, unused bits, trailing bytes

CHANGELOG.md

@@ -74,6 +74,20 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Fixed
 
+- Symbol-table placement was checked only against the whole file. Each table must
+  now lie in front of the row block, and the tables must tile that region
+  exactly, with no overlap and no gap (S1-9).
+- `Bias` values other than 0 and -2 decoded "successfully" with shifted or
+  NULL indices; they are now `UnsupportedBias` (S1-10).
+- `BitWidth` was not range-checked, so a wide field truncated the symbol index.
+  Widths above 33 (a u32 symbol count plus a bias of 2) are an error (S1-11).
+- A symbol index outside its table became a NULL in `rows()`, `to_write_table`,
+  Arrow conversion and the CLI. Every decoded column's indices are checked once
+  when the file opens, so the corruption is an error everywhere (S2-1).
+- Validation no longer scales with `NoOfRecords` when records are zero bytes
+  wide (a crafted 600-byte file declaring 2^32 - 1 rows took seconds).
+- Bits in a record that no field claims must be zero (S3-1), and bytes after the
+  row block are an error (S3-2). Both detect a misread layout.
 - XML entity references and CDATA in the header were silently deleted:
   `P&L` read as `PL` and `Customer's` as `Customers`. Predefined
   entities, numeric character references and CDATA are resolved; any other
@@ -114,6 +128,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Behavior changes
 
+- A file now fails to open if: a symbol index is out of range; a field's
+  `Bias` is not 0 or -2; its `BitWidth` is above 33; symbol tables overlap,
+  leave gaps, or reach the row block; a record has unclaimed bits set; or bytes
+  follow the row block. None of the 1,044 readable corpus files is affected.
+- `Qvd::open` does one pass over the row block to validate it. Opening with
+  `ReadOptions::columns` skips the index check for columns that are not
+  decoded; `schema()` in Python decodes none.
+
 - Files whose header lacks any numeric element, repeats one, has a non-empty
   `<Compression>`, or uses an undefined XML entity no longer open. None of the
   1,044 readable corpus files does.

crates/openqvd-py/python/openqvd/__init__.py

@@ -113,7 +113,11 @@
 
 
 def schema(path: Union[str, Path], *, leniency: LeniencyLike = None) -> Schema:
-    """Read a QVD file's metadata without decoding symbol tables or rows.
+    """Read a QVD file's metadata without decoding symbol tables.
+
+    The header and the row block are still validated (one pass over the
+    rows), so a structurally damaged file raises here, but no values are
+    decoded.
 
     Parameters
     ----------

crates/openqvd/src/error.rs

@@ -155,6 +155,78 @@ pub enum QvdError {
         body_len: usize,
     },
 
+    /// Bytes follow the row block.
+    #[error("invalid QVD structure: {extra} trailing bytes after the row block")]
+    TrailingBytes {
+        /// Number of bytes after the declared row block.
+        extra: u64,
+    },
+
+    /// A field is wider than any symbol index can need.
+    #[error(
+        "invalid QVD structure: field {field:?} BitWidth {bit_width} exceeds the maximum {max}"
+    )]
+    BitWidthOutOfRange {
+        /// Field name.
+        field: String,
+        /// `BitWidth`.
+        bit_width: u32,
+        /// The widest accepted field.
+        max: u32,
+    },
+
+    /// A field uses a `Bias` other than the two the format is known to use.
+    #[error("unsupported QVD feature: field {field:?} has Bias {bias}; only 0 and -2 are known")]
+    UnsupportedBias {
+        /// Field name.
+        field: String,
+        /// `Bias`.
+        bias: i32,
+    },
+
+    /// A symbol table lies (partly) inside the row block.
+    #[error(
+        "invalid QVD structure: field {field:?} symbol region [{start}..{end}) reaches the row block at {row_block_start}"
+    )]
+    SymbolRegionInRowBlock {
+        /// Field name.
+        field: String,
+        /// First byte of the region.
+        start: u64,
+        /// One past the last byte.
+        end: u64,
+        /// First byte of the row block.
+        row_block_start: u64,
+    },
+
+    /// Two symbol tables overlap.
+    #[error("invalid QVD structure: field {field:?} symbol region at {start} overlaps another")]
+    SymbolRegionOverlap {
+        /// Field name.
+        field: String,
+        /// First byte of the region.
+        start: u64,
+    },
+
+    /// Bytes between symbol tables, or between the last table and the row
+    /// block, belong to no field.
+    #[error("invalid QVD structure: bytes [{start}..{end}) in front of the row block belong to no symbol table")]
+    SymbolRegionGap {
+        /// First unclaimed byte.
+        start: u64,
+        /// One past the last unclaimed byte.
+        end: u64,
+    },
+
+    /// A record has a bit set that no field claims.
+    #[error("invalid QVD structure: row {row} byte {byte} has bits set outside every field")]
+    UnusedBitsSet {
+        /// Zero-based row.
+        row: u64,
+        /// Byte within the record.
+        byte: u32,
+    },
+
     /// A field's bit range does not fit in a record, or overflows.
     #[error(
         "invalid QVD structure: field {field:?} bits [{bit_offset}..+{bit_width}) exceed record size {record_bits} bits"
@@ -342,7 +414,7 @@ impl QvdError {
             | UnknownEntity { .. }
             | Xml { .. }
             | InvalidHeaderValue { .. } => ErrorKind::Header,
-            UnsupportedCompression { .. } => ErrorKind::Unsupported,
+            UnsupportedCompression { .. } | UnsupportedBias { .. } => ErrorKind::Unsupported,
             RowBlockSizeOverflow
             | RowBlockLengthMismatch { .. }
             | RowBlockOutOfRange { .. }
@@ -351,7 +423,13 @@ impl QvdError {
             | SymbolRegionOutOfRange { .. }
             | SymbolTableTruncated { .. }
             | SymbolTableTrailingBytes { .. }
-            | SymbolIndexOutOfRange { .. } => ErrorKind::Structure,
+            | SymbolIndexOutOfRange { .. }
+            | TrailingBytes { .. }
+            | BitWidthOutOfRange { .. }
+            | SymbolRegionInRowBlock { .. }
+            | SymbolRegionOverlap { .. }
+            | SymbolRegionGap { .. }
+            | UnusedBitsSet { .. } => ErrorKind::Structure,
             Truncated { .. }
             | UnterminatedString { .. }
             | UnknownSymbolType { .. }

crates/openqvd/src/reader.rs

@@ -17,6 +17,9 @@ use crate::warning::Warning;
 const MAX_PREALLOC_ROWS: usize = 256 * 1024 * 1024 / std::mem::size_of::<Cell>();
 const _: () = assert!(MAX_PREALLOC_ROWS > 0 && MAX_PREALLOC_ROWS < u32::MAX as usize);
 
+/// Widest bit field the reader accepts: a u32 symbol count plus a bias of 2.
+const MAX_BIT_WIDTH: u32 = 33;
+
 /// An in-memory QVD file with headers, symbol tables, and the packed row
 /// index block.
 ///
@@ -75,35 +78,108 @@ impl Qvd {
                 expected: expected_len,
             });
         }
-        // Bit-field coverage check.
-        let total_bits = header.record_byte_size.saturating_mul(8);
-        let mut used = vec![false; total_bits as usize];
-        for f in &header.fields {
+        // The row block must lie within the body, and nothing may follow it.
+        let row_start = u64::from(header.row_block_offset);
+        let row_end = row_start + u64::from(header.row_block_length);
+        if row_end > body.len() as u64 {
+            return Err(QvdError::RowBlockOutOfRange {
+                start: row_start,
+                end: row_end,
+                body_len: body.len(),
+            });
+        }
+        if row_end < body.len() as u64 {
+            return Err(QvdError::TrailingBytes {
+                extra: body.len() as u64 - row_end,
+            });
+        }
+
+        // Per-field bit layout. Widths are bounded by what an index can need:
+        // NoOfSymbols is a u32 and Bias is at most 2 below zero.
+        let total_bits = u64::from(header.record_byte_size) * 8;
+        let mut spans: Vec<(u64, u64, usize)> = Vec::new();
+        for (fi, f) in header.fields.iter().enumerate() {
+            if f.bit_width > MAX_BIT_WIDTH {
+                return Err(QvdError::BitWidthOutOfRange {
+                    field: f.name.clone(),
+                    bit_width: f.bit_width,
+                    max: MAX_BIT_WIDTH,
+                });
+            }
+            if !matches!(f.bias, 0 | -2) {
+                return Err(QvdError::UnsupportedBias {
+                    field: f.name.clone(),
+                    bias: f.bias,
+                });
+            }
             if f.bit_width == 0 {
                 continue;
             }
-            let out_of_range = || QvdError::FieldBitsOutOfRange {
-                field: f.name.clone(),
-                bit_offset: f.bit_offset,
-                bit_width: f.bit_width,
-                record_bits: total_bits,
-            };
-            let end = f
-                .bit_offset
-                .checked_add(f.bit_width)
-                .ok_or_else(out_of_range)?;
+            let start = u64::from(f.bit_offset);
+            let end = start + u64::from(f.bit_width);
             if end > total_bits {
-                return Err(out_of_range());
+                return Err(QvdError::FieldBitsOutOfRange {
+                    field: f.name.clone(),
+                    bit_offset: f.bit_offset,
+                    bit_width: f.bit_width,
+                    record_bits: total_bits.min(u64::from(u32::MAX)) as u32,
+                });
             }
-            for b in f.bit_offset..end {
-                if used[b as usize] {
-                    return Err(QvdError::FieldBitsOverlap {
-                        field: f.name.clone(),
-                        bit: b,
-                    });
-                }
-                used[b as usize] = true;
+            spans.push((start, end, fi));
+        }
+        spans.sort_by_key(|(start, _, _)| *start);
+        for pair in spans.windows(2) {
+            if pair[1].0 < pair[0].1 {
+                return Err(QvdError::FieldBitsOverlap {
+                    field: header.fields[pair[1].2].name.clone(),
+                    bit: pair[1].0 as u32,
+                });
+            }
+        }
+
+        // Symbol tables tile the region in front of the row block exactly:
+        // disjoint, in order, no gaps (SPEC 1.4.1).
+        let mut regions: Vec<(u64, u64, usize)> = header
+            .fields
+            .iter()
+            .enumerate()
+            .filter(|(_, f)| f.length > 0)
+            .map(|(fi, f)| {
+                let start = u64::from(f.offset);
+                (start, start + u64::from(f.length), fi)
+            })
+            .collect();
+        regions.sort_by_key(|(start, _, _)| *start);
+        let mut cursor = 0u64;
+        for (start, end, fi) in &regions {
+            let f = &header.fields[*fi];
+            if *end > row_start {
+                return Err(QvdError::SymbolRegionInRowBlock {
+                    field: f.name.clone(),
+                    start: *start,
+                    end: *end,
+                    row_block_start: row_start,
+                });
+            }
+            if *start < cursor {
+                return Err(QvdError::SymbolRegionOverlap {
+                    field: f.name.clone(),
+                    start: *start,
+                });
+            }
+            if *start > cursor {
+                return Err(QvdError::SymbolRegionGap {
+                    start: cursor,
+                    end: *start,
+                });
             }
+            cursor = *end;
+        }
+        if cursor != row_start {
+            return Err(QvdError::SymbolRegionGap {
+                start: cursor,
+                end: row_start,
+            });
         }
 
         let mut symbol_tables = Vec::with_capacity(header.fields.len());
@@ -121,25 +197,60 @@ impl Qvd {
             }
         }
 
-        // Row block must lie within the body.
-        let row_start = u64::from(header.row_block_offset);
-        let row_end = row_start + u64::from(header.row_block_length);
-        if row_end > body.len() as u64 {
-            return Err(QvdError::RowBlockOutOfRange {
-                start: row_start,
-                end: row_end,
-                body_len: body.len(),
-            });
-        }
-
-        Ok(Self {
+        let qvd = Self {
             header,
             header_raw,
             body,
             symbol_tables,
             decoded,
             warnings,
-        })
+        };
+        qvd.validate_rows(&spans)?;
+        Ok(qvd)
+    }
+
+    /// One pass over the row block: bits no field claims must be zero
+    /// (SPEC 1.4.3), and every decoded field's symbol index must exist.
+    fn validate_rows(&self, spans: &[(u64, u64, usize)]) -> Result<(), QvdError> {
+        let rbs = self.header.record_byte_size as usize;
+        let mut n = self.header.no_of_records as usize;
+        if rbs == 0 {
+            // Zero-width records carry no bits: every row is the same, so one
+            // row proves the lot. (A crafted NoOfRecords of 2^32 - 1 in a
+            // 600-byte file used to cost seconds here.)
+            n = n.min(1);
+        }
+        if n == 0 {
+            return Ok(());
+        }
+        // Mask of the bits that carry data, one byte per record byte.
+        let mut claimed = vec![0u8; rbs];
+        for (start, end, _) in spans {
+            for bit in *start..*end {
+                claimed[(bit / 8) as usize] |= 1 << (bit % 8);
+            }
+        }
+        let any_unclaimed = claimed.iter().any(|&m| m != 0xFF);
+        for idx in 0..n {
+            let rec = self.record(idx);
+            if any_unclaimed {
+                for (byte, (&b, &m)) in rec.iter().zip(&claimed).enumerate() {
+                    if b & !m != 0 {
+                        return Err(QvdError::UnusedBitsSet {
+                            row: idx as u64,
+                            byte: byte as u32,
+                        });
+                    }
+                }
+            }
+            let rec_int = le_bits_to_u128(rec);
+            for (i, f) in self.header.fields.iter().enumerate() {
+                if self.decoded[i] {
+                    self.resolve(idx, i, f, rec, rec_int)?;
+                }
+            }
+        }
+        Ok(())
     }
 
     /// Table name from `<TableName>`.

crates/openqvd/tests/edge_cases.rs

@@ -393,7 +393,7 @@ fn lf_terminator_accepted() {
 
 /// A hand-built record pointing at a symbol index that is out of range.
 #[test]
-fn rows_error_on_out_of_range_index() {
+fn out_of_range_index_is_rejected_at_open() {
     // One field, 2 bits, 2 symbols (indices 0 and 1).
     // We'll craft a row whose stored value is 3 (out of range).
     let sym_int = |v: i32| -> Vec<u8> {
@@ -441,11 +441,9 @@ fn rows_error_on_out_of_range_index() {
     bytes.extend(&sym_block);
     bytes.extend(&row_block);
 
-    let q = parse(bytes).unwrap();
-
-    // rows() reports the corruption instead of turning it into a NULL.
-    let results: Vec<_> = q.rows().collect();
-    let err = results[0].as_ref().unwrap_err();
+    // The corruption is reported when the file is opened, so neither rows()
+    // nor any conversion can turn it into a NULL.
+    let err = parse(bytes).unwrap_err();
     assert!(
         matches!(
             err,

crates/openqvd/tests/header.rs

@@ -247,7 +247,6 @@ fn earliest_terminator_wins_over_a_later_crlf_match_in_the_body() {
     v.push(0);
     v.extend_from_slice(&sym);
     v.push(0); // the row
-    v.extend_from_slice(b"\0"); // keeps the decoy followed by NUL somewhere
     let q = parse(v).unwrap();
     assert_eq!(q.num_rows(), 1);
     let rows: Vec<_> = q.rows().collect::<Result<_, _>>().unwrap();