ENH: Add regression tests for the bug fixes of the past two days - #123
Open
hjmjohnson wants to merge 42 commits into
Open
hjmjohnson wants to merge 42 commits into
hjmjohnson wants to merge 42 commits into
Conversation
The test hard-codes the path to the downstream example project:
cmake ... ../../nifti_clib/real_easy/minimal_example_of_downstream_usage
That only resolves when the build directory happens to be a sibling of a
source tree named exactly "nifti_clib". It fails for an in-tree build,
for a build directory named anything else, and on CI, where the checkout
lives at <workspace>/nifti_clib and the path resolves to
<workspace>/nifti_clib/nifti_clib/real_easy/...
CMake now passes CMAKE_SOURCE_DIR to the script, the way the other test
scripts in that directory already receive their arguments.
(cherry picked from commit 6458714)
cmake-multi-platform.yml ran ctest without --output-on-failure, so a red build reported which test failed and nothing about why. (cherry picked from commit 919dcea)
CMakeLists.txt sets policy CMP0169 unconditionally, to keep using the
deprecated FetchContent_Populate. CMP0169 was introduced in CMake 3.30,
and cmake_policy(SET) on an unknown policy is a hard error, so any CMake
older than that fails to configure at all:
CMake Error at CMakeLists.txt:175 (cmake_policy):
Policy "CMP0169" is not known to this version of CMake.
That includes the CMake 3.28 shipped by Ubuntu 24.04, which is what the
CI runners and the project's own Dockerfile use, and it sits well above
the cmake_minimum_required(VERSION 3.10.2) the project advertises.
Wrapping it in if(POLICY CMP0169) is the standard idiom. Verified
configuring with both CMake 3.28.3 and 4.4.2.
(cherry picked from commit 3aefe1b)
nifti_c22_copy_image has been failing on this machine since before any of
this work started. It converts an image i16 -> i64 -> i16 and asserts
the result matches the original, using
cmp out.c22.0.i16.nii.gz out.c22.2.0.i16.nii.gz
That compares gzip output, which is not reproducible across zlib
implementations. The system zlib here is zlib-ng 1.3.1, which Arch,
CachyOS and a growing number of distributions ship in place of stock
zlib; it encodes the same input differently. Both files come out at
exactly 642454 bytes and differ from byte 321594 on.
The conversion itself is fine. Decompressed, the two files are
byte-identical at 1114768 bytes each, so the round-trip through int64 and
back preserves the data exactly, which is what the test set out to check.
Confirmed the other way too: in an Ubuntu 24.04 container with stock
zlib 1.3, the test passes unmodified.
The test now decompresses before comparing, via a small nii_cmp helper
that falls back to plain cmp for uncompressed files. With this the suite
is 92 of 92 on both zlib-ng and stock zlib; previously it was 91 of 92
on any zlib-ng system.
(cherry picked from commit 7d92f16)
cmake-multi-platform.yml is the workflow that runs on every pull
request, and it passes no options at all. With the defaults that means
cifti/ and fsliolib/ are never compiled, and neither are the blocks
behind FSLSTYLE, PIGZ and REJECT_COMPLEX.
USE_CIFTI_CODE OFF
USE_FSL_CODE OFF
FSLSTYLE_NAME_CONFLICTS OFF -> -DFSLSTYLE
FSLSTYLE_PIGZ_SUPPORT OFF -> -DPIGZ
FSLSTYLE_REJECT_COMPLEX OFF -> -DREJECT_COMPLEX
So a PR that changes any of those files collects four green checks that
never built it. Several open PRs are in exactly that position.
The matrix gains an 'options' axis: 'default' is what a consumer gets
with no arguments, 'all' turns the optional libraries and the FSL parity
defines on. Both are kept, because the default build is what ships and
a change can break it while the fuller one still compiles. Four jobs
become eight, each about half a minute.
expat is installed for the Linux 'all' jobs; cifti needs it and the
default jobs do not.
Verified on master before writing this, with the test suite:
default build OK, tests pass
+cifti +fsl build OK, tests pass
+cifti +fsl +FSLSTYLE build OK, tests pass, doPigz/doPigz2
present in nifti2_io.c.o
COMPILE_NIFTIUNUSED_CODE is deliberately left out. It guards code the
project itself labels unused, so compiling it in CI would commit to
keeping it working; whether that code should exist at all is a separate
question.
(cherry picked from commit b6531f0)
The .clang-tidy file has been in the tree since 2025 but nothing has ever
run it, and it shows: two of its entries name clang-analyzer checks that
upstream renamed some releases ago, so --verify-config warned about them.
Run over the whole tree it produced 1697 findings, of which the great
majority came from three style checks. At that ratio nobody reads the
output.
Retuned for what this project actually is: C11, with a shipped ABI.
Measured with clang-tidy 22.1.8, USE_FSL_CODE=ON and USE_CIFTI_CODE=ON,
it now gives 331 findings across 15 checks, every one a defect class:
170 bugprone-macro-parentheses
44 bugprone-unchecked-string-to-number-conversion
25 bugprone-multi-level-implicit-pointer-conversion
19 readability-use-concise-preprocessor-directives
16 bugprone-switch-missing-default-case
13 readability-redundant-casting
5 bugprone-suspicious-string-compare
4 bugprone-suspicious-realloc-usage
3 readability-redundant-control-flow
2 readability-suspicious-call-argument
2 bugprone-misplaced-widening-cast
1 each: readability-implicit-bool-conversion,
readability-function-size, misc-redundant-expression,
bugprone-float-loop-counter
bugprone-macro-parentheses is re-enabled. It had been switched off, but
it is the check that catches the real precedence bugs in the NT_DT_*,
QSTR and NT_FILL macro families.
misc-use-internal-linkage is switched off permanently, with the reason
recorded in the file. It flags 29 functions, 13 of which are symbols
currently exported by libniftiio, libfslio and libcifti. Applying its
fix-it would delete them from the shared libraries. That it looks like a
tidy-up is exactly what makes it dangerous, so the file says so next to
the check rather than leaving the next person to rediscover it.
Every disabled check carries its finding count and the reason, so the
list can be re-argued from evidence instead of taste.
(cherry picked from commit 159d11d)
Add cmake/nifti_warnings.cmake with the warning flags the tree is already at zero under, so that a warning means a new defect rather than more noise. Fourteen flags are enabled: thirteen common to GCC and Clang, plus -Wcomma on Clang. Verified with NIFTI_WARNINGS_AS_ERRORS=ON in both the default and the USE_CIFTI_CODE/USE_FSL_CODE/FSLSTYLE configurations: zero compiler warnings. The flags that are wanted but not yet earned are recorded in a FUTURE SET comment block with their measured hit counts and the census command that produces them. They are promoted one at a time, each only after the change that fixes its warnings has landed, so CI stays green at every step. The GCC-only and MSVC branches are left empty: the GCC block has never been enabled anywhere so its counts are unknown, and no workflow builds on Windows at all. Co-Authored-By: Gabriel A. Devenyi <3001850+gdevenyi@users.noreply.github.com> (cherry picked from commit 596441b)
Run the Build and Test workflow on this repository's branch. Its
trigger named "main" while the branch is "master", so it had never
executed on master and did not execute on pull requests; valgrind,
AddressSanitizer, UndefinedBehaviorSanitizer, scan-build, gcov
coverage, and the only shared-library configuration were all
unexercised. Also move its macOS entry off the retired macos-11 image.
Restructure the per-PR matrix around the axes that change what is
compiled:
shared BUILD_SHARED_LIBS gates TEST_INSTALL, so the ON legs are
the first to run install_linking and cover the install
and export path.
fslstyle -DFSLSTYLE is not additive. It rewrites behaviour in
niftilib and nifti2 through global -DFSLSTYLE, -DPIGZ and
-DREJECT_COMPLEX, and one site has an #else that changes
the pixdim[0] value written to disk. Neither value
subsumes the other.
build_type One static Debug leg, which is what makes assert() live.
Drop both macos gcc legs: /usr/bin/gcc there is a clang shim reporting
AppleClang, so they duplicated the macos clang legs exactly. The
optional libraries are built everywhere rather than forming an axis,
since they are additive.
Add three jobs. The minimal configuration builds znzlib and niftilib
alone, which is what a downstream project vendoring the core reader
selects and which nothing else configures. The oldest supported CMake
job guards cmake_minimum_required, invisible to runners that all carry
a recent CMake. The exported symbol baseline diffs the dynamic symbol
set against a committed file, so a change to the ABI has to be updated
in the same commit rather than landing unnoticed.
Verified on Ubuntu 24.04 with GCC 13.3.0, the compiler the Linux
runners use:
shared, Release, FSLSTYLE=OFF 345/345, install_linking passed
static, Debug, FSLSTYLE=ON 344/344, no assertion aborts
minimal configures and builds, no tests
CMake 3.28.3 344/344
The baseline holds 448 symbols across six libraries, which is the
figure the linkage changes have been asserting without a way to check
it.
The workflow steps were exercised by running them directly in the
runner container image rather than through act, whose image omits
cmake.
(cherry picked from commit 77b7418)
The dashboard script restates the CTestConfig.cmake settings because CTest's delayed initialization does not pick them up, and the restated copy had drifted from the original. Submit over https rather than http. my.cdash.org serves plain http without redirecting, so submissions were going unencrypted. Use the 00:00:00 EST nightly start time that CTestConfig.cmake declares, rather than 01:00:00 UTC. The two differ by four hours, so nightly builds were filed under the wrong day on the dashboard. (cherry picked from commit 063e838)
The 2025 conversion to GitHub Actions replaced .travis.yml with two
workflows but left the dashboard script untouched, so it still
required a Travis environment. The workflow that drives it named the
wrong branch and never ran, which is why nothing surfaced the
mismatch.
Rename travis_dashboard.cmake to github_dashboard.cmake and take its
inputs from the runner that actually exists. CTEST_SITE now comes from
RUNNER_OS rather than the required TRAVIS_APP_HOST, whose absence
aborted the script before it configured anything, and the build name
carries RUNNER_OS in place of TRAVIS_OS_NAME.
Compare the branch name against its value rather than against the
literal string "ENV{BUILD_SOURCEBRANCHNAME}", so the Continuous and
Nightly models can be selected at all, and give the workflow a branch
name on push events as well as on pull requests.
Apply the same corrections to local_dashboard.cmake.
(cherry picked from commit f578e03)
FslGetHdrImgNames and FslSetIntensityScaling are defined here but declared nowhere, while FslGetIntensityScaling and FslInit are already published. axml_recur_find_xml sits beside axml_recur in afni_xml.h the same way. The upstream fslio was deleted in 2015 in favour of a C++ replacement, so this copy is the surviving one and its header is ours to correct. Additive; no symbol changes. (cherry picked from commit 4673c40)
Nine functions in fslio.c have external linkage and no declaration in any header. Nothing in the tree calls them across a translation unit, AFNI's vendored copy never calls them, and no public source outside a vendored copy of this file references them, so a caller would have had to declare them itself. Collect their prototypes in one block so the boundary between internal and published is visible in one place. FslSetVoxUnits and FslGetVoxUnits have no caller at all and are left under #if 0 rather than deleted; FslSetTimeUnits and FslGetTimeUnits are published, so the asymmetry is worth keeping visible. Removes nine symbols from libfslio, which the baseline records. (cherry picked from commit 8cdcf1a)
nifti1_io.c and nifti2_io.c each define nifti_fileexists with external linkage, so libniftiio and libnifti2 export the same name and ELF link order decides which one a caller linking both resolves to. Declaring it locally silences the warning without choosing between publishing it and making it static; that choice needs the duplication settled first, and it covers a hundred names, not this one. (cherry picked from commit 54e74e1)
Keeps FslGetAuxFile and FslSetAuxFile deriving the same length so the pair cannot drift if the field width changes. sizeof is 24, so the copy still writes at most 23 bytes. (cherry picked from commit 0c0e66c)
strncpy writes no terminator when the source fills the destination, so the copy is safe only because of the line that follows it, which is what -Wstringop-truncation reports. Copying at most sizeof(dest) - 1 makes the call safe on its own and takes the size from the buffer rather than a literal repeated twice. (cherry picked from commit 5c1665c)
act_add_exts() in both nifti_tool and nifti1_tool reads the extension data from a file into edata and, if nifti_add_extension() then fails, returns without freeing it. The success path a few lines below frees it already. Found by clang's static analyzer, on a path the test suite does not reach. (cherry picked from commit ab2d71c)
…ibute
nifti_image_from_ascii() walks the attributes of an ASCII header and
assigns the two filename fields with
nim->fname = nifti_strdup(rhs) ;
nim->iname = nifti_strdup(rhs) ;
Nothing stops a header from carrying header_filename or image_filename
twice, and nothing rejects the repeat, so the second assignment drops
the first string. The input comes from a file, so the leak is
attacker-controlled in size and count: one copy per repetition.
Direct leak of 5 byte(s) in 1 object(s) allocated from:
#0 malloc
#1 nifti_strdup nifti2_io.c:1301
#2 nifti_image_from_ascii nifti2_io.c:8874
Freeing before the assignment costs one call and keeps the last value,
which is what the function already documented by overwriting. The
fields are NULL until the first assignment, and free(NULL) is defined,
so no other path changes.
The same two lines exist in nifti1_io.c and are fixed there too.
Found by fuzzing nifti_image_from_ascii() with clang's libFuzzer under
AddressSanitizer.
(cherry picked from commit b4c6894)
act_mod_hdrs(), act_mod_hdr2s() and act_swap_hdrs() -- five functions
across the two tools -- read a header, then, when -prefix is given,
duplicate the dataset before writing the modified header back. Each of
the three ways that duplication can fail returns without freeing the
header, and the last of them also loses the strdup'd duplicate name:
nhdr = nt_read_header(fname, &nver, &swap, 0, ...);
...
if( opts->prefix ) {
nim = nt_image_read(opts, fname, 1, 1);
if( !nim ) { fprintf(...); return 1; } /* nhdr */
if( nifti_set_filenames(nim, opts->prefix, 1, 1) ) {
nifti_image_free(nim); return 1; /* nhdr */
}
dupname = nifti_strdup(nim->fname);
if( nifti_image_write_status(nim) ) {
nifti_image_free(nim); return 1; /* nhdr, dupname */
}
}
...
free(dupname);
free(nhdr);
The normal path frees both. Reproduced by pointing -prefix at a
directory that cannot be written:
nifti_tool -mod_hdr -prefix <read-only dir>/anat1 -infiles anat0.nii \
-mod_field qoffset_x -17.325
before: definitely lost: 348 bytes in 1 blocks
after: ERROR SUMMARY: 0 errors from 0 contexts
Separately, act_diff_nims() in both tools releases the first image with
free(nim0) when reading the second one fails. That is a shallow free: it
loses nim0's fname, iname and any data or extensions. It now calls
nifti_image_free() like the success path six lines below.
Found by running the test suite under valgrind. On the normal paths the
suite is clean -- 484 traced processes, no invalid access, no
uninitialised value and no leak in any nifti binary -- so these are
error-path defects that the tests do not otherwise reach.
(cherry picked from commit 40009a9)
Three -Wsign-compare warnings, each comparing a signed value against an
unsigned one, where the signed operand is silently converted and a
negative value would compare as enormous.
nifti1_tool.c, nifti_tool.c, fill_cmd_string()
`len < 0 || len >= remain`, len an int, remain a size_t. The
`len < 0` test short-circuits first, so the conversion could not
actually misfire, but the comparison relies on that ordering to be
correct. Made explicit, matching the idiom used a few lines above.
nifti_tool.c, read_file_text()
`bytes != len64`, size_t against int64_t. len64 is validated as
> 0 and <= INT_MAX immediately above, so the cast is lossless.
(cherry picked from commit 7b37e32)
Two of the five Build and Test jobs have never run to completion. sanitize-clang-linux invokes scan-build, which lives in clang-tools and was not installed, so the job exits 127 before configuring. rel-clang-macos asks brew for "sed", which is not a formula; brew fails the step and the job exits 1 before configuring. The GNU sed the dashboard scripts expect is gnu-sed. Both predate the workflow's first successful run, so neither has regressed; the trigger named the wrong branch until recently and the jobs never executed. (cherry picked from commit 3147438)
loc_strnlen measures a string that need not be terminated, but it dereferences before testing the bound, so when no NUL appears in the first maxlen bytes the last iteration reads str[maxlen]. Both callers pass an unterminated buffer: axml_read_buf takes the caller's buffer and its length, and axml_read_file measures what fread returned, which fills the buffer for any larger file. AddressSanitizer on a buffer sized to its content reports a heap-buffer-overflow read 0 bytes after a 324-byte region. The returned length is unchanged wherever the old order was in bounds. (cherry picked from commit 7476a12)
The only file in the tree without a trailing newline, and the only -Wnewline-eof warning. C11 5.1.1.2p1 requires a non-empty source file to end in a newline not immediately preceded by a backslash, so this is undefined behaviour rather than only a diff annoyance. (cherry picked from commit 91d1bb9)
…rter
nifti_convert_n2hdr2nim() uses nhdr.dim[0], the number of dimensions,
as a loop bound over the eight-element dim[] array:
for( ii=2 ; ii <= nhdr.dim[0] ; ii++ ) ...
for( ii=nhdr.dim[0]+1 ; ii <= 7 ; ii++ ) ...
for( ii=1 ; ii <= nhdr.dim[0] ; ii++ ) ...
It never checks that dim[0] is in [0,7]. The NIFTI-1 converter gets
that check for free, because need_nhdr_swap() rejects a dim[0] outside
[1,7] in either byte order, but the NIFTI-2 path decides swapping from
sizeof_hdr and reaches the loops with whatever the file said.
A NIFTI-2 header with a large negative dim[0] therefore starts the
second loop at a wild negative index, reads far outside the header and
segfaults. This is not confined to the header API: nifti_image_read()
gets there for any file with a valid 540 byte NIFTI-2 header, so a
604 byte file crashes nifti_tool:
$ nifti_tool -disp_nim -infiles bad_n2_dim0.nii
nifti2_io.c:5079: runtime error: index -6727636073941130588 out of
bounds for type 'int64_t[8]'
AddressSanitizer: SEGV ... in nifti_convert_n2hdr2nim
dim[0] is now range checked in the same place, and in the same style,
as the dim[1] check just below it. Zero stays acceptable, as it is on
the NIFTI-1 side. Valid headers are unaffected: dim[0] outside [0,7]
has no meaning in either format.
Found by fuzzing nifti_convert_n2hdr2nim() with clang's libFuzzer under
AddressSanitizer.
(cherry picked from commit 5e32002)
Both header converters multiply the dimensions into nim->nvox without
checking the product:
for( ii=1, nim->nvox=1; ii <= nhdr.dim[0]; ii++ )
nim->nvox *= nhdr.dim[ii];
Seven NIFTI-1 dimensions of 32767 are enough to overflow int64_t, and a
NIFTI-2 header needs only two dimensions to do it:
nifti2_io.c:4838: runtime error: signed integer overflow:
1152780773560811521 * 32767 cannot be represented in type 'int64_t'
Signed overflow is undefined, and what the compiler does produce is a
voxel count that no longer describes the file. nvox then goes on to
nifti_get_volsize(), which multiplies it by nbyper for another
unchecked product, and that result is used as an allocation size and a
read length.
Both products are now checked before they are made, in the way the
surrounding code already reports a bad header. A header that overflows
is rejected with a message instead of producing a silently wrong image.
No valid image is affected: an int64_t voxel count is more than any
file can hold.
Found by fuzzing the header converters with clang's libFuzzer under
UndefinedBehaviorSanitizer.
(cherry picked from commit 66c76c2)
The first commit guards both converters in nifti2/nifti2_io.c.
niftilib/nifti1_io.c has the same loop in nifti_convert_nhdr2nim(), on
the same untrusted path, and was left unguarded.
$ nifti1_tool -disp_nim -infiles huge.nii # dim[0]=7, dim[1..7]=32767
dim 32 8 7 32767 32767 32767 32767 32767 32767 32767
nvox 64 1 -1073512449
The wrapped count then becomes the data allocation size and the result
of nifti_get_volsize(). With the guard the header is refused:
** ERROR: nifti_convert_nhdr2nim: dim[] overflows the voxel count
nifti_image.nvox is a size_t in this library and an int64_t in the
NIFTI-2 one, so the bound here is SIZE_MAX rather than INT64_MAX.
nifti_get_volsize() is size_t * size_t to match. dim[0] is already
bounded by need_nhdr_swap(), and the loop that raises every dim to at
least 1 still runs first, so the dim[ii] > 0 test never has to reject.
Two more nvox loops remain in each library, in
nifti_update_dims_from_array() and update_nifti_image_for_brick_list().
Both take dims that a caller has already set rather than dims read from
a file, and reach them from nifti_tool's command line, so they are left
for a separate change.
(cherry picked from commit 147a07a)
…agic claims
swap_nifti_header() takes a void pointer and a version number, and picks
the struct to swap from the version:
if ( ni_ver == 0 ) nifti_swap_as_analyze((nifti_analyze75 *)hdr);
else if( ni_ver == 1 ) nifti_swap_as_nifti1((nifti_1_header *)hdr);
else if( ni_ver == 2 ) nifti_swap_as_nifti2((nifti_2_header *)hdr);
Two callers pass it a nifti_1_header, 348 bytes, together with the
version taken from that header's own magic string by NIFTI_VERSION().
A file whose magic is "n+2" therefore has 540 bytes swapped in place:
192 bytes of read and write past the end of the caller's stack object.
nifti_read_n1_hdr() reads the header straight from a file, so the bytes
that decide this come from the file being read. A 348 byte file with
sizeof_hdr = 348, dim[0] byte-swapped, magic = "n+2"
is enough; need_nhdr_swap() then reports that swapping is needed and the
overflow happens before any validity check. AddressSanitizer:
ERROR: AddressSanitizer: stack-buffer-overflow
READ of size 1 ...
#0 nifti_swap_4bytes nifti2_io.c:3051
#1 nifti_swap_as_nifti2 nifti2_io.c:3194
#2 nifti_read_n1_hdr nifti2_io.c:5394
Address ... is located in stack of thread T0 at offset 412 in frame
[64, 412) 'nhdr' (line 5339) <== Memory access ... overflows
nifti_convert_n1hdr2nim() has the same line and takes its header from
the caller, so any application that fills a nifti_1_header itself can
reach it too.
Both call sites know the struct they hold, so both now ask for the swap
that struct supports: analyze when the magic is absent, NIFTI-1
otherwise. Headers claiming versions 3 to 9, which swap_nifti_header()
previously refused with a message and left unswapped, are now swapped as
NIFTI-1 as well, which is the only interpretation 348 bytes allow.
nifti1_io.c is not affected: its swap_nifti_header() takes a
nifti_1_header * and a flag, so it cannot choose a wider struct.
Found by fuzzing nifti_convert_n1hdr2nim() with clang's libFuzzer under
AddressSanitizer.
(cherry picked from commit 16a6b67)
nifti_tool.c has the same defect this branch fixes in the library: it
passes NIFTI_VERSION(*nhdr), read from the magic string, to
swap_nifti_header(), which selects the struct to swap by that number.
act_mod_hdrs() and act_swap_hdrs() hold a nifti_1_header of 348 bytes.
A magic of "n+2" makes NIFTI_VERSION() return 2, so swap_nifti_header()
treats the allocation as a 540 byte nifti_2_header and reads and writes
past its end. Both functions refuse a header that is valid NIFTI-2, but
a header that is valid as neither version reaches the call.
$ printf ... > evil.nii # 348 bytes, sizeof_hdr byte swapped,
# magic "n+2"
$ nifti_tool -mod_hdr -mod_field descrip hello -overwrite \
-infiles evil.nii
==2747364==ERROR: AddressSanitizer: heap-buffer-overflow
#0 nifti_swap_4bytes nifti2_io.c:3029
#1 nifti_swap_as_nifti2 nifti2_io.c:3172
#2 swap_nifti_header nifti2_io.c:3131
#3 act_mod_hdrs nifti_tool.c:3389
0 bytes after 348-byte region allocated in nifti_read_n1_hdr()
Both now pass 1, or 0 for ANALYZE, which are the two 348 byte layouts.
act_mod_hdr2s() holds a nifti_2_header and gets the explicit 2, as
act_swap_hdrs() already does for its own NIFTI-2 display path.
old_swap_nifti_header() takes a nifti_1_header and a boolean, so the
calls beside these need no change.
(cherry picked from commit 522e624)
Under FSLSTYLE the block that reports "Multiple possible filenames" has
three defects, and they cannot be separated.
free(basename); <- freed here
char *gzname = calloc(...); <- unchecked
strcpy(gzname, hdrname); <- used at once
...
fprintf(stderr,"... %s\n", basename); <- read after free
exit(134); <- kills the caller
basename is read by the message, so it cannot be freed at the top.
gzname goes straight into strcpy(), so it has to be checked. And once
the function returns NULL instead of calling exit(), every path out has
to release what it holds, which is what ties the three together: moving
the free of basename decides what the error paths must free, and there
are no error paths to speak of until exit() is gone.
FSLSTYLE is off by default but is a supported configuration:
-DFSLSTYLE:BOOL=ON turns it on, and nifti2/Makefile defines it always.
Built and tested with -DFSLSTYLE=ON.
The nifti_image_read() half of the original change is a separate commit.
(cherry picked from commit 0ddf192)
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'".
(cherry picked from commit b8d5246)
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. (cherry picked from commit 06cb5a6)
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. (cherry picked from commit e96d1e9)
…atype The REJECT_COMPLEX block reads nim->datatype, and the test for nim being NULL sits immediately below it, so a header the converter refuses crashes the process in builds that define REJECT_COMPLEX. The refusal itself is correct and already reported; only the order is wrong. Found by the malformed-header tests added in this branch: the guards make the converter return NULL for exactly the inputs they cover, which is the case this block never handled. (cherry picked from commit f79d234)
A dim[0] outside 1..7 is used to index dim[] in nifti_convert_n2hdr2nim, so a header carrying a large negative value reads far outside the struct. The fixture is a 604-byte NIFTI-2 header holding such a value; without the check the tool segfaults in an ordinary Release build, so this test guards on every CI job rather than only the sanitizer one. (cherry picked from commit 2f6742c)
nifti_read_n1_hdr reads 348 bytes, so swapping the buffer as whatever width its magic names reaches past the end when the magic says n+2. The fixture is a big-endian NIFTI-1 header wearing an n+2 magic, which the sanitizer legs report as a stack-buffer-overflow without the fix. -check_hdr is the seam that reaches it; -disp_hdr does not. (cherry picked from commit e3990de)
act_mod_hdrs() and act_swap_hdrs() hold a 348-byte nifti_1_header and swapped it at the width its magic named, reading past the end for an n+2 magic. Both reuse the fixture added for the library-side check, and both rewrite their input, so each test runs against its own copy. (cherry picked from commit 3deb735)
Three fixtures, because the two guards are reached separately: a product that wraps nvox, the NIFTI-2 form of the same, and one whose nvox fits while nvox * nbyper does not. Without the guards the tool accepts all three and reports a negative nvox. The regex pins which refusal fired; an exit-code test would not tell this rejection apart from any other error. (cherry picked from commit 01ce2fa)
nifti_convert_nhdr2nim carries its own copy of the guards, so it needs its own tests; reverting the NIFTI-1 half alone leaves the nifti2 tests green and turns exactly these two red. (cherry picked from commit 329e869)
nifti_image_from_ascii overwrote fname and iname without releasing what they already held, so an ASCII header naming either attribute twice leaked the earlier copy. One fixture drives both libraries. The regex asserts the last value survives, so a fix that ignored the repeat instead of freeing the previous value could not pass. (cherry picked from commit d0b9413)
loc_strnlen read one byte past the buffer it was given. The reachable caller is axml_read_buf, where the buffer is an extension's edata, allocated esize-8 and filled exactly; axml_read_file allocates one spare byte and absorbs the overread, so it is not the seam. The fixture is a NIFTI-1 image carrying a CIFTI extension whose payload is space-padded to hold no NUL. The overread is reported by the sanitizer legs; the build stays green without them. (cherry picked from commit 457f8bf)
The tool writes both members of the pair itself, so no data is needed. The driver asserts exit 1 and the diagnostic, because a library reports the clash to its caller rather than ending the process, and a plain add_test cannot tell that exit from the abort it replaced. It then removes the .gz and repeats, so an unconditional NULL return could not pass. Guarded on FSLSTYLE_NAME_CONFLICTS, which is off by default, so the test is registered only where the code is compiled. (cherry picked from commit 7a0b98b)
Two paths returned without releasing the header, and one without releasing the duplicated name: a write into a directory that refuses it, and a duplication that fails because the input is the other NIfTI version. Both are reachable from the shipped tools with no data beyond what -make_im writes. The script asserts only that the paths are reached and reported; the leak is what the memcheck and sanitizer legs see. It skips itself for root, whom a read-only directory does not stop. (cherry picked from commit c051fd8)
c22_copy_image compares by content because gzip output is not reproducible across zlib implementations, but that property was only exercised where the system zlib is zlib-ng. Every CI leg ships stock zlib, so the comparison could revert to comparing compressed bytes and the suite would stay green. Extracting the helper lets it be tested directly. gzip -1 against -9 manufactures the byte difference on any zlib, and the two negative assertions pin that differing content is still reported as different and that the uncompressed path still works. (cherry picked from commit b5023c5)
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 #80, reverted from
masteron 2026-09-24. Content isunchanged from the original.
Position 1 of 11 in the deep stack. Base:
master.Stack order
stack/test-regression-coverage<- this PRmasterstack/pr-fix-alloc-null-checksstack/test-regression-coveragestack/fix-axml-skip-depthstack/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.