NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.46k stars 13.66k forks source link

GCC has unwanted flags #18995

Open andrewrk opened 7 years ago

andrewrk commented 7 years ago
andy@xps:~/tmp$ NIX_DEBUG=true gcc
(some good flags for dealing with /nix/store/* and then...)
  -O2
  -D_FORTIFY_SOURCE=2
  -fstack-protector-strong
  --param
  ssp-buffer-size=4
  -fPIC
  -fno-strict-overflow
  -Wformat
  -Wformat-security
  -Werror=format-security

These flags should be enabled when compiling nix packages, but they should not be always on for general compiler use.

This has caused a broken debugging experience for me on the project I was working on.

Most projects have a "debug build" that has optimizations off, and this prevents that from working correctly.

NixOS version 16.09pre90254.6b20d5b (Flounder)

andrewrk commented 7 years ago

I believe this was caused by #12895

andrewrk commented 7 years ago

Not only that, but these flags are in the extraAfter section, which means I can't even override the flags, for example with -Og in a Makefile or build script.

andrewrk commented 7 years ago

One workaround is putting

    hardeningDisable = [ "fortify" ];

in a nix-shell. Since I was already using a nix-shell for my environment, this is an acceptable workaround. Still, seems weird to assert -O2 by default and have to disable it to get previously expected behavior.

abbradar commented 7 years ago

I think this can be fixed by adding a special flag, NIX_ENFORCE_HARDENING, that is only set in stdenvs. This way the flags won't be enforced in regular gcc invocations and could be disabled in shells by setting this flag to 0.

BTW having -O2 always added is not nice because some applications might want -O3 or -Ofast (which would be overridden). We may want to make this conditional somehow...

rasendubi commented 7 years ago

/cc @globin @fpletz

fpletz commented 7 years ago

You can use

hardeningDisable = [ "all" ];

to disable all hardening flags. This should also work if set as an environment variable.

We will work on a solution after 16.09 using gcc spec files that can detect for example if libraries are built or debugging is enabled. This was just our first iteration and we agree that it is not perfect. Changes to the cc-wrapper always require a full rebuild, which is very painful. More feedback on #12895 would've been helpful.

We didn't anticipate and test that those flags would be propagated to regular gcc invocations outside of nix builds. Not entirely sure how to fix it yet but @abbradar's proposal sounds reasonable. I will look into it after 16.09.

-O2 is required for -D_FORTIFY_SOURCE=2. Not sure though if higher optimizations also work.

abbradar commented 7 years ago

FWIW I found that level 1 fortifying requires inlining:

As I said in that post, the special fortified functions (those that are available in the form of __$func_chk in the libc.so file and provide warnings at build time, and proper stack traces at runtime) only get enabled if inline functions are enabled, so are totally ignored at -O0 (simply disabling inlines, by using -fno-inline won’t stop them from being used, though).

I haven't been able to find anything except of anecdotal evidence that -O2 is required for level 2 (I'm not questioning your choice, rather I was trying to find a list of optimizations that are required so that we can enable them individually).

fpletz commented 7 years ago

(I'm not questioning your choice, rather I was trying to find a list of optimizations that are required so that we can enable them individually).

I'm always open for suggestions. Thanks! :smiley: We'll have to try that.

abbradar commented 7 years ago

Unfortunately it may need peeking into rat nest GCC/glibc's codebases to determine this. Meanwhile I think it's okay to leave -O2.

copumpkin commented 7 years ago

Just as a quick reminder when you talk about gcc: there's also clang to consider on Darwin (and in some cases on Linux too)

jxy commented 7 years ago

So this is why recently gdb suddenly tells me different things in my code and gcc no longer does a good job optimizing my code. I almost thought gcc 5.4 had optimization regressions.

How do I get back a reasonable working gcc? Which nixpkgs version should I revert to?

Does this mean I should put hardeningDisable = [ "all" ]; in all of my custom nix files for my HPC production code, eg, mpich3?

I'm using nixpkgs on a ubuntu without root privilege.

It is NOT okay to leave -Oanything in extraAfter!

andrewrk commented 7 years ago

We're all in agreement here @jxy and going to fix it soon. This is why it's called unstable!

The workaround you mentioned is working for me and is probably the reasonable thing to do until this is fixed.

jxy commented 7 years ago

Right, thanks for the workaround. It wasn't easy to find this thread. I guess I'll need some performance regression tests after changing my system.

jxy commented 7 years ago

Does NixOS 16.09 release version have this issue? If so, I'll put off upgrading until this is fixed.

vcunat commented 7 years ago

Yes, both 16.09 and unstable/master.

fpletz commented 7 years ago

I'm currently working on a fix.

andrewrk commented 7 years ago

even though I knew about this, I temporarily forgot and it caused me to file a bogus issue report on another project: https://github.com/thejoshwolfe/legend-of-swarkland/issues/36

mboisson commented 7 years ago

I'm running into this issue when compiling older versions of GCC.

gcc 4.8 compiles fine, but at run time, it tries to use stack-protector-strong, which is not supported.

I've wasted about 8 hours so far trying to fix this broken thing.... [mboisson@build-node easybuild-easyconfigs]$ gcc --version gcc: erreur: unrecognized command line option ‘-fstack-protector-strong’ gcc (GCC) 4.8.5

Our fork of nixpkgs is based on 16.09 and is here: github.com/computecanada/nixpkgs

is there any commit I could pull to fix this ?

globin commented 7 years ago

There is no optimal fix yet, workaround is still to export hardeningDisable=all in your shell while developing.

mboisson commented 7 years ago

Thanks.

Ralith commented 7 years ago

This cost me untold hours of frustration; eventually I just gave up getting anything useful out of a debugger on NixOS. Eagerly looking forward to a fix.

It's important to me that building nix packages with useful debug info (i.e. without -O2) even with nix-build (rather than nix-shell) is possible as well. For example, debugging software that uses LLVM is much easier with a debug build of LLVM which I'd rather not have to make by hand.

viric commented 7 years ago

What about not adding any hardening if IN_NIX_SHELL=1 ? What is bad in this?

Ralith commented 7 years ago

That would be a huge improvement, but would still make it more difficult than necessary to build nix packages with debug symbols. IMO no -O flags should be supplied (let alone in extraAfter!) if debug symbols are requested by the derivation.

edolstra commented 7 years ago

It's not desirable to have nix-shell behave very different from a regular build. The whole point of nix-shell after all is to get an interactive build environment that is otherwise identical to a regular Nix build.

Not supplying -O when debug symbols are requested doesn't sound right either. I do optimized builds with debug symbols all the time... After all, it's pretty useful to be able to get meaningful results out of a production core dump.

However: The real issue here is that add-hardening.sh adds -O2 after the user's CFLAGS. So you might think you're building with -O3, but you're not. That's a pretty critical bug...

A possible fix would be for add-hardening.sh to not add -O2 if any previous -O flag was seen.

globin commented 7 years ago

Just a note if this is not clear, -D_FORTIFY_SOURCE=2 needs at least -O2 to work, but yes generally -O3 should not be overwritten. But I don't think we should not stop adding the -O2 if we see a lower optimisation level or -g because that will remove the hardening on all derivations that have a broken build environment and that are exactly those that need hardening more than others.

edolstra commented 7 years ago

@globin It's probably not a big deal if it overwrites -O1, but it should not overwrite -O0. Maybe the best thing to do in that case is to exit with an error message informing the user that there is a conflict between optimization and hardening flags.

globin commented 7 years ago

@edolstra -O1 disables some checks -O0 all of them (only talking about the fortify stuff).. I think the problem here is that we can't check if the build just defaults to setting one of those which we do want to override or if a user has specified it explicitly e.g. in nix-shell.

joachifm commented 7 years ago

Has anybody looked at basing the hardening logic on an existing wrapper, e.g. https://github.com/thestinger/hardening-wrapper?

rszibele commented 7 years ago

This also caused me countless hours of frustration from being unable to determine the cause of not being able to debug any of my programs through any debugger on NixOS. I was also puzzled why a simple test program with ~10 LOC compiled with "g++ -g -O0 main.cpp" kept jumping around during debugging.

I'll be using export hardeningDisable=all until this is fixed as it seems to work just fine, though it might be a good idea to insert a warning when using g++ while this is still an issue so others wont have to waste time trying to figure out why their debugging just does not work as expected.

musteresel commented 7 years ago

Why are the hardening flags added after the user specified flags after all? Why not add them before, to make them override-able? https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/cc-wrapper.sh#L124

mboisson commented 7 years ago

Why add them period ? If the end-user wants to use hardening flags, he or she can add them.

cstrahan commented 7 years ago

Rather than have the cc-wrapper require opt-out, how about we do this: make hardening opt-in, but we'll make the stdenv do the opting-in by default. We could have add-hardening.sh look for something like $NIX_CC_HARDENING_ENABLE, which would have all the non-disabled flags handed off from the Nix expression side -- the idea being that this should require no changes to the API of stdenv.mkDerivation. If that sounds good, I could try to whip up a PR over the next week or two (a little too busy to get to it at the moment) -- or someone else can take a stab at it now, if they'd like.

lambdafu commented 7 years ago

In GnuPG we use the kernel's jitter entropy gatherer, which does not compile when OPTIMIZE is defined. I spent quite a bit of time tracking down why -O0 does not work as expected, and finally found this issue. This is really unconventional.

cstrahan commented 7 years ago

So, 5 thumbs up on my last recommendation. I'll create a PR over the next day or two, and then hopefully we can finally call this resolved.

If anyone thinks this isn't a good solution, please tell me now.

Ralith commented 7 years ago

It's not immediately obvious to me how that gives the correct behavior (i.e. no involuntary -O2) for nix-shell. Also, as @edolstra said:

However: The real issue here is that add-hardening.sh adds -O2 after the user's CFLAGS. So you might think you're building with -O3, but you're not. That's a pretty critical bug...

Wouldn't the proposed change fail to address the root problem, while also being more complex than just supplying flags in the correct sequence in the first place? I don't think -O2 by default is a problem, so long as explicit user-provided -O flags aren't overridden.

cstrahan commented 7 years ago

It's not immediately obvious to me how that gives the correct behavior (i.e. no involuntary -O2) for nix-shell.

Here are my thoughts:

If I install gcc as a systemPackage in NixOS, I shouldn't then have to stuff export hardeningDisable=all; in my profile to ensure that I get the expected result when I'm building with e.g. my IDE. In reallity, it's more complicated than that, as there are many scenarios where env vars are dropped (e.g. sudo), and I'd have to work around each of those cases.

However, for nix-shell, this isn't a bug -- things are working as intended. As @edolstra says:

It's not desirable to have nix-shell behave very different from a regular build.

If you want to use nix-shell simply to bring a bunch of dev tools/libraries into your environment, and not necessarily because you want to replicate the build environment of a package, then the solution is straight-forward: add hardeningDisable = "all"; to your shell.nix. Here's (a rather minimal) one from a recent project:

with import ~/src/nixpkgs { };

let
  # from TARGET_RECIPES
  deps = [
    c-ares
    backward-cpp
    libevent
    pythonPackages.gcovr
    gtest
    gperftools
    http-parser
    /* lightstep */
    nghttp2
    /* libnghttp2 */
    protobuf3_2
    tclap
    rapidjson
    spdlog
    boringssl
  ];

