BUG: Use memcpy instead of casting to over-aligned pointer types - #44
hjmjohnson merged 1 commit into
Conversation
a2d2af0 to
441054e
Compare
7dd1e42 to
1ea2295
Compare
1ea2295 to
48c682d
Compare
48c682d to
956dccd
Compare
|
Rebased onto No regression test applies — the UB is latent, not reachable in tree. Details below. Message cleanupRemoved the Also dropped the paragraph referring to the unmerged upstream attempt; that relationship belongs in the PR description rather than in permanent history. Why there is no red-green test (NOT-DEMONSTRABLE)Unaligned access is real UB but invisible on x86-64 and arm64, so the only observer is UndefinedBehaviorSanitizer. I built the tree without this fix at No test trips. The reason is structural, not a gap in coverage:
A test could only fail by deliberately handing The fix still stands on its own: it removes 19 Build and testRelease, Same 362 pass with the fix applied under Whitespace churn check: |
19 -Wcast-align warnings, and behind them undefined behavior on any
target that cares about alignment.
modify_field() writes a value into a header field at a byte offset
parsed from a field table:
((short *)((char *)basep + field->offset))[fc] = (short)val;
field->offset is a byte offset into a packed on-disk header, so that
cast produces an address only correctly aligned by coincidence. The
same pattern appears for int, int64_t, float and double, in both tool
files. Each becomes a memcpy of the right width at the right byte
offset.
The second group reads a pointer back out of a structure through a
byte offset -- `sp = *(char **)((char *)str + fp->offset)` and the
nifti1_extension equivalents -- and becomes a memcpy into an aligned
local.
The third is nifti_header_version(), which cast its `const char * buf`
argument, a buffer straight off a file read with no alignment
guarantee, to both nifti_1_header * and nifti_2_header * and read
fields through them. It now copies into aligned locals first, exactly
the sizeof(nifti_1_header) bytes the function already checks are
present.
Verified by round-tripping int16, int32, int64, float32, float64 and
string fields through nifti_tool -mod_hdr2.
956dccd to
81c2953
Compare
eb4f831
into
InsightSoftwareConsortium:master
2 participants
19 -Wcast-align warnings, and behind them undefined behaviour on any
target that cares about alignment.
modify_field() writes a value into a header field at a byte offset parsed
from a field table:
field->offset is a byte offset into a packed on-disk header, so that cast
produces an address only correctly aligned by coincidence. The same
pattern appears for int, int64_t, float and double, in both tool files.
Each becomes a memcpy of the right width at the right byte offset. This
is the fix attempted in NIFTI-Imaging#172, which was never
merged and still carries assert(0) and "// TEMP" scaffolding; it is
re-derived here cleanly.
The second group reads a pointer back out of a structure through a byte
offset --
sp = *(char **)((char *)str + fp->offset)and thenifti1_extension equivalents -- and becomes a memcpy into an aligned
local.
The third is nifti_header_version(), which cast its
const char * bufargument, a buffer straight off a file read with no alignment guarantee,
to both nifti_1_header * and nifti_2_header * and read fields through
them. It now copies into aligned locals first, exactly the
sizeof(nifti_1_header) bytes the function already checks are present.
Verified by round-tripping int16, int32, int64, float32, float64 and
string fields through nifti_tool -mod_hdr2.
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