Modernize: uniform style, C11, a warning set held at zero, and the memory-safety defects that fell out

[gdevenyi]

Modernizes the codebase: a uniform code style enforced by CI, an ISO C11 build, a warning set the tree holds at zero under both gcc and clang, and the memory-safety defects that fell out of turning all that on.

It is 40 commits, one change type each, meant to be read commit by commit. Each one is also a separate PR in my fork if that is easier to review in slices: gdevenyi#1 has the index. GitHub cannot stack PRs across repositories, so upstream it has to arrive as one.

What it does

Commits
1	removes nifticdf (see the caveat below)
2	fixes the CI trigger
3–5	.clang-format, the reformat, .git-blame-ignore-revs
6–8	C11, a shared warning set, a reworked .clang-tidy
9–31	one warning or defect class per commit
32	CI jobs for clang-tidy and -Werror

Real defects fixed, not just warnings silenced

• MIM_disp_funcs[mind] guarded by if (kid >= 0) in axio_show_mim_summary(). kid is the loop counter and never negative, so the guard is always true; get_map_index() returns -1 for any element name not in MIM_kids[]. A CIFTI file with an unrecognised element under MatrixIndicesMap reads a function pointer from before the table and calls it.
• int offset in FslSeekVolume() holds a byte position into the image file. Any volume past 2GB overflows it, and znzseek() takes a 64-bit offset.
• Use-after-free in nifti_findhdrname() under -DFSLSTYLE: basename is freed, then read by the fprintf that reports the ambiguous-filename error. Same block had an unchecked calloc, a leak of hdrname, and exit(134).
• nifti_image_read() under -DREJECT_COMPLEX tested nim->datatype before the if (nim == NULL) check immediately below it.
• Alignment UB in modify_field() — 8 sites writing through ((short *)((char *)base + offset)) where offset is a byte offset into a packed header. Now memcpy.
• i and j uninitialised in nifti_mat44_to_orientation() — set only inside a switch whose default is assert(0), which disappears under NDEBUG. k was already fixed; the other two were missed.
• Nine allocations dereferenced without a NULL check, two leaks, an unclosed stream, and a search loop in cifti_tool that never advanced its pointer.
• Division by zero in FslReadSliceSeries() and FslReadRowSeries() — both divide by swapsize, which nifti_datatype_sizes() sets to 0 for DT_UINT8, DT_INT8 and DT_RGB24. Nothing stops a header declaring an 8-bit datatype with the opposite byte order.
• 32-bit overflow throughout fslio's size arithmetic — size_t volbytes = xdim * ydim * zdim * wordsize; and size_t FslGetVolSize(...) { return nx * ny * nz; } do the multiply as int and widen only the result, so the destination type bought nothing past 2GB.
• znzread(), znzwrite() and nifti_read_buffer() return -1 from size_t functions, so an error reaches the caller as SIZE_MAX. Callers happen to detect it by comparing against the requested count, but the documented contract is one the return type cannot express. Made explicit, not changed — fixing it properly is a contract change.
• Stack buffer overflow in doPigz()/doPigz2() — a shell command is built in char command[768] with strcat(command, nim->fname) and handed to popen(). PATH_MAX is 4096, so an ordinary deep path overruns it. PR ENH: code review of recent nifti_image_conflicted_names PR #11 grows the buffer to 4096, which does not fix it; this bounds the write instead.
• Unbounded vsprintf() in znzprintf(), into a buffer sized strlen(format) + 1000000 with the comment /* overkill I hope */. Now vsnprintf, and the va_list is ended on the allocation-failure path it previously leaked.

Verification

At the tip, for default, -DUSE_FSL_CODE=ON, and -DFSLSTYLE=ON (PIGZ), each under gcc 16.1.1 and clang 22.1.8 — six configurations, with the full warning set including -Wsign-conversion:

• builds clean with -DNIFTI_WARNINGS_AS_ERRORS=ON
• 92/92 tests pass

Also: valgrind reports 0 errors and 0 bytes lost across 91 tests with --trace-children=yes (which matters — without it valgrind only inspects the shell wrappers and reports a falsely clean run); ASan+UBSan with -fno-sanitize-recover is clean.

The reformat commit is verified by comparing compiled output rather than by reading a 58k-line diff: 14 of 16 object files are byte-identical before and after at -O2 -g0, and the two that differ are both nifti_tester001.c.o, whose PrintTest macro expands __LINE__ — disassembly shows the only differing instructions are immediates decoding to exactly the shifted line numbers.

External interface

Unchanged throughout, apart from the deliberate removal in commit 1. Every commit was checked with nm -D over all five shared libraries, the preprocessed installed headers, and gcc -dM -E over every installed macro. Across all 32 commits the only macro text that differs is FSL_RADIOLOGICAL, which becomes (-1) instead of -1; its value is unchanged, verified by compiling against the installed header.

This is why misc-use-internal-linkage is disabled in .clang-tidy with the reason recorded next to it: it flags 29 functions, 13 of which are exported symbols, and its fix-it would delete them from the shared libraries.

Two things to decide

Commit 1 removes libnifticdf, nifticdf.h and nifti_stats. That is a real interface change and a maintainer call, which is why it is first and standalone. It does not affect AFNI, SPM, MINC, MIRTK, BROCCOLI or Slicer — all of them vendor their own copy of nifticdf.c rather than linking the library, and ITK vendors only niftilib and znzlib. It does affect anyone linking -lnifticdf or using the NIFTI::nifticdf CMake target. Say the word and I will drop that commit and rebase the rest.

