Conversation
090c71c to
4964fc9
Compare
4964fc9 to
758c8a2
Compare
|
Rebased onto current Maintainer decision needed: What changed in this update
Build and testRelease, |
758c8a2 to
1a727b9
Compare
seanm
left a comment
There was a problem hiding this comment.
Don't think I'm a fan of this one either.
If someone wants to build a library with some compiler-specific options, they should just pass those in C_FLAGS when they build.
I assume this is AI blather, but this is not true at all. #24 aims to remove those old functions because they just aren't supported with -fbound-safety at all.
|
The C library then bounds-checks the string and memory functions wherever the compiler can determine the destination size, and aborts instead of writing past the end. Level 3, which gcc 12+ and clang 15+ support, also covers sizes known only at run time -- the strlen(x) + n shape this library uses throughout its filename construction. The level is chosen by compiling a test rather than by checking versions, since both the compiler and the C library have to support it. It is applied only to optimized configurations because glibc warns if it is defined without optimization. NIFTI_ENABLE_FORTIFY_SOURCE=OFF disables it.
1a727b9 to
0528545
Compare
hjmjohnson
left a comment
There was a problem hiding this comment.
I agree with @seanm. As part of CI, but not an implicit part of the library.
|
Sure. I have discovered since then that most distros already inject these flags into their default builds anyways so much of this is actually redundant anyways. |
The C library then checks the destination size of the string and memory
functions wherever the compiler can determine it, and aborts instead of
writing past the end. Level 3, which gcc 12+ and clang 15+ support, also
covers sizes only known at run time -- the strlen(x) + n shape this
library uses throughout its filename construction.
This is the low-churn way to get what upstream PR #24 is after. That PR
replaces every strcpy()/strcat() with strlcpy()/strlcat() for
-fbounds-safety compatibility; fortification checks the same calls
without editing any of them, and the two complement each other --
fortification catches a bad size at run time, strlcpy and
-fbounds-safety make the bound explicit at compile time.
The level is chosen by compiling a test rather than checking versions,
since both the compiler and the C library have to support it, and it is
applied only to optimised configurations because glibc warns if it is
defined without optimisation. NIFTI_ENABLE_FORTIFY_SOURCE=OFF disables
it.
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