Repository navigation
Conversation
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. |
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.