Issue #2366: a /Prev reference to an object holding the offset reads that offset

iText's indirectPrev.pdf writes /Prev 6 0 R, and object 6, listed in the
same update, holds 470, the offset of the first section. Both readers took
the object number as the offset, so cos found no section at 6, rebuilt the
file from object headers and lost its revision list, and hayro's chain
stopped there.

When /Prev is a reference, the sections read so far (cos) or the section's
own entries (hayro, which now reads them before following /Prev) are asked
for that object first. If it holds a non-negative integer, that is the
offset; otherwise the object number still is, as the tiff2pdf files need.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NH3itd85jPuM6wNjgcGfEH

crates/cos/src/document.rs

@@ -1476,7 +1476,7 @@ impl Document {
                 }
             }
             sections.push(XrefRevisionSection { primary, hybrid });
-            next = prev_offset(&sect_trailer, off, log);
+            next = prev_offset(data, &sect_trailer, off, |num| sections.iter().find_map(|s| s.lookup(num, data)), log);
             if trailer.is_none() {
                 trailer = Some(sect_trailer);
             }
@@ -1764,7 +1764,7 @@ impl Document {
             for (num, e) in sect {
                 entries.entry(num).or_insert(e);
             }
-            next = prev_offset(&sect_trailer, off, log);
+            next = prev_offset(data, &sect_trailer, off, |num| entries.get(&num).copied(), log);
             if trailer.is_none() {
                 trailer = Some(sect_trailer);
             }
@@ -1991,15 +1991,36 @@ impl Document {
 }
 
 /// The offset of the section before the one whose trailer this is (`/Prev`), if any. Some writers
-/// (tiff2pdf; JHOVE's PDF-HUL-157 files) give it as a reference, `N 0 R`: other readers rebuild
-/// such a file, and the sections before are at N, so it is read as that, with a note. Any other
-/// value that is not an offset is noted and ends the chain.
-fn prev_offset(trailer: &Dict, off: u64, log: &mut Vec<String>) -> Option<u64> {
+/// give it as a reference, `N 0 R`. When the sections read so far (`lookup`) place that object
+/// and it holds an offset (iText's `indirectPrev.pdf`), that is the offset; otherwise, as the
+/// tiff2pdf files in JHOVE's PDF-HUL-157 cases need, the sections before are at N. Either way the
+/// reference is noted. Any other value that is not an offset is noted and ends the chain.
+fn prev_offset(data: &[u8], trailer: &Dict, off: u64, lookup: impl Fn(u32) -> Option<XrefEntry>, log: &mut Vec<String>) -> Option<u64> {
     match trailer.get(b"Prev") {
         Some(Object::Int(p)) if *p >= 0 => Some(*p as u64),
         Some(Object::Ref(r)) => {
-            log.push(format!("the trailer at {off} gives /Prev as the reference {} 0 R; reading it as offset {}", r.num, r.num));
-            Some(u64::from(r.num))
+            let held = match lookup(r.num) {
+                Some(XrefEntry::InFile { offset, generation }) if generation == r.generation => {
+                    usize::try_from(offset).ok().and_then(|at| parse_indirect(data, at, &|_| None).ok()).and_then(|(at, value)| match value {
+                        Object::Int(p) if at == *r && p >= 0 => Some(p as u64),
+                        _ => None,
+                    })
+                }
+                _ => None,
+            };
+            match held {
+                Some(p) => {
+                    log.push(format!(
+                        "the trailer at {off} gives /Prev as the reference {} {} R; reading the offset {p} it holds",
+                        r.num, r.generation
+                    ));
+                    Some(p)
+                }
+                None => {
+                    log.push(format!("the trailer at {off} gives /Prev as the reference {} 0 R; reading it as offset {}", r.num, r.num));
+                    Some(u64::from(r.num))
+                }
+            }
         }
         Some(_) => {
             log.push(format!("the trailer at {off} has a /Prev that is not an offset; the sections before it are unreachable"));
@@ -2890,6 +2911,39 @@ endobj
         );
     }
 
+    /// iText's `indirectPrev.pdf` in miniature: the update's trailer gives `/Prev 4 0 R`, and
+    /// object 4, listed in that update, holds the offset of the first section. Read as offset 4
+    /// (inside the header), the chain broke and the file was rebuilt without its revisions.
+    #[test]
+    fn a_prev_reference_to_an_object_holding_the_offset_reads_that_offset() {
+        let mut bytes = build(
+            &["<< /Type /Catalog /Pages 2 0 R >>", "<< /Type /Pages /Kids [3 0 R] /Count 1 >>", "<< /Type /Page /Parent 2 0 R >>"],
+            "/Root 1 0 R",
+        );
+        let prev = String::from_utf8_lossy(&bytes).rfind("xref\n0 4").unwrap();
+        let mut at = Vec::new();
+        for (num, body) in
+            [(2, "<< /Type /Pages /Kids [3 0 R 5 0 R] /Count 2 >>".to_string()), (4, prev.to_string()), (5, "<< /Type /Page /Parent 2 0 R >>".into())]
+        {
+            at.push(bytes.len());
+            bytes.extend_from_slice(format!("{num} 0 obj\n{body}\nendobj\n").as_bytes());
+        }
+        let xref = bytes.len();
+        bytes.extend_from_slice(
+            format!(
+                "xref\n2 1\n{:010} 00000 n \n4 2\n{:010} 00000 n \n{:010} 00000 n \ntrailer\n<< /Size 6 /Root 1 0 R /Prev 4 0 R >>\nstartxref\n{xref}\n%%EOF\n",
+                at[0], at[1], at[2]
+            )
+            .as_bytes(),
+        );
+        let doc = Document::open(Arc::new(bytes)).unwrap();
+        assert_eq!(doc.revisions().len(), 2, "{:?}", doc.repair_log());
+        assert_eq!(doc.repair_log().len(), 1, "a note about the /Prev, and no rebuild: {:?}", doc.repair_log());
+        assert!(doc.repair_log()[0].contains(&format!("the offset {prev}")), "{:?}", doc.repair_log());
+        assert_eq!(doc.get(ObjRef::new(3, 0)).as_dict().and_then(|d| d.name(b"Type")), Some(&b"Page"[..]));
+        assert_compact_matches_materialized(&doc);
+    }
+
     #[test]
     fn damaged_xref_is_reconstructed() {
         let mut bytes = build(&["<< /Type /Catalog /Pages 2 0 R >>", "<< /Type /Pages /Kids [] /Count 0 >>"], "/Root 1 0 R");

vendor/hayro-syntax/src/xref.rs

@@ -759,14 +759,31 @@ fn populate_xref_impl<'a>(data: &'a [u8], pos: usize, xref_map: &mut XrefMap) ->
 }
 
 /// The offset of the previous xref section (`Prev`).
-// PdfCraft patch: (#2366) some writers (tiff2pdf, in JHOVE's PDF-HUL-157 files) give `Prev` as
-// a reference `N 0 R`; its object number is the offset, as other readers take it.
-fn prev_offset(trailer: &Dict<'_>) -> Option<usize> {
-    let prev = trailer
-        .get::<i32>(PREV)
-        .or_else(|| trailer.get_ref(PREV).map(|r| r.obj_number))?;
+// PdfCraft patch: (#2366) some writers give `Prev` as a reference `N G R`. When this section's
+// own entries (`own`) place that object and it holds an offset (iText's `indirectPrev.pdf`), that
+// is the offset; otherwise its object number is (tiff2pdf, in JHOVE's PDF-HUL-157 files), as
+// `pdfcraft-cos` and other readers take it.
+fn prev_offset(data: &[u8], trailer: &Dict<'_>, own: &XrefMap) -> Option<usize> {
+    if let Some(prev) = trailer.get::<i32>(PREV) {
+        return usize::try_from(prev).ok();
+    }
 
-    usize::try_from(prev).ok()
+    let r = trailer.get_ref(PREV)?;
+    let id = ObjectIdentifier::new(r.obj_number, r.gen_number);
+    let held = match own.get(&id) {
+        Some(EntryType::Normal(at)) => {
+            let mut reader = Reader::new(data);
+            reader.jump(*at);
+            reader
+                .read_with_context::<IndirectObject<i32>>(&ReaderContext::dummy())
+                .filter(|o| o.id() == &id)
+                .map(|o| o.get())
+                .filter(|p| *p >= 0)
+        }
+        _ => None,
+    };
+
+    usize::try_from(held.unwrap_or(r.obj_number)).ok()
 }
 
 /// Maximum number of allowed xref `Prev` pointers before we abort.
@@ -844,17 +861,8 @@ fn populate_from_xref_table<'a>(
     reader.skip_white_spaces();
 
     let mut max_obj = 0;
-
-    if let Some(prev) = prev_offset(&trailer) {
-        // First insert the entries from any previous xref tables.
-        populate_xref_impl_inner(data, prev, insert_map, visited)?;
-    }
-
-    // In hybrid files, entries in `XRefStm` should have higher priority, therefore we insert them
-    // after looking at `PREV`.
-    if let Some(xref_stm) = trailer.get::<i32>(XREF_STM) {
-        populate_xref_impl_inner(data, xref_stm as usize, insert_map, visited)?;
-    }
+    // PdfCraft patch: (#2366) this section's entries are read first, for `prev_offset`.
+    let mut own = XrefMap::default();
 
     while let Some(header) = reader.read_without_context::<SubsectionHeader>() {
         reader.skip_white_spaces();
@@ -870,7 +878,7 @@ fn populate_from_xref_table<'a>(
             // Specification says we should ignore any object number > SIZE, but probably
             // not important?
             if entry.used {
-                insert_map.insert(
+                own.insert(
                     ObjectIdentifier::new(obj_number as i32, entry.gen_number),
                     EntryType::Normal(entry.offset),
                 );
@@ -878,6 +886,19 @@ fn populate_from_xref_table<'a>(
         }
     }
 
+    if let Some(prev) = prev_offset(data, &trailer, &own) {
+        // First insert the entries from any previous xref tables.
+        populate_xref_impl_inner(data, prev, insert_map, visited)?;
+    }
+
+    // In hybrid files, entries in `XRefStm` should have higher priority, therefore we insert them
+    // after looking at `PREV`.
+    if let Some(xref_stm) = trailer.get::<i32>(XREF_STM) {
+        populate_xref_impl_inner(data, xref_stm as usize, insert_map, visited)?;
+    }
+
+    insert_map.extend(own);
+
     Some(trailer.data())
 }
 
@@ -891,11 +912,6 @@ fn populate_from_xref_stream<'a>(
         .read_with_context::<IndirectObject<Stream<'_>>>(&ReaderContext::dummy())?
         .get();
 
-    if let Some(prev) = prev_offset(stream.dict()) {
-        // First insert the entries from any previous xref tables.
-        let _ = populate_xref_impl_inner(data, prev, insert_map, visited)?;
-    }
-
     let size = stream.dict().get::<u32>(SIZE)?;
 
     let [f1_len, f2_len, f3_len] = stream.dict().get::<[u8; 3]>(W)?;
@@ -912,6 +928,8 @@ fn populate_from_xref_stream<'a>(
     }
 
     let xref_data = stream.decoded().ok()?;
+    // PdfCraft patch: (#2366) this section's entries are read first, for `prev_offset`.
+    let mut own = XrefMap::default();
     let mut xref_reader = Reader::new(xref_data.as_ref());
 
     if let Some(arr) = stream.dict().get::<Array<'_>>(INDEX) {
@@ -925,21 +943,20 @@ fn populate_from_xref_stream<'a>(
                 f1_len,
                 f2_len,
                 f3_len,
-                insert_map,
+                &mut own,
             )?;
         }
     } else {
-        xref_stream_subsection(
-            &mut xref_reader,
-            0,
-            size,
-            f1_len,
-            f2_len,
-            f3_len,
-            insert_map,
-        )?;
+        xref_stream_subsection(&mut xref_reader, 0, size, f1_len, f2_len, f3_len, &mut own)?;
     }
 
