Undefine _FORTIFY_SOURCE only on what's needed

[dotnwat]

Seastar cmake build contains

target_compile_options (seastar
  PUBLIC
    -U_FORTIFY_SOURCE)

Which disables FORTIFY_SOURCE on all Seastar targets and is inherited by anything depending on Seastar.

This PR undefines this only on src/core/thread.cc which is apparently the only thing that needs it, and avoids leaking this out to dependents.

[dotnwat]

This is added in a separate commit that we can drop.

It's only real purpose here is to demonstrate that the tests pass with _FORTIFY_SOURCE=2 while being undefined only on src/core/thread.cc.

[tchaikov]

this works if we use a single-configuration generator. but it does not work if we use a multi-config generator to generate the rules. if we use

cmake -G "Ninja Multi-Config" 

to generate build.ninja, we cannot get the "current" build mode via CMAKE_BUILD_TYPE anymore. as this variable is determined at the configure time. but with multi-config generator, one can pick which configuration at build time, like:

cmake --build build --config Dev --target seastar

so, i'd suggest use the generator expression, which is expanded to different value specific to each build configuration, so we could use something like:

set(Seastar_RELEASE_BUILD_TYPES "Release;RelWithDebInfo")
target_compile_definitions (seastar
  PRIVATE
    $<$<IN_LIST:$<CONFIG>,${Seastar_RELEASE_BUILD_TYPES}>:-U_FORTIFY_SOURCE)

set(Seastar_FORTIFY_BUILD_TYPES "Debug;Sanitize;Dev")
target_compile_definitions (seastar
  PRIVATE
    $<$<IN_LIST:$<CONFIG>,${Seastar_FORTIFY_BUILD_TYPES}>:-U_FORTIFY_SOURCE;-D_FORTIFY_SOURCE=2)

in which,

1. use PRIVATE so we don't force the parent project's to use the same fortification level as seastar does
2. instead of using add_definitions() use target_compile_definitions(), more specific this way
3. use the generator expression as explained above.
4. disable fortification when building release build to restore the behavior of release builds, as suggested in my review of the previous commit.
5. enable fortification when building debug builds to detect the potential security issues.

[dotnwat]

@tchaikov i'm not suggesting that we leave this here, it is merely for testing. do you want fortify sources enabled?

[tchaikov]

@dotnwat okay. i am inclined to agree with you that we should leave it to the user of library to decide what fortification level they want to use or even disable it. so let's drop the 2nd commit.

do you want fortify sources enabled?

if you are asking in the context of this specific PR. i think we don't have to be opinioned here. if, in general, the proposed change reflects what i have in my mind. i'd like to include it in the cmake-based projects i am working on.

[xemul]

To educate myself.
Before the patch FORTIFY_SOURCE was off for the whole seastar. Now it's off for thread.cc only, is that correct?
If so, do we expect any performance penalties once it's enabled for most of the seastar?

[tchaikov]

To educate myself. Before the patch FORTIFY_SOURCE was off for the whole seastar.

yes, it's undefined for seastar, and also for the projects which use seastar via its .pc file and those which use seastar via its CMake config files.

Now it's off for thread.cc only, is that correct?

correct.

If so, do we expect any performance penalties once it's enabled for most of the seastar?

i think so. as the fortified version of glibc function do perform extra checks. i think, the next question would be "how much?". as an application can always override the level of the fortification level at the compile time using something like -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3. so the answer might depend on what the compile options are used.

if the overhead is not negligible, or we don't have a benchmark to justify the change, i think the safe move is probably to

1. keep this -U_FORTIFY_SOURCE private and apply it only to src/core/thread.cc as in this commit
2. add this option back globally in Seastar as a private option when compiling in release builds.

[tchaikov]

sorry, i haven't read the 2nd commit. so it has been implemented. thanks!

[xemul]

2nd commit does this

+set(FORTIFY_BUILD_TYPES "Release;RelWithDebInfo;Dev")
+if(CMAKE_BUILD_TYPE IN_LIST FORTIFY_BUILD_TYPES)
+  add_definitions(-D_FORTIFY_SOURCE=2)
+endif()
+

I see the final result as -- set FORTIFY_SOURCE=2 for Release build. Is level 2 considered to be "no penalty"?

[tchaikov]

no, i don't think so. it incurs less penalty than level 3. to be on the safe side, i think we should undefine this macro for Release and RelWithDebInfo. as to Dev, we can enable level 2 or even level 3.

[dotnwat]

If so, do we expect any performance penalties once it's enabled for most of the seastar?

Just to clarify, I think to some extent this is the choice of the user of Seastar or even the build system (e.g. Bazel turns this on by default. I didn't observe CMake doing so), and doesn't necessarily need to be Seastar making this decision. I only added the second commit which turns on FORTIFY_SOURCES as a demonstration, that when turned on across the code base except for thread.cc, that things still work. Having FORTIFY_SOURCES enabled by Seastar is a decision for Seastar dev.

[avikivity]

I agree Seastar should leave it to the application and not change defaults.

Can we just add #undef (plus comment explaining why) to the source file?

[tchaikov]

@dotnwat hi Noah, do you plan to continue working on this PR? i still think _FORTIFY_SOURCE is a nice feature to identify runtime issues. and the first commit would suffice. seastar applications can opt out of this feature if desired.

[dotnwat]

@tchaikov yes, thanks for the ping. I can push an update tomorrow. But please feel free to cherry-pick the first commit if you want.

[dotnwat]

@dotnwat hi Noah, do you plan to continue working on this PR? i still think _FORTIFY_SOURCE is a nice feature to identify runtime issues. and the first commit would suffice. seastar applications can opt out of this feature if desired.

@tchaikov I dropped the second commit.

[tchaikov]

thank you Noah!

[tchaikov]

lgtm

[tchaikov]

@scylladb/seastar-maint hello maintainers, could you help review / merge this change?