InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

ImageIOBase validates geometry indices but never values, and the image-size multiplication is unchecked

Open
#6,817 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

## Summary

`ImageIOBase` validates the **axis index** on every geometry setter and never the **value**. Zero, negative, NaN, and infinite spacing are all accepted and handed to downstream filters; the pixel-count multiplication that sizes every read buffer has no overflow check. Because every `ImageIO` subclass routes through these, this is the one place a fix reaches all readers at once — including the ones that delegate header parsing to a vendored library and are therefore out of reach of any per-reader change.

Opening this as a **discussion first**, not a patch: the fix is a behavioral tightening whose blast radius needs measuring before anything is written.

## What the base class does today

**Setters check the index, not the value** (`Modules/IO/ImageBase/src/itkImageIOBase.cxx`):

```cpp
void
ImageIOBase::SetSpacing(unsigned int i, double spacing)
{
if (i >= m_Spacing.size())
{
itkExceptionMacro("Index: " << i << " is out of bounds, expected maximum is " << m_Spacing.size());
}
this->Modified();
m_Spacing[i] = spacing;
}
```

`SetSpacing` (L125), `SetOrigin` (L114), `SetDirection` (L136, L147) and `SetDimensions` (L103) are all this shape. A reader that computes `spacing = extent / size` from a malformed header stores `inf` without complaint; one that computes it as `0` stores `0`.

**The getter is asymmetric with the setter.** `SetDimensions(i, dim)` throws on a bad index, but

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/ImageBase/include/itkImageIOBase.h#L182-L184

is a bare unchecked `return m_Dimensions[i];`.

**The allocation-sizing multiplication is unchecked:**

```cpp
ImageIOBase::SizeType
ImageIOBase::GetImageSizeInPixels() const
{
SizeType numPixels = 1;
for (unsigned int i = 0; i < m_NumberOfDimensions; ++i)
{
numPixels *= m_Dimensions[i];
}
return numPixels;
}
```

`GetImageSizeInComponents()` (L228) and `GetImageSizeInBytes()` (L234) multiply further on top, also unchecked. Header dimensions whose product exceeds `SizeType` wrap silently, and the wrapped value sizes the buffer that the subsequent read fills.

**`SetNumberOfDimensions`** (L264) resizes with no upper bound, so a large file-derived count escapes as a non-ITK `std::bad_alloc`/`std::length_error` rather than an `itk::ExceptionObject`.

## Why this is the high-leverage location

A survey of 19 IO modules found the per-reader picture is uneven and, more importantly, **not uniformly fixable**:

- **Delegated readers** — GDCM, NIFTI, NRRD (teem), MINC, Meta (MetaIO), JPEG, PNG, TIFF, JPEG2000, LSM — parse headers inside a vendored library. No ITK-level per-reader helper applies to them, but they all call these base-class setters and `GetImageSizeInBytes()`.
- **ITK-parsed readers** range from good (HDF5 compares every header vector length against `numDims`; MRC bounds `nx/ny/nz`; VTI checks stream state and orthonormality) to none at all (see #6815).
- **There is no shared validation helper anywhere.** `itkStringConvert.h` (`StringToInt32` and siblings) validates scalar text→number conversion only. `itkAssertOrThrowMacro` appears at 7 IO sites, none header-related. `.at()` is never used for header-derived indices.

So per-reader fixes are necessary but cannot be sufficient, and a shared *accessor* would have essentially one caller. The base class is the only common path.

## The part that needs discussion before any patch

**This is a behavioral tightening, not an ABI or API break.** No signature changes; no downstream source edits. The risk is entirely that **files which load today would start throwing.**

That population is unknown and must be measured, not assumed. Specific questions for maintainers:

1. **Is zero spacing ever legitimate?** Some formats and some degenerate-but-real clinical data may carry `0` along an axis. Rejecting it outright may break working pipelines.
2. **Is `inf`/`NaN` ever legitimate?** Almost certainly not, which makes non-finite rejection the safest first increment — narrower than a positivity requirement and probably uncontroversial.
3. **Should the overflow check throw or saturate?** Throwing from `GetImageSizeInBytes()` — a `const` getter that currently cannot fail — changes the contract of a widely-called method.
4. **Should `GetDimensions(i)` be made symmetric with its setter?** Cheap, but it is on a hot path.

A staged approach seems right: **(a)** reject non-finite spacing/origin/direction, **(b)** range-check the size multiplication, **(c)** revisit zero/negative spacing only after (a) and (b) have been through a release. Steps (a) and (b) look safe; (c) is the one that needs evidence.

The forest build testbed is the natural place to measure the real-world impact across ITK, SimpleITK, Slicer, BRAINSTools, ANTs and elastix before committing to (c).

## Context

Found during a survey of IO header-field validation prompted by the review of #6761. Per-reader instances: #6813 (`ITKIOBruker`), #6815 (`ITKIOStimulate`), #6816 (`ITKIOVTK`). Related: #5084 (OpenSSF scorecard; ITK's Fuzzing score is 0 and IO readers are unfuzzed, which is why these survive).

Contributor guide

Open the contributing guide

Research direction

Start with Modules/IO/ImageBase/src/itkImageIOBase.cxx and itkImageIOBase.h, then review the listed reader modules and the forest build testbed. Measure which existing files rely on zero or non-finite geometry and oversized dimensions, and discuss whether staged validation should throw. Done means maintainers agree on scope, compatibility evidence, and tests before implementation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, computer-vision
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.