Repository navigation
BUG: Fix an out-of-bounds indirect call in axio_show_mim_summary - #109
Open
hjmjohnson wants to merge 3 commits into
Open
hjmjohnson wants to merge 3 commits into
hjmjohnson wants to merge 3 commits into
Conversation
hjmjohnson
marked this pull request as ready for review
September 24, 2026 12:17
hjmjohnson
force-pushed
the
pr/fix-oob-indirect-call
branch
from
October 6, 2026 14:35
4a52c54 to
ec5ea2f
Compare
hjmjohnson
force-pushed
the
pr/fix-oob-indirect-call
branch
from
October 14, 2026 19:18
ec5ea2f to
336afd8
Compare
seanm
reviewed
Oct 19, 2026
hjmjohnson
force-pushed
the
pr/fix-oob-indirect-call
branch
2 times, most recently
from
November 2, 2026 20:37
28ee1da to
51f7943
Compare
mind = get_map_index(xt->xchild[kid]);
if( kid >= 0 ) MIM_disp_funcs[mind](ofp, xt->xchild[kid], verb);
The guard tests kid, the loop counter, which is never negative, so it is
always true. The index actually used is mind, and get_map_index()
returns -1 for any element name not in MIM_kids[]. A CIFTI file
containing an unrecognized element under MatrixIndicesMap therefore reads
a function pointer from before the start of MIM_disp_funcs and calls it.
Reported by the clang static analyzer as security.ArrayBound, "Out of
bound access to memory preceding 'MIM_disp_funcs'".
process() returned without releasing either the afni_xml_t it parsed or the nifti_image the non-cext path fills in, so every run leaked the whole tree. The non-cext path also leaked the image when the NIfTI part was read but held no CIFTI extension. Both free routines already accept NULL, so neither path needs a guard. LeakSanitizer is on by default under AddressSanitizer on Linux but not on Apple, so the leak ended the process with a non-zero status on the Linux sanitizer job alone. With this commit reverted, the tests added in the next commit, cifti_mim_summary_unknown_child and cifti_mim_summary_known_child, both fail there with a LeakSanitizer report.
Drives cifti_tool with -eval_type show_summary over two XML fixtures. The first holds a MatrixIndicesMap child whose name get_map_index() does not resolve, and fails without the fix to axio_show_mim_summary: the Linux sanitizer job reports a global-buffer-overflow, an 8-byte read 8 bytes before MIM_disp_funcs. The second holds a BrainModel and requires the BrainStructure attribute that only disp_brainmodel_child prints, so that skipping the unresolved name cannot pass by skipping every name. Both tests fail on a sanitizer report even when the process exits with status 0, since a pass regular expression overrides the exit status. These are the first tests for the cifti library. The fixtures are small enough to keep in tree, so neither needs the external testing data.
hjmjohnson
force-pushed
the
pr/fix-oob-indirect-call
branch
from
November 6, 2026 15:13
51f7943 to
64050a6
Compare
Member
Author
|
@greptileai review |
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.
Fixes an out-of-bounds indirect call in
axio_show_mim_summary(the guard tested the loop counter instead of the map index), frees whatcifti_toolparses, and adds the first cifti tests.Re-submission of #40, reverted from
masteron 2026-09-24 so it can be reviewed before merging. Base:master. Independent: nothing has to land before it.Commits
Tests and red-green
cifti_mim_summary_unknown_child: fails with the guard reverted (sanitizer abort).cifti_mim_summary_known_child: requires theBrainStructureattribute that onlydisp_brainmodel_childprints; fails when every child is skipped.Ordering for all the re-submitted work is tracked in #84.