Turn a corrupt PNG's decode crash into a catchable error - #1788
Closed
afonsojanu wants to merge 1 commit into
Closed
Turn a corrupt PNG's decode crash into a catchable error#1788afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
png-js decodes IDAT pixel data through Node's async zlib.inflate and rethrows any inflate error from inside that callback. That happens on a later tick, so it can't be caught by a try/catch around document.image(), and it takes the whole process down (issue foliojs#1747). Added an inflateSync pass on the same compressed bytes right before handing them to png-js's decodePixels, in all three places pdfkit calls it (splitAlphaChannel, loadIndexedAlphaChannel, decodeData). Since it runs synchronously inside embed(), which is itself called synchronously from document.image(), a bad IDAT stream now throws in the same spot PDFImage.open() already throws for other malformed input, instead of crashing later from a spot nothing can catch. The #zlib abstraction only had deflateSync before this (used for re-encoding decoded pixels), so extended both implementations with an inflateSync as well: Node's zlib.inflateSync for the node build, and fflate's unzlibSync for the browser build, since that's the inverse of the zlibSync it already uses for deflateSync. Added a regression test reproducing the exact corrupted PNG from the issue and confirming document.image() now throws synchronously. Confirmed it fails on the unmodified code (as an uncaught exception, matching the reported crash) and passes with the fix. Also added direct round-trip and corrupt-data tests for the new inflateSync on both zlib implementations. Full suite (unit + visual, 494 tests) and eslint both pass.
Member
|
png.js is being reworked to improve the error handling and allow sync decode foliojs/png.js#100 we will update png.js once a new release lands |
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
2 participants
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.
Fixes #1747.
png-js decodes IDAT pixel data through Node's async
zlib.inflateand rethrows any inflate error from inside that callback. That happens on a later tick, so it can't be caught by a try/catch arounddocument.image(), and it takes the whole process down instead.I added an
inflateSyncpass on the same compressed bytes right before handing them to png-js'sdecodePixels, at all three places pdfkit calls it (splitAlphaChannel,loadIndexedAlphaChannel,decodeData). That runs synchronously insideembed(), which is itself called synchronously fromdocument.image(), so a bad IDAT stream now throws in the same spotPDFImage.open()already throws for other malformed input, rather than crashing later from somewhere nothing can catch.The
#zlibabstraction only exposeddeflateSyncbefore this (used for re-encoding decoded pixels), so I extended both implementations withinflateSynctoo: Node'szlib.inflateSyncfor the node build, and fflate'sunzlibSyncfor the browser build, which is the inverse of thezlibSyncit already uses fordeflateSync.Added a regression test reproducing the exact corrupted PNG from the issue and confirming
document.image()now throws synchronously instead of crashing the process. Confirmed it fails on the unmodified code (surfaces as an uncaught exception, matching the reported bug) and passes with the fix. Also added direct round-trip and corrupt-data tests for the newinflateSyncon both zlib implementations.Full suite (unit + visual, 494 tests) and eslint both pass.