BUG: Keep the XML skip depth at the element that started the skip - #125
Open
hjmjohnson wants to merge 5 commits into
Open
hjmjohnson wants to merge 5 commits into
hjmjohnson wants to merge 5 commits into
Conversation
znzread(), znzwrite() and nifti_read_buffer() all return size_t and
all returned -1 to report an error. In a size_t that value is
SIZE_MAX, larger than any length a caller can have asked for, so a
caller that tests the result with '<' reads the error as a complete
transfer:
ii = nifti_read_buffer(fp, nim->data, ntot, nim);
if( ii < ntot ){ ... } /* SIZE_MAX < ntot is false */
The visible effect is that a truncated image loads as if it were
whole: nifti_image_read() prints its short-read warning, ignores the
failure and returns an image whose tail is uninitialized heap.
FslReadVolumes() has the same problem one level up, dividing the
returned byte count by the volume size to report SIZE_MAX/volbytes
volumes read.
These now return 0, which every one of these callers already treats
as failure and which is the only value a size_t function has for
"nothing was transferred". The published return types in znzlib.h
and nifti1_io.h are unchanged.
The nifti2 equivalents return int64_t, where -1 is representable,
and are left alone.
(cherry picked from commit b344fe4)
Writes a 31x31x31 float image, copies it 1000 bytes short, and asserts nifti_image_read() returns NULL for the copy and a populated image for the original, so a blanket refusal cannot pass. Without the fix the truncated file comes back accepted with nvox=29791 and an uninitialized tail. (cherry picked from commit 9b31abf)
afni_xml's parser holds sixteen levels of open elements in a fixed array and refuses to push a seventeenth by entering a skip block: epush() records the depth in xd->dskip and stops touching the stack until the matching pop. Two details of that bookkeeping are wrong. epush() overwrote dskip on every push past the limit, so the skip was recorded as starting at the innermost depth. epop() then cleared dskip before the stack was handled, so the pop that ends the skip fell through to "xd->stack[xd->depth-1] = NULL" and wrote a pointer eight bytes past the sixteen-entry array, which lives in the file-scope afni_xml_control. The next pop found dskip clear and dereferenced xd->stack[16]. epush() now keeps the outermost skip depth, since that is the depth whose pop ends the skip, and epop() clears dskip only after the element has been skipped. Depths within the limit are unaffected. The XML comes from the CIFTI extension of a NIfTI file, or from any file read by axml_read_file(), so the nesting is chosen by the input. Found by fuzzing axml_read_buf() with libFuzzer under AddressSanitizer. (cherry picked from commit 6ddc454)
process_popped_element() read xd->stack[xd->depth-1] and dereferenced it without a test. A slot is filled by the matching epush(), and an element that was skipped never fills one, so the read is only safe because the preceding commit keeps the skip running to the element that started it. Guard it rather than rely on that: check the depth is inside the stack, and that the slot holds a struct with a name, before the strcmp(). (cherry picked from commit d34f28c)
deep_nesting.xml nests twenty elements inside a CIFTI extension whose stack holds sixteen, which is the shape that walked off the end of xd->stack. The test drives it through cifti_tool and pins that the BrainModel sibling is still summarized, so a fix that refused the whole document would not pass. (cherry picked from commit 0e0033d)
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
3 participants
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.
Re-submission of #65, reverted from
masteron 2026-09-24. Content isunchanged from the original.
Position 3 of 11 in the deep stack. Base:
stack/pr-fix-alloc-null-checks.Based on the pull request above it in the stack, so the diff shown here is
this change alone. Merge the stack bottom-up.
Stack order
stack/test-regression-coveragemasterstack/pr-fix-alloc-null-checksstack/test-regression-coveragestack/fix-axml-skip-depth<- this PRstack/pr-fix-alloc-null-checksstack/pr-fix-analyzer-leaksstack/fix-axml-skip-depthstack/pr-fix-sign-conversionstack/pr-fix-analyzer-leaksstack/fix-fslio-64bit-arithmeticstack/pr-fix-sign-conversionstack/fix-cifti-null-streamstack/fix-fslio-64bit-arithmeticstack/pr-fix-calloc-transposed-argsstack/fix-cifti-null-streamstack/pr-fix-shorten-64-to-32stack/pr-fix-calloc-transposed-argsstack/fix-image-read-complex-checkstack/pr-fix-shorten-64-to-32stack/pr-fix-xml-read-errorsstack/fix-image-read-complex-checkThe order is the order these changes sat on
masterbefore the revert, soit builds and tests at every step.
Commits introduced by this PR
Ordering for all the re-submitted work is tracked in #84.