Ignore a "Q" that has no matching "q"

[wouterschreurs]

Ignore a Q that has no matching q

Small, well-scoped bug fix, with a test.

Problem

PDFObject::getTextArray() pushes the current font and graphics matrix onto $clipped_font / $clipped_position_cm on q, and pops them on Q:

case 'Q':
    list($current_font, $current_font_size) = array_pop($clipped_font);
    $current_position_cm = array_pop($clipped_position_cm);
    break;

Neither pop is guarded. When a stream restores a graphics state that was never pushed, both stacks are already empty, so array_pop() returns null and:

• $current_position_cm is null, and the next text-showing operator reads offsets off it → Warning: Trying to access array offset on null (PDFObject.php:909 and :910);
• $current_font is null and reaches getTJUsingFontFallback() → TypeError: Argument #1 ($font) must be of type Font, null given.

How a real file gets there

Not by authoring a stray Q. This came from a product catalog (20 pages, PDF-1.7) that arrived as an email attachment and hit our mail importer in production, producing 276 warnings in one parse.

Counting operators in the raw file gives 2111 q and 2111 Q — balanced. The imbalance is created at the lexer: one page content stream (92,247 bytes) begins

qqqqqqqqqqqqqqqqqqqqq
0 0 841.8900 594.7640 re
W n

— 21 save-state operators concatenated with no separators. That same stream holds 355 properly separated q and 376 properly separated Q, and 355 + 21 = 376, so the file's intent is balanced; only the encoding of those 21 saves is broken.

Per 7.2.2 a keyword token runs until white-space or a delimiter, so qqqq…q is a single unknown keyword rather than 21 operators. PdfParser is right not to treat it as q — but then nothing is pushed while the 21 matching Qs still pop. Instrumenting the Q case on that file reports exactly 21 underflows.

Introduced by #634, which replaced the single stored state with a stack; first released in v2.8.0.

Fix

Skip each pop when its stack is empty, keeping the current font and matrix — which is how a viewer treats the same stream. No attempt is made to lex the run as 21 q operators: per spec it is not 21 operators.

Test

PDFObjectTest::testGetTextWithUnmatchedGraphicsStateRestore reproduces the real shape, no PDF fixture needed:

$content = "qq\nBT /F1 12 Tf 10 10 Td (Hello) Tj ET\nQ\nQ\nBT 10 -20 Td (World) Tj ET";

static::assertSame("Hello\nWorld ", (new PDFObject($document, null, $content, new Config()))->getText());

On master this errors with 2 warnings; with the fix it passes. The properly separated form (q\nq\n…) behaves identically before and after — the trigger is the concatenation, not the operator count.

Scope of the impact

On the file above the text still comes out: 54,219 characters, byte-identical with and without this patch (sha1 1cad49de23f860ada2b84fe9fed847836676a4ba). The cost there is 276 logged errors, not lost content. Losing the remainder of the stream requires no Tf to follow the Q, so the font stays null and the TypeError fires — real, but not what this file hits.

Checks

Run against master on PHP 8.4.19:

• PHPUnit — 194 tests, 1185 assertions, all green (193 before this PR's test).
• PHP-CS-Fixer — Found 0 of 82 files that can be fixed.
• PHPStan — the 4 pre-existing findings are identical with and without this change; it adds none. (Run with PHPStan 2.x rather than the pinned ^1, since dev-tools could not be installed in my environment.)

The change is PHP 7.1-compatible, in line with composer.json.

[k00ni]

Introduced by #634, which replaced the single stored state with a stack; first released in v2.8.0.

CC @GreyWyvern

[GreyWyvern]

This is a simple enough change. I believe I actually had a check like this in place earlier on when my own code was reading extra Q's and q's in certain special text strings unintentionally as commands. When I fixed that, I was really happy to remove this check because all of the PDFs in the test suite were now balanced. :)

I suppose there are malformed PDFs out there that would still require this.

I'm not at my PC right now, but I would like to know if there is a REAL live PDF file example out there of this error, and if so, would that file load up successfully in Acrobat?

[wouterschreurs]

Yes — there is a real file, and chasing it down changed my understanding of the bug, so I've updated the PR and its test.

The file. A 20-page, 1.9 MB, PDF-1.7 product catalog that arrived as an email attachment on one of our customer-service inboxes. Our mail importer parses PDF attachments to look for order references, and this one produced 276 warnings in a single parse (138 text-showing operators × 2). It's the only occurrence in a week of production traffic, so: rare, but real, and not synthetic.

It is not an authored stray Q. That was my assumption and it was wrong. Counting operators in the raw file gives 2111 q and 2111 Q — balanced. The imbalance is created at the lexer.

One page content stream (92,247 bytes) begins, literally:

qqqqqqqqqqqqqqqqqqqqq
0 0 841.8900 594.7640 re
W n

21 q operators concatenated with no separators. In that same stream there are 355 properly separated q and 376 properly separated Q — and 355 + 21 = 376. The file's intent is balanced; only the encoding of those 21 saves is broken.