+    if let Some(prev) = prev_offset(data, stream.dict(), &own) {
+        // First insert the entries from any previous xref tables.
+        let _ = populate_xref_impl_inner(data, prev, insert_map, visited)?;
+    }
+
+    insert_map.extend(own);
+
     Some(stream.dict().data())
 }
 
@@ -1402,6 +1419,44 @@ mod tests {
         assert!(xref_map.contains_key(&ObjectIdentifier::new(1, 0)));
     }
 
+    #[test]
+    fn prev_as_reference_to_an_object_holding_the_offset_reads_that_offset() {
+        let mut pdf = b"%PDF-1.0\n1 0 obj\n<< /Type /Catalog /Pages 2 0 R >>\nendobj\n".to_vec();
+        let first_xref_pos = pdf.len();
+        pdf.extend_from_slice(
+            format!(
+                "xref\n\
+                 0 2\n\
+                 0000000000 65535 f \n\
+                 0000000009 00000 n \n\
+                 trailer\n<< /Size 2 /Root 1 0 R >>\n\
+                 startxref\n{first_xref_pos}\n%%EOF\n"
+            )
+            .as_bytes(),
+        );
+        // Object 3 holds the offset; read as offset 3, `Prev` would point inside the header.
+        let held_pos = pdf.len();
+        pdf.extend_from_slice(format!("3 0 obj\n{first_xref_pos}\nendobj\n").as_bytes());
+        let second_xref_pos = pdf.len();
+        pdf.extend_from_slice(
+            format!(
+                "xref\n\
+                 3 1\n\
+                 {held_pos:010} 00000 n \n\
+                 trailer\n<< /Size 4 /Root 1 0 R /Prev 3 0 R >>\n\
+                 startxref\n{second_xref_pos}\n%%EOF"
+            )
+            .as_bytes(),
+        );
+
+        let mut xref_map = FxHashMap::default();
+        let xref_pos = find_last_xref_pos(pdf.as_ref()).unwrap();
+        assert_eq!(xref_pos, second_xref_pos);
+        populate_xref_impl(pdf.as_ref(), xref_pos, &mut xref_map).unwrap();
+        assert!(xref_map.contains_key(&ObjectIdentifier::new(1, 0)));
+        assert!(xref_map.contains_key(&ObjectIdentifier::new(3, 0)));
+    }
+
     #[test]
     fn find_last_xref_uses_last_startxref() {
         let pdf = b"%PDF-1.0\nstartxref\n5\n%%EOF\nstartxref\n42\n%%EOF";