Repository navigation
Improve image size - #1
Merged
Merged
Conversation
… bugs An infinite loop inside imageSize cannot be caught by the timeout option of node:test, because the loop is synchronous and the event loop never gets a chance to fire the timer. imageSizeIsolated runs the payload in a child process that is killed from the outside instead, so a hang shows up as a failed assertion rather than a frozen suite. Baseline measurements captured against the current code, to be used as the budgets of the upcoming regression tests: JPEG, 512KB 12837ms, then a RangeError HEIF, 4000 ispe boxes 3551ms TIFF, 512KB 970ms ICNS/HEIF/JXL, size 0 never terminate
getView built its DataView without a length, so it spanned the rest of the
underlying ArrayBuffer rather than the Uint8Array it was given. Any parser
reading past the end of a short input therefore returned adjacent memory.
Node allocates small Buffers out of a shared 8KB pool, so those bytes
routinely belong to unrelated data.
Proven from the public API: an 8-byte view onto a 4096-byte buffer filled
with 0x41 made imageSize return { width: 1094795585, height: 1094795585 },
which is 0x41414141 read out of bounds.
Passing the length also turns the RangeError that leaked out of DataView on
truncated inputs into the documented TypeError.
readBox mishandled three cases of the box size field. A size of zero means the box runs to the end of the file and a size of one means the real size follows as a 64-bit value, but both were read as literal byte counts. Sizes between two and seven are impossible, yet findBox happily crawled forward one byte at a time through them. readBox also gave up entirely on any box declaring more bytes than the input held, which is what made a cropped HEIF fail with 'no ipco box found' (issue #452). Clamping the box to what survived the cut keeps the metadata that is present readable. Boxes now report their headerSize, so HEIF, JP2 and JXL locate payloads after a 64-bit header instead of assuming eight bytes. The findBox assertions in utils.spec and malformed.spec encoded the old behaviour and were updated: a zero-sized box is now the last box of the file, and an over-declared box is reported at its real extent.
) An ICNS entry declares its own length, and the parser added that length to the cursor with no lower bound. A crafted entry of length zero left the cursor where it was and spun the event loop for ever. Proven from the public API with the payload from GHSA-w3rx-r6r6-pgpr, run out of process: before this change the child had to be SIGKILLed after two seconds, it now returns a TypeError. Unknown OSType codes are skipped as well. They used to produce entries with undefined width and height that travelled all the way back to the caller.
The ipco walk called findBox for ispe and again for clap on every turn, and findBox restarts its scan from the offset it is given. A file packed with image properties therefore cost quadratic time: 4000 of them, in 78KB, took 2122ms of blocked event loop, and 512KB would have run for minutes. Reading the direct children of ipco in a single pass brings the same file under 10ms. It also gives clap its correct meaning: it now crops the image property it follows, where the old scan could apply the first clap of the file to every image before it. The zero-sized ispe of GHSA-5p2g-fcmc-qvqq no longer spins either, since box sizes are read to spec; the payload is kept here as a regression test.
A jxlc box with no payload, and jxlp boxes carrying only their header, both produced a zero-length codestream that passed the truthiness check and blew up inside BitReader with a bare Error reading 'Reached end of input'. The container now reports the real problem, as the TypeError the rest of the library throws. The zero-sized jxlp box of GHSA-5p2g-fcmc-qvqq no longer spins either, since box sizes are read to spec; the payload is kept here as a regression test.
Realigning on a bad segment length did input = input.slice(1), copying the whole remainder for every byte skipped. A 512KB file of bytes that never spell a marker took 15168ms of blocked event loop and then failed with a RangeError. imageSizeFromFile hands the parsers exactly 512KB, so any file that large reached this path. Walking the same buffer with an index brings it to 21ms and a TypeError, and closes issue #96, 'Corrupt JPG, exceeded buffer limits'. Also handles a file that opens straight on its frame header. Skipping four bytes past the signature stepped over the marker of a FF D8 FF C0 file, so a valid baseline JPEG was reported as having no size (issue #434).
extractTags resliced the rest of the buffer for every 12-byte entry, so a 512KB file whose tag list never terminates took 1279ms of blocked event loop. Reading the entries in place brings it to 11ms. Requiring a whole entry to be present before reading it also removes the truncated read at the end of a cropped directory. tiff.ts also imported node:fs without ever using it, which dragged a Node builtin into every browser bundle of the library.
Running out of bits threw a bare Error, so a JXL container holding a codestream cut short in its header escaped the TypeError contract that the rest of the library keeps. Callers filtering on TypeError saw it as an internal failure rather than a malformed image. Reachable from the public API through a jxlc box carrying only the two byte codestream signature. The signed-overflow risk on 30-bit dimensions turned out not to exist: a 30-bit read tops out at 2^30-1, which never reaches the sign bit, and an exhaustive check across all eight bit alignments confirms it.
The entry count comes straight from the file. A 22 byte header announcing 65535 icons produced an array of 65535 entries whose width and height were undefined, and those travelled back to the caller. The count is now capped by what the input can actually hold, and a header with no entry behind it is rejected instead of returning one made of undefined. CUR gets the same fix, since it shares this code path.
PNG.validate threw 'Invalid PNG' when the header chunk was not an IHDR. A validator is asked whether the input is of its format, and answering with an exception means one broken candidate can abort the whole detection sweep. The signature check now answers that question and calculate reports the corruption, with the same message and the same behaviour through the public API, as the invalid.spec fixtures show.
Detection asks every format in turn whether the input is theirs. Since reads are bounded, a validator handed a buffer shorter than the header it wants throws, and that exception ended the sweep for every format behind it. A two byte JXL codestream was reported as 'Truncated input, cannot read past the end of the data', the complaint of an unrelated parser that happened to be earlier in the list, instead of being recognised and rejected as the truncated JXL stream it is. A throwing validator now simply means no.
Several parsers returned dimensions that no image has: a PNM header whose dimensions are not numbers came back as NaN by NaN, one with negative numbers came back as -5 by -5, and a BMP or GIF declaring an empty surface came back as 0 by 0. Callers sizing a layout from those values got silent nonsense rather than an error. A single check after the format specific work rejects anything that is not a positive integer on both sides. It is a net rather than a parser fix: it also catches whatever a future format gets wrong.
Documents the three advisories and the five further problems found while auditing, the measured before and after on each hostile payload, and the behaviour changes an integration will notice. Also records a limitation left alone on purpose: SVG detection reads only the first 1000 bytes, so an SVG behind a longer comment is missed.
The hardening changes what imageSize does with malformed input: sizes of zero, NaN and negative numbers now raise instead of being returned, and RangeError and bare Error give way to TypeError. Valid images are unaffected, but that is still a breaking change for anyone relying on the old answers, so it goes out as a major rather than a minor.
…file calculate decoded every byte of the file into one string, split it into an array of lines, then consumed that array with shift(), which recopies it on every call. Two costs followed from that: the header scan grew with the pixel data behind it, and a file made only of comment lines was quadratic. 512KB of comment lines, the size imageSizeFromFile hands the parsers, took 1847ms of blocked event loop. It now takes 32ms. Valid files gain even more, since the dimensions live in the first two lines and the megabytes after them were being decoded for nothing. Measured with npm run bench on a 2MB payload: ppm 178 -> 296,533 decode/s ppm/ascii 435 -> 304,286 decode/s pam 179 -> 250,153 decode/s PNM was the slowest format in the benchmark by three orders of magnitude and is now among the fastest. Small files improve too, 225,094 -> 301,993. Behaviour is unchanged, including on CRLF headers, which fail the same way they did before.
The two guards each tested the other format's signature, in the negative. A VP8 chunk was accepted whenever byte 0 was not the lossless 0x2f, and a VP8L chunk whenever bytes 3 to 6 were not the lossy start code. The comments were swapped to match. Both directions were wrong. A lossy chunk with no 9d 01 2a start code was sized from whatever the following bytes held, fabricating a 320x240 from arbitrary data; the dimensions are positive integers, so the output check added in 3.0.0 cannot catch them. A lossless chunk with no 0x2f signature fabricated a 321x177 the same way. In the other direction, a valid VP8L stream whose packed dimension bits happen to spell 9d 01 2a was rejected outright. The benchmark fixture had a byte pinned to zero to work around exactly that, which is what put me onto this; the workaround is gone with the bug.
When boxes learned to report their headerSize, HEIF and JP2 were switched over but this one call site kept a hardcoded offset + 8. A JXL container whose ftyp box carries a 64-bit largesize had its brand read from the middle of that size field, so a valid file was reported as an unsupported type.
handle.read ignored its bytesRead result, so a short read left the tail of the buffer at zero and the parsers were handed bytes that were never in the file. That trades an error for a wrong answer, which is the worse of the two. Sizing the input to bytesRead means a truncated read is either parsed correctly from what arrived or rejected, never guessed at. The plan called for a retry loop instead. A single read plus subarray does the same job in one line, with no risk of spinning on a read that keeps returning zero, and no branch the tests cannot reach. Callers already accept a truncated view of large files, since MaxInputSize caps every input at 512KB by design. Empty file also threw a bare Error, outside the TypeError contract that 3.0.0 established for bad input.
calculate decoded the whole file to UTF-8 before matching a tag that validate has already guaranteed lies within the first kilo-byte. A root tag that runs past that window fails validate, so calculate never sees one. npm run bench on a 2MB payload: 1,869 -> 237,313 decode/s. SVG had become the slowest format once PNM was fixed, and its cost no longer depends on file size at all, 237,313 on 2MB against 239,137 on 512 bytes. The thirteen SVG files in specs/images/valid cover the attribute forms and are unchanged.
toUTF8String and toHexString both took a slice, which copies, only to read it once. A subarray is a view over the same bytes and decodes identically. Every format runs its validate through one of these two, so the gain is across the board. npm run bench, mean over all formats: 512 B payload 1,189,480 -> 1,936,778 decode/s 2 MB payload 1,118,203 -> 1,820,690 decode/s The formats that lean on string decoding gain most, roughly doubling: pnm 302,715 -> 643,531 and svg 237,313 -> 459,657 on the 2MB payload. The copies left in jpg and jxl are deliberate, since those buffers outlive the call that makes them.
Adds the quadratic PNM header scan and the unsigned WebP streams to Security, the JXL ftyp brand and the fromFile short read to Fixed, and the WebP signature requirement to the breaking changes. Replaces the claim that valid images were unaffected, which the second pass disproved: they are about twice as fast, measured against the 2.x benchmark rather than estimated. Notes TGA validation, the fixed J2C SIZ offset and PNM CRLF headers as known limitations, so the things left alone are on the record alongside the things fixed.
Every call pushed one job and immediately drained the queue itself, and nothing tracked what was already running. Each caller therefore started its own batch: the queue never accumulated, and the limit of 100 never engaged. Measured with fs.promises.open instrumented, 500 files at once: 500 handles open simultaneously, against the 100 the queue is there to enforce. That is the EMFILE the comment says it prevents. It now peaks at exactly 100, and the batch takes the same time, 11ms against 12ms. The queue is now drained by a pump that starts a job only when a slot is free, and starts the next one as each job settles. That also retires the setTimeout retry: it existed to pick up a backlog that, with the accounting broken, could never form.
…iner extractCodestream copied the entire jxlc payload so that the stream parser could read the nine bytes of header at its start. On a 32MB containerized image that is a 32MB allocation and memcpy per call: 2.252ms, against 0.009ms now, for the same dimensions. The bit reader only indexes the array it is given, so a view behaves identically. The jxlp partials become views too, which spares them a copy on the way into the concatenation buffer. That buffer stays a real copy, since the partials have to end up contiguous, and it is bounded by the input. imageSizeFromFile caps its input at 512KB, so this is for callers handing imageSize a whole buffer of their own.
validateExifBlock copied the whole APP1 segment, up to 64KB, and extractOrientation then copied each twelve byte IFD entry out of that copy, one allocation per tag. Both are read-only walks. A segment packed with 5000 entries: 1.221ms per call, against 0.339ms now, for the same answer. Every read already goes through getView, which honours the view's byteOffset and bounds, so the entries can be read in place. The bounds behaviour is unchanged: an entry that runs past the end of the block raised a TypeError from getView before and still does, since the short slice it used to produce failed the same check.
The concurrency limit in Fixed, and the two copies removed from the JXL container and the EXIF walk in Performance, kept apart from the benchmark table since neither shape appears in its fixtures.
The two lines at the end of the file were reference-style link definitions for the CVE ids used in the Security section. Markdown does not render them, but in the raw file they sat under Known limitations and read as if the advisories were a limitation. They were redundant besides: each pointed at the same GitHub advisory the entry already linked inline. Each CVE id now links to its NVD record and the GHSA id to the advisory, two different resources instead of the same one twice, and the file ends on the limitations. Adds the one commit that had no entry, ea5cf96: a JXL container holding no codestream now says so, where it used to fail inside the bit reader with 'Reached end of input'. Also corrects the opening count. There are two advisories, not three; the second covers both HEIF and JXL.
The isolated harness gave each child two seconds to load ts-node and run imageSize. On a GitHub runner that budget was not enough, and every isolated proof failed as if the payload hung the library, including the harness's own self-test on a valid 8x8 PNG. The ICNS branch those proofs cover then showed up as missing from the coverage report. Two changes, because the budget was wrong in two ways. The child no longer type checks. It only has to run the library, and the test config pulls every spec file in with it, so checking again cost more than the run: a bare spawn goes from 455ms to 203ms here. The parent still type checks lib, and the build after it, so nothing is lost. The timeout is now 30 seconds. It exists to notice a loop that never ends, which never ends whatever the budget, and spawnSync returns the moment the child exits, so a generous value costs nothing on the passing path. The margin goes from roughly 5x to 145x. A short timeout still kills, so a real regression is still caught.
Fix JPEG lossless and arithmetic-coded frames read as corrupt TIFF: the wrong page of a multi-page file Remove Dataview
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.
No description provided.