in

runCommand "dummy" {
  hardeningDisable = "all";
  buildInputs = [
    bazel
  ] ++ deps;
  shellHook = ''
  '';
} ""

I suspect most people are reporting here because they're running into issues with the gcc wrapper installed globally, rather than within nix-shell. To prevent future headaches for nix-shell users, we should provide a warning in the docs where nix-shell is introduced.

Also, as @edolstra said:

However: The real issue here is that add-hardening.sh adds -O2 after the user's CFLAGS. So you might think you're building with -O3, but you're not. That's a pretty critical bug...

I'll also try to address that too, thought I haven't decided how exactly. Perhaps I can make the logic be: if passed -ON where N < 2, add the -O2; else leave the commands as is. Thoughts?

First, off to bed ;)

Ralith commented 7 years ago

It's not desirable to have nix-shell behave very different from a regular build.

The current behavior is just as wrong in a regular build as it is in a nix-shell. If any -O is passed explicitly, it should not be clobbered.

Perhaps I can make the logic be: if passed -ON where N < 2, add the -O2; else leave the commands as is. Thoughts?

The only thing that needs to be done is to move the injected options before, rather than after, the user-supplied options, so that the user-supplied options will correctly override them. As has been discussed at length above, overriding -O2 with lower optimizations is an immensely important capability so as to generate debuggable binaries.

