diff --git a/assets/pdfs/with_incremental_updates.pdf b/assets/pdfs/with_incremental_updates.pdf new file mode 100644 index 000000000..06a4027ef Binary files /dev/null and b/assets/pdfs/with_incremental_updates.pdf differ diff --git a/src/core/PDFContext.ts b/src/core/PDFContext.ts index 0544aa214..8af7d3445 100644 --- a/src/core/PDFContext.ts +++ b/src/core/PDFContext.ts @@ -62,6 +62,16 @@ class PDFContext { private pushGraphicsStateContentStreamRef?: PDFRef; private popGraphicsStateContentStreamRef?: PDFRef; + /** + * Tracks whether new objects have been registered in this context. + * When true, objects will be sorted by object number during enumeration. + * When false, objects maintain their original order (important for PDFs + * with incremental updates where reordering can cause corruption). + * + * @see https://github.com/Hopding/pdf-lib/issues/951 + */ + private needsReordering: boolean; + private constructor() { this.largestObjectNumber = 0; this.header = PDFHeader.forVersion(1, 7); @@ -69,6 +79,7 @@ class PDFContext { this.indirectObjects = new Map(); this.rng = SimpleRNG.withSeed(1); + this.needsReordering = false; } assign(ref: PDFRef, object: PDFObject): void { @@ -80,6 +91,7 @@ class PDFContext { nextRef(): PDFRef { this.largestObjectNumber += 1; + this.needsReordering = true; return PDFRef.of(this.largestObjectNumber); } @@ -178,10 +190,24 @@ class PDFContext { return undefined; } + /** + * Returns all indirect objects in this context. + * + * If new objects have been registered (needsReordering=true), objects are + * sorted by ascending object number to ensure proper XRef table generation. + * + * If no new objects have been registered (needsReordering=false), objects + * maintain their original parsing order. This is crucial for PDFs with + * incremental updates, where reordering objects can cause corruption. + * + * @see https://github.com/Hopding/pdf-lib/issues/951 + */ enumerateIndirectObjects(): [PDFRef, PDFObject][] { - return Array.from(this.indirectObjects.entries()).sort( - byAscendingObjectNumber, - ); + const entries = Array.from(this.indirectObjects.entries()); + if (this.needsReordering) { + return entries.sort(byAscendingObjectNumber); + } + return entries; } obj(literal: null | undefined): typeof PDFNull; diff --git a/tests/core/PDFContext_ordering.spec.ts b/tests/core/PDFContext_ordering.spec.ts new file mode 100644 index 000000000..04e9cd85b --- /dev/null +++ b/tests/core/PDFContext_ordering.spec.ts @@ -0,0 +1,169 @@ +import fs from 'fs'; +import { PDFDocument } from 'src/index'; +import PDFContext from 'src/core/PDFContext'; +import PDFRef from 'src/core/objects/PDFRef'; + +/** + * Tests for Issue #951 - Corrupted PDF on save with incremental updates + * @see https://github.com/Hopding/pdf-lib/issues/951 + * + * The issue occurs when pdf-lib loads and saves PDFs that use the "Incremental + * Update" feature of the PDF format. The previous implementation always sorted + * objects by object number during enumeration, which corrupted PDFs by + * invalidating the byte offsets in the XRef table. + * + * The fix tracks whether new objects have been registered, and only sorts + * objects when necessary (when new objects are added that need to be + * integrated into the object number sequence). + */ +describe('Issue #951 - Corrupted PDF on save with incremental updates', () => { + describe('PDFContext.enumerateIndirectObjects ordering', () => { + it('should NOT sort objects when no new objects are registered', () => { + const context = PDFContext.create(); + + // Simulate loading objects from a PDF - assign directly without using register() + // This mimics what the parser does when loading an existing PDF + const obj1 = context.obj({ Type: 'Font' }); + const obj2 = context.obj({ Type: 'Page' }); + const obj3 = context.obj({ Type: 'Catalog' }); + + // PDFRef.of creates refs with specific object numbers + // Assign in non-sequential order to test ordering behavior + // In real parsing, objects may not come in sequential order + context.assign(PDFRef.of(5), obj1); + context.assign(PDFRef.of(2), obj2); + context.assign(PDFRef.of(8), obj3); + + // When NO new objects are registered via register(), + // enumeration should preserve insertion order (not sorted) + const entries = context.enumerateIndirectObjects(); + const objectNumbers = entries.map(([ref]) => ref.objectNumber); + + // Should maintain insertion order: 5, 2, 8 (not sorted as 2, 5, 8) + expect(objectNumbers).toEqual([5, 2, 8]); + }); + + it('should sort objects when new objects ARE registered', () => { + const context = PDFContext.create(); + + // Assign existing objects + const obj1 = context.obj({ Type: 'Font' }); + const obj2 = context.obj({ Type: 'Page' }); + context.assign(PDFRef.of(5), obj1); + context.assign(PDFRef.of(2), obj2); + + // Register a NEW object (this triggers needsReordering = true) + const newObj = context.obj({ Type: 'NewObject' }); + context.register(newObj); + + // Now enumeration SHOULD sort by object number + const entries = context.enumerateIndirectObjects(); + const objectNumbers = entries.map(([ref]) => ref.objectNumber); + + + // Should be sorted: 2, 5, 6 (the new one gets objectNumber 6 since largestObjectNumber was 5) + expect(objectNumbers).toEqual([2, 5, 6]); + }); + + it('should sort objects when using nextRef() + assign() pattern (embedder code path)', () => { + const context = PDFContext.create(); + + // Assign existing objects (simulates loading from PDF) + const obj1 = context.obj({ Type: 'Font' }); + const obj2 = context.obj({ Type: 'Page' }); + context.assign(PDFRef.of(5), obj1); + context.assign(PDFRef.of(2), obj2); + + // This is the pattern used by embedders (JavaScriptEmbedder, CustomFontEmbedder, etc.) + // They call nextRef() first, then later assign() with that ref + const ref = context.nextRef(); // This should trigger needsReordering = true + const newObj = context.obj({ Type: 'EmbeddedObject' }); + context.assign(ref, newObj); + + // Now enumeration SHOULD sort by object number + const entries = context.enumerateIndirectObjects(); + const objectNumbers = entries.map(([r]) => r.objectNumber); + + // Should be sorted: 2, 5, 6 (the new one from nextRef) + expect(objectNumbers).toEqual([2, 5, 6]); + }); + + it('should correctly track largestObjectNumber when loading/assigning objects', () => { + const context = PDFContext.create(); + + context.assign(PDFRef.of(5), context.obj({ Type: 'Font' })); + context.assign(PDFRef.of(2), context.obj({ Type: 'Page' })); + context.assign(PDFRef.of(8), context.obj({ Type: 'Catalog' })); + + // After parsing/assigning, largestObjectNumber should be updated + // Next ref should be 9 (8 + 1) + const newRef = context.nextRef(); + expect(newRef.objectNumber).toBe(9); + }); + }); + + describe('Integration tests with real PDFs', () => { + const incrementalUpdatesPdfPath = 'assets/pdfs/with_incremental_updates.pdf'; + const normalPdfPath = 'assets/pdfs/normal.pdf'; + + const incrementalFileExists = fs.existsSync(incrementalUpdatesPdfPath); + const normalFileExists = fs.existsSync(normalPdfPath); + + (incrementalFileExists ? it : it.skip)( + 'should preserve PDF with incremental updates when loading and saving without modifications', + async () => { + const inputBytes = fs.readFileSync(incrementalUpdatesPdfPath); + + // Load the PDF without making any modifications + const pdfDoc = await PDFDocument.load(inputBytes); + const pageCount = pdfDoc.getPageCount(); + + // Save the PDF + const savedBytes = await pdfDoc.save(); + + // Verify we can reload the saved PDF + const reloadedDoc = await PDFDocument.load(savedBytes); + expect(reloadedDoc.getPageCount()).toBe(pageCount); + }, + 30000 // 30 second timeout for large PDF processing + ); + + (incrementalFileExists ? it : it.skip)( + 'should preserve PDF with incremental updates when saving with useObjectStreams: false', + async () => { + const inputBytes = fs.readFileSync(incrementalUpdatesPdfPath); + + const pdfDoc = await PDFDocument.load(inputBytes); + const pageCount = pdfDoc.getPageCount(); + + // Save with useObjectStreams: false (as mentioned in the issue) + const savedBytes = await pdfDoc.save({ useObjectStreams: false }); + + // Verify reload + const reloadedDoc = await PDFDocument.load(savedBytes); + expect(reloadedDoc.getPageCount()).toBe(pageCount); + }, + 30000 // 30 second timeout for large PDF processing + ); + + (normalFileExists ? it : it.skip)( + 'should still correctly save PDFs when new content is added', + async () => { + const inputBytes = fs.readFileSync(normalPdfPath); + + const pdfDoc = await PDFDocument.load(inputBytes); + const originalPageCount = pdfDoc.getPageCount(); + + // Add a new page (this should trigger reordering) + pdfDoc.addPage(); + + const savedBytes = await pdfDoc.save(); + + // Verify the new page was added correctly + const reloadedDoc = await PDFDocument.load(savedBytes); + expect(reloadedDoc.getPageCount()).toBe(originalPageCount + 1); + } + ); + }); +}); +