geotiff: parse_all_ifds raises on IFD chain cycle (#1913)#1920
Merged
Conversation
Cyclic IFD chains used to be silently truncated. The loop condition `while offset != 0 and offset not in seen` exited cleanly when a repeat offset showed up and returned whatever had been parsed so far. The other two malformed-chain branches in the same function (past-EOF and MAX_IFDS) both raise ValueError, so the cycle case was the odd one out. The loop now raises ValueError on a repeat offset with the same "file is malformed" wording as the sibling errors. Callers get a clear error instead of a half-parsed file that looks plausible. test_header.py's existing TestIFDChainLoop asserted the old silent-truncation behaviour; updated to assert the new raise.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the GeoTIFF IFD-chain parser by treating cyclic IFD chains as malformed input and raising a ValueError, aligning cycle handling with existing malformed-chain error branches (past-EOF and MAX_IFDS cap) and preventing callers from receiving a plausible-looking but truncated parse result.
Changes:
- Update
parse_all_ifdsto explicitly detect repeated IFD offsets and raiseValueErrorwith “file is malformed” wording. - Update existing cycle-related tests to assert the new raising behavior (including self-loop).
- Add a new targeted regression test module covering cycle detection (LE/BE), message content, and guarding the sibling malformed-chain branches plus normal chains.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
xrspatial/geotiff/_header.py |
Makes IFD-chain cycles raise ValueError for consistent malformed-file handling. |
xrspatial/geotiff/tests/test_header.py |
Updates existing cycle-loop tests to expect the new exception-based contract. |
xrspatial/geotiff/tests/test_ifd_cycle_1913.py |
Adds regression coverage for cycle detection across endianness and for existing malformed-chain branches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Fixes #1913.
parse_all_ifdsinxrspatial/geotiff/_header.pysilently truncated the IFD list on a cyclic chain. The loop condition waswhile offset != 0 and offset not in seen, so a repeat offset exited the loop and returned whatever had been parsed before the cycle. The other two malformed-chain branches in the same function (offset past EOF, chain over MAX_IFDS) raise ValueError, so the cycle case was the odd one out.The fix restructures the loop to raise ValueError on a repeat offset with the same
file is malformedwording. Callers now get a clear error rather than a half-parsed file that looks plausible.Changes
xrspatial/geotiff/_header.py: cycle branch now raises ValueError. Comment explains why (consistency with the sibling errors).xrspatial/geotiff/tests/test_header.py: existing TestIFDChainLoop asserted the old silent-truncation behaviour; updated to assert the new raise (two-IFD cycle and self-loop).xrspatial/geotiff/tests/test_ifd_cycle_1913.py: new test file covering cycle rejection on little-endian and big-endian, error message content, and regression guards for the two sibling malformed-chain branches and the normal multi-IFD chain.Test plan
pytest xrspatial/geotiff/tests/test_ifd_cycle_1913.py -q(8 passed)pytest xrspatial/geotiff/tests/test_header.py -qpytest xrspatial/geotiff/tests/test_ifd_chain_cap.py -qtest_predictor2_big_endian_gpu_1517.py::test_gpu_predictor2_big_endian_int32_tiled_reproducermonkeypatch target missing,test_size_param_validation_gpu_vrt_1776.py::test_tile_size_positive_workstile_size mismatch). Both reproduce onmainatb4f0eee.