Update a couple of functions to know buffer length

[seanm]

Without the buffer length, the function can't really be implemented safely. For now, only use the new length parameter in an assert.

[gdevenyi]

Closed PR comments

This overlaps #26 but is not subsumed by it, so flagging the interaction rather than claiming otherwise.

#26 rewrites the same code in modify_field() — the ((short *)((char *)basep + field->offset))[fc] = ... writes become memcpy, because field->offset is a byte offset into a packed header and that cast produces an address only correctly aligned by coincidence. 19 -Wcast-align sites. That is the fix attempted in NIFTI-Imaging#172, which was never merged and still has assert(0) and // TEMP scaffolding in it; #26 re-derives it clean.

What #26 does not take from this PR is the baselen parameter. The reason is your own note — "For now, only use the new length parameter in an assert" — and assert compiles out under NDEBUG, so in a release build the parameter buys nothing. Since modify_field writes at an offset parsed from a field table, the bounds check seems worth having for real rather than only in debug builds. That is a behaviour change on bad input, so I left it out of a warning-fix series.

If you want to take it further, the version I would suggest is the check in the shipped binary rather than an assert, returning an error the way the surrounding failure paths do. Both prototypes live in uninstalled headers, so the signature change is free.

Rebasing this over #26 should be mechanical — the lines it touches are adjacent to, not the same as, the ones #26 changes.

[hjmjohnson]

Rebased onto current master (b4876bf) and the commit message reworded. The diff is unchanged.

Rebase

master's last 44 commits were rewritten on 2026-09-22 (messages only, trees byte-identical), so this branch's base no longer existed and GitHub reported it CONFLICTING. Rebased with git rebase --onto master 443a28b^; the single commit applied without conflict, and git diff master HEAD is byte-identical to the previous git diff <old-base> 443a28b.

Commit message

Subject given the project's ENH: prefix and wrapped to the 78/72 limits. Nothing else was removed; there were no tool trailers or transient URLs.

No regression test — and why

The change is a signature change plus assert(field->offset + field->len <= baselen). Under the default Release build NDEBUG compiles the assert out, so there is no observable behavior to pin. Under Debug the assert can only fire if the static g_hdr*_fields / g_nim*_fields tables disagree with the struct they describe, which is not reachable from the command line — a red proof would require editing the field table itself, which is not what this PR changes. NOT-DEMONSTRABLE.

What is worth recording is that the new bound is live-exercised rather than dead: mod_header_test.sh, c21_c_make_im.sh and mod_header_errpaths.sh all drive -mod_field descrip, i.e. the NT_DT_STRING case the assert guards.

Build and test

macOS/arm64, Ninja, -DNIFTI_BUILD_APPLICATIONS=ON -DUSE_NIFTI2_CODE=ON -DUSE_CIFTI_CODE=ON -DUSE_FSL_CODE=ON:

• Release: builds clean, 100% tests passed, 0 tests failed out of 362.
• Debug (assert active): 100% tests passed, 0 tests failed out of 362.

[hjmjohnson]

Rebased onto current master and pushed as 41fa064. The 4 red checks were real,
and fixing them turned out to mean finishing the change rather than quieting a
warning.

The bounds check did not exist in any shipping build. baselen's only use was
an assert(), and assert compiles away under NDEBUG — which is exactly what
the -O3 release legs use. That is why baselen read as an unused parameter:
coverage-gcc-linux, valgrind-gcc-linux, sanitize-clang-linux and
rel-clang-macos all build with -Wunused-parameter, and the dashboard treats a
warning as fatal. use_prefix-gcc-linux, which passes no extra warning flags, was
the one leg that passed.

What the 2 warnings actually were

Reproduced locally with the rel-clang-macos flag set:

niftilib/nifti1_tool.c:2909:39: warning: unused parameter 'baselen' [-Wunused-parameter]
nifti2/nifti_tool.c:3857:39: warning: unused parameter 'baselen' [-Wunused-parameter]

Exactly the 2 the dashboard counted, in both tool files.

The check is now real, and covers every write path

modify_field() has roughly eight write paths — one per datatype case — and each
writes field->len elements at field->offset. The assert guarded only
case NT_DT_STRING:.

The check now sits ahead of the switch, so one test covers all of them, and it
uses the element size the field table already carries:

/* every case below writes field->len elements at field->offset */
if( field->offset < 0 || field->size < 0 || field->len < 0 ||
    (size_t)field->offset + (size_t)field->size * (size_t)field->len > baselen )
{
   fprintf(stderr,"** field '%s' (offset %d, %d x %d bytes) does not fit "
                  "in a %zu byte structure\n",
           field->name, field->offset, field->len, field->size, baselen);
   return 1;
}

It reports and returns 1 like every other failure in the function rather than
aborting, which also keeps it out of the way of the library-must-not-kill-host
rule. <assert.h> was added by this PR for the assert and is removed with it.

Note the arithmetic differs from the assert's: the assert compared
offset + len against baselen, which is only right when the element size is 1.
For any wider type it would have under-counted the write.

No red-green test, and why none is possible

The condition is unreachable from user input. check_total_size() already
requires each field table's offsets to tile its structure exactly, so no field in
g_hdr_fields, g_nim_fields, g_hdr1_fields or g_hdr2_fields can exceed the
sizeof passed at the two call sites.

So this is hardening against a future edit to those tables, not a fix for a
reachable defect, and there is no red state to observe. Classification: BUILD /
defensive. The instrument that is meaningful here is the warning-flag CI legs,
which is what caught the incomplete version.

Verification

Configuration	Result
Release, cifti + FSL + FSLSTYLE=ON, applications on	370/370
rel-clang-macos flag set (-Wall -Wextra -Wshadow -Wwrite-strings -Wunused-parameter)	0 compiler warnings

The 370 include the mod_hdr tests that exercise modify_field, so the new check
accepts every valid field.

[seanm]

Great, thanks for reviewing this one!

I should get back to my fbound-safety work... it was getting pretty close...

[seanm]

Wait a sec, you changed my code.

Please don't change my code, give me no time to review your changes, then merge it under my name. I am listed as commit author and I do not approve/like the changes you made.

This PR had been open for 9 months, you don't look at it in all that time (that's fine!), then you modify my work and merge it all within 17 hours (that's not fine!).