fix: preserve PDF object precedence during recovery

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.

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

src/odr/internal/pdf/pdf_document_parser.cpp

@@ -1950,16 +1950,20 @@ 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<ObjectReference> candidates;
+  // 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<std::pair<std::uint32_t, ObjectReference>> 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 +1976,13 @@ 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
+        // 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() &&
+            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),

src/odr/internal/pdf/pdf_file_parser.cpp

@@ -500,7 +500,11 @@ std::pair<Xref, Dictionary> FileParser::recover_xref() {
 
     if (const std::optional<ObjectReference> ref =
             match_object_start(content)) {
-      // last definition of an id wins (operator[] overwrites)
+      // 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);
+      }
       xref.table[*ref] = Xref::Entry(Xref::UsedEntry{
           checked_file_index(static_cast<std::uint64_t>(position) + lead)});
       // A compact object may inline its dictionary and the `stream` token on

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<std::istringstream>(pdf));
-  const std::unique_ptr<Document> document = parser.parse_document();
-  const std::vector<Page *> 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<std::istringstream>(pdf));
+    const std::unique_ptr<Document> document = parser.parse_document();
+    const std::vector<Page *> 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<std::istringstream>(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");
+    }
+  }
+}