Skip to content

Reject zero-denominator RATIONAL/SRATIONAL tags (#2313)#2317

Merged
brendancol merged 2 commits into
mainfrom
issue-2313
May 22, 2026
Merged

Reject zero-denominator RATIONAL/SRATIONAL tags (#2313)#2317
brendancol merged 2 commits into
mainfrom
issue-2313

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2313.

Summary

  • _read_value in xrspatial/geotiff/_header.py used to coerce a malformed RATIONAL or SRATIONAL value (denominator=0) to 0.0. It now raises ValueError with the offending tag and denominator in the message, matching the actionable failure style from PR Pin actionable failure modes for unsupported COG writer inputs (#2286 prod-ready wave B) #2301.
  • The tag id is threaded through the three call sites in parse_ifd, so the message can name XResolution / YResolution / ResolutionUnit directly when it's one of those.
  • The two existing tests that pinned the old silent behaviour are flipped to assert the new failure, and a new regression test (test_rational_zero_denominator_2313.py) builds a TIFF whose XResolution rational has a zero denominator and confirms both the header parser and open_geotiff fail loudly.

Backend coverage

Header decoding is shared across all four backends (numpy, cupy, dask+numpy, dask+cupy), so the change applies uniformly. No backend-specific code is touched.

Test plan

  • pytest xrspatial/geotiff/tests/test_header.py
  • pytest xrspatial/geotiff/tests/test_rational_zero_denominator_2313.py
  • pytest xrspatial/geotiff/tests/ -k "not gpu and not cuda" (4282 passed, 51 skipped)

`_read_value` used to coerce a malformed (denominator=0) RATIONAL or
SRATIONAL to 0.0, which let corrupted `XResolution` / `YResolution`
metadata round-trip through the reader as if the file were valid.
Raise `ValueError` instead, name the offending tag and the
denominator, and update the two existing tests that pinned the old
silent behaviour.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 22, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Reject zero-denominator RATIONAL/SRATIONAL tags (#2313)

Blockers

None.

Suggestions

  • test_header.py:261 and test_header.py:272: the two new tests do from xrspatial.geotiff._header import TAG_X_RESOLUTION (and Y) inside the method body. The module is already imported at the top of the file (_read_value, parse_all_ifds, parse_header); folding the tag constants into that same import keeps the block consistent and avoids the local import.

Nits

  • _header.py:88-96: _TAG_NAMES covers only X/YResolution and ResolutionUnit, which is the right scope for this PR. If a follow-up threads tag= through other error sites, growing this map will help. Flagging it so it doesn't get forgotten.
  • test_rational_zero_denominator_2313.py:147-208: test_yresolution_zero_denominator_named_in_error reimplements most of _build_tiff_with_rational_xres. Adding a which: int = 282 parameter to the helper (default XResolution, 283 for YResolution) would let the YResolution test reuse the helper instead of duplicating ~60 lines.

What looks good

  • Error message follows the "refusing to parse possibly malformed TIFF" pattern already used by parse_ifd (lines 666/672 in the original).
  • All three _read_value call sites in parse_ifd get tag=tag, including the pre-scan path that swallows ValueError. That swallow is safe because _DIMENSION_TAGS doesn't include any RATIONAL tag.
  • The regression test exercises both parse_all_ifds and open_geotiff, so the failure is verified at the public entry point, not just the internal helper.
  • The two existing tests that pinned the old silent behaviour are flipped to assert the new failure, not deleted. A regression that reintroduces the silent path would fail loudly.
  • BigTIFF coverage is implicit: parse_ifd uses the same _read_value for both classic and BigTIFF entries.
  • Error message includes the element index, so a malformed entry inside a multi-element RATIONAL array gets pinpointed.

Checklist

  • Algorithm matches spec (TIFF treats denom=0 as malformed)
  • NaN handling unchanged (rationals never produced NaN before either)
  • Edge cases covered (count>1 in test_rational_denominator_zero_in_array_raises)
  • No premature materialization or unnecessary copies
  • Benchmark coverage: not needed (error path)
  • README feature matrix: not applicable (pure bug fix)
  • Docstrings present and accurate (_read_value docstring documents the new behaviour)

- Pull `TAG_X_RESOLUTION` / `TAG_Y_RESOLUTION` up into the top-level
  import in `test_header.py` instead of importing inside the test
  bodies.
- Add a `which` parameter to `_build_tiff_with_malformed_resolution`
  in `test_rational_zero_denominator_2313.py` so the YResolution test
  reuses the helper instead of duplicating ~60 lines.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up review after ca86979

Re-checked the two items that needed action and confirmed the new commit addresses them.

Disposition

  • Suggestion (hoist local imports in test_header.py): fixed in ca86979. TAG_X_RESOLUTION and TAG_Y_RESOLUTION are now in the top-level import block, and the two local imports inside the test methods are gone.
  • Nit (share helper between XResolution and YResolution tests): fixed in ca86979. _build_tiff_with_malformed_resolution now takes a which parameter, and test_yresolution_zero_denominator_named_in_error is a one-liner that reuses the helper. Net -41 lines in the test file.
  • Nit (_TAG_NAMES map could grow): deferred. The map only exists to label tags in error messages produced by _read_value, and the only error sites that take a tag= kwarg today are the RATIONAL / SRATIONAL branches. Adding entries for tags that no error site references would be dead code. If a future PR threads tag= through other error sites, growing the map should be part of that PR.

Tests: pytest xrspatial/geotiff/tests/test_header.py xrspatial/geotiff/tests/test_rational_zero_denominator_2313.py -> 44 passed.

No new findings on this pass.

@brendancol brendancol merged commit df2331b into main May 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reject zero-denominator RATIONAL/SRATIONAL values in TIFF header reader

1 participant