Read image metadata out of the file's structure, correct Exif Orientation on request, and re-encode with less memory - #35
Conversation
|
Disable by default, run /simplify and a performance test over this. |
c05e2c5 to
0c4d473
Compare
87fbac8 to
4557e18
Compare
Follow-up: read the remaining PNG chunks out of the chunk index, not with
|
| Site | Chunk | Feeds |
|---|---|---|
ImageProcessor.php:276 |
tRNS |
alpha channel for an indexed image |
ImageProcessor.php:296 |
tRNS |
alpha channel for greyscale/truecolour |
ImageProcessor.php:352 |
tRNS |
the transparent colour, as /Mask |
ImageProcessor.php:1265 |
tRNS |
alpha channel for an indexed image, second copy |
ImageProcessor.php:1287 |
tRNS |
alpha channel for greyscale/truecolour, second copy |
ImageProcessor.php:1405 |
iCCP |
the embedded colour profile |
They all have this shape:
$p = strpos($data, 'tRNS');
if ($p) {
$n = $this->fourBytesToInt(substr($data, $p - 4, 4)); // the length, read backwards from the type
$transparency = substr($data, $p + 4, $n);$chunk['size'] and $chunk['offset'] + 4 from pngChunk() drop straight in, exactly as they did for pHYs and gAMA in #35.
A third chunk walk
ImageProcessor.php:1480 has a hand-rolled $p = 33; do { ... } while ($n) loop that reads PLTE, tRNS, IDAT and iCCP correctly — it is the branch that assembles the image stream when no conversion is needed. It should become the same index the rest of the class uses, which is the larger part of this job and the part with the most to gain.
One design decision it forces: pngChunks() currently stops at the first IDAT, because every chunk read through it so far has to precede the image data and stopping there is 95–484× cheaper on a large PNG (measured). This walk needs IDAT and IEND, so it wants either a parameter or a second entry point.
Why it matters
The two strpos presence tests fixed in #35 turned out to be real, demonstrable defects, and these are the same class:
- a
tEXtchunk mentioningtRNSmade mPDF build a soft mask for an image with no transparency, adding a second image object to the PDF - a
tEXtchunk mentioningiCCPmade mPDF decode and re-encode an indexed image through GD to strip a colour profile that was not there
The six reads above are worse in one respect: they do not merely mis-set a flag, they read a length and an offset from wherever the match landed.
Test plan
PngChunksTest in #35 is the pattern — it builds its fixtures from a GD-generated PNG rather than committing binaries, and each test has a decoy chunk plus the real behaviour it must not disturb. This work needs fixtures per colour type (0, 2 and 3), per bit depth, and for palette and non-palette images, since the six sites divide along exactly those lines.
4557e18 to
ca09d0f
Compare
… the whole file Seven places were finding metadata with a substring search over the entire image, and the string each searched for is as likely to turn up in a comment, or among the compressed samples, as it is to be the marker or chunk that introduces the thing it wanted. A JPEG's marker segments and a PNG's chunks are now indexed properly, by jpgSegments() and pngChunks(), and all seven ask the index instead: - the JFIF density, which strpos($data, 'JFIF') would take from any four such bytes - the ICC profile, which strpos($data, "ICC_PROFILE\0") would assemble out of anything - the PNG density, from strpos($data, 'pHYs') - the PNG gamma, from strpos($data, 'gAMA'), and the sRGB chunk that overrides it, whose mere mention in a comment was enough to suppress a correction that should have happened - whether a PNG carries tRNS or iCCP at all, from two bare strpos() tests with no length check of any kind, which between them decide whether the image is embedded as it stands or decoded and re-encoded through GD: a comment mentioning tRNS was enough to build a soft mask for an image that has no transparency, and one mentioning iCCP to re-encode an indexed image for a colour profile it does not have - jpgHeaderFromString(), which walked to the start of frame by hand, stepping over segment lengths without checking that any of them landed inside the file Both walks read only the header structures: the JPEG one stops at the start of scan and the PNG one at the image data, which every chunk read here has to precede, so neither touches a byte of compressed samples. That also makes them far cheaper than the searches they replace, which read the whole file whenever the thing they were looking for was not in it - reading the ICC segments off an 8.6 MB JPEG that has none goes from 2.7 ms to 5 us. It also settles a hang. The ICC scan advanced by a length read from the two bytes before wherever "ICC_PROFILE\0" happened to match, and those bytes are only a length if the match was a real APP2 segment. A comment carrying the fourteen bytes 00 00 ICC_PROFILE\0 moves the search backwards onto itself, and it never terminates: a 20 KB JPEG built that way pins mPDF indefinitely on gravitypdf and renders in well under a second here. Both walkers yield rather than return. A JPEG can pack a million four-byte segments into under 4 MB, and a PNG a million empty chunks into 11 MB; indexing those eagerly costs 394 MB and 432 MB of PHP memory, against 20 MB and 28 MB when the entries go one at a time. The ICC read now also requires a segment large enough to hold the 14-byte header it indexes into, which it was checking the file for rather than the segment. $nom, read out of every ICC segment and never used, goes with them. Each parser names the clause it implements - ITU-T T.81 B.1.1.4 and Table B.1 for the marker segments, T.871 6.3 for JFIF, ICC Technical Note 10-21 for the profile chunks, and PNG (Third Edition) 5.2, 5.3 and 11.3.x for the chunks - so the offsets can be checked against the standard rather than trusted. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ca09d0f to
d9f3aea
Compare
d9f3aea to
6ad03ee
Compare
A photo off a phone is nearly always stored the way the sensor read it, with an Exif Orientation tag saying which way round that is. Browsers have honoured that tag by default since image-orientation: from-image became the initial value, so an image that sits the right way up on the page it was copied from arrives in a PDF on its side, or mirrored. Turn useImageExifOrientation on and processJpg() reads the tag, and when it says anything other than "as stored", rotates and mirrors the samples through GD before the rest of the pipeline sees them. Doing it there rather than at the drawing sites means the intrinsic width and height come back off the corrected image, so layout, background sizing, and every place that scales an image get the right numbers without knowing anything about Exif. The setting is off by default. Correcting an image costs a decode and a re-encode of it, and a document laid out around the samples as stored should not move under an upgrade. The quality of that re-encode is imageJpegQuality, which the WebP and AVIF conversions now read as well - they were already re-encoding at GD's implicit default, and 75 is that default, so their output is unchanged to the pixel. Raising it is the obvious temptation and it is a real change: at 85 those two conversions grow about a quarter, which moves the four snapshots that carry a WebP. 85 is the knee on a real photo, worth about 7 dB for 6% more bytes, and the setting is there for anyone who wants it. The snapshot asks for 95 because its fixtures are four saturated flat colours meeting at hard edges, which is the one thing chroma subsampling handles badly at any quality below 90. It is there to show which way up the samples are, not what JPEG does to a colour boundary. The Exif is read out of the marker segments jpgSegments() indexes, and the TIFF IFD behind them is parsed directly, in both byte orders, rather than through ext-exif, which is not a dependency and cannot read from a string. Anything malformed reads as orientation 1 and the image is passed through untouched. Only images that carry an orientation to correct are re-encoded; everything else, including every JPEG that has no Exif at all, comes through byte for byte as before. CMYK images are left alone because GD reads their samples without their colour space. An ICC profile is copied across to the re-encoded file, since the samples are still in the space it describes. The snapshot renders all eight orientations at a fixed width and no height, so a sample that was not corrected is drawn twice as tall as its neighbours rather than half as tall. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
6ad03ee to
67d5f8d
Compare
The previous commit moved seven metadata reads onto jpgSegments() and pngChunks() and left six
behind, along with a third hand-rolled chunk walk. They are the same defect and a worse form of
it: rather than mis-setting a flag, they take a length and an offset from wherever the four
letters happened to match and read a value out of that.
$p = strpos($data, 'tRNS');
if ($p) {
$n = $this->fourBytesToInt(substr($data, $p - 4, 4));
A tEXt chunk saying "tRNS" puts $n at the four bytes before the match, which are comment text,
so the read runs off with a nonsense length. All six now ask pngChunk(), which stops at IDAT
because PNG 5.6 has every chunk read through it precede the image data.
The hand-rolled walk becomes a foreach over pngChunks(). That gains it the bounds check the
index does, and loses it a real bug: the walk was `do { ... } while ($n)`, so a chunk carrying
no data - a comment with an empty value is enough - ended the scan, and the image came out as
the error placeholder because no IDAT was ever collected.
pngChunks() now walks to IEND rather than stopping at IDAT, because assembling the image stream
needs the chunks after it. Stopping early moves to pngChunk(), where it belongs: the generator
is the traversal and the early exit is policy. Nothing pays for the longer walk, since a caller
that stops still only advances the generator as far as it reads.
Two pieces of duplication go with it. The index now yields a payload offset the way jpgSegments()
already did, so no caller adds four to an offset to find the data. And the ICC profile check -
'acsp', RGB in, XYZ out - was written three times in this class; usableIccProfile() is the one
copy, and an indexed PNG no longer inflates a profile that the next branch discards unread.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three places hand GD more work, or more memory, than the result needs. A half turn is both flips. imagerotate() allocates a second image the size of the first and copies every pixel into it; imageflip() works in place. Orientation 3 is the common one - a photo taken upside down - and it now costs no allocation at all, leaving only the quarter turns of 5 to 8 needing a copy. The transforms table still says 180 degrees, because that is what CIPA DC-008 Figure 11 says orientation 3 means and the table is checked against the figure; which GD call gets there is the executor's business. On a 4000x3000 JPEG this takes peak RSS from 139 MB to 92 MB, and the encoded bytes are identical. The PNG alpha branch built a second full-size truecolor image purely to drop the alpha channel, while the first was still alive. Turning the alpha off and writing the image where it stands does the same job - byte for byte the same PNG - and the branch just below already did it that way. A palette image still has to be copied, because writing it as it stands would hand processPng() an indexed image where it had a truecolor one. Measured on a 2400x1800 RGBA PNG, peak RSS goes from 114 MB to 96 MB, which is the w * h * 4 the second image was taking. The source image is also released once both temp files are written rather than held to the end of the branch, which matters on the PHP versions where destroyImage() still does something. Both of those writes, and the one below them, want the same four calls on the image first, so writeFlatPng() is the one place that knows what "flat enough for processPng() to read back" means. Clearing the transparent colour is part of it: a truecolour PNG with tRNS arrives with one set, and writing it back out would give the image a colour key mask on top of the soft mask already built from the same chunk. convertImage() decided the colour space three times per pixel and built two arrays per pixel to compare a transparent colour it had in hand. None of that changes between pixels. Deciding it once takes the greyscale conversion of a 4 MP image from 0.62 s to 0.57 s; the CMYK one is unchanged, its cost being the conversion itself. Deciding $ncols with it settles a variable PHPStan had reported as possibly undefined, so the baseline loses that entry. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
b7dd83c to
288e68d
Compare
|
Done in 15fde02, as part of #35 rather than after it. All six reads now ask On the design decision it forced. Neither a parameter nor a second entry point, in the end. Two things turned up that were not in the write-up. The walk was
Tests. The chunk-building helpers moved to Nothing else moved. 7 built PNGs — truecolour RGBA, the same interlaced, palette with |
Summary
Four commits: three on how images are read, and one on what the re-encoding they sometimes need
costs. Each stands on its own.
Read a JPEG's and a PNG's metadata out of their structure. Seven places were finding metadata with a substring search over the whole image —
strposforJFIF,ICC_PROFILE\0,pHYs,gAMA, thesRGBthat overrides it, and the bare presence tests fortRNSandiCCP— and the string each searched for turns up in a comment, or among the compressed samples, just as readily as in the marker or chunk that was wanted. Every one of them is a defect you can trigger with a comment:JFIFICC_PROFILEsRGBpHYstRNSiCCPjpgSegments()andpngChunks()now index the real structures and all seven ask the index.jpgHeaderFromString(), which walked to the start of frame by hand without checking that any segment length landed inside the file, goes through the same index.Read the rest of a PNG's chunks out of the index too. Six reads and a third hand-rolled chunk walk were left behind by that first pass, and they are the same defect in a worse form: rather than mis-setting a flag, they take a length and an offset from wherever the four letters matched and read a value out of that. A
tEXtchunk sayingtRNSputs the length at the four bytes in front of the match, which are comment text. All six now ask the index, for every colour type and both bit depths. Folding the hand-rolled walk into the index gains it a bounds check and loses it a real bug: it rando { ... } while ($n), so a chunk carrying no data — an empty comment is enough — ended the scan, and the image came out as the error placeholder with noIDATcollected.Show JPEGs the way up their Exif Orientation tag says. A photo taken on a phone is nearly always stored the way the sensor read it, with an Exif
Orientationtag recording which way round that was. Browsers have honoured that tag by default ever sinceimage-orientation: from-imagebecame the initial value, so an image that sat the right way up on the page it was copied from arrives in a PDF on its side, or mirrored. TurnuseImageExifOrientationon andprocessJpg()reads the tag and, when it says anything other than "as stored", rotates and mirrors the samples through GD before the rest of the pipeline sees them. Correcting the pixels rather than the drawing matrix means the intrinsic width and height come back off the corrected image, so layout, background sizing, and every other place that scales an image get the right numbers without having to know anything about Exif.The setting is off by default. Correcting an image costs a decode and a re-encode of it, and a document laid out around the samples as stored should not move under an upgrade. Only images that actually carry an orientation to correct are ever re-encoded — every JPEG with no Exif, or whose Exif says it is already the right way up, comes through byte for byte as before.
The quality of that re-encode is a new
imageJpegQualitysetting, defaulting to 75. The WebP and AVIF conversions read it too: they were already re-encoding at GD's implicit default, and 75 is that default, so their output is unchanged to the pixel. Raising it is tempting and measurably better — 85 is the knee on a real camera photo, worth 7 dB for 6% more bytes — but it is not free: at 85 those two conversions grow about a quarter, which moves every existing document carrying a WebP or an AVIF. The setting is there for anyone who wants to make that trade.Reading the structure rather than searching for it also fixes a hang. The ICC scan advanced by a length taken from the two bytes before wherever
ICC_PROFILE\0matched, and those bytes are a length only if the match was a real APP2 segment. A comment carrying the fourteen bytes00 00 ICC_PROFILE\0moves the search backwards onto itself and it never terminates. A 20 KB JPEG built that way pins mPDF indefinitely ongravitypdf; it renders in under a second here.Ask GD for less memory when re-encoding an image. Three places hand GD more memory than the result needs. A half turn is both flips, and
imageflip()does them in place whereimagerotate()allocates a second image and copies every pixel into it — so orientation 3, a photo taken upside down, now costs no allocation at all, and a 4000 × 3000 JPEG peaks at 92 MB RSS instead of 139 MB for identical output bytes. The PNG alpha branch built a second full-size truecolor image purely to drop the alpha channel, while the first was still alive; turning the alpha off and writing the image where it stands produces the same PNG byte for byte, and takes a 2400 × 1800 RGBA PNG from 114 MB to 96 MB. AndconvertImage()decided the colour space three times per pixel and built two arrays per pixel to compare a colour it already had in hand.The first three commits stand on their own — no new configuration and nothing to opt into, just the reads corrected — so any of them can land or be reverted without the ones after it.
Try it
With the setting off (the default) — the image is drawn 60 × 120 mm, portrait, turned a quarter of the way anticlockwise from how it looks in a browser or in Preview. The red quadrant is at the bottom left.
With it on — the image is drawn 60 × 30 mm, landscape, the way up every other viewer shows it. Red top left, green top right, blue bottom left, yellow bottom right.
Test plan
Full suite goes from 1192 tests on
gravitypdfto 1202, 1216, 1224 and 1224 as the four commits land, all passing, withcomposer csclean and PHPStan quiet at each.Reading the structure —
JpgMarkerSegmentsTestandPngChunksTest, 10 tests, 6 of which fail ongravitypdf. They build their fixtures rather than adding binaries: a JPEG comment segment sayingJFIFwith a density of 100 in it, against an image whose real density is 300; a JPEG comment carrying a complete, valid-looking ICC profile, against an image that has none; a PNGtEXtchunk claiming 99,999 pixels per metre placed ahead of a realpHYssaying 300 dpi, so a search finds the decoy first; a PNGtEXtmerely mentioningsRGB, which must not suppress the gamma; a PNGtEXtmentioningtRNS, which used to make mPDF build a soft mask for an image with no transparency; and one mentioningiCCP, which used to make it decode and re-encode an indexed image through GD to strip a profile that was not there. Alongside them, the cases that must not move: a real 150,000-byte ICC profile split across three APP2 segments written in reverse sequence order has to come back whole, a realsRGBchunk still has to overridegAMA, and the dimensions still have to come off the frame and header chunks.Reading the values, not just the names —
PngChunkValuesTest, 8 tests, 6 of which fail without the third commit. One per site, each pairing an honest file against the same file with atEXtdecoy in front of the real chunk, so the two have to render identically: a greyscaletRNSat 8 and at 16 bits a sample, a truecolour one, a palette one placed afterPLTEwhere PNG 5.6 wants it, and aniCCPon an indexed image — the case where a profile mPDF can use sends the image through GD to come outDeviceRGB, so reading the wrong one leaves it indexed and the difference is visible in the result. The remaining two guard the walk that was folded into the index rather than demonstrating a defect: image data split across threeIDATchunks still has to be assembled whole, which is what stopspngChunks()being left stopping at the first one, and an emptytEXtchunk must not end the scan.Orientation —
ExifOrientationTest, 14 tests, plus a snapshot. The snapshot renders all eight orientations at a fixed width and no height, so a sample that was not corrected is drawn twice as tall as its neighbours rather than half as tall; all nine images in it come out 40 × 20 and are drawn in an identical landscape box. One fixture per orientation, each holding the same picture stored a different way up. All eight have to come out 40 × 20 with the same four colours in the same four corners. Orientations 1–4 carry little-endian Exif and 5–8 big-endian, so the set exercises both. The fixtures were checked against an independent implementation before being committed:magick exif-orientation-N.jpg -auto-orientproduces the canonical image for all eight, andmagick identify -format '%[orientation]'reads them back asTopLeftthroughLeftBottomin order.Also covered: with the setting at its default the orientation 6 fixture stays 20 × 40 and its bytes are untouched; a JPEG with no Exif at all, and one whose Exif says orientation 1, both come back byte-identical to the file on disk even with the setting on; an ICC profile survives the re-encode;
imageJpegQualitychanges the size of the result; and the drawn box turns with the image, the content stream'scmmatrix being landscape with the setting on and portrait with it off.Crafted input — the files from the security audit were each run against both revisions: a JPEG whose comment contains the bytes that used to send the ICC scan into an infinite loop, one whose APP2 segment ends exactly at EOF, and files packing a million empty segments or chunks into a few megabytes.
Nothing else moved — every image fixture in the repo was rendered before and after and compared on width, height, colour space, bit depth, channels, dpi, ICC profile and image bytes. Separately, 7 built PNGs — truecolour RGBA, the same interlaced, palette with
tRNS, greyscale + alpha, 16-bit RGBA, one carryinggAMA, and a JPEG — were rendered under four configurations each (default,restrictColorSpacegreyscale,restrictColorSpaceCMYK, and PDF/A, which between them reach all threeconvertImage()targets) and compared byte for byte: 28 of 28 identical. Run against the crafted files instead, the only documents that change are the decoy ones and the one with an empty chunk — which is the point. The WebP and AVIF paths, which now pass a quality where they previously let GD default it, were checked pixel by pixel ontiger.webp: 0 of 321,489 pixels differ, and the file is 3 bytes shorter because libgd encodes the quantisation tables slightly differently when it is given a number.Performance
PHP 8.5 with GD on an otherwise busy laptop, each figure the minimum of repeated runs in fresh processes, so treat the millisecond columns as indicative and the byte columns as exact. "Rotate" is a JPEG tagged orientation 6, the only case that costs anything.
gravitypdfwith no commit at all, peak memory and output bytes are identical at all three sizes, and the timings differ by less than the noise floor.imagecreatefromstring→imagerotate→imagejpegwith no mPDF involved measured 455 MB RSS at 48 MP against mPDF's 464 MB for the whole pipeline — mPDF adds about 1.8%.memory_limitto catch it. Real process RSS runs 3–4× PHP's ownmemory_get_peak_usage(), because GD's pixel buffers live outside the Zend heap.imagerotateholds source and destination at once, so a 48 MP photo peaks near 440 MB RSS while PHP reports 110 MB. Where a 128M limit breaks is content-dependent rather than a clean function of megapixels — around 48–50 MP for detailed photos, and 100–110 MP for 256M, which is within reach of shipping phone sensors.w × h × 4GD buffer, and in both the output is byte-identical. Note thatmemory_get_peak_usage()reports neither, GD's buffers being outside the Zend heap — this only shows up in RSS, which is what the OOM killer looks at.Security
The new parsers read attacker-supplied bytes, so they were audited against the base. The crafted files below were each run against both revisions to confirm the behaviour; they are not committed, since every one of them is a malformed image whose only assertion would be "does not hang" or "does not allocate a gigabyte".
memory_limiteither way, but it was a cheap request-level DoS.imagecreatefromstringhas no pixel-count guard, so a decompression bomb can allocate far beyond what the file size suggests — but that pattern is already on default-on paths, andprocessWebpandprocessAvifhold more buffers than this does. (The PNG alpha branch used to as well; the fourth commit takes one of them away, which does not make it guarded.) A zero-dimension JPEG throwsDivisionByZeroErrorongravitypdftoo. Both want fixing where they live.More info — how the tag is read, and what is left alone
Parsing.
ext-exifis not a dependency and cannot read from a string anyway, so the Exif is taken from the APP1 segmentjpgSegments()indexes andexifOrientationFromTiff()reads tag0x0112out of IFD0 directly. Both TIFF byte orders are handled. Every malformed shape reads as orientation 1: a bad byte order mark, a magic number that is not 42, an IFD offset that points outside the segment, an entry count that runs off the end, a value that is not aSHORT, or a number outside 1–8. A truncated or nonsense Exif segment therefore leaves the image exactly as it was, and the probe confirms none of these paths raises a diagnostic.The eight cases. Orientation records where row 0 and column 0 of the stored samples belong on screen, which comes to a rotation, a mirror, or one of each:
Why the samples and not the matrix. The alternative is to leave the JPEG alone, swap the reported width and height, and compose an extra matrix into the
cmat each drawing site. That is lossless, but there are eight of those sites acrossMpdf,Form,SvgandBackgroundWriter, and threepreg_matchcalls inMpdf.phpthat recognise an image-only block by the exact shape of that operator — they match a literal0 0in the b and c slots, which any quarter turn makes nonzero, so they would silently stop recognising rotated images and change page-break behaviour. Beyond the drawing sites,$info['w']and$info['h']feed around forty aspect-ratio and scale calculations inMpdf.phpalone; a matrix-only fix would leave every one of them laying a portrait photo into a landscape box. Correcting the samples keeps all of that untouched, and is what the CMYK, WebP, AVIF and BMP paths in this class already do.Cost of the index. A metadata read touches only the header structures — the JPEG walk stops at the start of scan, and
pngChunk()at the firstIDAT, which PNG 5.6 has every chunk it looks for precede — so neither reads a byte of compressed samples, and both yield their entries one at a time rather than building an index.pngChunks()itself runs toIEND, because assembling the image stream needs the chunks afterIDAT; a caller that stops earlier pays nothing for that, since a generator only advances as far as it is read. That makes them cheaper than the searches they replace, which read the whole file whenever what they were looking for was absent: taking the ICC segments off an 8.6 MB JPEG that has none goes from 2.7 ms to 5 µs.What is deliberately left alone.
eXIfchunk and WebP an Exif chunk, but in practice orientation comes off cameras as JPEG, and the PNG path embeds its stream without decoding it. WebP would be additive if it is ever wanted — the RIFF chunk plus a call to the existing helper — because the transcode already re-entersprocessJpg().Costs, for the images that are re-encoded. The JPEG is decoded, transformed and written back at
imageJpegQuality, so there is one generation of loss and, briefly, two full-size truecolor buffers in memory — a large photo needs the headroom for that. The ICC profile is carried across because the samples are still in the space it describes, and it is inserted after the JFIFAPP0that GD writes, which is where JFIF wants its own segment to stay. The Exif itself is not carried across, which is what stops the correction being applied twice.