Skip to content

BUG: Fix destructive write and silent short reads in GiplImageIO#6611

Merged
hjmjohnson merged 9 commits into
InsightSoftwareConsortium:mainfrom
physwkim:bug-gipl-io-6575
Jul 13, 2026
Merged

BUG: Fix destructive write and silent short reads in GiplImageIO#6611
hjmjohnson merged 9 commits into
InsightSoftwareConsortium:mainfrom
physwkim:bug-gipl-io-6575

Conversation

@physwkim

Copy link
Copy Markdown
Contributor

GiplImageIO truncates the target file before validating the write, and reports truncated reads as success. Addresses B53–B56 of #6575, plus the INT/UINT support the B53 investigation turned up.

The five commits
  • B53 — destroys existing data. Write() opened (and truncated) the output file and wrote the 256-byte header before the switch (m_ComponentType) that decides image_type. An unsupported component type threw only after the target was already destroyed, leaving a 256-byte stub where a valid image used to be. The switch now runs before the file is opened.
  • B54 — truncated read reports success. The uncompressed Read() tested success = !m_Ifstream.bad(). A short read sets failbit/eofbit, never badbit, so a truncated file "succeeded" over an uninitialized buffer tail. Now compares gcount() against GetImageSizeInBytes().
  • B55 — same, compressed path. success = (p != nullptr) on the caller's always-non-null output buffer, with gzread's byte count discarded. Now checks the returned count.
  • B56 — vector images write a corrupt file. image_type came from the component type alone while the byte count multiplied by the component count, so an N-component image wrote a scalar header followed by N times the pixel bytes. GIPL's header has no component-count field and ReadImageInformation() hardcodes SCALAR, so the format cannot represent a vector pixel at all — a multi-component write is now rejected before any file I/O rather than encoded lossily.
  • ENH — INT/UINT support. The B53 investigation found Write()'s switch already emitted GIPL_INT (32) and GIPL_U_INT (31), but SwapBytesIfNecessary() had no arms for them, so an int image threw "Pixel Type Unknown" from deep inside Write(). The two gates disagreed, and that disagreement is what let INT/UINT reach the destructive late throw. The codes exist in the format, so the missing swap arms are added rather than the types declared unsupported. Kept as a separate ENH: commit so the four BUG: commits stay honest — with only the B53 commit applied, an int write still throws, but without destroying the file.
Local validation

New GoogleTest itkGiplImageIOGTest.cxx (the module had no GTest driver). Fixtures are written by the test itself — no ExternalData added.

Test Item Fail-before Pass-after
WriteOfUnsupportedComponentTypePreservesExistingFile B53 existing file became a 256-byte stub file intact after the throw
ReadOfTruncatedUncompressedFileThrows B54 no throw, uninitialized tail throws
ReadOfTruncatedCompressedFileThrows B55 no throw throws
WriteOfMultiComponentImageThrows B56 wrote a corrupt scalar-header file throws
RoundTripsIntAndUnsignedIntComponentTypes ENH n/a passes

ctest -R Gipl with ExternalData fetched, at main and at the branch tip: itkGiplImageIOTest, itkGiplImageIOGzTest, itkGiplImageIOTest2 all Passed at both states. Those three exercise the normal (non-truncated) compressed and uncompressed round-trip against the real ramp.gipl fixture, which is exactly what the new byte-count checks could have broken — they do not misfire on valid data.

Adjacent gap found, not fixed here

ReadImageInformation() reads all 15 header fields through unchecked gzread / ifstream::read pairs, and CanReadFile()'s magic-number probe is unchecked too. That is the same silent-corruption class as B54/B55 but a different shape — no success flag is computed at all, so there is nothing to correct locally; it wants its own change (read the 256-byte header in one call and check the count once). Flagged on #6575 rather than folded in.

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 GIPL ImageIO read and write behavior. The main changes are:

  • Validate unsupported write cases before opening the output file.
  • Reject multi-component GIPL writes.
  • Check compressed and uncompressed reads against the expected byte count.
  • Add INT and UINT byte swapping support.
  • Add GoogleTest coverage for the fixed cases.

Confidence Score: 5/5

This looks safe to merge after considering the large compressed-read edge case.

The changed write path now validates before file I/O, the scalar GIPL write contract is enforced before data is written, and the new tests follow the module's GoogleTest pattern.

Modules/IO/GIPL/src/itkGiplImageIO.cxx still has a non-blocking large-file count narrowing edge case.

T-Rex T-Rex Logs

What T-Rex did

  • Reviewed the general contract validation evidence, including the GIPL validation log and the attempted cmake configuration.
  • Noted that no GIPL test failures were observed in this environment because the build/test driver could not be configured.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/IO/GIPL/src/itkGiplImageIO.cxx Moves write validation before file opening, rejects multi-component writes, adds INT/UINT swapping, and checks read byte counts.
Modules/IO/GIPL/test/CMakeLists.txt Adds the GIPL GoogleTest driver with the module output directory definition.
Modules/IO/GIPL/test/itkGiplImageIOGTest.cxx Adds tests for failed writes preserving files, truncated reads throwing, multi-component rejection, and INT/UINT round trips.
Modules/IO/GIPL/itk-module.cmake Adds ITKGoogleTest as a test dependency.

Reviews (1): Last reviewed commit: "ENH: Support INT and UINT component type..." | Re-trigger Greptile

