Bound-check the uint32_t offset case in TiffEntryBase::writeOffset - #9480
Open
afonsojanu wants to merge 1 commit into
Open
Bound-check the uint32_t offset case in TiffEntryBase::writeOffset#9480afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
The uint16_t branch already rejected an offset past its range before calling us2Data, throwing kerOffsetOutOfRange. The uint32_t branch right below it skipped that check entirely and went straight into static_cast<uint32_t>(offset), so any offset above UINT32_MAX just wrapped around silently instead of failing loudly. Since offset is a 64-bit size_t on most platforms, that's an easy value to hit with a crafted or unusually large TIFF structure, and the result is a subtly wrong offset written into the file with no indication anything went wrong. Added the same range check the short case already has, comparing against numeric_limits<uint32_t>::max() before truncating. Covered it with a small unit test file exercising both long variants plus a control case for the short branch, so a future change here can't quietly drop either check again.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new unit test file has compile/portability issues (missing <limits> include and size_t overflow on 32-bit) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prevents silent truncation when writing TIFF entry offsets by adding a missing upper-bound check for the uint32_t offset cases in TiffEntryBase::writeOffset, and introduces unit tests to cover the boundary/error behavior.
Changes:
- Add
UINT32_MAXbounds checking forttUnsignedLong/ttSignedLongoffset writes to avoid lossy casts fromsize_t. - Add a new unit test file covering the
uint32_tboundary and an existinguint16_tcontrol case. - Wire the new unit test into both CMake and Meson test builds.
File summaries
| File | Description |
|---|---|
src/tiffcomposite_int.cpp |
Adds uint32_t max bound-check before writing long offsets. |
unitTests/test_tiffentrybase_writeoffset.cpp |
New unit tests for writeOffset boundary and error conditions. |
unitTests/CMakeLists.txt |
Registers the new unit test source in the CMake test target. |
unitTests/meson.build |
Registers the new unit test source in the Meson test target. |
Review details
Suppressed comments (1)
unitTests/test_tiffentrybase_writeoffset.cpp:33
- Same overflow/wrap issue as above: on 32-bit platforms the
size_t+ 1 wraps, so the test may not validate the intended failure behavior. Use a wider intermediate and skip whensize_tcannot exceed UINT32_MAX.
const size_t tooLarge = static_cast<size_t>(std::numeric_limits<uint32_t>::max()) + 1;
- Files reviewed: 4/4 changed files
- Comments generated: 2
- 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
+3
to
+6
| #include <exiv2/error.hpp> | ||
| #include <gtest/gtest.h> | ||
| #include <tiffcomposite_int.hpp> | ||
|
|
Comment on lines
+26
to
+30
| TEST(ATiffEntryBaseWriteOffset, rejectsAnOffsetPastUint32MaxForUnsignedLong) { | ||
| const size_t tooLarge = static_cast<size_t>(std::numeric_limits<uint32_t>::max()) + 1; | ||
| byte buf[4] = {}; | ||
| EXPECT_THROW(TestableTiffEntryBase::writeOffset(buf, tooLarge, ttUnsignedLong, littleEndian), Error); | ||
| } |
Collaborator
|
Please remove the unit test. It's too trivial to be worth including in our codebase. If you can create an actual image file that triggers this overflow, then that would be useful to include as a test. Otherwise, let's just do the 2-line change in src/tiffcomposite_int.cpp. Out of interest, was this found by an AI tool? |
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.
writeOffset already rejects an offset that's too big for the uint16_t branch (throws kerOffsetOutOfRange before us2Data), but the uint32_t branch right underneath it has no equivalent check, it just casts straight into static_cast<uint32_t>(offset). offset comes in as a size_t, 64-bit on most platforms these are built for, so a large enough value truncates silently instead of failing, and the file ends up with a wrong offset baked in with no error raised anywhere.
Fixed by adding the same numeric_limits<uint32_t>::max() check the short case already has. Added a small unit test file for TiffEntryBase::writeOffset covering the unsigned/signed long cases past the boundary plus a control case for the existing short-side check, since there wasn't one before.
Ran the full unit test suite locally (356/356 passing) and confirmed the two new failure-path tests actually fail without the fix and pass with it (checked via git stash).