edolstra commented 6 years ago

We got bitten at work by this again today, i.e. a debug build at -O0 silently got changed into -O2. This really needs to be fixed since this issue has been a blocker for the last two releases.

How about this: cc-wrapper adds -O2 only if it hasn't seen a preceding -O flag. If it has seen -O where n >= 2, it does nothing.If is has seen -O0 or -O1, it should fail with an error message like:

cc-wrapper: Hardening is incompatible with -O0. Set hardeningDisable = ["bla"] to disable hardening.

This way hardening can be enabled by default, but we don't get silent optimization with -O0.

(What about -Os?)

vcunat commented 6 years ago

Oh, I had been wondering why my debug builds don't feel like compiled with -O0.

gebner commented 6 years ago

If is has seen -O0 or -O1, it should fail with an error message like:

I agree that it's better to fail loudly instead of silently changing flags. But it won't solve this issue: either way using gcc outside of nixpkgs is broken. nix-shell -p gcc --run "gcc -O0 foo.c" should work and call gcc with the -O0 flag and no further optimization. I really expect this to just work when developing C/C++ code.

IMHO stdenv and the standalone gcc/clang attributes should use different wrappers. stdenv should have hardening enabled by default, and fail with -O0 as you suggested. The standalone attributes should use the minimal wrapper that is necessary to compile programs, but not enable hardening. In this setup, you can still use nix-shell -A to troubleshoot a regular build since it would use the stdenv wrapper.

