Skip to content

Conversation

@lgritz
Copy link
Collaborator

@lgritz lgritz commented Oct 11, 2025

IffInput::read_native_tile was simply incorrect for images with nonzero data window origin. Fix!

Also switch the forumulation to use spancpy rather than memcpy, to be a little more careful that we are't overwriting the presumed sizes of the buffers.

And while I'm in there, I realized we can hold the mutex for less time.

@lgritz lgritz added bug Crash or wrong behavior of an existing feature. file formats Image file formats, ImageInput, ImageOutput labels Oct 11, 2025
@lgritz lgritz marked this pull request as ready for review October 16, 2025 17:54
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 16, 2025

Any comments about this potentially serious bug? This was found externally via fuzzing by people looking for security issues, so I'd like to get it integrated as soon as possible.

IffInput::read_native_tile was simply incorrect for images with
nonzero data window origin. Fix!

Also switch the forumulation to use spancpy rather than memcpy,
to be a little more careful that we are't overwriting the presumed
sizes of the buffers.

And while I'm in there, I realized we can hold the mutex for less
time.

Signed-off-by: Larry Gritz <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a critical bug in IFF image reading when handling images with non-zero data window origins and improves buffer safety through the use of span-based copying instead of raw memcpy.

  • Corrects tile coordinate calculations to account for non-zero image origin offsets
  • Replaces unsafe memcpy operations with spancpy for buffer overflow protection
  • Optimizes mutex lock scope to reduce contention
Comments suppressed due to low confidence (1)

src/iff.imageio/iffinput.cpp:529

  • After adjusting x to be relative to the data origin (line 521), this calculation may be incorrect. If x is negative after adjustment, tw could be larger than intended or negative. Similarly, the comparison should likely be against the image width considering the origin. Add validation that x >= 0 and ensure width calculations account for the adjusted coordinate space.
    int tw = std::min(x + static_cast<int>(m_header.tile_width),
                      static_cast<int>(m_header.width))
             - x;

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 19, 2025

This fixes an important issue and has gone for a week without objection. Merging.

@lgritz lgritz merged commit d423a14 into AcademySoftwareFoundation:main Oct 19, 2025
57 of 58 checks passed
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 19, 2025
…cademySoftwareFoundation#4925)

IffInput::read_native_tile was simply incorrect for images with nonzero
data window origin. Fix!

Also switch the forumulation to use spancpy rather than memcpy, to be a
little more careful that we are't overwriting the presumed sizes of the
buffers.

And while I'm in there, I realized we can hold the mutex for less time.

---------

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 19, 2025
…cademySoftwareFoundation#4925)

IffInput::read_native_tile was simply incorrect for images with nonzero
data window origin. Fix!

Also switch the forumulation to use spancpy rather than memcpy, to be a
little more careful that we are't overwriting the presumed sizes of the
buffers.

And while I'm in there, I realized we can hold the mutex for less time.

---------

Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz deleted the lg-iff branch October 19, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Crash or wrong behavior of an existing feature. file formats Image file formats, ImageInput, ImageOutput

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant