From 0289ec2f06252a969a9dd195d047f1972b5aba39 Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:13:00 +0000 Subject: [PATCH 1/4] Keep and verify page checksums A page whose fNElements is negative has an XXH3-64 checksum stored little-endian right after it, over the stored (sealed) bytes. The locator's size does not include it (spec, Page Locations; ROOT's reader adds the eight bytes back itself, RPageStorage.cxx:297). rootfilespec neither read nor checked it. - RPageDescription keeps fNElements as stored (the sign is the flag, and the stored value is what reproduces the bytes) and gains n_elements, has_checksum and stored_size. size stays the locator's size, as the spec defines it. - RPageDescription.page_locator is a new RPageLocator covering the stored bytes (offset, stored_size). Its read_from verifies the checksum and returns an RPage with the raw bytes and the checksum; nothing is decompressed. PageListEnvelope.page_locators returns these, and get_page uses it. Fixes #55 Assisted-by: claude-code:claude-opus-5-5 --- src/rootfilespec/rntuple/RPage.py | 4 +- src/rootfilespec/rntuple/pagelist.py | 8 +- src/rootfilespec/rntuple/pagelocations.py | 84 ++++++++++++++-- tests/test_rntuple_pages.py | 114 ++++++++++++++++++++++ 4 files changed, 200 insertions(+), 10 deletions(-) create mode 100644 tests/test_rntuple_pages.py diff --git a/src/rootfilespec/rntuple/RPage.py b/src/rootfilespec/rntuple/RPage.py index 6e45576..3f6c6db 100644 --- a/src/rootfilespec/rntuple/RPage.py +++ b/src/rootfilespec/rntuple/RPage.py @@ -8,7 +8,9 @@ class RPage(ROOTSerializable): """A class to represent an RNTuple page.""" page: bytes - """The RNTuple page raw data.""" + """The RNTuple page raw data, as stored (still compressed if the page is).""" + checksum: int | None = None + """The page's XXH3-64 checksum, verified against ``page``, or None if it has none.""" # TODO: Flush out RPage class @classmethod diff --git a/src/rootfilespec/rntuple/pagelist.py b/src/rootfilespec/rntuple/pagelist.py index ce4182d..982563c 100644 --- a/src/rootfilespec/rntuple/pagelist.py +++ b/src/rootfilespec/rntuple/pagelist.py @@ -8,6 +8,7 @@ from rootfilespec.rntuple.pagelocations import ( PageLocations, RPageDescription, + RPageLocator, ) from rootfilespec.rntuple.RFrame import ListFrame, RecordFrame from rootfilespec.rntuple.RPage import RPage @@ -66,16 +67,19 @@ class PageListEnvelope(REnvelope): """The Page Locations Triple Nested List Frame""" @property - def page_locators(self) -> list[list[list[RPageDescription]]]: + def page_locators(self) -> list[list[list[RPageLocator]]]: """Get locators for all pages in this page list. + Each locator covers a page's stored bytes, checksum included, and verifies + the checksum when read (see ``RPageDescription.page_locator``). + Returns a triple-nested list structure: - Top level: clusters - Middle level: columns - Inner level: pages """ return [ - [list(pagelist) for pagelist in columnlist] + [[page.page_locator for page in pagelist] for pagelist in columnlist] for columnlist in self.pageLocations ] diff --git a/src/rootfilespec/rntuple/pagelocations.py b/src/rootfilespec/rntuple/pagelocations.py index 677b8f1..84c353b 100644 --- a/src/rootfilespec/rntuple/pagelocations.py +++ b/src/rootfilespec/rntuple/pagelocations.py @@ -1,6 +1,9 @@ from collections.abc import Callable +from dataclasses import dataclass from typing import Annotated, cast +import xxhash + from rootfilespec.bootstrap.compression import RCompressionSettings from rootfilespec.rntuple.RFrame import Item, ListFrame from rootfilespec.rntuple.RLocator import RLocator @@ -13,6 +16,46 @@ ) from rootfilespec.structutil import Fmt, OptionalField +PAGE_CHECKSUM_SIZE = 8 +"""A page checksum is an XXH3-64, stored little-endian right after the page""" + + +@dataclass(frozen=True) +class RPageLocator: + """The locator of a page's stored bytes: the page and, if it has one, its checksum + + ``size`` is the stored size, so fetching ``(offset, size)`` gets everything + ``read_from`` needs. ``read_from`` verifies the checksum over the stored + (sealed, possibly compressed) bytes and does not decompress the page. + """ + + offset: int + """The byte offset of the page in the file.""" + size: int + """The stored size: the page, plus the checksum if it has one.""" + has_checksum: bool + """Whether an XXH3-64 checksum follows the page.""" + + def read_from(self, buffer: ReadBuffer) -> RPage: + if len(buffer) != self.size: + msg = ( + f"RPageLocator.read_from: expected {self.size} bytes, got {len(buffer)}" + ) + raise ValueError(msg) + if not self.has_checksum: + page, _ = RPage.read(buffer) + return page + data, buffer = buffer.consume(self.size - PAGE_CHECKSUM_SIZE) + (checksum,), buffer = buffer.unpack(" int: @property def size(self) -> int: - """The (compressed) size of the page data.""" + """The (compressed) size of the page data, as in the locator. + + As the spec says, it does not include the checksum; see ``stored_size``.""" return self.locator.size + @property + def n_elements(self) -> int: + """The number of elements in the page.""" + return abs(self.fNElements) + + @property + def has_checksum(self) -> bool: + """Whether an XXH3-64 checksum is stored right after the page.""" + return self.fNElements < 0 + + @property + def stored_size(self) -> int: + """The number of bytes the page takes in the file, checksum included.""" + return self.size + (PAGE_CHECKSUM_SIZE if self.has_checksum else 0) + + @property + def page_locator(self) -> RPageLocator: + """A locator for the page's stored bytes, which verifies the checksum.""" + return RPageLocator(self.offset, self.stored_size, self.has_checksum) + def read_from(self, buffer: ReadBuffer) -> RPage: - """Read the page from the given buffer. + """Read the page from the given buffer, without its checksum. - Pages are wrapped in compression blocks (like envelopes). + Pages are wrapped in compression blocks (like envelopes). Prefer + ``page_locator``, which also reads and verifies the checksum. """ #### Read the page from the buffer page, buffer = RPage.read(buffer) @@ -63,11 +133,11 @@ def read_from(self, buffer: ReadBuffer) -> RPage: def get_page( self, fetch_data: Callable[[Locator[ROOTSerializable]], ReadBuffer] ) -> RPage: - """Reads the page data from the data source using the locator. + """Reads the page, and verifies its checksum, using ``page_locator``. Pages are wrapped in compression blocks (like envelopes). """ - buffer = fetch_data(self) - return self.read_from(buffer) + loc = self.page_locator + return loc.read_from(fetch_data(loc)) @serializable diff --git a/tests/test_rntuple_pages.py b/tests/test_rntuple_pages.py new file mode 100644 index 0000000..780668d --- /dev/null +++ b/tests/test_rntuple_pages.py @@ -0,0 +1,114 @@ +from pathlib import Path + +import pytest +import xxhash + +from rootfilespec.bootstrap import BOOTSTRAP_CONTEXT +from rootfilespec.reader import open_path +from rootfilespec.rntuple.pagelocations import RPageDescription, RPageLocator +from rootfilespec.rntuple.RNTuple import RNTuple +from rootfilespec.serializable import BufferContext, ReadBuffer + +DATA = Path(__file__).parent.parent / "reference" / "root-io-spec" / "data" / "rntuple" +pytestmark = pytest.mark.skipif( + not DATA.exists(), reason="reference/root-io-spec not checked out" +) + + +def _page_descriptions(path: Path) -> list[RPageDescription]: + out: list[RPageDescription] = [] + with open_path(path) as reader: + keylist = reader.keylist() + for name in keylist: + key = keylist[name] + if key.fClassName.fString != b"ROOT::RNTuple": + continue + rntuple = RNTuple.from_anchor(reader.fetch(key), reader.fetch.buffer) + for pagelist in rntuple.pagelistEnvelopes: + for cluster in pagelist.pageLocations: + for column in cluster: + out.extend(column) + return out + + +def _buffer(raw: bytes, offset: int, size: int) -> ReadBuffer: + return ReadBuffer( + memoryview(raw[offset : offset + size]), + 0, + BOOTSTRAP_CONTEXT, + BufferContext(abspos=offset), + ) + + +def test_page_with_checksum(): + """Issue #55: the page at 550 in rntuple/anchor.root, checked against its bytes + + Its fNElements is -3 (the sign flags a checksum), its locator says 12 bytes, + and the XXH3-64 of those 12 bytes is stored little-endian at 562..570. + """ + path = DATA / "anchor.root" + raw = path.read_bytes() + (page,) = [p for p in _page_descriptions(path) if p.offset == 550] + assert page.fNElements == -3 + assert page.n_elements == 3 + assert page.has_checksum + assert page.size == 12 + assert page.stored_size == 20 + + loc = page.page_locator + assert (loc.offset, loc.size, loc.has_checksum) == (550, 20, True) + assert raw[562:570].hex() == "de3ce2c4a5a407be" + read = loc.read_from(_buffer(raw, loc.offset, loc.size)) + assert read.page == raw[550:562] + assert read.checksum == int.from_bytes(raw[562:570], "little") + assert read.checksum == xxhash.xxh3_64_intdigest(raw[550:562]) + + +def test_corrupted_page_raises(): + path = DATA / "anchor.root" + raw = bytearray(path.read_bytes()) + (page,) = [p for p in _page_descriptions(path) if p.offset == 550] + raw[555] ^= 0x01 + loc = page.page_locator + with pytest.raises(ValueError, match="Page checksum mismatch at offset 550"): + loc.read_from(_buffer(bytes(raw), loc.offset, loc.size)) + + +def test_wrong_length_raises(): + loc = RPageLocator(offset=0, size=20, has_checksum=True) + with pytest.raises(ValueError, match="expected 20 bytes"): + loc.read_from(_buffer(bytes(12), 0, 12)) + + +def test_page_without_checksum(): + raw = b"0123456789" + page = RPageLocator(offset=0, size=10, has_checksum=False).read_from( + _buffer(raw, 0, 10) + ) + assert page.page == raw + assert page.checksum is None + + +@pytest.mark.parametrize("name", sorted(p.name for p in DATA.glob("*.root"))) +def test_every_page_verifies(name: str): + """Every page of every root-io-spec RNTuple fixture passes its checksum + + Pages are read by their stored bytes, never decompressed, and several page + descriptions may name the same bytes (same-page merging): each still reads. + """ + path = DATA / name + raw = path.read_bytes() + pages = _page_descriptions(path) + assert pages + for page in pages: + loc = page.page_locator + assert loc.offset + loc.size <= len(raw) + read = loc.read_from(_buffer(raw, loc.offset, loc.size)) + assert read.checksum is not None + assert len(read.page) == page.size + + +def test_shared_page_ranges(): + """rntuple/map.root has page descriptions that name the same bytes""" + ranges = [(p.offset, p.stored_size) for p in _page_descriptions(DATA / "map.root")] + assert len(set(ranges)) < len(ranges) From 2c308fe29c13f95e19c9156ac0bc4b68edf5d04b Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:34:54 +0000 Subject: [PATCH 2/4] Ignore the missing xxhash stubs in the pre-commit mypy environment The pre-commit mypy hook installs only pytest, numpy and tomli, so it cannot find xxhash. bootstrap/compression.py already imports it the same way. Assisted-by: claude-code:claude-opus-5-5 --- src/rootfilespec/rntuple/pagelocations.py | 2 +- tests/test_rntuple_pages.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rootfilespec/rntuple/pagelocations.py b/src/rootfilespec/rntuple/pagelocations.py index 84c353b..5d14d8b 100644 --- a/src/rootfilespec/rntuple/pagelocations.py +++ b/src/rootfilespec/rntuple/pagelocations.py @@ -2,7 +2,7 @@ from dataclasses import dataclass from typing import Annotated, cast -import xxhash +import xxhash # type: ignore[import-not-found] from rootfilespec.bootstrap.compression import RCompressionSettings from rootfilespec.rntuple.RFrame import Item, ListFrame diff --git a/tests/test_rntuple_pages.py b/tests/test_rntuple_pages.py index 780668d..140b28c 100644 --- a/tests/test_rntuple_pages.py +++ b/tests/test_rntuple_pages.py @@ -1,7 +1,7 @@ from pathlib import Path import pytest -import xxhash +import xxhash # type: ignore[import-not-found] from rootfilespec.bootstrap import BOOTSTRAP_CONTEXT from rootfilespec.reader import open_path From 13224db92d3504427a30cc9a2669318ccde38868 Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Wed, 30 Sep 2026 14:16:23 +0000 Subject: [PATCH 3/4] Rename tests/test_rntuple_pages.py to tests/test_page_checksums.py Name the module after what it tests, so it does not read as the home of a broad set of tests (review on #125). Assisted-by: claude-code:claude-opus-5-5 --- tests/{test_rntuple_pages.py => test_page_checksums.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/{test_rntuple_pages.py => test_page_checksums.py} (100%) diff --git a/tests/test_rntuple_pages.py b/tests/test_page_checksums.py similarity index 100% rename from tests/test_rntuple_pages.py rename to tests/test_page_checksums.py From 94c6b0067b52670ee4a9d342c59bf4b84555773c Mon Sep 17 00:00:00 2001 From: Samantha Abbott <52800387+sathabbott@users.noreply.github.com> Date: Wed, 30 Sep 2026 16:57:45 -0500 Subject: [PATCH 4/4] Verify page checksums in RPageDescription, drop RPageLocator RPageDescription already is the page's locator. Instead of a second locator class next to it, it now fetches and verifies the checksum itself, as suggested on #55: - size is the number of bytes to fetch: locator.size, plus the 8-byte checksum when fNElements is negative. The spec's page size, which excludes the checksum (Page Locations), stays in locator.size. - read_from splits off the trailer, verifies XXH3-64 over the stored bytes and returns RPage(page, checksum). get_page goes through it. RPageLocator flattened the RLocator into an in-file (offset, size) and left RPageDescription as a second, unverified way to read a page. Keeping the RLocator leaves room for further locator types (the spec reserves 0x02-0x7f), and the checksum check depends only on the fetched bytes. PageListEnvelope.page_locators is unchanged from main, so this PR no longer changes any API. Assisted-by: claude-code:claude-opus-5-5 --- src/rootfilespec/rntuple/pagelist.py | 8 +- src/rootfilespec/rntuple/pagelocations.py | 89 +++++++---------------- tests/test_page_checksums.py | 43 ++++++----- 3 files changed, 50 insertions(+), 90 deletions(-) diff --git a/src/rootfilespec/rntuple/pagelist.py b/src/rootfilespec/rntuple/pagelist.py index 982563c..ce4182d 100644 --- a/src/rootfilespec/rntuple/pagelist.py +++ b/src/rootfilespec/rntuple/pagelist.py @@ -8,7 +8,6 @@ from rootfilespec.rntuple.pagelocations import ( PageLocations, RPageDescription, - RPageLocator, ) from rootfilespec.rntuple.RFrame import ListFrame, RecordFrame from rootfilespec.rntuple.RPage import RPage @@ -67,19 +66,16 @@ class PageListEnvelope(REnvelope): """The Page Locations Triple Nested List Frame""" @property - def page_locators(self) -> list[list[list[RPageLocator]]]: + def page_locators(self) -> list[list[list[RPageDescription]]]: """Get locators for all pages in this page list. - Each locator covers a page's stored bytes, checksum included, and verifies - the checksum when read (see ``RPageDescription.page_locator``). - Returns a triple-nested list structure: - Top level: clusters - Middle level: columns - Inner level: pages """ return [ - [[page.page_locator for page in pagelist] for pagelist in columnlist] + [list(pagelist) for pagelist in columnlist] for columnlist in self.pageLocations ] diff --git a/src/rootfilespec/rntuple/pagelocations.py b/src/rootfilespec/rntuple/pagelocations.py index 5d14d8b..dc1e7dd 100644 --- a/src/rootfilespec/rntuple/pagelocations.py +++ b/src/rootfilespec/rntuple/pagelocations.py @@ -1,5 +1,4 @@ from collections.abc import Callable -from dataclasses import dataclass from typing import Annotated, cast import xxhash # type: ignore[import-not-found] @@ -20,43 +19,6 @@ """A page checksum is an XXH3-64, stored little-endian right after the page""" -@dataclass(frozen=True) -class RPageLocator: - """The locator of a page's stored bytes: the page and, if it has one, its checksum - - ``size`` is the stored size, so fetching ``(offset, size)`` gets everything - ``read_from`` needs. ``read_from`` verifies the checksum over the stored - (sealed, possibly compressed) bytes and does not decompress the page. - """ - - offset: int - """The byte offset of the page in the file.""" - size: int - """The stored size: the page, plus the checksum if it has one.""" - has_checksum: bool - """Whether an XXH3-64 checksum follows the page.""" - - def read_from(self, buffer: ReadBuffer) -> RPage: - if len(buffer) != self.size: - msg = ( - f"RPageLocator.read_from: expected {self.size} bytes, got {len(buffer)}" - ) - raise ValueError(msg) - if not self.has_checksum: - page, _ = RPage.read(buffer) - return page - data, buffer = buffer.consume(self.size - PAGE_CHECKSUM_SIZE) - (checksum,), buffer = buffer.unpack(" int: @property def size(self) -> int: - """The (compressed) size of the page data, as in the locator. + """The number of bytes to fetch: the page as stored, then its checksum if it has one. - As the spec says, it does not include the checksum; see ``stored_size``.""" - return self.locator.size + The spec's page size, which excludes the checksum, is ``locator.size``.""" + return self.locator.size + (PAGE_CHECKSUM_SIZE if self.has_checksum else 0) @property def n_elements(self) -> int: @@ -105,39 +67,42 @@ def has_checksum(self) -> bool: """Whether an XXH3-64 checksum is stored right after the page.""" return self.fNElements < 0 - @property - def stored_size(self) -> int: - """The number of bytes the page takes in the file, checksum included.""" - return self.size + (PAGE_CHECKSUM_SIZE if self.has_checksum else 0) - - @property - def page_locator(self) -> RPageLocator: - """A locator for the page's stored bytes, which verifies the checksum.""" - return RPageLocator(self.offset, self.stored_size, self.has_checksum) - def read_from(self, buffer: ReadBuffer) -> RPage: - """Read the page from the given buffer, without its checksum. + """Read the page from the given buffer, and verify its checksum if it has one. - Pages are wrapped in compression blocks (like envelopes). Prefer - ``page_locator``, which also reads and verifies the checksum. + The buffer holds ``size`` bytes. The checksum covers the page as stored + (sealed, possibly compressed), wherever those bytes were fetched from. + Pages are wrapped in compression blocks (like envelopes); nothing is + decompressed here. """ + if len(buffer) != self.size: + msg = f"RPageDescription.read_from: expected {self.size} bytes, got {len(buffer)}" + raise ValueError(msg) + #### Read the page from the buffer - page, buffer = RPage.read(buffer) + if not self.has_checksum: + page, _ = RPage.read(buffer) + return page - if buffer: - msg = "RPageDescription.read_from: buffer not empty after reading page." + data, buffer = buffer.consume(self.locator.size) + (checksum,), buffer = buffer.unpack(" RPage: - """Reads the page, and verifies its checksum, using ``page_locator``. + """Reads the page data from the data source using the locator. Pages are wrapped in compression blocks (like envelopes). """ - loc = self.page_locator - return loc.read_from(fetch_data(loc)) + buffer = fetch_data(self) + return self.read_from(buffer) @serializable diff --git a/tests/test_page_checksums.py b/tests/test_page_checksums.py index 140b28c..bf2bed0 100644 --- a/tests/test_page_checksums.py +++ b/tests/test_page_checksums.py @@ -5,7 +5,8 @@ from rootfilespec.bootstrap import BOOTSTRAP_CONTEXT from rootfilespec.reader import open_path -from rootfilespec.rntuple.pagelocations import RPageDescription, RPageLocator +from rootfilespec.rntuple.pagelocations import RPageDescription +from rootfilespec.rntuple.RLocator import StandardLocator from rootfilespec.rntuple.RNTuple import RNTuple from rootfilespec.serializable import BufferContext, ReadBuffer @@ -44,7 +45,8 @@ def test_page_with_checksum(): """Issue #55: the page at 550 in rntuple/anchor.root, checked against its bytes Its fNElements is -3 (the sign flags a checksum), its locator says 12 bytes, - and the XXH3-64 of those 12 bytes is stored little-endian at 562..570. + and the XXH3-64 of those 12 bytes is stored little-endian at 562..570. The + page description's size covers both, so one fetch gets what read_from needs. """ path = DATA / "anchor.root" raw = path.read_bytes() @@ -52,13 +54,11 @@ def test_page_with_checksum(): assert page.fNElements == -3 assert page.n_elements == 3 assert page.has_checksum - assert page.size == 12 - assert page.stored_size == 20 + assert page.locator.size == 12 + assert page.size == 20 - loc = page.page_locator - assert (loc.offset, loc.size, loc.has_checksum) == (550, 20, True) assert raw[562:570].hex() == "de3ce2c4a5a407be" - read = loc.read_from(_buffer(raw, loc.offset, loc.size)) + read = page.read_from(_buffer(raw, page.offset, page.size)) assert read.page == raw[550:562] assert read.checksum == int.from_bytes(raw[562:570], "little") assert read.checksum == xxhash.xxh3_64_intdigest(raw[550:562]) @@ -69,24 +69,24 @@ def test_corrupted_page_raises(): raw = bytearray(path.read_bytes()) (page,) = [p for p in _page_descriptions(path) if p.offset == 550] raw[555] ^= 0x01 - loc = page.page_locator - with pytest.raises(ValueError, match="Page checksum mismatch at offset 550"): - loc.read_from(_buffer(bytes(raw), loc.offset, loc.size)) + with pytest.raises(ValueError, match=r"Page checksum mismatch at .*offset=550"): + page.read_from(_buffer(bytes(raw), page.offset, page.size)) def test_wrong_length_raises(): - loc = RPageLocator(offset=0, size=20, has_checksum=True) + page = RPageDescription(-3, StandardLocator(12, 0)) with pytest.raises(ValueError, match="expected 20 bytes"): - loc.read_from(_buffer(bytes(12), 0, 12)) + page.read_from(_buffer(bytes(12), 0, 12)) def test_page_without_checksum(): raw = b"0123456789" - page = RPageLocator(offset=0, size=10, has_checksum=False).read_from( - _buffer(raw, 0, 10) - ) - assert page.page == raw - assert page.checksum is None + page = RPageDescription(3, StandardLocator(10, 0)) + assert not page.has_checksum + assert page.size == 10 + read = page.read_from(_buffer(raw, 0, 10)) + assert read.page == raw + assert read.checksum is None @pytest.mark.parametrize("name", sorted(p.name for p in DATA.glob("*.root"))) @@ -101,14 +101,13 @@ def test_every_page_verifies(name: str): pages = _page_descriptions(path) assert pages for page in pages: - loc = page.page_locator - assert loc.offset + loc.size <= len(raw) - read = loc.read_from(_buffer(raw, loc.offset, loc.size)) + assert page.offset + page.size <= len(raw) + read = page.read_from(_buffer(raw, page.offset, page.size)) assert read.checksum is not None - assert len(read.page) == page.size + assert len(read.page) == page.locator.size def test_shared_page_ranges(): """rntuple/map.root has page descriptions that name the same bytes""" - ranges = [(p.offset, p.stored_size) for p in _page_descriptions(DATA / "map.root")] + ranges = [(p.offset, p.size) for p in _page_descriptions(DATA / "map.root")] assert len(set(ranges)) < len(ranges)