Skip a dictionary value that has no key rather than refusing the document by jafin · Pull Request #64 · jafin/PdfSharpCore

jafin · 2026-08-09T04:23:28Z

Reproduces and fixes empira/PDFsharp#376 in this fork.

What was wrong

Parser.ReadDictionary walked the parsed items two at a time and threw name expected the moment the item in the key position was not a PdfName.

pdfTeX has broken that rule for decades. It writes its PTEX.FullBanner into the document information dictionary as two bare strings with no key in front of them:

<< /Creator (LaTeX with hyperref) /CreationDate (D:...) (PTEX.FullBanner)
(This is pdfTeX, Version 3.141592653-2.6-1.40.29 \(TeX Live 2026\)) >>

Every document TeX Live produces was therefore unreadable in every open mode, with no option to suppress it. Acrobat, Chrome and Edge all step over the stray values and read the rest of the file.

The fix

The pairing loop now advances one item at a time. A name takes the item after it as its value; anything else is a value with nothing to key it under, and is stepped over on its own.

That matters beyond the pdfTeX case, which happens to leave an even number of items. A single stray value leaves an odd number, and a loop that always advances by two reads the following key as its value and the value after that as a key — quietly mispairing every entry to the end of the dictionary. AValueWithNoKeyDoesNotPairTheEntriesAfterItUpWrongly pins that down.

Dropping the dangling key of a truncated dictionary falls out of the same loop, so the Debug.Assert that guarded that case goes away with it. The existing UnterminatedObjectTests still cover it.

Tests

PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs — 8 tests, all failing before the change:

the pdfTeX information dictionary opens in Import, Modify and ReadOnly
the entries that do have keys are still read
the values with no key are dropped
a single stray value does not mispair what follows it
a dictionary that is nothing but values reads as an empty one
such a document can be saved and reopened

RawPdf.Build gained an optional extraTrailerEntries parameter so a test can hang an /Info dictionary off the trailer the way pdfTeX does.

Full suite: 1048 passed, 1 skipped, on both net8.0 and net10.0.

…ment A dictionary entry is a name followed by a value, and the parser threw "name expected" the moment it found anything else where a key belonged. pdfTeX has broken that rule for decades: it writes its PTEX.FullBanner into the document information dictionary as two bare strings with no key in front of them, so every document TeX Live produces was unreadable in every open mode. Adobe Acrobat, Chrome and Edge all step over the stray values and read the rest of the file. The pairing loop now walks the items one at a time instead of two at a time. A name takes the item after it as its value; anything else is a value with nothing to key it under, and is stepped over on its own. That matters beyond the pdfTeX case: a single stray value leaves an odd number of items, and a loop that always advances by two would read the following key as its value and the value after that as a key, quietly mispairing every entry to the end of the dictionary. Dropping the dangling key of a truncated dictionary still falls out of the same loop, so the assertion that guarded that case goes away with it.

coderabbitai · 2026-08-09T04:23:40Z

Warning

Review limit reached

@jafin, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 02a017c7-d121-4303-8d26-e63b5ac48ca7

📥 Commits

Reviewing files that changed from the base of the PR and between a08c635 and fd60ef8.

📒 Files selected for processing (3)

PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs
PdfSharpCore.Test/IO/RawPdf.cs
PdfSharpCore/Pdf.IO/Parser.cs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

qodo-code-review · 2026-08-09T04:24:13Z

PR Summary by Qodo

Tolerate keyless values in PDF dictionaries during parsing

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Parse malformed dictionaries by skipping values that lack a preceding name key.
• Prevent odd-item dictionaries from mispairing subsequent key/value entries.
• Add regression tests covering pdfTeX /Info dictionaries and save/reopen behavior.

Diagram

graph TD
T["DictionaryEntryWithoutKeyTests"] --> R["RawPdf.Build"] --> O["PdfReader.Open"] --> P["Parser.ReadDictionary"] --> I["Parsed items"] --> D{"Key is PdfName?"}
D -->|"yes"| A["pair key+value"] --> I
D -->|"no"| S["skip stray value"] --> I

High-Level Assessment

The following are alternative approaches to this PR:

1. Strict-by-default with opt-in permissive mode

➕ Preserves spec conformance for users who want hard failures
➕ Makes malformed PDFs more visible by default
➖ Keeps TeX Live/pdfTeX outputs unreadable without configuration
➖ More API surface area (options/config) to maintain

2. Permissive parse + diagnostics/warnings hook

➕ Matches behavior of Acrobat/Chromium while still surfacing corruption
➕ Avoids breaking loads while enabling observability
➖ Requires a logging/diagnostics mechanism (or event) in the parser layer
➖ Potential noise if many PDFs contain minor spec violations

Recommendation: The chosen approach (skip unkeyed values while preserving valid pairs) is a good default for real-world interoperability and also fixes a more subtle mispairing risk with odd item counts. If the project later adds a diagnostics channel, emitting a non-fatal warning when skipping a stray value would provide visibility without reverting to hard failures.

Files changed (3) +132 / -7

Bug fix (1) +12 / -5

Parser.csSkip unkeyed dictionary values instead of throwing during ReadDictionary +12/-5

Skip unkeyed dictionary values instead of throwing during ReadDictionary

• Reworks the dictionary pairing loop to advance one item at a time, pairing only when the current item is a PdfName. Non-name items are treated as stray values and skipped, and truncated trailing keys are naturally dropped without assertions.

PdfSharpCore/Pdf.IO/Parser.cs

Tests (2) +120 / -2

DictionaryEntryWithoutKeyTests.csAdd regression tests for pdfTeX-style keyless dictionary values +115/-0

