Repository navigation
Enhance image handling and decoding robustness across formats - #974
Merged
Merged
Conversation
Handlers report a width and a height of 0 when the dimensions cannot be read from the image, so the documented range of the values was wrong. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Every decoder and handler unpacked integers on its own, with a private copy of the same wrapper, and truncated data produced PHP warnings or silently wrong values instead of an error. Add ByteReader, which reads bytes, integers and strings in sequence and throws on any read beyond the data, and Bytes, which packs and unpacks integers at an offset in either byte order and applies several replacements to a string at once using the offsets of the original data. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The handler patched the metadata of AVIF files in place, recomputing the offsets by hand at every step. The byte surgery left dangling offsets behind: removing the Exif item did not move the item locations, removing a color profile did not renumber the property associations, an added profile was never associated with the primary item, and the length of a replaced Exif item was not updated. Parse the file into the boxes of the Isobmff namespace instead, change them as objects, and rebuild the file in a single step: the item data is laid out in one media data box and every offset is recomputed in one place, so no operation can leave the file inconsistent. The new model also covers the layouts the previous code mishandled: 64-bit and unsized boxes, item location boxes of any version, base offsets, extent indexes, items stored in the item data box, and media data placed before the metadata. Exif items now get the content description reference decoders need to find them, and image sequences are rejected instead of being corrupted. Image sequences report their number of frames and how many times they are played, read from the sample table and the edit list of the first track, and monochrome images are reported as grayscale. The decoder reads boxes through ByteReader and rejects boxes that are truncated or smaller than their header. Add the tests for the decoder, the handler and the whole Isobmff namespace, over an encoded image and over files built with a controlled box layout. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The decoder returned truncated chunks and kept reading after the end chunk. The handler duplicated the color profile and the Exif chunk when they were set on an image already holding one, and a corrupt profile raised PHP warnings. Read chunks through ByteReader, so that truncated data is rejected, and stop at IEND. Replace existing iCCP and eXIf chunks instead of adding new ones, give profiles without a name a valid one and report invalid compressed profiles with an exception. Add GdImages, which encodes small real images for the handler tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Removing a color profile or Exif data edited the image while iterating over its segments, so every segment following a removed one was skipped and profiles split in several segments were left half removed. Setting them added a second copy, before the JFIF segment, and Exif data longer than a segment was silently truncated. Require the start of image marker, skip marker padding, handle markers without length and stop at the end of image. Collect the changes while iterating and apply them at once, replacing existing segments, placing new ones after the JFIF segment and rejecting data that does not fit in a segment or in the 255 chunks of a profile. Recognize every start of frame marker and incomplete or inconsistent color profiles. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The decoder read blocks without checking their bounds and failed on data following the trailer. The handler took the delay of a single frame as a sign of animation, reported the color resolution as number of colors and allocated the transparent color on the source image instead of the one being encoded. Read blocks through ByteReader, stop at the trailer and count the frames to tell animations from still images. Report the size of the global color table as number of colors and allocate the transparent color on the destination image. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Chunk values included their padding byte, so color profiles of odd size were corrupted, and chunks were read past the end of the RIFF container. Metadata could be set only on files that already had a VP8X chunk, and setting it twice added a second copy. Read chunks through ByteReader, bound them to the RIFF container and leave padding out of the values. Edit images as a list of chunks that is serialized in one step, so that the RIFF size and the VP8X flags always match their content, extending simple files with a VP8X chunk when needed, replacing existing metadata and placing Exif data before XMP. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The dimensions were cast to integers as they were, so relative or suffixed values gave wrong sizes, such as 100 for "100%" and 0 for documents sized only by their view box, and prefixed root elements were rejected. Convert absolute lengths to pixels, fall back to the view box for missing or relative sizes and match the root element by its local name. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A truncated tag table, a tag outside the profile or a string with invalid bounds raised PHP warnings or generic errors, and the signature was looked up in the whole profile. Check the signature at its offset, read the tag table and the string tags through ByteReader and reject profiles whose tag table is invalid with an InvalidArgumentException. Malformed tag values fall back to the default value instead of failing. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Processing always produces RGB pixels through GD, so carrying over a CMYK or grayscale profile made the converted image inconsistent, and some decoders (e.g. AVIF in browsers) refused to display it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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.
This pull request introduces a new binary data handling subsystem for image processing, refactoring existing decoders and color profile parsing to use a new set of utility classes. The main improvements are the introduction of the
ByteReader,Bytes, andEndiannessclasses, which provide safer and more readable ways to handle binary data, and the refactoring of the AVIF and GIF decoders, as well as ICC color profile parsing, to leverage these utilities. This results in more robust, maintainable, and exception-safe code when dealing with binary formats.New Binary Data Utilities:
ByteReader, a utility class for reading bytes, unsigned integers, and strings from binary data with bounds checking and endianness support (formwork/src/Images/Binary/ByteReader.php).Bytes, a static utility class for packing/unpacking unsigned integers and performing safe binary replacements (formwork/src/Images/Binary/Bytes.php).Endiannessenum to represent big and little endian byte orders (formwork/src/Images/Binary/Endianness.php).Refactoring and Improvements in Decoders:
AvifDecoderandGifDecoderto useByteReaderfor all binary parsing, replacing manual offset and unpack logic. This improves safety and readability, and removes custom unpack helpers (formwork/src/Images/Decoder/AvifDecoder.php,formwork/src/Images/Decoder/GifDecoder.php) [1] [2] [3] [4].ICC Color Profile Parsing Enhancements:
ColorProfileto useByteReaderandBytesfor parsing ICC profile tags and values, improving error handling for truncated or invalid profiles and ensuring proper bounds checking (formwork/src/Images/ColorProfile/ColorProfile.php) [1] [2] [3] [4] [5].These changes make binary parsing code more robust against malformed data and easier to maintain by centralizing low-level operations in dedicated utility classes.