vcunat commented 6 years ago

-O0 silently decreasing hardening might be "unexpected" as well, but outside nix build/shell it seems OK to be without hardening by default. The wrapper would better be the same; stdenv may e.g. set some specific variable like we have IN_NIX_SHELL already.

edolstra commented 6 years ago

@gebner Yes, I guess hardeningDisable should be hardeningEnable. I.e. if that variable (which would be set by default in stdenv) is not set, you don't get hardening so -O0 works as expected outside of a Nix build or nix-shell. Inside a nix-shell hardening would still be enabled by default, but you would get an error when using -O0.

Ralith commented 6 years ago

@vcunat

outside nix build/shell it seems OK to be without hardening by default

Silently breaking -O0 inside nix-shell is not okay. Using nix-shell to manage development environments is one of Nix's most compelling features.

Having an explicit gcc/clang build input change the behavior would be weird and unintuitive, especially when you want to build a nix package with debug symbols (which is very common when e.g. developing against LLVM). It's not really an improvement on the current state of affairs; in either case, nix silently defeats your efforts unless you know exactly the right magic incantation.

@edolstra

How about this: cc-wrapper adds -O2 only if it hasn't seen a preceding -O flag. If it has seen -O where n >= 2, it does nothing.If is has seen -O0 or -O1, it should fail with an error message like:

Why can't we just place all automatically-injected options before manually supplied ones? This would be massively more intuitive, require less complexity, and protect against similar issues in the future. Yes, there might(!) exist some small number of packages which won't be fully hardened as a result; those packages are broken, and can be fixed.

vcunat commented 6 years ago

@Ralith. ~I didn't mean to claim that at all. I'm sorry for the misunderstanding.~ I'm sorry; I'm too tired.

ElvishJerricco commented 6 years ago

I agree that if the only cost of simply putting the arguments before the supplied ones is that some packages break, that seems like by far the most useful and intuitive approach. The problem is tracking down which packages are broken and verifying that we actually can fix them

vcunat commented 6 years ago

@Ralith: the problem about nix-shell is that some people expect it to be an interactive nix-build and some use it just to bring packages into scope. EDIT: before I managed to finish the thought properly, Gebner did it for me below.

gebner commented 6 years ago

If hardening is only enabled by stdenv, then

AFAICT this would address both usages of nix-shell.

Ralith commented 6 years ago

@vcunat Those aren't mutually exclusive, we just need to make the nix-build semantics sane, i.e. don't clobber explicitly passed options.

@gebner As discussed in my prior comment, that doesn't really improve things. I maintain build environments with a shell.nix containing a derivation and often want debug builds of nix packages, so stdenv very much applies. Adding an explicit build input for a compiler that stdenv would otherwise supply is just another magic incantation required to prevent nix from silently sabotaging me.

vcunat commented 6 years ago

Using mkDerivation from nixpkgs is indistinguishable from a real nixpkgs build. If you want something different, you should explicitly differ "in some way".

Ralith commented 6 years ago

@ElvishJerricco To be clear, respecting explicitly supplied arguments will almost certainly not break any packages, it only risks marginally weaken the hardening applied to any packages in the unlikely event that they have hardcoded -O0 in their makefiles or similar. I'm skeptical that there even exist any packages which do this and aren't for some reason specifically unable to use higher optimization levels, in which case this could actually fix some packages.