BUG: Bound the aux_file copy by sizeof rather than a repeated 24 - #46
Conversation
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.
4452889 to
6f45656
Compare
|
Rebased onto current With #37 in, this is the last GCC warning in the default configuration: Commit message trimmedThe body narrated the previous form of the code and used a British spelling. It now states only what the change is for: The getter half landed in #37
VerificationChecked on macOS with AppleClang after the rebase: builds clean, 345/345 tests, no warnings. AppleClang does not implement |
33ac84e
into
InsightSoftwareConsortium:master
|
The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: This PR's commit on the rewritten
The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves. |
2 participants
FslSetAuxFile() copied with strncpy(dest, src, 24) into a char[24] and
then wrote the terminator at [24-1]. That is correct, but only because
of the second line: strncpy() writes no terminator when the source is 24
characters or longer, which is what -Wstringop-truncation points at.
Copy at most sizeof(dest) - 1 and terminate at sizeof(dest) - 1, so the
call is self-evidently safe without depending on the line after it, and
the buffer size comes from the buffer rather than a literal repeated in
two places. Follows the direction of commit 7b08ead, "Use sizeof()
instead of repeating raw constant".
Behaviour is unchanged for every input.
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