BUG: Fix signed/unsigned comparisons in the nifti tools - #51
Conversation
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.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KSPnbwpDjVcAYqDdVqLkMU
01d0529 to
fd56ac7
Compare
|
Rebased onto current
With this branch, the same build gives 0 warnings and 345/345 tests passing. The casts are value-preservingEach comparison is between a
No input changes behaviour; the casts make the conversion the compiler was performing implicitly visible at the call site. What this unblocks
Together with #78 this takes all five |
e039528
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
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. Thelen < 0test short-circuits first, so the conversion could notactually 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.
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