-Wsign-conversion is opt-in rather than fixed. Now fixed properly — the flag is in the default warning set and the tree holds at zero under it. All 225 findings are cleared across six commits, split by what the conversion actually does: fslio's size and offset arithmetic, the voxel-count and brick-size products, allocation sizes, copy lengths, read/write lengths, and the remaining offsets and counters.

The audit is the interesting part. Tracing the invariants showed most of these conversions are provably safe rather than latent bugs: nifti_update_dims_from_array() rejects dim[0] outside [1,7] and clamps every dim[i] < 1 up to 1, nifti_datatype_sizes() leaves nbyper at 0 for an unknown datatype and every caller checks it, and nifti_image_load() refuses to proceed unless nbyper > 0 && nvox > 0. So the casts document invariants the code already enforces. Each commit message says which invariant it is relying on.

Four real defects did fall out of it, described below.

Relationship to the open PRs and issues

• Closes Make indentation uniform; add .editorconfig file #10 — .clang-format plus .editorconfig, uniform indentation, enforced by CI.
• Supersedes WIP: added .editorconfig file, applied uncrustify to make indentation uniform #12. @hjmjohnson asked there for clang-format over uncrustify to stay in sync with ITK; this uses ITK's .clang-format with six documented overrides.
• Subsumes the defect classes behind More clang-tidy fixes #22 and Fixed some warnings from the new cppcheck 2.19 #23, and the exit() removal that is the point of ENH: code review of recent nifti_image_conflicted_names PR #11.
• Overlaps Update a couple of functions to know buffer length #21 and conflicts with WIP: Replace dangerous old string functions with less horrible variants, for -fbounds-safety compatibility #24; see the comments on those.

On the string functions (upstream PR #24)

I audited every strcpy/strcat call rather than replacing them wholesale. Two were genuine defects and are fixed above. The other 145 are correct: 86 copy into a buffer allocated as strlen(source) + slack a few lines above, 10 write into a fixed array that fits exactly (extcopy[8] takes name + len - 7; the elist[] entries are the char[8] buffers whose declaration comment says "(leave space for .gz)"), and the rest are literals into much larger struct fields.

So instead of rewriting 145 correct calls, this enables _FORTIFY_SOURCE (level chosen by compiling a test, applied only to optimised builds). The C library then checks the destination size of every one of those calls wherever the compiler can determine it — including the run-time-known strlen(x) + n sizes this library uses — and aborts rather than overflowing. Verified it catches the doPigz defect above.

That complements PR #24 rather than replacing it: fortification catches a bad size at run time, strlcpy and -fbounds-safety aim to make the bound explicit at compile time. If #24 lands, this stays useful. Note also that #24 defines strlcpy/strlcat at global scope on glibc < 2.38, which claims names the C library reserves, and its #if defined(__GLIBC__) guard omits the fallback entirely on musl and MSVC.

Sorry for the size — the reformat cannot be small, and everything after it is deliberately one class per commit so the interesting parts are reviewable on their own.

[seanm]

Boy LLMs sure are good at generating text! But this is way too large to review. No one has even reviewed small PRs that have been open for months. Generating walls of text is easy these days, human review is in short supply.

...so upstream it has to arrive as one.

"has to"? I don't see why one PR per thing cannot be created.

As to all the formatting changes, even though I've advocated for them, I'm not actually convinced anymore it's a good idea, as it will make merging with upstream harder. At least, it's probably better to do them last, after fixing actual bugs, so that upstream, should it re-activate, can more easily cherry pick them.

At least the commits seem generally well divided. I suggest picking one commit, starting with the most important and/or simplest, and create a PR for it. Then human reviewers can digest it. Then create another PR, and repeat.

[gdevenyi]

"has to"? I don't see why one PR per thing cannot be created.

I was trying to use https://docs.github.com/en/pull-requests/get-started/about-stacked-prs but it seems you can't do that for PRs on repos that aren't yours.

As to all the formatting changes, even though I've advocated for them, I'm not actually convinced anymore it's a good idea, as it will make merging with upstream harder.

OK, I'm dropping all the clang-format work.

[seanm]

I was trying to use https://docs.github.com/en/pull-requests/get-started/about-stacked-prs but it seems you can't do that for PRs on repos that aren't yours.

It's quite a new feature I believe; perhaps not very capable yet.

It looks like there's a lot of nice stuff here, but it's just overwhelmingly huge.

I have the impression you are gungho on this new tool :), but perhaps a little more coordination is advisable before going too far too fast?

OK, I'm dropping all the clang-format work.

Maybe @hjmjohnson will also chime in on formatting changes... upstream still has no commits since 1.5 years, but there was a bit of activity in an Issue recently. If upstream is moribund forever, then perhaps this fork will essentially be the new upstream. I had hoped when creating this fork that this could be a proving ground, and eventually integrated into upstream too, but not sure that will ever be...

[gdevenyi]

@seanm @hjmjohnson I rebased all these fixes, minus any formatting changes into individual decoupled PRs.

[seanm]

@seanm @hjmjohnson I rebased all these fixes, minus any formatting changes into individual decoupled PRs.

I'm just back from vacation and will try to take a look at them little by little...