Skip to content

BUG: Fix silent-corruption defects in VTKImageIO read and write#6610

Merged
hjmjohnson merged 7 commits into
InsightSoftwareConsortium:mainfrom
physwkim:bug-vtk-io-6575
Jul 14, 2026
Merged

BUG: Fix silent-corruption defects in VTKImageIO read and write#6610
hjmjohnson merged 7 commits into
InsightSoftwareConsortium:mainfrom
physwkim:bug-vtk-io-6575

Conversation

@physwkim

Copy link
Copy Markdown
Contributor

Six defects in VTKImageIO's reader and writer: truncated files read as success, sscanf results ignored over uninitialized locals, substring-based attribute dispatch, ASCII precision loss, and CanReadFile() throwing. Addresses B57–B60, B62 of #6575 (and the ReadTensorBuffer sibling of B61).

The six commits
  • B57VTKImageIO::Read()'s non-streaming binary path discarded ReadBufferAsBinary()'s bool, so a truncated file returned partially-uninitialized pixels as success. The other call sites (in StreamReadBufferAsBinary / StreamWriteBufferAsBinary) already throw internally before returning false — distinct, untouched.
  • B58 — six sscanf calls in InternalReadImageInformation() discarded their returned field count while writing into uninitialized locals, so a malformed DIMENSIONS 5 line sized the image from indeterminate values. All six destinations are now zero-initialized and the field count is checked, throwing and naming the malformed line. color_scalars/scalars numComp already had a default assigned before the sscanf — an undercount leaves a defined value there, so those are distinct and untouched.
  • B59 — attribute dispatch was an ordered substring test (text.find("vector") < text.length()), so SCALARS vector_field float matched the vector arm and was misparsed as a 3-component VECTORS line. Now dispatches on the line's first whitespace-delimited token.
  • B60 — ASCII write set precision(16) on the header stream, but the pixel data went out through a second stream at the default 6 significant digits. The precision is now set on the stream that actually writes the pixels.
  • B62 — four GetNextLine() calls sat outside CanReadFile()'s try block, so a .vtk file shorter than four lines threw "Premature EOF" out through the IO factory. CanReadFile() must never throw; the calls moved inside.
  • B61 siblingReadTensorBuffer() has the identical defect B61 fixes in ImageIOBase (see BUG: Check stream state after each ASCII component read #6609): a PrintType temp hoisted outside the read loop with no stream check, so one bad field latches its stale value into every later component. Fixed here because it is the same defect family and lives in this file. Each of the nine per-tensor extractions now goes through a helper that zero-initializes a fresh value, extracts, and throws on is.fail() — including the redundant symmetric "skip" reads, which previously swallowed corruption silently.

Sweep of the rest of the file for the same anchors turned up nothing else: the os << ASCII writes have no stale-latch failure mode, and the remaining .find() calls are single-purpose presence gates, not differentiated dispatch.

Local validation

New GoogleTest itkVTKImageIOGTest.cxx (the module had no GTest driver). Every fixture is synthesized byte-for-byte in the test body — no ExternalData added.

Test Fail-before Pass-after
TruncatedBinaryFileThrowsOnRead (B57) no throw, silent short read throws
MalformedDimensionsLineThrows (B58) no throw throws
ScalarsNameContainingVectorSubstringParsesAsScalars (B59) parsed as 3-component VECTOR 1-component SCALAR
AsciiWriteRetainsDoublePrecision (B60) EXPECT_NEAR failed passes
CanReadFileReturnsFalseOnShortFile (B62) threw returns false
TensorOverflowingFieldThrowsOnRead (B61 sibling) no throw throws

ctest -R "VTKImageIO|VTIImageIO": 46/46 pass, no pre-existing test changed result.

AI assistance

@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 Jul 13, 2026
@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens legacy VTK image IO read and write behavior. The main changes are:

  • Checked malformed VTK header fields before using parsed values.
  • Switched attribute dispatch from substring matching to first-token matching.
  • Raised errors for truncated binary and malformed tensor ASCII reads.
  • Preserved higher precision when writing ASCII pixel data.
  • Added GoogleTest coverage for the fixed reader and writer paths.

Confidence Score: 4/5

The changed VTK keyword parsing path needs a whitespace fix before merging.

Indented valid header keywords can stop matching after the first-token change. The binary read, tensor read, precision, and CanReadFile changes look consistent with the inspected contracts. The new tests cover the intended bug fixes but do not cover leading whitespace before keywords.

Modules/IO/VTK/src/itkVTKImageIO.cxx

T-Rex T-Rex Logs

What T-Rex did

  • The focused C++ harness against a legacy VTK fixture reproduced the issue where indented SPACING and SCALARS lines yield empty keywords and fail at EOF.
  • The local toolchain availability check showed g++/gcc were available but cmake/ctest/ninja were not on PATH, so a focused harness was used instead of the ITK CMake test target.
  • Environment constraints from the second proof were confirmed: cmake/ctest/ninja unavailable while C++ compilers were present; an attempted ctest command failed with exit code 127; ITK VTKImageIO tests are listed in ITKIOVTKGTests and wired to the GoogleTest driver.
  • Artifacts from both proofs were collected and labeled for review, including the failing harness output, the focused harness source, the legacy fixture, and the toolchain/test-wiring logs.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/IO/VTK/src/itkVTKImageIO.cxx Hardens VTK parsing and data reads, but the new keyword extraction rejects indented header lines that previously matched.
Modules/IO/VTK/test/CMakeLists.txt Adds the new VTKImageIO GoogleTest source to the existing test driver.
Modules/IO/VTK/test/itkVTKImageIOGTest.cxx Adds focused tests for malformed headers, short reads, tensor parse failures, precision, and CanReadFile behavior.

Reviews (1): Last reviewed commit: "BUG: Check stream state after each ASCII..." | Re-trigger Greptile

Comment thread Modules/IO/VTK/src/itkVTKImageIO.cxx Outdated
@hjmjohnson

Copy link
Copy Markdown
Member

First force-push (3f7b178708c4e2) = plain rebase on main; second (708c4e211aeb00) = the indented-keyword fix only.

@hjmjohnson

Copy link
Copy Markdown
Member

Follow-up push = one review commit from a high-effort automated review: ReadHeaderSize now shares the first-token attribute dispatch (its old substring scan could stop at a different line than the rewritten header parser — e.g. a FIELD array named vector_x — and corrupt the streamed-write offset), DIMENSIONS rejects negative tokens (previously wrapped through %u to ~4e9 and OOM'd instead of throwing), and the tensor test is named for its actual non-numeric fixture. New NegativeDimensionsLineThrows regression test; ctest -R VTKImageIO: 19/19 pass.

Intentional-strictness notes (not changed)
  • 2-value SPACING/ORIGIN/DIMENSIONS lines and the singular VECTOR keyword now throw where the old parser accepted them by accident (uninitialized stack reads / substring dispatch). The legacy VTK format mandates 3 values and the plural keyword, so the strictness is kept — flagging here in case a reviewer wants lenient 2D handling instead.
  • COLOR_SCALARS keeps its unchecked count with a defined default of 1, per the PR body's stated scope (no uninitialized read there).

physwkim and others added 7 commits July 13, 2026 20:59
A truncated BINARY .vtk file left the tail of the pixel buffer
unset instead of raising an error, since the read status was
discarded.

Addresses item B57 of InsightSoftwareConsortium#6575.
DIMENSIONS, SPACING, ORIGIN, and the VECTORS/SCALARS/TENSORS type
tokens were scanned into uninitialized locals without checking how
many fields sscanf actually filled, so an under-filled line sized
or typed the image from indeterminate values.

Addresses item B58 of InsightSoftwareConsortium#6575.
The header parser matched attribute keywords (SPACING, ORIGIN,
VECTORS, COLOR_SCALARS, SCALARS, TENSORS) as substrings, so a
SCALARS array whose name contained another keyword, such as
"vector_field", was misparsed as that other attribute.

Addresses item B59 of InsightSoftwareConsortium#6575.
The 16-digit precision set on the header stream did not carry over
to the separate stream used to write pixel data, so ASCII output
silently dropped to the default 6 significant digits.

Addresses item B60 of InsightSoftwareConsortium#6575.
GetNextLine's premature-EOF exception propagated out of
CanReadFile when the header was fewer than four lines, so a short
.vtk file threw instead of yielding a plain false.

Addresses item B62 of InsightSoftwareConsortium#6575.
ReadTensorBuffer kept one temp across all nine per-row extractions
and never checked the stream, so a malformed or overflowing tensor
field left failbit set and silently replayed the same stale value
into every remaining component instead of failing.

Addresses item B61 of InsightSoftwareConsortium#6575.
ReadHeaderSize now uses the same first-token attribute dispatch as
InternalReadImageInformation, so the streamed-write header re-scan
cannot stop at a different line (e.g. a FIELD array whose name merely
contains "vector"). DIMENSIONS tokens are parsed as signed values and
rejected when non-positive; %u silently wrapped negatives to huge
sizes. The tensor test name now states the non-numeric-field fixture.
@hjmjohnson

Copy link
Copy Markdown
Member

Force-push = plain rebase onto current main, folding the new GTest source into the ITKIOVTKGTests list introduced by #6589 (avoids the duplicate creategoogletestdriver configure error in the CI merge build). Local configure clean; ctest -R 'VTKImageIO|VTIImageIO': all pass.

@hjmjohnson
hjmjohnson merged commit 0f3de1f into InsightSoftwareConsortium:main Jul 14, 2026
21 checks passed
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.

2 participants