cc-wrapper: add fortify flags after invocation args, not before

[risicle]

Description of changes

An alternative to #246067, attempting to address issues raised in #224822

This splits hardeningCFlags into hardeningCFlagsAfter and hardeningCFlagsBefore (where most flags still remain) to allow us to append -D_FORTIFY_SOURCE= values to the command-line, forcing our choice of fortify level and avoiding potential redefinition warnings/errors through use of -U_FORTIFY_SOURCE.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

cc @emilazy

[emilazy]

If we go down this route, would it make sense to move more (or even all) of these to the end, so that the UI is a consistent "hardening flags are forced and must be overridden out-of-band"?

[ghost]

👍

Building:

• x86_64-linux
• powerpc64le-linux
• aarch64-linux (cross from x86_64-linux)
• mips64el-linux mixed n32/64 (cross from x86_64-linux)
• pkgs.tests.cross.sanity

[emilazy]

We should be able to revert a bunch of the fortify3 disables now, right?

[risicle]

If it works as intended and we don't have to revert this

[yu-re-ka]

Had to add hardeningDisable = [ "fortify3" ]; for pkgsMusl.thunderbird-unwrapped to build on staging-next

[risicle]

Out of interest what error were you getting without that?

Also, is it only required for pkgsMusl?

[yu-re-ka]

I still have to debug a bit further to fully understand what's happening.

One error was undefined symbol: __builtin_va_arg_pack

[emilazy]

I think that musl does not implement _FORTIFY_SOURCE and you need to get the functions from a separate library. Though I don't know why that'd only be a problem with fortify3; maybe we have some logic to disable fortification on musl that this is overriding, or maybe musl added the fortify stuff but isn't up to date?

[risicle]

Also recently merged: #219421

[risicle]

Was it one of the calls in /nix/store/...-fortify-headers-1.1alpine1/include/stdio.h ?

[emilazy]

Also recently merged: #219421

Ah, I knew I'd seen something related to this recently. I guess it's more complex than just "fortify3 is on by default but the fortify compatibility library for musl doesn't support it", then?

[risicle]

It might take me a while to build all the musl build-deps for thunderbird. If you're able to narrow it down to a test case to add to #217390 that would help things along.

[risicle]

Yup, it's in /nix/store/5702fsdnzk6a6ibb5679chgsqcg6hmv3-fortify-headers-1.1alpine1/include/stdio.h, guessing an snprintf call. Could be related to this apparently using clang. Will dig further in the morning.

[risicle]

https://clang.llvm.org/docs/UsersManual.html#gcc-extensions-not-implemented-yet

clang does not support __builtin_va_arg_pack/__builtin_va_arg_pack_len.

Having tested this, it's actually #219421 that's the culprit. The build fails in a similar way if the cc-wrapper makes no attempt to alter _FORTIFY_SOURCE.

Suggest we disable the fortify-headers for musl/clang until we figure out how best to selectively omit the sprintf/snprintf hardening that causes the problem.

[risicle]

Ok I think I have a fix, it moves the compilation of pkgsMusl.thunderbird-unwrapped forward to an error stating:

/nix/store/jmrg5dkmya3nrgscl291pwfhnrwwzi28-gcc-12.3.0/include/c++/12.3.0/stdlib.h:59:7: error: no member named 'moz_xcalloc' in namespace 'std'; did you mean '::moz_xcalloc'?
using std::calloc;
      ^~~~~
/build/thunderbird-115.1.1/mozobj/dist/include/mozilla/mozalloc.h:80:16: note: '::moz_xcalloc' declared here

Was this a pre-existing error?

[yu-re-ka]

Sorry I did not provide the complete information.
I also already found the fix for the __builtin_va_arg_pack error, which was simply updating the fortify-headers to the latest alpine patch version.
I'm now bisecting to find the source of the moz_xcalloc errors (they are new (in the last two staging cycles)).

[risicle]

Oh good.

My vague theory with the moz_xcalloc errors is that it's not a clang vs. fortify-headers thing, instead it's just that mozilla are themselves doing some screwy (and potentially brittle) stuff with #include_next (see thesystem_wrappers and stl_wrappers), and the fortify-headers unexpectedly importing stdlib.h in wchar.h:26 breaks things.

This makes me wish there were a nice way to omit the fortify headers on a package-by-package basis. Ideally they would omit themselves if _FORTIFY_SOURCE is 0 or undefined, but that's not how things currently work.

[accelbread]

Hey, with this change, how would we disable FORTIFY_SOURCE for just one invocation? tpm2-pytss is broken since it needs to disable fortify for running the preprocessor when parsing types, but it doesnt disable fortify package-wide as it builds objects which should have fortify.

[risicle]

The ultimate thing that controls the behaviour of the cc-wrapper is the NIX_HARDENING_ENABLE environment variable. However I'd be careful playing with it too much because its default value has already had all the platform/toolchain-specific disablements/tweaks applied to it. (edit: not quite true)

Suggest grepping nixpkgs for examples of its (rare) use. libffi for instance selectively disables fortify by doing:

NIX_HARDENING_ENABLE=${NIX_HARDENING_ENABLE/fortify3/}
NIX_HARDENING_ENABLE=${NIX_HARDENING_ENABLE/fortify/}

(note the order of the two is important there, and consider why)

However, that being said, doing a hardeningDisable = [ "fortify" ] doesn't necessarily totally disable fortify. It stops the nix cc-wrapper from applying extra flags, but (currently) the cc-wrapper doesn't explicitly disable any fortify flags the package adds itself. And if the package is already micro-managing which files need fortify and which don't, it's probably doing a good job of successfully enabling it for all the other files.