BUG: Fix an out-of-bounds indirect call in axio_show_mim_summary - #40
hjmjohnson merged 3 commits into
Conversation
062285e to
84fe587
Compare
84fe587 to
98389a4
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 unrecognised 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'".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KSPnbwpDjVcAYqDdVqLkMU
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 preceding commit: the sanitizer legs report a global-buffer-overflow, an 8-byte read 8 bytes before MIM_disp_funcs. The second holds a BrainModel and asserts it is still displayed, so that skipping the unresolved name cannot pass by skipping every name. 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.
98389a4 to
e2f30ed
Compare
|
Rebased onto current AddressSanitizer on unmodified Eight bytes before a 64-bit function-pointer array is The two tests, and why there are twoBoth drive
To confirm the first test is actually tied to the fix, I reverted the one-line change while keeping both tests: The regression test fails on its own and passes once the guard is restored. Verification
The change is inside a function body, so no symbol appears or disappears; the comparison above confirms it. Two notes on the change itselfI tightened the added comment to one line describing what the code does. The original mentioned that the test "used to be on kid", and history of that kind reads better from Worth recording separately: these are the first tests the cifti library has. |
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. 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 sanitizer job alone. That also discarded the buffered standard output, which is why the summary text went missing there rather than merely being followed by a leak report.
|
The sanitizer job failed on the previous push, and the cause was a second real bug rather than a problem with the tests. Pushed a third commit that fixes it; the same configuration now passes.
Why it failed on Linux only, and why the output vanishedLeakSanitizer runs by default under AddressSanitizer on Linux, and does not on Apple. So the same binary that exits 0 locally on macOS exits 1 on the That also explains the second, stranger symptom. Both symptoms are the one leak. The fix axml_free_xml_t(ax);
nifti_image_free(nim);Both routines already return early on NULL, so neither path needs a guard, and the Verification, on Linux under the failing configurationUbuntu, clang, Debug,
The |
6484a6d
into
InsightSoftwareConsortium:master
|
The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: This PR's commits on the rewritten
The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves. |
2 participants
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 unrecognised 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'".
Interface impact: none. On the union of all these changes, configured with
USE_FSL_CODE=ONandUSE_CIFTI_CODE=ON: all 448 exported symbols acrosslibniftiio,libnifti2,libznz,libfslio,libnifticdfandlibciftiare identical tomasterundernm -D --defined-only, and all ten installed headers are identical undergcc -E -P. Undergcc -dM -Eone macro definition differs, intentionally and only in text: #61 makesFSL_RADIOLOGICALread(-1)so it is safe inside an expression. Its value is still-1, checked by compiling against each installedfslio.hand printing it.Verification. This branch: builds with gcc 16.1.1,
ctestunchanged frommaster(2 of 345 fail onmasteritself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8,ctest345/345 under each, and the whole suite under valgrind memcheck with--trace-children=yesgives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9
-Wsign-conversion(5 infslio.cfor #22, 2 innifti2_io.cand 2 innifti_tester001.cfor #24) and 8-Wcalloc-transposed-argsinnifti_findhdrnameandnifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.One of a set of independent, single-purpose PRs. Each bases on
masterand can be merged on its own, in any order.The full set of PRs (35)
The union of all of them is on the fork as
all-changes, if you want to build and test the lot at once.CI and build
Configuration and documentation
Defects
Warning and check classes