Repository navigation
Fix parseBuffer with offset buffers - #437
Conversation
modesty
left a comment
modesty
left a comment
There was a problem hiding this comment.
The actual bug is in the engine and is still there base/core/stream.js: Stream.makeSubStream builds sub-streams from this.bytes.buffer, which loses the view's byte offset. This is the root cause.
Changing it to new Stream(this.bytes, start, length, dict) passes all 12 new tests on Node 22 and 24 without any copying.
The approach in this PR copies the whole PDF on every parse of a buffer it doesn't own, doubling peak memory for large inputs. And it copies again each time a cached buffer is re-parsed, and copies small stream inputs twice; This would leave the same offset bug in place for other engine code that builds a Stream from a view (for example fonts.js:5437, fonts.js:5448). Fix the root cause would avoid the memory issue and other offset places would benefit too.
Currently, the new test fails 8 of 12 cases on master and passes all 12 on the PR branch, on both Node 22.23.3 and Node 24.21.0.
| return bytes; | ||
| } | ||
| return Buffer.from(new Uint8Array(bytes).buffer); | ||
| } |
There was a problem hiding this comment.
this would break typed arrays other than Uint8Array : #toOwnedBuffer uses new
Uint8Array(bytes), which converts value by value instead of copying the raw bytes. So parseBuffer(new Uint16Array(pdfBytes.buffer)) parses on master but fails on this branch with "Invalid XRef stream".
The fix is to always build a Uint8Array view over the same bytes and, when a copy is needed, use
buffer.slice(byteOffset, byteOffset + byteLength).
5a9af2c to
054a4dd
Compare
|
@modesty feedback addressed. |
modesty
left a comment
modesty
left a comment
There was a problem hiding this comment.
it's very close, but has a regression, see inline comments for details.
Also, the following items are cheap to fix:
- test/_test_parseBuffer_offset.cjs:77: The tests depend on running in order. Running one test alone fails with a misleading SyntaxError. Move the baseline parse into a before() hook.
- test/_test_parseBuffer_offset.cjs:8: is its header on inputs "must be copied" valid?
- src/types/pdfparser.d.ts:41 : Widen pdfBuffer: Buffer to Uint8Array to match what the PR now supports.
| } | ||
|
|
||
| this.#startParsingPDF(pdfBufferParse); | ||
| this.#startParsingPDF(pdfBuffer); |
There was a problem hiding this comment.
regression: typed arrays other than Uint8Array stopped working, it fails with Invalid XRef stream when passing a Uint16Array (or Int32Array, Float32Array, etc.) over PDF bytes, while on master it parses ok.
It results from the removing the guard: it also turned non-byte typed arrays into a byte view. Without it, each element gets cut down to 8 bits, which corrupts the data.
Consider a safer bet like:
const bytes = pdfBuffer instanceof Uint8Array
? pdfBuffer
: new Uint8Array(pdfBuffer.buffer, pdfBuffer.byteOffset, pdfBuffer.byteLength);
this.#startParsingPDF(bytes);Also consider to dd a Uint16Array case to test/_test_parseBuffer_offset.cjs so it can't break again.
054a4dd to
d16d668
Compare
|
Thanks, I've applied fixes for all of the above. |
- parseBuffer: accept any non-empty ArrayBufferView, including DataView,
which the old check rejected. Inputs that are not views still throw.
- loadPDF/parseBuffer: use so verbosity 0 (ERRORS)
is honored instead of falling back to WARNINGS.
- Stream: check explicitly, so a sub-stream at
offset 0 with length 0 is empty instead of the whole buffer.
- Field tab indexes (TI): number them per document, from 0, in page
order. The counter was shared by the whole process, so TI depended on
how many documents had been parsed before, including concurrently.
- types: parseBuffer(pdfBuffer: ArrayBufferView) in .d.ts and .d.cts,
matching the typed-array support the implementation already had.
- JSDoc: parseBuffer and #startParsingPDF describe the accepted inputs
and the throw.
Tests:
- _test_parseBuffer_offset: add a DataView case; force Buffer.from to
use the shared pool at a non-zero offset (before, the test passed even
with the bug on Node 22's 8 KiB pool); compare full output now that TI
is deterministic; the helper uses once() and always calls destroy();
add unit tests for Stream sub-streams against base/core/stream.js.
- _test_errors: inputs that are not views and an empty DataView throw;
verbosity 0 hides engine warnings for both parseBuffer and loadPDF.
- _test_testMultipleDataPDF: TI starts at 0 for repeated, same-parser
and concurrent parses.
Docs: README API reference documents parseBuffer inputs, the
verbosity levels and how TI is numbered; unit suite counts updated.
Behavior change: when several PDFs are parsed in one process (e.g. the
CLI on a directory), TI now starts at 0 for each file instead of
continuing from the previous one. Corpus check: across 1064 test:forms
output files, the only differences from master are TI values.
parseBufferfailed on buffers that are views into a largerArrayBufferat a non-zerobyteOffset, because PDF.js'smakeSubStreamreads frombytes.bufferwith absolute positions and the old guard rebuilt the buffer as another view instead of copying it.Node 24.18 raised
Buffer.poolSizeto 64 KiB, so anyBuffer.from/subarrayresult under 32 KiB now lands at an arbitrary pool offset and parsing fails with "Invalid XRef stream header".Whilst I encountered the bug after a Node 24.15 -> 24.21 upgrade, tests don't depend on using the latest Node version.
Given that #424 hasn't moved forward in the last 3 months, I've created a new PR.