Per 7.2.2 a keyword token runs until white-space or a delimiter, so qqqq…q is a single unknown keyword, not 21 operators. PdfParser is right not to treat it as q — but nothing is pushed, and the 21 matching Qs later pop anyway. Instrumenting the Q case on the real file reports exactly 21 underflows.

So your recollection was on the money — same family of problem, opposite direction. I deliberately did not try to lex the run as 21 qs: per spec it isn't 21 operators, and that's precisely the kind of handling you were glad to delete.

What it actually costs today. On this file the text still comes out — 54,219 characters, byte-identical with and without the patch (sha1 1cad49de23f860ada2b84fe9fed847836676a4ba). So for us the damage was 276 logged errors, not lost content. Total loss needs a stream where no Tf follows the Q, which is what my original synthetic test had; the TypeError from the null font is real but not what this file hits. I've corrected the PR description accordingly — it previously overstated the impact.

Acrobat. I can't answer that one honestly: I have no PDF renderer available in the environment where I have the file, so I haven't opened it. My expectation is that it renders fine — viewers ignore an unknown keyword, and a graphics-state underflow is likewise ignored — but that's reasoning, not a test. If it would help, I can get someone to open it in Acrobat and report back.

The test now reproduces the real shape rather than a lone Q:

$content = "qq\nBT /F1 12 Tf 10 10 Td (Hello) Tj ET\nQ\nQ\nBT 10 -20 Td (World) Tj ET";

static::assertSame("Hello\nWorld ", (new PDFObject($document, null, $content, new Config()))->getText());

Without the guard that errors with 2 warnings; with it, it passes. The properly separated form (q\nq\n…) is unaffected either way, which is the point — the trigger is the concatenation, not the operator count. Full suite is green (194 tests, 1185 assertions), PHP-CS-Fixer reports 0 of 82 files to fix.

I'd have attached the original, it's a third-party marketing catalog we received by email. I'm happy to produce a minimal synthetic PDF that reproduces the same qqq…q run if you'd like one for the test suite.
Cleaning Wipes (Dry & Wet).pdf

[k00ni]

For what its worth, you should check https://github.com/PrinsFrank/pdfparser. We use it in production and it is way more reliable than this PDF parser.

I converted your PR to a draft, in case you are still working on it. Please mark it "ready for review" when you are done.

[wouterschreurs]

Thanks — that's a generous pointer coming from a collaborator here, and we took it seriously rather than politely. We benchmarked both against real attachments from our ticket system and are migrating to prinsfrank/pdfparser, so you were right.

What tipped it wasn't speed. It was a supplier order confirmation (PDF 1.2, uncompressed content streams, 474 Tj/TJ operators — the text is plainly there in a hex dump) that this parser cannot read at all: it resolves the structure correctly, 2 pages and 30 objects, but every page returns 0 bytes of content, so getText() yields nothing. No exception, no warning. prinsfrank/pdfparser reads the same file to 4,115 characters with the order reference intact.

On a German delivery note the two were equivalent — identical 8 references recovered — except this parser welded three adjacent table headers into one token (ckEinzelpreisWarenwert) where the other kept Einzelpreis and Warenwert separate. That matters for us, since we regex order references out of the extracted text.

For balance, it isn't one-sided. prinsfrank/pdfparser throws on /Root 25 0 R — two spaces before the R — which is legal per §7.2.2; deleting those two bytes makes the same document parse fine. And on a PDF using a subsetted font with a custom encoding it emitted raw glyph codes rather than applying the ToUnicode CMap, where this parser correctly returned fewer but accurate words. Both are getting reported there.

So this is a fit-for-our-corpus decision, not a verdict on the library.

On the PR: it's ready for review — I've pushed an update since your comment. Chasing down a real-world example of the bug (which the other thread asked for) showed my original diagnosis was wrong, so both the description and the test now reflect what actually happens: the file's q/Q operators are balanced, but 21 of the saves are written as a single unseparated qqqqqqqqqqqqqqqqqqqqq token, which lexes as one unknown keyword rather than 21 operators — so nothing is pushed while the 21 matching Qs still pop. The guard itself is unchanged.

I don't have permission to flip the draft flag myself, so please mark it ready for review, or let me know and I'll ask a colleague to.

[k00ni]

@PrinsFrank the feedback here might be of interest to you:

#838 (comment)

---

Unfortunately, I am super busy right now, so I can't give any timeline when a merge will happen. But I am glad that we could refine the PR so fast that it might be an easy task.

[PrinsFrank]

@k00ni Thanks for tagging me and the recommendation!

@wouterschreurs I've fixed the bug with whitespaces in references in prinsfrank/pdfparser. I'll tag a new release shortly. I'm happy to look into the issue with toUnicodeCMaps, but I need an example file that has the issue as I'm unable to reproduce it at the moment. Feel free to open an issue with the sample, or you can also send me it in private if the sample shouldn't be publicly available!

[k00ni]

@wouterschreurs and @etrias-nl: Small ping to make sure this one was closed intentionally.