Repository navigation
Conversation
This was referenced Oct 12, 2026
Owner
|
hi @Cwooper, thank you for the changes. The README is correct now. The object number limit is now the uint32 limit. The sparse table is a good solution. Before we merge, please do these steps:
After these steps, this PR is ready for review. Thank you. |
Content stream operators:
- Q with an empty graphics stack indexed gstack[-1]. A content stream may
hold more Q than q, and Page.Content has no error return, so this
panicked out of a public API on a one-byte content stream.
- Tm in walkTextBlocks read args[4] and args[5] without checking how many
operands were supplied.
ToUnicode CMaps:
- Entry counts were declared by the file and trusted. "268435456
beginbfchar endbfchar" appended 2.7e8 empty entries, about 8.6GB, from a
76-byte stream. A count is now only honoured if the operands to fill it
were actually supplied, which bounds the work by real input.
- A codespace range's width indexes cmap.space[4], so a 5-byte range was
out of range.
- Interpret's own panics (mismatched begin/end, def without a dict) escaped
readCmap into Page.Content, which cannot report them. A malformed cmap
now degrades to no cmap, as a failed parse already did.
Cyclic references. /Parent, /Kids, /First and /Next are object references, so
a file can point them back at themselves. Walking them looped forever, and
the outline case recursed until the goroutine stack was exhausted, which is a
fatal error a caller cannot recover from. All four traversals are bounded.
GetTextByColumn and GetTextByRow recovered into unnamed results, so the
deferred function only reassigned its own locals: a panic surfaced as
(nil, nil) and the error was silently dropped. Naming the results fixes it.
Reader.GetPlainText and Reader.GetStyledTexts now recover too. Both call into
object resolution and Page.Content, which panic on malformed input, so they
could not honour their error returns.
Page.Content is documented as panicking, since it has no way to report a
malformed content stream and callers handling untrusted files need to know.
Conflict resolution (cherry-picked onto ledongthuc/pdf master):
- GetTextByColumn/GetTextByRow: master's b3c860c already names the results
and recovers through recoverTo; kept master's version and dropped this
commit's copy of the same fix.
- bfrange scaling: master's addOffset (upstream PR 95) already handles an
empty destination, so this commit's len(s) > 0 guard on the old
last-byte increment is dropped in favor of master's carry.
The loop read s[i+1] on the final iteration of an odd-length string. The callers that guard length (Value.Text, TextFromUTF16) made this look safe, but cmap.Decode passes bfchar and bfrange replacement strings taken verbatim from the file, so a replacement of an odd number of bytes panicked. A trailing odd byte cannot form a UTF-16 code unit, so it is dropped.
seekForward computed a buffer position that could be negative, for a seek to a negative offset or to one behind the data still held in the buffer. That left pos negative, which indexed buf out of bounds on the next read rather than at the seek itself. Conflict resolution (rebased onto ledongthuc/pdf master): - readHexString: this commit also stopped both whitespace-skipping loops at end of input, so an unterminated hex string no longer spun forever. Master's d0d294e (upstream PR 91) makes the same fix, so this commit's copy is dropped and its subject now names what remains.
The cross-reference table is preallocated and indexed from values taken
straight out of the file, none of which were checked.
- /Size in an xref stream sized make([]xref, size) directly. A negative
value panicked; a large one exhausted memory. Because a mid-range value
reaches the allocator rather than the length check, this surfaced as a
Go out-of-memory error, which is fatal and cannot be recovered.
- /Index and classic subsection headers name object numbers used to index
the table. A two-element /Index of [4000000000 1] grew it by ~128GB on
one entry of input.
- Growing the table's capacity does not necessarily grow its length past
the index being written, so table[x] could still be out of range. The
xref stream path was missing the reslice the classic table path has.
- A negative trailer /Size reached table[:size] as a negative bound.
- A /W field width was used as a slice bound without a sign check, and
summed into the size of the read buffer without a magnitude check.
The table is no longer bounded by the file length: an xref stream is
compressed, so a small file can legitimately describe far more objects than
it has bytes. Instead the preallocation is capped on its own, with the table
still growing to whatever the file really contains, and object numbers are
bounded separately.
Also in the same untrusted path: startxref and /Prev offsets are range
checked before being handed to the lexer, which reports an unreadable stream
by panicking; a FlateDecode /Columns is checked before it sizes a row
buffer; object stream /Extends chains and nesting are bounded, since a cycle
there recursed until the stack was exhausted; and the object stream pair
loop stops at the end of the data instead of trusting the declared /N.
NewReaderEncrypted now converts the lexer's panics into errors. The lexer
signals malformed input by panicking, and every caller opening an untrusted
file had to recover for itself or crash.
Fixes the trailing-newline underflow too: the hand-rolled strip loop read
buf[len(buf)-1] after emptying buf, because && binds tighter than ||, so a
file ending in newlines indexed buf[-1]. The following TrimRight already
does that job correctly, so the loop is simply gone.
Conflict resolution (cherry-picked onto ledongthuc/pdf master):
- readXrefStream/readXrefTable /Prev loops: master's b3c860c folded both
into readPrevXrefs, so the sectionReader range check moved there (one
site instead of two) and the negative prev /Size check went into its
closure.
- Table growth: master's ensureXrefLen already grows the length past x,
so this commit's reslice is dropped as a duplicate.
- NewReaderEncrypted: master already recovers panics into errors; kept
master's recover and added only the too-short check. Dropped
Reader.errorf, which master removed as unused.
101 assertions across 17 tests. Every malformed case was confirmed to panic, hang, or allocate without bound before the corresponding fix, and each runs under a timeout so a regression that reintroduces an infinite loop fails rather than hanging CI. The suite also pins the behaviour the bounds must not break: a well-formed PDF, one that keeps its objects in an object stream indexed by a cross-reference stream, a well-formed multi-entry CMap, a table larger than the preallocation cap, and a compressed xref stream describing 100000 objects in under 8KB, which is the case that rules out bounding the table by file length. There were no tests in the repository before this, so the CI job that runs go test ./... had nothing to run.
/Index [8388600 1] in a 223-byte file allocated over a gigabyte growing the cross-reference slice to the named object number. The table is now a dense slice that grows in proportion to the entries stored, with far-off numbers kept in a map, so memory follows the entries a file actually contains rather than the numbers it declares, and a legally sparse file (one object numbered 500000 among a handful) still resolves. A Reader that never loaded a table treats every reference as unresolvable instead of dereferencing nil. Split out of the author's 0b37be3 in chappihappymeal/pdf ("close the gaps found reviewing the absorbed hardening"), so it can travel with upstream PR 78, whose review asked for it. Dense growth reuses master's ensureXrefLen.
The 1 << 23 object-number cap refused legal files that number their objects sparsely. With the sparse table, memory follows the entries stored rather than the numbers named, so object numbers only need to fit an objptr. What still needs a bound is the entry count: a few kilobytes of compressed xref stream can describe billions of entries, which the old cap prevented only by accident.
Object numbers go up to the uint32 range objptr holds, but the xref readers built each one as int(start) + i and put took an int. On a 32-bit build a number at or past 1<<31 wrapped negative, missed the dense-growth check, and then indexed dense out of range. put now takes the uint32 an objptr carries, and both readers form the number in int64 before narrowing, since the subsection bounds are already checked against that range. TestSparseObjectNumberResolves now also opens a file whose catalog is object 3000000000, which panicked under GOARCH=386 before this change.
size returned the dense slice's length plus the sparse entries, which is neither the span of object numbers its comment claimed nor the number of entries stored. Only two tests called it, to check that a table past the preallocation cap is read in full, and reading each entry back checks that directly.
Author
|
Thanks for the review! Rebased onto master and everything's in. The Great catch on 386. I removed CI only runs on amd64, so nothing would catch that breaking again. I'd recommend adding a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This carries #78 forward, since @gage-marshall handed it over. Their five commits are unchanged apart from the rebase onto master, so the description and review over there still apply.
What's different, following @ledongthuc's review on #78:
README.mdis back to master.objptralready holds, so a legal sparse file isn't refused anymore. The limit is now on how many entries a table actually stores (1<<23)./Index [8388600 1]file stays small now, and one object numbered 500000 among a handful still resolves. Both are tests inhardening_test.go.@chappihappymeal, I only took the sparse-table part, so the rest of your follow-ups are still yours to send once this lands.