From 30cea8640be6b02a0fc672e14db299b2a92d71ff Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 07:30:25 +0200 Subject: [PATCH] fix: validate PDF cross-reference targets before caching objects --- CHANGELOG.md | 3 ++ src/odr/internal/pdf/AGENTS.md | 3 ++ src/odr/internal/pdf/pdf_document_parser.cpp | 12 ++++-- src/odr/internal/pdf/pdf_document_parser.hpp | 21 ++-------- test/src/internal/pdf/pdf_document_parser.cpp | 42 +++++++++++++++++++ 5 files changed, 59 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 412ee7a54..9bd439f9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- PDF object reads reject mismatched cross-reference targets and recover + from a stream error before seeking to another object. + - PDF incremental updates keep deleted and replaced objects from resolving through older generations. Hybrid tables discard superseded free entries. diff --git a/src/odr/internal/pdf/AGENTS.md b/src/odr/internal/pdf/AGENTS.md index daa8917ea..8b7d074c2 100644 --- a/src/odr/internal/pdf/AGENTS.md +++ b/src/odr/internal/pdf/AGENTS.md @@ -61,6 +61,9 @@ output. `std::streambuf`. Random access seeks, sequential tokenizing peeks one char. An object parses when referenced and is cached by reference. Positions are `std::uint32_t`, so a file of 4 GiB or more is out of scope. + Parsers live for one render. Their caches retain whole decoded object streams, + so reading another member does not inflate or parse that stream again. + Object IDs and generations must match the cross-reference entry. - **Fail early on malformed structure, tolerate unknown content.** A missing `obj`, `endobj`, `stream`, `endstream`, `xref` or `startxref`, an unexpected char, an unknown page-tree type or an exhausted stream throws diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index 8427d3805..53d32d0b4 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -1637,8 +1637,13 @@ DocumentParser::read_object(const ObjectReference &reference) { << " not in cross-reference table, " "treating as null"); } else if (const Xref::Entry &entry = entry_it->second; entry.is_used()) { + in().clear(); in().seekg(entry.as_used().position); object = parser().read_indirect_object(); + if (object.reference != reference) { + throw std::runtime_error( + "PDF object reference does not match its xref entry"); + } // Decrypt string leaves (7.6.2). The /Encrypt dictionary needs no special // case here: it is read and cached during construction before the // decryptor is installed, so its un-decrypted /O,/U,… strings are served @@ -1657,10 +1662,8 @@ DocumentParser::read_object(const ObjectReference &reference) { throw std::runtime_error("object stream member index out of range"); } if (members[index].id != reference.id) { - ODR_WARNING(m_logger, "pdf: object stream " - << stream_id << " member " << index - << " has id " << members[index].id - << ", expected " << reference.id); + throw std::runtime_error( + "PDF object stream member does not match its xref entry"); } object.object = members[index].object; } else { @@ -1731,6 +1734,7 @@ std::string DocumentParser::read_object_stream(const IndirectObject &object) { Object length = object.object.as_dictionary().get("Length"); resolve_object(length); + in().clear(); // a stream object always carries a stream position // NOLINTNEXTLINE(bugprone-unchecked-optional-access) in().seekg(object.stream_position.value()); diff --git a/src/odr/internal/pdf/pdf_document_parser.hpp b/src/odr/internal/pdf/pdf_document_parser.hpp index c063edbcf..a102e6dd9 100644 --- a/src/odr/internal/pdf/pdf_document_parser.hpp +++ b/src/odr/internal/pdf/pdf_document_parser.hpp @@ -23,26 +23,11 @@ namespace odr::internal::pdf { struct Document; -/// Resolution/memoization layer on top of the sequential `FileParser`: maps -/// `ObjectReference`s to file positions via the cross-reference table and hands -/// out resolved objects by reference. -/// -/// The caches are intrinsic, not a convenience: reading one compressed object -/// means inflating and parsing the whole object stream holding it, and only -/// this class knows from the xref which objects share one. They grow -/// monotonically and are never evicted, which is safe only because a -/// `DocumentParser` is transient per render — build one, produce a `Document`, -/// read its lazy streams, discard it. Never share one across documents. +/// Resolves and caches objects for one PDF; construct a parser per render. class DocumentParser { public: - /// Takes ownership of the input stream (move-only: the parser is the - /// top-level handle for reading a PDF, so the stream's lifetime is tied to - /// it). The constructor walks the trailer chain and, if the file declares an - /// `/Encrypt` dictionary, builds the `Authenticator`. Pass an already - /// authenticated `decryptor` (from a prior `authenticate()` / `decryptor()`) - /// to re-open an encrypted file without the password; otherwise pass - /// `std::nullopt` to parse an unencrypted file or to call `authenticate()` - /// later. The logger is borrowed and must outlive the parser. + /// Owns the input and accepts a decryptor from an authenticated parser. + /// Otherwise probes the empty password; authenticate() unlocks later reads. explicit DocumentParser(std::unique_ptr in, std::optional decryptor = std::nullopt, const Logger &logger = Logger::null()); diff --git a/test/src/internal/pdf/pdf_document_parser.cpp b/test/src/internal/pdf/pdf_document_parser.cpp index 35f1efcea..a25c3c48d 100644 --- a/test/src/internal/pdf/pdf_document_parser.cpp +++ b/test/src/internal/pdf/pdf_document_parser.cpp @@ -790,3 +790,45 @@ TEST(DocumentParser, deleted_objects_do_not_resolve_to_an_older_generation) { EXPECT_TRUE(parser.read_object(ObjectReference{3, 0}).object.is_null()); EXPECT_TRUE(parser.read_object(ObjectReference{3, 1}).object.is_null()); } + +TEST(DocumentParser, rejects_mismatched_indirect_object_references) { + for (const std::string header : {"9 0 obj", "1 1 obj"}) { + std::string source = two_object_mini_pdf(true); + source.replace(source.find("1 0 obj"), header.size(), header); + DocumentParser parser(std::make_unique(source)); + ASSERT_FALSE(parser.is_recovered()); + EXPECT_THROW((void)parser.read_object(ObjectReference{1, 0}), + std::runtime_error); + } +} + +TEST(DocumentParser, object_reads_reset_stream_errors) { + DocumentParser parser( + std::make_unique(two_object_mini_pdf(true))); + parser.in().setstate(std::ios::eofbit | std::ios::failbit); + EXPECT_EQ(parser.read_object(ObjectReference{1, 0}) + .object.as_dictionary() + .get("Type") + .as_name(), + "Catalog"); + for (std::uint32_t attempt = 0; attempt < 2; ++attempt) { + parser.in().setstate(std::ios::eofbit | std::ios::failbit); + EXPECT_EQ(parser.read_decoded_stream(ObjectReference{4, 0}), "BT ET"); + } +} + +TEST(DocumentParser, rejects_mismatched_object_stream_members) { + PdfFileBuilder builder; + builder.stream_object("/Type /ObjStm /N 1 /First 4", "9 0 (wrong)") + .object("null") + .trailer("/Root 2 0 R"); + std::string source = builder.build_xref_stream(); + const std::size_t start = source.find("stream\n", source.find("/Type /XRef")); + ASSERT_NE(start, std::string::npos); + // Entry 2: compressed in object stream 1, member 0 (builder uses W [1 4 2]). + source.replace(start + 7 + 2 * 7, 7, std::string("\x02\0\0\0\x01\0\0", 7)); + DocumentParser parser(std::make_unique(source)); + ASSERT_FALSE(parser.is_recovered()); + EXPECT_THROW((void)parser.read_object(ObjectReference{2, 0}), + std::runtime_error); +}