BUG: Check the allocations whose result is used immediately - #124
Open
hjmjohnson wants to merge 11 commits into
Open
hjmjohnson wants to merge 11 commits into
hjmjohnson wants to merge 11 commits into
Conversation
doPigz and doPigz2 are called only from the file that defines them and appear in no header, so they are internal. Declaring them so is what lets -Wmissing-declarations be promoted to an error on the FSLSTYLE path, where they were the only offenders. The copy of doPigz2 in the NIFTI-1 library is byte-identical to doPigz beside it and nothing calls it, so it is excluded from the build rather than given linkage it does not need. The exported symbol set is unchanged: this code compiles only under PIGZ, which the shared build behind the baseline does not define. (cherry picked from commit 1abbd85)
-Wmissing-declarations is already in the project's clean set, but a warning in a build that passes anyway is not read, so two changes that added declarations reached master with nothing to stop the next one. Both FSLSTYLE settings are covered. The flag catches nifti_fileexists on one side and axml_recur_find_xml, FslGetHdrImgNames and FslSetIntensityScaling on the other, each of which reached master as a warning nobody acted on. (cherry picked from commit b4876bf)
Twelve source files carry WIN32 or _MSC_VER guards and no workflow has ever compiled them, so a change that breaks the Windows path is invisible here and surfaces only when a consumer reports it. Static and shared, because the ZNZ_API and NIFTI_API decorations differ between them and only the shared build exercises the dllexport path. zlib and expat come from vcpkg, which the runner image already carries. VCPKG_INSTALLATION_ROOT is set by the runner image rather than by the workflow, so the toolchain path is read in PowerShell and checked before cmake runs, which reports a missing toolchain as itself rather than as a CMake error several lines removed from the cause. (cherry picked from commit 2505325)
afni_xml_io.h decorates its declarations with CIF_API and afni_xml.h decorated none of its own, so a Windows shared build produced a DLL missing every axml_ entry point and both cifti tools failed to link against the library they are built with. The macro definition moves to afni_xml.h, which afni_xml_io.h includes at its top, so one definition now serves both headers rather than each carrying its own. Nothing changes where the attribute expands to default visibility: the exported set of the shared build is byte-identical. Found by the Windows job added in the preceding commit. (cherry picked from commit 63b361a)
znzprintf() built its output with vsprintf(), which has no bound; the buffer was sized strlen(format) + 1000000 under a comment reading "overkill I hope". A single %s argument longer than a megabyte writes past the end of the allocation. vsnprintf() with the size already being computed turns that overflow into a reported truncation. The same block also returned on calloc failure without calling va_end(), leaving the va_list unterminated. This is the only unbounded formatted write in the tree; there is no sprintf() and no gets() anywhere. (cherry picked from commit 57a61a7)
The preceding commit bounds the write but still returned the gzprintf count after a truncation, so a caller could not tell. It also returned 0 for an allocation failure, and the function returns 0 for a NULL stream. The printf family reports failure with a negative value; 0 is a successful empty write, so it cannot carry either meaning. All three now return -1, and a truncated result is not written at all: a partial record reported as a whole one is worse than nothing. znzprintf sits inside COMPILE_NIFTIUNUSED_CODE in both znzlib.c and znzlib.h and is absent from libznz.a in a default build, so this is a latent defect in code no released configuration compiles. (cherry picked from commit ac7ec09)
znzprintf has no caller in this tree and is compiled only under COMPILE_NIFTIUNUSED_CODE, so nothing reached it. The test builds its own copy of znzlib.c with that definition and drives a 2 MB argument through the 1000001 byte buffer. Without the bound the write overflows the allocation, and the call reports success; the test pins the negative return, and pins the ordinary write and the resulting file content so an over-broad fix that refuses everything does not pass. (cherry picked from commit 0350b30)
znzflush() and znzeof() are defined inside COMPILE_NIFTIUNUSED_CODE and declared nowhere, unlike znzgets(), znzputc() and znzgetc() beside them. Nothing compiled that block until a test in this branch did, so the omission was invisible. They are declared where the rest of the block is declared, so the warning set the project already requires stays clean when the block is built. (cherry picked from commit 8449a1f)
Three allocations had their result used without a NULL check, in every
case within a line or two, so an allocation failure is a null
dereference rather than an error.
nifti2_io.c nifti_read_n2_hdr() malloc(nifti_2_header), then
nifti_convert_nim2n2hdr() fills it
afni_xml.c new_afni_xml(), strdup results stored straight
axml_add_attrs() into the structure
Each returns an error the way the failure paths beside it do.
One line further on, nifti_read_n2_hdr() released the image it had
just converted with free(nim) rather than nifti_image_free(), leaking
the filename strings nifti_read_ascii_image() had allocated. That is
corrected in the same place.
The four fslio.c allocations in the same class are left out: the
idiom that file uses to report an allocation failure, FSLIOERR, calls
exit(), and whether the library may end its host process is for the
maintainers to decide.
(cherry picked from commit 923e869)
axml_add_attrs() returns 1 when it cannot copy an attribute, but make_afni_xml() ignored the result, so the element was returned with a partial attribute list and nothing said so. Every failure the function reports is an allocation failure, so there is no case where continuing is right. Propagate it: epush() already treats a NULL from make_afni_xml() by entering a skip block, which is what the allocation-failure paths beside it do. axml_free_xml_t() releases what was built. It walks the attributes with attrs.length, which axml_add_attrs() lowers to the pair it failed on, so the half of that pair that was copied is freed and the entries past it are never read. That matters because the two arrays come from malloc(), not calloc(), so those entries hold uninitialized pointers. (cherry picked from commit 0685f7f)
nifti_read_n2_hdr() builds a nifti_image to convert an ASCII header and then released it with free(), which leaves its filename strings behind. No test reached that path, so the suite reported 362 of 362 either way. The fixture is the one the ASCII attribute test already uses, read through -disp_hdr2 rather than -disp_nim so the NIFTI-2 reader is the one exercised. The leak is visible to the memcheck and sanitizer legs; an ordinary build stays green with or without the fix. (cherry picked from commit c6410f5)
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 #55, reverted from
masteron 2026-09-24. Content isunchanged from the original.
Position 2 of 11 in the deep stack. Base:
stack/test-regression-coverage.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-checks<- this PRstack/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.