BUG: Check the allocations whose result is used immediately - #55
hjmjohnson merged 3 commits into
Conversation
|
Pushed a second commit. The error return this PR added to It now propagates. Worth recording why the Note this code is behind |
124701e to
67a160d
Compare
67a160d to
34085f2
Compare
34085f2 to
f8b9662
Compare
|
Rebased onto Why the splitAll four fslio hunks report the allocation failure through /* fsliolib/fslio.c:46 */
#define FSLIOERR(x) { fprintf(stderr,"Error:: %s\n",(x)); fflush(stderr); exit(EXIT_FAILURE); }So the change adds four new The PR's own reasoning for using
So "just return instead of exiting" is not uniformly available, and where it is, it changes published signatures or contracts. That is the decision I am asking for, and it is bigger than this PR: it is whether Nothing is lost by holding them — the current code null-dereferences on allocation failure, and so would the What is on the branch nowTwo commits, both clean:
Test: leak fix proven, null checks not demonstrableThe leak is proven. LeakSanitizer is not available on this platform ( With the fix, on the same input: I did not register this as a ctest. It needs a leak checker to observe, the project has no leak-checking test infrastructure, and a test that runs The null checks are NOT-DEMONSTRABLE. Every one of them fires only when Message cleanup and buildRemoved the Build: |
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.
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.
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.
f8b9662 to
a1e0d8c
Compare
|
Rebased onto current Why the fsliolib hunks were removed rather than rewrittenThey reported failure through #define FSLIOERR(x) { ... ; exit(EXIT_FAILURE); }and this project's standing position is that a library returns errors rather than terminating its host — it is vendored into ITK, AFNI, FSL, dcm2niix and Slicer, where an Rewriting them in place is not uniformly possible: Worth knowing before anyone takes that on: roughly 50 call sites are written as braceless The leak, and a test that catches it
Nothing reached that path, so I measured rather than assumed: with the fix reverted, the suite still reported 362 of 362 passing. The path is reachable from a shipped tool, though, and the leak is observable: So the test is registered. It uses the fixture the ASCII attribute test already carries, read through Red proof, reverting only the The test only bites under a leak checker. A plain Release build is 363/363 with or without the fix, confirmed. It fails on the The allocation checksThree, all in
|
c6410f5
into
InsightSoftwareConsortium:master
2 participants
Nine 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.
fslio.c FslInit() calloc(FSLIO), then FslSetInit()
writes through the pointer
FslGetHdrImgNames() two callocs, then strcpy into both
FslWriteVolumes() calloc of the byte-swap buffer,
then written in the reorder loop
FslClose() calloc(dsr), then FslReadRawHeader()
fills it in
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
fslio.c already uses FSLIOERR to report allocation failure (d3matrix,
d4matrix), so these follow that idiom rather than adding a second
convention to one file. The others return an error the way the failure
paths beside them 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.
Deliberately not included, to avoid overlapping open PRs:
both functions in both libraries;
FslReadAllVolumes() and FslReadHeader() -- Fixed some warnings from the new cppcheck 2.19 #23 removes them.
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