Comment thread Modules/IO/GIPL/src/itkGiplImageIO.cxx Outdated
physwkim and others added 8 commits July 13, 2026 10:23
GiplImageIO::Write truncated and partially wrote the header before
determining whether the component type had a Gipl encoding,
destroying any pre-existing file when the type turned out to be
unsupported. Move the component-type validation before the file is
opened so an unsupported type is rejected without touching the
target file.

Addresses item B53 of InsightSoftwareConsortium#6575.
GiplImageIO::Read treated any non-bad ifstream as success, but a
short read only sets failbit/eofbit, so a truncated file returned an
uninitialized tail as if it were valid pixel data. Use the
ImageIOBase::ReadBufferAsBinary helper, which compares gcount()
against the expected byte count, checks fail(), and range-checks the
streamsize cast.

Addresses item B54 of InsightSoftwareConsortium#6575.
GiplImageIO::Read used the caller's output buffer pointer, which is
never null, as the success flag for the compressed path, so gzread's
actual byte count was never inspected. A truncated .gipl.gz file
reported success over a partially filled buffer. Read in chunks of
at most 1 GiB, since gzread rejects requests over INT_MAX and a
single 32-bit count also truncates for images of 4 GiB or more, and
require the 64-bit total to reach the expected byte count.

Addresses item B55 of InsightSoftwareConsortium#6575.
GiplImageIO::Write derived the header image_type from the component
type alone but sized the pixel payload by component count, so a
vector image wrote a scalar header with N times the expected bytes.
The Gipl header has no field for component count, so reject
multi-component images instead of writing an unreadable file.

Behavior change: pipelines that previously wrote RGB or vector
images to .gipl produced a malformed-but-present file; they now
throw before any file is written.

Addresses item B56 of InsightSoftwareConsortium#6575.
Gipl has real type codes for INT (32) and UINT (31), but
SwapBytesIfNecessary had no cases for them, so a write hit an
unrelated "Pixel Type Unknown" throw after the file was already
truncated. Prior commits closed that hole by rejecting INT/UINT
before any file I/O. Add the missing swap cases and accept both
types again now that they round-trip correctly.

Addresses item B53 of InsightSoftwareConsortium#6575.
Read() accumulated the pixel count in a uint32_t. Header dimensions
are unsigned short, so a volume can exceed 2^32 pixels, wrapping the
count passed to SwapBytesIfNecessary and leaving the buffer tail
un-swapped on opposite-endian machines.
SwapBytesIfNecessary repeated the LittleEndian/BigEndian if-else
block once per component type; a templated helper reduces each case
to one line.
The Gipl GTests repeated the ImageFileWriter/ImageFileReader setup
in six places and the truncate-then-read block in two; local helper
templates collapse each to one call.
@hjmjohnson

Copy link
Copy Markdown
Member

Maintainer pass (with @physwkim's commits preserved and two of them rewritten in place): the compressed-read check is now 64-bit safe, plus one additional pre-existing bug fix and two STYLE dedups. Local: 5/5 GTests, 5/5 module ctests, pre-commit clean.

What changed and why
  • Compressed short-read commit (rewritten): the previous check compared gzread's return against static_cast<int>(GetImageSizeInBytes()) after requesting static_cast<unsigned int>(...) bytes. zlib's gzread rejects requests over INT_MAX (returns -1), so valid 2-4 GiB files would always throw, and for >= 4 GiB images both casts truncate identically, so a short read could still pass. Now reads in <= 1 GiB chunks and requires the 64-bit total to reach the expected count.
  • Uncompressed short-read commit (rewritten): replaced the inline read + gcount() comparison with the existing ImageIOBase::ReadBufferAsBinary helper, which additionally checks fail() and range-checks the streamsize cast.
  • New BUG: Accumulate Gipl pixel count in SizeValueType: header dims are unsigned short, so >2^32-pixel volumes are representable; the uint32_t accumulator wrapped the count passed to SwapBytesIfNecessary, leaving the buffer tail un-swapped on opposite-endian reads. Pre-existing, but the new INT/UINT swap cases inherit it, so fixed here.
  • Multi-component reject commit: message now documents the behavior change (previously wrote a malformed file, now throws before writing).
  • Two STYLE commits: templated SwapRange<T> helper collapses the six duplicated byte-order blocks; test helpers collapse the six duplicated writer/reader/truncate blocks.

Open question, not addressed: ReadImageInformation maps GIPL_BINARY (image_type 1) to UCHAR, but packed binary GIPL files may store fewer than 1 byte/voxel; if such files exist in the wild, the strict completeness check turns them from readable to hard error. No fixture available to confirm — flagging for awareness rather than blocking.

A bare "Error reading image data." hides the 8:1 mismatch that
identifies a packed GIPL_BINARY file; the counts make any short
read self-explanatory.
@hjmjohnson

Copy link
Copy Markdown
Member

Follow-up to the GIPL_BINARY note above: short-read errors now report the byte counts (Error reading image data: read X of Y expected bytes., commit 6e11267), so a packed 1-bit-per-voxel file is self-identifying by its 8:1 mismatch. Packed-bit unpacking itself remains out of scope until a real fixture surfaces.

@hjmjohnson hjmjohnson merged commit 82050a3 into InsightSoftwareConsortium:main Jul 13, 2026
24 of 25 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