metaUtils: report uncompression failures to the caller#142
Merged
Conversation
MET_PerformUncompression always returned true, and neither MetaImage nor MetaArray looked at the result, so a truncated or corrupt stream produced a partly written buffer and a successful read. Its byte accounting also could not support a success check: dest_pos was advanced after the test that breaks out of the loop, so the output of the final inflate call -- the one reporting Z_STREAM_END -- was never counted, leaving dest_pos at 0 for a stream that decompressed in one pass. Count each call's output before acting on its status, then require that the expected number of bytes was produced. When the compressed size is absent from the header, the readers assumed the data began at the start of the file. For a single-file image that is the start of the ASCII header, which was then fed to inflate. Measure from the current position instead, which is where the element data actually begins. MetaArray::M_ReadElements also leaked its compressed buffer.
hjmjohnson
marked this pull request as ready for review
July 22, 2026 11:42
Contributor
Author
|
@dzenanz Fixes before updating the ITK vendored copy two PR's pushed upstream: MetaIO PRs are on Kitware/MetaIO:
|
dzenanz
approved these changes
Jul 22, 2026
Member
|
Consider addressing #143, to have a reliable test here before integrating into ITK/VTK. |
This was referenced Jul 23, 2026
Closed
dzenanz
pushed a commit
that referenced
this pull request
Jul 23, 2026
MET_PerformUncompression validated the number of bytes produced but not that the zlib stream terminated with Z_STREAM_END. When the output fills the destination buffer exactly, the next inflate() returns Z_BUF_ERROR (deliberately excluded from the diagnostic), the loop exits, and the byte-count check passes -- so a stream truncated at the size boundary is accepted without validating its trailing Adler-32 checksum. Reject the data unless inflate reported Z_STREAM_END, in addition to the existing byte-count check. This closes the exact-size truncation gap left by the byte-count check added in #142. Fixes #144.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A truncated or corrupt compressed stream is read as a successful image.
MET_PerformUncompressionalways returnstrue, and neitherMetaImagenorMetaArrayinspects the result.Found while auditing MetaIO as vendored by ITK (InsightSoftwareConsortium/ITK#6575, items B65 and B66). New test
testMeta14ImageCompressedcovers both shapes and fails before this change.The byte accounting had to be fixed first
MET_PerformUncompressionends in an unconditionalreturn true;— every error path merelybreaks out of the loop. The natural success criterion is "did we produce the expected number of bytes?", butdest_poscould not answer that:The output of the final
inflatecall — the one returningZ_STREAM_END— was never added, so a stream that decompressed in a single pass leftdest_posat 0. Counting each call's output before acting on its status makesdest_posa true total, after which requiringdest_pos == uncompressedDataSizeis a sound check. Both call sites now honour the result.Compressed size absent from the header
When
CompressedDataSize/CompressedElementDataSizeis missing, the readers took the whole file as the payload:For a single-file (
LOCAL) image the file begins with the ASCII header, so the header text itself was handed toinflate. Measuring from the current position — where the element data actually begins — is correct for both theLOCALand separate-data-file cases, since the latter is already positioned at 0.MetaArray::M_ReadElementscarried the same assumption, and additionally never freed its compressed buffer; both are fixed here.Testing
testMeta14ImageCompressedwrites a compressedLOCALimage, stripsCompressedDataSizefrom the header, and requires the data to read back correctly; then truncates the stream and requires the read to fail.Verified fail-before/pass-after — before the change the first case reports
element 1 is 0, expected 1, i.e. a silently zero-filled buffer returned as success. Full suite 13/13 passing.Also run through ITK's MetaImageIO with no regressions in its Meta test suite.