From dfa37e1d6eee831e1c3184d45fae784aeeeaf9bb Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 07:51:18 +0200 Subject: [PATCH 1/2] fix: preserve PDF object precedence during recovery --- CHANGELOG.md | 3 + src/odr/internal/pdf/AGENTS.md | 4 +- src/odr/internal/pdf/pdf_document_parser.cpp | 18 +++-- src/odr/internal/pdf/pdf_file_parser.cpp | 5 +- test/src/internal/pdf/pdf_document_parser.cpp | 66 ++++++++++++++----- 5 files changed, 72 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0261466c..f2505f4d9 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 recovery removes obsolete generations and uses the latest compressed + copy when no direct definition exists. + - PDF file indices are checked before narrowing; malformed cross-reference widths, counts and object-stream offsets cannot wrap or drive unchecked allocation. Incomplete cross-reference index pairs trigger recovery. diff --git a/src/odr/internal/pdf/AGENTS.md b/src/odr/internal/pdf/AGENTS.md index 75caddf47..ce166ecae 100644 --- a/src/odr/internal/pdf/AGENTS.md +++ b/src/odr/internal/pdf/AGENTS.md @@ -75,7 +75,9 @@ output. numeric value, a free or absent reference resolves to null with a warning. A throw in the cross-reference layer is caught once, and the file is forward-scanned to rebuild a synthetic xref (`recover_xref`; the last - definition of an id wins). If the trailer has no `/Root`, `recover_root` + definition of an id wins). Direct definitions take precedence over compressed + copies; otherwise the latest object stream supplies an id. If the trailer + has no `/Root`, `recover_root` takes the first `/Type /Catalog` it finds in id order. - **Diagnostics go through `Logger`.** `DocumentParser` and `extract_text` take a `const Logger &` (default `Logger::null()`). `Logger` is a value diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index c5a0c226e..fc51665be 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -1950,16 +1950,17 @@ void DocumentParser::recover_xref() { } void DocumentParser::index_object_streams() { - // Snapshot the directly recovered objects: reading object streams adds - // compressed entries, which would invalidate an in-flight iterator. - std::vector candidates; + std::vector> candidates; for (const auto &[reference, entry] : m_xref.table) { if (entry.is_used()) { - candidates.push_back(reference); + candidates.emplace_back(entry.as_used().position, reference); } } + std::ranges::sort(candidates, [](const auto &left, const auto &right) { + return left.first > right.first; + }); - for (const ObjectReference &reference : candidates) { + for (const auto &[position, reference] : candidates) { try { const IndirectObject &object = read_object(reference); if (!object.has_stream || !object.object.is_dictionary()) { @@ -1972,7 +1973,12 @@ void DocumentParser::index_object_streams() { } const ObjectStream &members = load_object_stream(reference); for (std::size_t i = 0; i < members.size(); ++i) { - // a directly recovered object wins over its compressed copy + const auto existing = + m_xref.table.lower_bound(ObjectReference(members[i].id, 0)); + if (existing != m_xref.table.end() && + existing->first.id == members[i].id) { + continue; + } m_xref.table.try_emplace( ObjectReference(members[i].id, 0), Xref::Entry(Xref::CompressedEntry{checked_file_index(reference.id), diff --git a/src/odr/internal/pdf/pdf_file_parser.cpp b/src/odr/internal/pdf/pdf_file_parser.cpp index e8c8a60b1..a2d5e5dd4 100644 --- a/src/odr/internal/pdf/pdf_file_parser.cpp +++ b/src/odr/internal/pdf/pdf_file_parser.cpp @@ -500,7 +500,10 @@ std::pair FileParser::recover_xref() { if (const std::optional ref = match_object_start(content)) { - // last definition of an id wins (operator[] overwrites) + const auto previous = xref.table.lower_bound(ObjectReference(ref->id, 0)); + if (previous != xref.table.end() && previous->first.id == ref->id) { + xref.table.erase(previous); + } xref.table[*ref] = Xref::Entry(Xref::UsedEntry{ checked_file_index(static_cast(position) + lead)}); // A compact object may inline its dictionary and the `stream` token on diff --git a/test/src/internal/pdf/pdf_document_parser.cpp b/test/src/internal/pdf/pdf_document_parser.cpp index 1921f836f..2c9a15432 100644 --- a/test/src/internal/pdf/pdf_document_parser.cpp +++ b/test/src/internal/pdf/pdf_document_parser.cpp @@ -565,23 +565,32 @@ TEST(DocumentParser, recovers_root_from_catalog_scan) { // Recovery: an id defined more than once (e.g. a botched incremental update) // resolves to the last definition in the file. TEST(DocumentParser, recovery_last_definition_wins) { - const std::string pdf = - "%PDF-1.7\n" - "1 0 obj\n<< /Type /Catalog /Pages 2 0 R >>\nendobj\n" - "2 0 obj\n<< /Type /Pages /Kids [3 0 R] /Count 1 >>\nendobj\n" - "3 0 obj\n<< /Type /Page /Parent 2 0 R /MediaBox [0 0 100 100] " - "/Resources << >> /Contents 4 0 R >>\nendobj\n" - "4 0 obj\n<< /Length 5 >>\nstream\nBT ET\nendstream\nendobj\n" - "3 0 obj\n<< /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] " - "/Resources << >> /Contents 4 0 R >>\nendobj\n" - "trailer\n<< /Root 1 0 R >>\n%%EOF\n"; - - DocumentParser parser(std::make_unique(pdf)); - const std::unique_ptr document = parser.parse_document(); - const std::vector pages = document->collect_pages(); + for (const std::string generation : {"0", "1"}) { + const std::string pdf = + "%PDF-1.7\n" + "1 0 obj\n<< /Type /Catalog /Pages 2 0 R >>\nendobj\n" + "2 0 obj\n<< /Type /Pages /Kids [3 " + + generation + + " R] /Count 1 >>\nendobj\n" + "3 0 obj\n<< /Type /Page /Parent 2 0 R /MediaBox [0 0 100 100] " + "/Resources << >> /Contents 4 0 R >>\nendobj\n" + "4 0 obj\n<< /Length 5 >>\nstream\nBT ET\nendstream\nendobj\n" + "3 " + + generation + + " obj\n<< /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] " + "/Resources << >> /Contents 4 0 R >>\nendobj\n" + "trailer\n<< /Root 1 0 R >>\n%%EOF\n"; + + DocumentParser parser(std::make_unique(pdf)); + const std::unique_ptr document = parser.parse_document(); + const std::vector pages = document->collect_pages(); - ASSERT_EQ(pages.size(), 1); - EXPECT_EQ(pages[0]->media_box.as_array()[2].as_real(), 200.0); + ASSERT_EQ(pages.size(), 1); + EXPECT_EQ(pages[0]->media_box.as_array()[2].as_real(), 200.0); + if (generation == "1") { + EXPECT_TRUE(parser.read_object(ObjectReference{3, 0}).object.is_null()); + } + } } // Recovery: an object that inlines its dictionary and the `stream` token on a @@ -844,3 +853,28 @@ TEST(DocumentParser, malformed_xref_index_requires_recovery) { .as_name(), "Catalog"); } + +TEST(DocumentParser, recovery_keeps_the_latest_compressed_copy) { + PdfFileBuilder builder; + builder.object("<< /Type /Catalog /Pages 2 0 R >>") + .object("<< /Type /Pages /Kids [] /Count 0 >>") + .stream_object("/Type /ObjStm /N 1 /First 4", "6 0 (older)") + .stream_object("/Type /ObjStm /N 1 /First 4", "6 0 (newer)") + .trailer("/Root 1 0 R"); + for (const bool direct : {false, true}) { + std::string source = "broken\n" + builder.build_classic(); + if (direct) { + source += "6 1 obj\n(direct)\nendobj\n"; + } + DocumentParser parser(std::make_unique(source)); + ASSERT_TRUE(parser.is_recovered()); + if (direct) { + EXPECT_TRUE(parser.read_object(ObjectReference{6, 0}).object.is_null()); + EXPECT_EQ(parser.read_object(ObjectReference{6, 1}).object.as_string(), + "direct"); + } else { + EXPECT_EQ(parser.read_object(ObjectReference{6, 0}).object.as_string(), + "newer"); + } + } +} From 16e474c1852d85a2497cbf83b2bb1cdf09d34cd2 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 6 Oct 2026 20:19:03 +0200 Subject: [PATCH 2/2] fix: keep the reasons beside the recovery precedence The rewrite dropped the comments that say why the candidates are copied before indexing, which definition of an id recovery keeps, and that a direct definition beats a compressed copy. They stand again, adapted to the new order: latest object stream first, and the last definition of an id wins whatever its generation. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MxyTMutqSUJRGfxA8CyzMc --- src/odr/internal/pdf/pdf_document_parser.cpp | 4 ++++ src/odr/internal/pdf/pdf_file_parser.cpp | 1 + 2 files changed, 5 insertions(+) diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index fc51665be..950442c41 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -1950,6 +1950,9 @@ void DocumentParser::recover_xref() { } void DocumentParser::index_object_streams() { + // Snapshot the directly recovered objects, latest in the file first: reading + // object streams adds compressed entries, which would invalidate an + // in-flight iterator. std::vector> candidates; for (const auto &[reference, entry] : m_xref.table) { if (entry.is_used()) { @@ -1973,6 +1976,7 @@ void DocumentParser::index_object_streams() { } const ObjectStream &members = load_object_stream(reference); for (std::size_t i = 0; i < members.size(); ++i) { + // a direct definition, or a later object stream, already wins const auto existing = m_xref.table.lower_bound(ObjectReference(members[i].id, 0)); if (existing != m_xref.table.end() && diff --git a/src/odr/internal/pdf/pdf_file_parser.cpp b/src/odr/internal/pdf/pdf_file_parser.cpp index a2d5e5dd4..8fb380a82 100644 --- a/src/odr/internal/pdf/pdf_file_parser.cpp +++ b/src/odr/internal/pdf/pdf_file_parser.cpp @@ -500,6 +500,7 @@ std::pair FileParser::recover_xref() { if (const std::optional ref = match_object_start(content)) { + // the last definition of an id wins, whatever its generation const auto previous = xref.table.lower_bound(ObjectReference(ref->id, 0)); if (previous != xref.table.end() && previous->first.id == ref->id) { xref.table.erase(previous);