Skip to content

COMP: Enable _FORTIFY_SOURCE in optimized builds - #34

Closed
gdevenyi wants to merge 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fortify-source
Closed

gdevenyi wants to merge 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fortify-source

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

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=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

Configuration and documentation

Defects

Warning and check classes

This was referenced Aug 15, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Rebased onto current master (b4876bf) and pushed as 758c8a2. No test added: this is a compiler-flag change with no runtime seam a ctest can observe.

Maintainer decision needed: _FORTIFY_SOURCE aborts the process on a violation. This repository has a standing rule that a library must not kill its host — a fortification abort inside libniftiio takes down whatever application linked it, with no chance to return an error. As written the flag is added with add_compile_options(), so it applies to this build only and is not exported to consumers via usage requirements, which keeps the blast radius to binaries built from this tree. Worth deciding explicitly whether the default should stay ON for the installed library build, or whether hardening of this kind belongs only in the test/CI configurations.

What changed in this update
  • Rebased onto master; one conflict in CMakeLists.txt, resolved on the merits — master added include(nifti_warnings) on the same line this PR adds include(nifti_hardening), and both belong.
  • Commit message: dropped the Co-Authored-By: naming an AI tool and the Claude-Session: URL; replaced the bare #24 cross-reference with words (this is a fork, so the number names a different PR here); trimmed the body to fit the 72-column/12-line convention.
  • US English: optimised/optimisation/optimiser → optimized/optimization/optimizer, in both the commit message and cmake/nifti_hardening.cmake.
  • Trimmed the 27-line comment banner at the head of cmake/nifti_hardening.cmake to 11 lines. The removed text argued against an alternative approach and restated the commit message; that belongs in this PR body, not in the source.
Build and test

Release, NIFTI_BUILD_APPLICATIONS=ON USE_NIFTI2_CODE=ON USE_CIFTI_CODE=ON USE_FSL_CODE=ON, Ninja, macOS/AppleClang:

-- Performing Test NIFTI_HAVE_FORTIFY_3 - Success
-- Using _FORTIFY_SOURCE=3 for optimized builds
...
100% tests passed, 0 tests failed out of 362

@hjmjohnson hjmjohnson changed the title COMP: Enable _FORTIFY_SOURCE in optimised builds COMP: Enable _FORTIFY_SOURCE in optimized builds Sep 22, 2026

@seanm seanm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seanm

seanm commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

This is the low-churn way to get what upstream PR #24 is after.

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.

_FORTIFY_SOURCE is indeed great, and running Ci with it would be nice, but I think that belongs in CI, not within the library itself.

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.

@hjmjohnson hjmjohnson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @seanm. As part of CI, but not an implicit part of the library.

@gdevenyi

Copy link
Copy Markdown
Author

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.

@gdevenyi gdevenyi closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants