Conversation
Collaborator
Author
|
Comments or concerns? |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved thumbnail and channel-length validation issues remain, along with missing regression coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens the PSD reader against corrupt length fields and conflicting layer-info sections discovered through fuzzing.
Changes:
- Validates color, thumbnail, and layer-channel lengths.
- Replaces the
Lr16/Lr32layer-info assertion with an error. - Adds a thumbnail corruption regression test and expected output.
File summaries
| File | Description |
|---|---|
testsuite/psd/run.py |
Runs the thumbnail corruption test. |
testsuite/psd/ref/out.txt |
Records expected failure output. |
src/psd.imageio/psdinput.cpp |
Adds PSD length validation and duplicate-section handling. |
Review details
Suppressed comments (2)
src/psd.imageio/psdinput.cpp:1471
lengthincludes the 28-byte thumbnail header, but this check runs before that header is consumed and only comparesjpeg_length. A block declared up to 28 bytes past EOF can therefore pass this guard and allocate nearly the entire remaining file before the later read fails. Compare the full resourcelengthwith the bytes remaining at the resource start (or subtract the header from the available bytes) before allocating.
if (int64_t(jpeg_length) > file_size - iotell()) {
src/psd.imageio/psdinput.cpp:1099
- Please add a PSD regression fixture for this new color-data length check. The added test only exercises the thumbnail path, so a truncated color-mode-data section could regress to allocating the reported length without being caught by
testsuite/psd.
if (size_t(iotell()) + m_color_data.length > ioproxy()->size())
- Files reviewed: 3/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1783
to
+1784
| int64_t file_size = ioproxy() ? ioproxy()->size() : 0; | ||
| if (int64_t(channel_info.data_length) > file_size - channel_info.data_pos) { |
|
|
||
| // A channel's stored data cannot extend past the end of the file. | ||
| int64_t file_size = ioproxy() ? ioproxy()->size() : 0; | ||
| if (int64_t(channel_info.data_length) > file_size - channel_info.data_pos) { |
Comment on lines
+2001
to
+2004
| if (layer_info.length != 0) { | ||
| errorfmt("[Global Additional Layer Info] unexpected second layer info " | ||
| "section"); | ||
| return false; |
Fuzzing the psd reader turned up several corrupt-input failures: - load_resource_thumbnail() computed jpeg_length = length - 28 with no lower bound check, so a short resource underflowed into a multi-GB allocation. - load_color_data() allocated from the declared length without checking it against the file size. - load_layer_channel() sized compressed_data from the raw channel data_length. Bound it by the bytes remaining from the start of the channel, compared unsigned: data_length is 64 bits for PSB, where a corrupt value cast to int64_t wraps negative and slips past the guard into an uncaught bad_alloc. - The RLE path subtracted the RLE length table from data_length without checking that it fit, underflowing on corrupt input. - load_layers_16_32() asserted layer_info.length == 0, so a file carrying both a normal layer info section and an Lr16/Lr32 block aborted a debug build. Return an error instead. Adds fixtures for each new rejection path. Assisted-by: Claude Code / Claude Opus 5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
Collaborator
Author
|
Updated based on review comments |
lgritz
changed the title
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.
Fuzzing the psd reader turned up several corrupt-input failures:
lower bound check, so a short resource underflowed into a multi-GB
allocation.
against the file size.
data_length. Bound it by the bytes remaining from the start of the
channel, compared unsigned: data_length is 64 bits for PSB, where a
corrupt value cast to int64_t wraps negative and slips past the guard
into an uncaught bad_alloc.
checking that it fit, underflowing on corrupt input.
both a normal layer info section and an Lr16/Lr32 block aborted a debug
build. Return an error instead.
Adds fixtures for each new rejection path.
Assisted-by: Claude Code / Claude Opus 5