BUG: Give i and j a defined value in nifti_mat44_to_orientation

[gdevenyi]

nifti_mat44_to_orientation() and its dmat44 twin end with

*icod = i;  *jcod = j;  *kcod = k;

where i, j and k are each assigned only inside a switch whose default arm
is assert(0). Under NDEBUG -- which is to say in every release build --
that arm compiles to nothing, so an unexpected switch value falls through
and the function writes an indeterminate value into the caller's
orientation code. k was already declared k=0, so somebody hit this
once and fixed a third of it. i and j now match.

Found by clang's -Wconditional-uninitialized.

---

Interface impact: none. On the union of all these changes, configured with USE_FSL_CODE=ON and USE_CIFTI_CODE=ON: all 448 exported symbols across libniftiio, libnifti2, libznz, libfslio, libnifticdf and libcifti are identical to master under nm -D --defined-only, and all ten installed headers are identical under gcc -E -P. Under gcc -dM -E one macro definition differs, intentionally and only in text: #61 makes FSL_RADIOLOGICAL read (-1) so it is safe inside an expression. Its value is still -1, checked by compiling against each installed fslio.h and printing it.

Verification. This branch: builds with gcc 16.1.1, ctest unchanged from master (2 of 345 fail on master itself 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, ctest 345/345 under each, and the whole suite under valgrind memcheck with --trace-children=yes gives 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 in fslio.c for #22, 2 in nifti2_io.c and 2 in nifti_tester001.c for #24) and 8 -Wcalloc-transposed-args in nifti_findhdrname and nifti_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 master and 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

• COMP: Show test output when the starter workflow fails #28 — COMP: Show test output when the starter workflow fails
• COMP: Give install_linking the source directory instead of guessing it #29 — COMP: Give install_linking the source directory instead of guessing it
• COMP: Guard the CMP0169 policy setting so older CMake still configures #30 — COMP: Guard the CMP0169 policy setting so older CMake still configures
• BUG: Compare NIfTI test output by content, not by compressed bytes #31 — BUG: Compare NIfTI test output by content, not by compressed bytes
• COMP: Make the 'Build and Test' analysis workflow run #57 — COMP: Make the 'Build and Test' analysis workflow run

Configuration and documentation

• ENH: Rework the clang-tidy configuration for C11 #32 — ENH: Rework the clang-tidy configuration for C11
• COMP: Add a shared compiler warning set #33 — COMP: Add a shared compiler warning set
• COMP: Enable _FORTIFY_SOURCE in optimized builds #34 — COMP: Enable _FORTIFY_SOURCE in optimised builds
• COMP: Build as ISO C11 with no compiler extensions #38 — COMP: Build as ISO C11 with no compiler extensions
• DOC: Describe the CMake build and how to run the memory checks #58 — DOC: Describe the CMake build and how to run the memory checks
• COMP: Add CI jobs for clang-tidy and for building with -Werror #59 — COMP: Add CI jobs for clang-tidy and for building with -Werror

Defects

• BUG: Fix cdfbin's argument range check, which could never fire #39 — BUG: Fix cdfbin's argument range check, which could never fire
• BUG: Fix an out-of-bounds indirect call in axio_show_mim_summary #40 — BUG: Fix an out-of-bounds indirect call in axio_show_mim_summary
• BUG: Bound the formatted write in znzprintf, and end the va_list on failure #41 — BUG: Bound the formatted write in znzprintf, and end the va_list on failure
• BUG: Report XML read errors instead of treating them as end of input #42 — BUG: Report XML read errors instead of treating them as end of input
• BUG: Stop returning -1 from functions that return size_t #43 — BUG: Stop returning -1 from functions that return size_t
• BUG: Fix the ambiguous-filename path in nifti_findhdrname #54 — BUG: Stop the library calling exit(), and fix the defects beside it
• BUG: Check the allocations whose result is used immediately #55 — BUG: Check the allocations whose result is used immediately
• BUG: Fix cifti_tool's CIFTI extension search, which never advanced #56 — BUG: Fix cifti_tool's CIFTI extension search, which never advanced
• BUG: Free the extension data when nifti_add_extension fails #62 — BUG: Free the extension data when nifti_add_extension fails
• BUG: Free the header on nifti_tool's duplicate-file failure paths #60 — BUG: Free the header on nifti_tool's duplicate-file failure paths
• BUG: Use memcpy instead of casting to over-aligned pointer types #44 — BUG: Use memcpy instead of casting to over-aligned pointer types
• BUG: Stop casting away const in fslio and cifti #45 — BUG: Stop casting away const in fslio and cifti
• BUG: Bound the aux_file copy by sizeof rather than a repeated 24 #46 — BUG: Bound the aux_file copy by sizeof rather than a repeated 24
• ENH: Make the 64-to-32 bit truncations explicit #52 — BUG: Fix 64-to-32 bit truncations, including a real overflow in fslio
• BUG: Fix signed/unsigned comparisons in the nifti tools #51 — BUG: Fix signed/unsigned comparisons in the nifti tools

Warning and check classes

• STYLE: Add the missing newline at end of nifti1_tool.h #35 — STYLE: Add the missing newline at end of nifti1_tool.h
• ENH: Give the cifti tools' gopt internal linkage #36 — ENH: Give the cifti tools' gopt internal linkage
• COMP: Declare the exported functions that no header declares #37 — COMP: Declare the exported functions that no header declares
• BUG: Give i and j a defined value in nifti_mat44_to_orientation #47 — BUG: Give i and j a defined value in nifti_mat44_to_orientation
• ENH: Zero-initialize the matrix locals clang cannot prove are filled #48 — ENH: Zero-initialise the matrix locals clang cannot prove are filled
• BUG: Give the statement macros a do/while(0) body and drop the stray semicolons #49 — BUG: Give the statement macros a do/while(0) body
• ENH: Pass calloc its arguments in the documented order #50 — ENH: Pass calloc its arguments in the documented order
• BUG: Parenthesize macro arguments and replacement lists #61 — BUG: Parenthesise macro arguments and replacement lists
• ENH: Make the implicit sign conversions explicit #53 — ENH: Make the implicit sign conversions explicit

[hjmjohnson]

Rebased onto master (b4876bf). 362/362 tests pass.

No test added — the defect is not demonstrable at runtime. The default: assert(0) arm is unreachable from any input, so a ctest cannot distinguish the patched and unpatched builds. Details below; the change is still worth taking as hardening plus a real warning fix.

Why no test

The switch selectors are ibest*pbest, jbest*qbest and kbest*rbest. All six are seeded before the search loop:

vbest = -666.0F ; ibest=pbest=qbest=rbest=1 ; jbest=2 ; kbest=3 ;

and the loops only ever reassign ibest/jbest/kbest from i,j,k in 1..3 and pbest/qbest/rbest from p,q,r in {-1,+1}. The products are therefore always in {±1, ±2, ±3} — exactly the six case labels. Every degenerate input (NULL pointers, singular matrix, detQ == 0) returns before the switch.

There is no sanitizer path either: AddressSanitizer does not detect uninitialized reads, and MemorySanitizer is not configured for this project — and would not fire here anyway, since the read never happens.

A test that called nifti_mat44_to_orientation and checked the returned codes would pass identically with and without the fix, so none was written.

Message and comment cleanup

Removed the AI Co-Authored-By: trailer and the Claude-Session: chat URL. The added comment was cut to one line stating why.

The commit body was also corrected: it claimed an unexpected switch value "falls through and the function writes an indeterminate value". That is true of the code shape but not reachable today, and the body now says so rather than overstating the bug.

Build

Release, Ninja, full option set. 362/362 pass. Functional diff unchanged: int i=0,j=0,k=0 in niftilib/nifti1_io.c and in both the mat44 and dmat44 functions in nifti2/nifti2_io.c.

[seanm]

Does the 0 init of i actually affect the warning given? i is unconditionally init to 1 at the start of the for loop, unless I need more coffee.

assert(0) should mean that reaching that branch is impossible. If it's in fact possible, then we could put default: i = 0; there instead, which is probably clearer that it's returning the error value.

[hjmjohnson]

You were right on both counts, and the second suggestion turns out to be better than what the PR did. Reworked to follow it, in f06ee47.

Measured, -Wconditional-uninitialized across the tree:

	warnings
master	11
the previous approach — init at declaration	8
this one — default: assigns	4

The four that remain are an unrelated variable C in nifti2_io.c, present on master and untouched here.

On your first question: yes, the zero-init of i did nothing

You were right that i is unconditionally assigned — for( i=1 ; i <= 3 ; i++ ) leaves it defined on every path, and the compiler sees that.

I checked rather than guessed. Of the three warnings the old approach removed, all three were j, none was i:

nifti2_io.c:2807  variable 'j' may be uninitialized when used here
nifti2_io.c:2990  variable 'j' may be uninitialized when used here
nifti1_io.c:2138  variable 'j' may be uninitialized when used here

So i=0 was silencing nothing, and k=0 was already on master. The declaration was carrying two initialisers that did no work and one whose reason was three screens away from the code that needed it.

On your second: assert(0) is right, and the default should still assign

The assert(0) is correct — that arm is unreachable. ibest and jbest and kbest are seeded to 1, 2, 3 and only ever reassigned from the loop counters, which run 1..3; pbest, qbest, rbest are seeded to 1 and only reassigned from p, q, r, which are -1 or +1. So the switch selector is always in {±1, ±2, ±3} — exactly the six case labels.

But assert(0) is nothing under NDEBUG, so in a release build the fall-through leaves the variable holding the loop counter that last wrote it — 4, after for(i=1;i<=3;i++) exits — which is not an orientation code. Valid codes are 1..6 and there is no NIFTI_UNKNOWN, so 0 is the natural out-of-range value, as you suggested.

Both are kept: the assert still fires in a debug build, and the assignment makes the release path defined.

default: assert(0) ; i = 0 ; break ;

One thing your suggestion also reached that the old approach missed

nifti2_io.c's nifti_mat44_to_orientation() — the float sibling of nifti_dmat44_to_orientation() — has the same three switches but with a bare default: break; and no assert. Initialising at the declaration silenced it; assigning in the default arm needed that function handled too, which is why this touches nine switches rather than six.

That is also why the count lands at 4 rather than 5.

Verification

Rebased onto current master first, as its own push, then this change on top.

• Release, USE_CIFTI_CODE=ON USE_FSL_CODE=ON, applications on: 367/367
• Diff is 9 changed lines, no insertions at declarations, no comments added