Skip to content

BUG: Validate Stimulate image dimensions - #6943

Open
agourakis82 wants to merge 3 commits into
InsightSoftwareConsortium:mainfrom
agourakis82:fix/stimulate-num-dim
Open

agourakis82 wants to merge 3 commits into
InsightSoftwareConsortium:mainfrom
agourakis82:fix/stimulate-num-dim

Conversation

@agourakis82

@agourakis82 agourakis82 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Validate Stimulate dimensions and geometry fields. Match header keys at the start of each line so data-file paths remain readable. Fixes #6815.

Implementation

Bound numDim to the four-element parsing buffers, initialize geometry arrays, check parsed field lengths, and reject missing or zero dimensions when deriving spacing. InternalReadImageInformation now matches complete keys including the colon at column zero, so stimFileName values cannot enter geometry branches.

New GoogleTest coverage writes and reads signed-short images through paths containing header keywords, checking pixels, dimensions, origin and spacing. The existing malformed-header test now checks a specific diagnostic for every input. The module explicitly declares its GoogleTest dependency.

Native validation

Linux x86_64, GCC 13.3, C++23:

  • Against the same SDK, the published parser reads the plain control but fails the five dim, fov, dim_fov_path, dimitri and fovea regressions. The corrected parser passes them.
  • The focused harness passes 23 CTest entries: 22 path round trips and one test covering nine malformed headers.
  • All 22 round trips pass three repetitions with four CTest workers.
  • ASan and UBSan pass the same 23 entries, with leak detection enabled. Instrumentation covers the changed parser and test executables; cached SDK libraries are not instrumented.
  • The official ITKIOStimulateGTestDriver and ITKIOStimulateTestDriver build from source. All 24 selected official CTest entries pass, including cleanup; the existing itkStimulateImageIOTest also passes separately.
  • Official pre-commit and commit-message hooks pass.

These are scoped results, not a full ITK suite or a new Windows/macOS validation. The BigEndian external-data test was not rerun for this update.

AI assistance

AI assistance supported investigation, implementation and test preparation.

Signed-off-by: Demetrios Agourakis <demetrios@agourakis.med.br>
@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:IO Issues affecting the IO module labels Oct 12, 2026
@greptile-apps

greptile-apps Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium impact] Adds validation to image file format parsing.

The PR appears safe to merge, with a non-blocking gap in tests for the new geometry checks.

Findings

  1. P2 Geometry checks lack tests ▶

Summary

The PR bounds numDim, initializes parsing arrays, checks geometry field lengths, and guards spacing derived from fov.

  • The Stimulate reader checks header dimensions and geometry before using them.

Reviews (1) · Last reviewed commit: "BUG: Validate Stimulate image dimensions" · Reviewed by Greptile

@hjmjohnson hjmjohnson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @agourakis82, this closes all three defects in #6815 cleanly. One blocking change before merge: the new per-field count checks make files written by ITK itself unreadable when their path contains "dim" or "fov", because keys are matched anywhere in the line. Anchoring each key at the start of the line fixes it; details, a reproduction, and a suggested test are below. Everything else here is optional and can wait for a follow-up PR.

Blocking: key matching reaches the stimFileName path

InternalReadImageInformation selects a branch with text.find("dim"), text.find("fov"), and so on, anywhere in the line, and the dim branch (line 231) and fov branch (line 281) come before stimFileName: (line 421). The writer always emits stimFileName: <full .sdt path> (line 596). For a path such as /data/dimensions/image.sdt or /scans/fovea/image.sdt, that line now enters the dim or fov branch, sscanf reads zero values, and the new count check throws. Before this PR the same line read zero values and was harmless, so those files read correctly.

Every key in the format, both in the writer's output and in BigEndian.spr, is key: at the start of its line, so a start-of-line match with the colon is safe:

const auto hasKey = [&text](const char * key) { return text.rfind(key, 0) == 0; };

if (hasKey("numDim:"))
{
  // ...
}
else if (hasKey("dim:"))
{
  // ...
}
else if (hasKey("origin:"))
// ... the same for extent:, fov:, interval:, dataType:, displayRange:, fidName:,
// sdtOrient:, dsplyThres:, endian:, mapParmFileName:, mapTypeName:, stimFileName:

This also stops the older substring cases (a path containing "origin" or "interval") that copied uninitialized values before the arrays were initialized.

Suggested regression test: write a small short image with StimulateImageIO into a directory whose name contains both substrings, for example ${ITK_TEST_OUTPUT_DIR}/dim_fov_path/image.spr, and read it back with StimulateImageIO. It passes on main and fails with this PR as it stands.

Reproduction

Built the reader from main and from this PR against the same ITK build, wrote a 4x4 short image with the Stimulate writer into three directories, and read each back:

Directory main This PR
.../plain/image.spr reads reads
.../dimitri/image.spr reads Stimulate dim must specify one value for each dimension, got: stimFileName: .../dimitri/image.sdt
.../fovea/image.spr reads Stimulate fov must specify one value for each dimension, got: stimFileName: .../fovea/image.sdt
Optional, fine for a follow-up PR
  • Documentation/AI/testing.md asks for new tests to be GoogleTest (itkStimulateImageIOGTest.cxx with EXPECT_THROW) rather than a new legacy driver test.
  • Four of the nine cases (numDim 9, UINT_MAX, malformed numDim, fov without dim) pass no expected message, so any exception satisfies them; checking the text would prove the intended guard fired.
  • Fold the test-only second commit into the first, and give the first commit a one- or two-line body with the why (an unbounded numDim drove reads past the four-element arrays).
  • The repository's prose rules ask that PR bodies not name the AI tool; keep the disclosure, drop the tool name.
  • Related cases from the same survey, out of scope here: #6813 (same class in ITKIOBruker), the unchecked sscanf for COLOR_SCALARS in the VTK reader, and ImageIOBase accepting zero or NaN spacing. A zero or huge dim value with an interval line still passes silently.

Data-file paths can contain geometry field names. Substring matching
interprets those paths as header values and rejects writer output.
@agourakis82

Copy link
Copy Markdown
Contributor Author

Thanks for the reproducer. Fixed in 7709270: InternalReadImageInformation now matches complete keys at column zero, including the colon, so stimFileName paths cannot enter geometry branches.

Added GoogleTest round trips for 22 path variants, including dim_fov_path, dimitri and fovea. Five cases fail with the published parser and pass with the fix; all nine malformed-header inputs still produce their expected diagnostics. The scoped tests also pass with ASan/UBSan. The description records validation scope and removes the tool identifier from the assistance disclosure.

@agourakis82

Copy link
Copy Markdown
Contributor Author

@hjmjohnson Could you re-review 770927082 when convenient? The fix and scoped validation are recorded in the response to your reproducer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stimulate reader: unbounded numDim causes out-of-bounds stack reads of header arrays

2 participants