Add regression tests for pdfTeX-style keyless dictionary values
• Introduces a new test suite reproducing pdfTeX writing bare strings into the /Info dictionary. Verifies documents open across modes, keyed entries are preserved, stray values are dropped, and save/reopen continues to work.
PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs

RawPdf.csAllow adding extra trailer entries in RawPdf.Build +5/-2

Allow adding extra trailer entries in RawPdf.Build
• Extends the test PDF builder to accept optional extra trailer entries (e.g., /Info 5 0 R). Enables tests to mirror how pdfTeX attaches the information dictionary via the trailer.
PdfSharpCore.Test/IO/RawPdf.cs

Copilot

Pull request overview

This PR improves PdfSharpCore’s resilience when reading malformed PDF dictionaries by tolerating stray dictionary values without keys (notably the PTEX.FullBanner strings produced by pdfTeX), aligning behavior with common PDF readers and fixing the failure described in empira/PDFsharp#376.

Changes:

Updated Parser.ReadDictionary to advance one item at a time, pairing only PdfName keys with the following value and skipping unkeyed values.
Extended the RawPdf.Build test helper to optionally inject extra trailer entries (used to model pdfTeX’s /Info trailer reference).
Added a focused regression test suite covering pdfTeX-style /Info dictionaries, odd-item mispairing prevention, and save/reopen behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
PdfSharpCore/Pdf.IO/Parser.cs	Makes dictionary parsing tolerant of unkeyed values to avoid rejecting otherwise readable PDFs.
PdfSharpCore.Test/IO/RawPdf.cs	Adds optional trailer augmentation to support constructing pdfTeX-like PDFs in tests.
PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs	Adds regression coverage for unkeyed dictionary values and ensures correct pairing and round-tripping.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    ///   Anything in <paramref name="extraTrailerEntries" /> is written into the trailer beside
+    ///   the size and the root.


qodo-code-review · 2026-08-09T04:26:51Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. ~~Parser tests missing xUnit timeouts~~ ✓ Resolved 📘 Rule violation ▣ Testability

Description

The new malformed-input parser tests are synchronous and lack xUnit timeouts, so a parser/scanner
hang would block the test runner indefinitely. This violates the requirement to make such tests
async Task with [Fact(Timeout=...)]/[Theory(Timeout=...)] and to execute the potentially
blocking call inside Task.Run(...) and await it.

Code

PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs[R36-38]
+    [Fact]
+    public void TheEntriesThatDoHaveAKeyAreStillRead()
+    {

Relevance

●●● Strong
Team has accepted prior test-quality improvements; adding async+Timeout to prevent hangs is
low-risk, policy-aligned.
PR-#18
PR-#19

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2557504 requires malformed-input lexer/parser tests to be async with xUnit timeouts
and to wrap the potentially blocking parser invocation in Task.Run and await it. In
DictionaryEntryWithoutKeyTests, multiple tests use [Fact]/[Theory] with public void ... and
call PdfReader.Open(...) directly, with no timeout and no async/task wrapper.

Rule 2557504: Use async xUnit tests with timeouts for malformed input scanner tests
PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs[36-43]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests exercising malformed PDF dictionary parsing are synchronous and have no xUnit timeout, so a hang can stall the entire test run.

## Issue Context
Compliance requires malformed/adversarial lexer/parser tests to be `async Task`, annotated with a timeout, and to run the potentially blocking parse call inside `Task.Run(...)` and `await` it.

## Fix Focus Areas
- PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs[23-97]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context used

✅ Compliance rules (platform): 14 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

A parser change can spin rather than throw, and these tests feed the parser dictionaries no writer should produce. They now open the document on a worker thread under a timeout, the way the content lexer tests already do, so a pairing loop that stops advancing fails the run instead of stalling the test host on it. The trailer entries the builder takes are copied in verbatim, so say so: a caller that leaves the leading solidus off gets a malformed document rather than an argument error.

Copilot AI lite review requested due to automatic review settings August 9, 2026 04:23

Copilot started reviewing on behalf of jafin August 9, 2026 04:23

View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Comment thread PdfSharpCore.Test/IO/RawPdf.cs Outdated

Comment on lines +15 to +16

/// Anything in <paramref name="extraTrailerEntries" /> is written into the trailer beside

/// the size and the root.

qodo-code-review Bot reviewed Aug 9, 2026

View reviewed changes

Comment thread PdfSharpCore.Test/IO/DictionaryEntryWithoutKeyTests.cs Resolved

jafin merged commit e5c2a69 into master Aug 9, 2026
2 checks passed

jafin deleted the fix/dictionary-entry-without-key branch August 9, 2026 05:10

This was referenced Aug 9, 2026

Read an object that ends without saying so #69

Merged

Scan a hex string by the digits in it rather than for a '>' that never comes #70

Merged

Navigation Menu

Repository navigation

Skip a dictionary value that has no key rather than refusing the document - #64

Skip a dictionary value that has no key rather than refusing the document#64
jafin merged 2 commits into
masterfrom
fix/dictionary-entry-without-key

jafin commented Aug 9, 2026

Uh oh!

coderabbitai Bot commented Aug 9, 2026 •

edited

Loading

Review limit reached

Uh oh!

qodo-code-review Bot commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

qodo-code-review Bot commented Aug 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		/// Anything in <paramref name="extraTrailerEntries" /> is written into the trailer beside
		/// the size and the root.

Conversation

jafin commented Aug 9, 2026

What was wrong

The fix

Tests

Uh oh!

coderabbitai Bot commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

qodo-code-review Bot commented Aug 9, 2026

PR Summary by Qodo

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

qodo-code-review Bot commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 9, 2026 •

edited

Loading

qodo-code-review Bot commented Aug 9, 2026 •

edited

Loading