Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

{cc,bintools}-wrapper: fix removal of unsupported hardening flags #38941

Merged
merged 1 commit into from Apr 14, 2018
Merged

{cc,bintools}-wrapper: fix removal of unsupported hardening flags #38941

merged 1 commit into from Apr 14, 2018

Conversation

pbogdan
Copy link
Member

@pbogdan pbogdan commented Apr 14, 2018

Motivation for this change

add-hardening.sh is sourced with nullglob in effect, this apparently causes unset -v hardeningEnableMap["$flag"] to expand to unset -v and the unsupported flags aren't removed as a result, causing build failures like so https://hydra.nixos.org/build/72810060/nixlog/1

I only verified this change by injecting set -x into the bintools wrapper and inspecting the results, and rebuilding a small part of x86_64-{linux,darwin} stdenv.

I'm also far from a bash expert so please review with that in mind.

/cc @Ericson2314 @cstrahan

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

pbogdan referenced this pull request Apr 14, 2018
@Ericson2314
Revert "Revert "Merge pull request #28029 from cstrahan/hardening-fix""
0884027 
This reverts commit 6c064e6, reapplying
PR #28029 which was supposed to have gone to staging all along.
@LnL7
Copy link
Member

LnL7 commented Apr 14, 2018

@GrahamcOfBorg build stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.binutils

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.binutils

Partial log (click to expand)

building '/nix/store/yl5brqial68pmk99fh3sxc591i51j1j7-binutils-wrapper-2.30.drv'...
unpacking sources
patching sources
configuring
no configure script, doing nothing
installing
post-installation fixup
checking for references to /build in /nix/store/kbwd3n1wvimd86bnpkbb7ma68ldfyrws-binutils-wrapper-2.30...
Using dynamic linker: '/nix/store/fx00n1d1s16rrlir6n7pb3ip7wkqxx0f-bootstrap-stage0-glibc/lib/ld-linux-x86-64.so.2'
/nix/store/kbwd3n1wvimd86bnpkbb7ma68ldfyrws-binutils-wrapper-2.30

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.binutils

Partial log (click to expand)

strip is /nix/store/4q1c84f1ynfvsd85a932375sg0jc89v8-bootstrap-tools/bin/strip
building '/nix/store/97bydhz59mnqx3dy163casp9lvfwvwrk-binutils-wrapper-2.30.drv'...
unpacking sources
patching sources
configuring
no configure script, doing nothing
installing
post-installation fixup
Using dynamic linker: '/usr/lib/dyld'
/nix/store/a7r5zax21zxc1n9ig620w6klkyybvvj8-binutils-wrapper-2.30

@LnL7
Copy link
Member

LnL7 commented Apr 14, 2018

@GrahamcOfBorg build stdenv

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.binutils

Partial log (click to expand)

building '/nix/store/438pbb7kw6w67hap8kqirx408sjxny3p-binutils-wrapper-2.30.drv'...
unpacking sources
patching sources
configuring
no configure script, doing nothing
installing
post-installation fixup
checking for references to /build in /nix/store/y0arp14xr8iphrrwmcngg1xp69x8wc19-binutils-wrapper-2.30...
Using dynamic linker: '/nix/store/n5dfz05iknrvqkx8jvcvs2a9wmzim4nb-bootstrap-stage0-glibc/lib/ld-linux-aarch64.so.1'
/nix/store/y0arp14xr8iphrrwmcngg1xp69x8wc19-binutils-wrapper-2.30

@cstrahan
Copy link
Contributor

Ugh. I made the mistake of thinking that the usual array indexing syntax would suffice, but that only applies when you’re doing something like parameter expansion (I.e. $foo[bar] or ${foo[bar]}. The unset command expects a variable name, and since we already know that name statically, use something of the form unset foo[bar], but then foo[bar] in this context, just as any other, would be treated as a glob matching any of foob, fooa, or foor. Without those files present and assuming nullglob Isn’t set, foo[bar] expands to itself, which unset then takes and does the expected thing. Otherwise foo[bar] is a null expansion.

The takeaway is that if you’re using arrays with unset, you should always quote the argument -if your code works without quoting, it just means you’re getting lucky (with respect to both the contents of the working dir, and the present setting of nullglob). And, since it doesn’t hurt anything, it makes sense to always quote the argument as a policy, so that you get the expected results out of habit.

Thanks for catching this, @pbogdan. I’ll let any remaining borg runs complete, give @Ericson2314 a chance to indicate if he wants to look at it in the next couple hours, and then merge.

Sources: http://mywiki.wooledge.org/glob#nullglob

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

shrinking /nix/store/bq1xnvq1x8rgyv78j9xiq4vphpkp1p4v-findutils-4.6.0/libexec/code
shrinking /nix/store/bq1xnvq1x8rgyv78j9xiq4vphpkp1p4v-findutils-4.6.0/libexec/bigram
gzipping man pages under /nix/store/bq1xnvq1x8rgyv78j9xiq4vphpkp1p4v-findutils-4.6.0/share/man/
strip is /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/strip
stripping (with command strip and flags -S) in /nix/store/bq1xnvq1x8rgyv78j9xiq4vphpkp1p4v-findutils-4.6.0/libexec  /nix/store/bq1xnvq1x8rgyv78j9xiq4vphpkp1p4v-findutils-4.6.0/bin 
checking for references to /build in /nix/store/bq1xnvq1x8rgyv78j9xiq4vphpkp1p4v-findutils-4.6.0...
shrinking RPATHs of ELF executables and libraries in /nix/store/81d27scszz5210p3lji66g6zyj4c1gfg-findutils-4.6.0-info
strip is /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/strip
checking for references to /build in /nix/store/81d27scszz5210p3lji66g6zyj4c1gfg-findutils-4.6.0-info...
building '/nix/store/74cl24469l86a95n09ln644g7rj64ph8-stdenv-linux.drv'...

@GrahamcOfBorg
Copy link

Failure on x86_64-darwin (full log)

Attempted: stdenv

Partial log (click to expand)

cannot build derivation '/nix/store/sxxnq272din879v9n750bqmwiclqfw6y-cctools-port-895.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/7zrqhnxkgdpsb2nmwvlz4n5g0k3ld620-hook.drv': 6 dependencies couldn't be built
cannot build derivation '/nix/store/2vq3yv7dsa3jmb9jgxv980bbrc3rykyy-ICU-osx-10.10.5.drv': 3 dependencies couldn't be built
cannot build derivation '/nix/store/l1as3zrvb5nsd69cci8dz8xfazxigk1l-cctools-binutils-darwin.drv': 4 dependencies couldn't be built
cannot build derivation '/nix/store/6n0xsv14izpjw9bhm1frcxsdsf0x1c8g-gnutar-1.30.drv': 3 dependencies couldn't be built
cannot build derivation '/nix/store/0ir69dpcpv62d5lm4v3i3gs9pf23bqz9-CF-osx-10.10.5.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/z58rr82fvp6m825kircywd5x6576i6za-cctools-binutils-darwin-wrapper.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/x3y9m55j7irfh53as5wlxbgz8lxarp1n-clang-wrapper-5.0.1.drv': 9 dependencies couldn't be built
cannot build derivation '/nix/store/hyadi4idk8jxzvd9g7ji1dw8xgnb662q-stdenv-darwin.drv': 35 dependencies couldn't be built
�[31;1merror:�[0m build of '/nix/store/hyadi4idk8jxzvd9g7ji1dw8xgnb662q-stdenv-darwin.drv' failed

LnL7
LnL7 approved these changes Apr 14, 2018
View reviewed changes
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

I figured the stdenv build would probably timeout, but the bootPackages are fixed now so it's probably fine.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

shrinking /nix/store/8vgw7mm8swl1h51a4clvalmcqcsr43kq-findutils-4.6.0/bin/find
gzipping man pages under /nix/store/8vgw7mm8swl1h51a4clvalmcqcsr43kq-findutils-4.6.0/share/man/
strip is /nix/store/gfgczbs0cy0blibb0acv39cayq7qbplg-bootstrap-tools/bin/strip
stripping (with command strip and flags -S) in /nix/store/8vgw7mm8swl1h51a4clvalmcqcsr43kq-findutils-4.6.0/libexec  /nix/store/8vgw7mm8swl1h51a4clvalmcqcsr43kq-findutils-4.6.0/bin
checking for references to /build in /nix/store/8vgw7mm8swl1h51a4clvalmcqcsr43kq-findutils-4.6.0...
shrinking RPATHs of ELF executables and libraries in /nix/store/wxh45y7dhh0cl5sv16ql2v6r0bhp6l47-findutils-4.6.0-info
strip is /nix/store/gfgczbs0cy0blibb0acv39cayq7qbplg-bootstrap-tools/bin/strip
checking for references to /build in /nix/store/wxh45y7dhh0cl5sv16ql2v6r0bhp6l47-findutils-4.6.0-info...
building '/nix/store/mb332b6bqv9vjix83sy2pg1fzch4zkwd-stdenv-linux.drv'...
/nix/store/c7mmflzhbxkhi97ny6b2p38sb5rbldgk-stdenv-linux

Ericson2314
Ericson2314 approved these changes Apr 14, 2018
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Indeed I absolutely agree with you @cstrahan that "over-quoting" is best idiom given bash's insanity.

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 14, 2018 •
edited

Thanks @pbogdan for catching and fixing this!

@Ericson2314 Ericson2314 merged commit c0e58f8 into NixOS:staging Apr 14, 2018
@matthewbauer
Copy link
Member

Just FYI: this appears to break GHC on staging-

cc1: error: -Wformat-security ignored without -Wformat [-Werror=format-security]
cc1: some warnings being treated as errors

https://hydra.nixos.org/build/72961629/nixlog/1

@pbogdan
Copy link
Member Author

pbogdan commented Apr 18, 2018 •
edited

AFAICT first broken build is from prior to this change - https://hydra.nixos.org/build/72807800 I would expect that failure to be more related to the change of order in which the hardening flag are injected. Could be wrong though, also not sure what's the proper way to fix it :-( (patching GHC to drop -Wno-format for that file does produce a working build with no compilation warnings / errors).

EDIT: to elaborate - IIUC prior to 0884027 gcc -Wno-format would expand to gcc -Wno-format -Wformat -Wformat-security so -Wno-format would be effectively ignored because of order of the flags. After that commit it ends up being gcc -Wformat -Wformat-security -Wno-format and it blows up as per the error from the previous comment. And for reference here's the needed GHC change pbogdan/ghc@e818a8e if no other solution can be found.

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 18, 2018 •
edited

Hmm I forgot about the order change. That was the one controversial part of @cstrahan's original PR IIRC---I wish we had landed the rest without that so we could just bikeshed that part separately.

So either changing the order back (quite a can of worms), or adding the GHC patch is the right order.

@globin
Copy link
Member

globin commented Apr 18, 2018

As I've said on other occasions IMHO builds should break if the build systems supplies flags that conflict with hardening flags, iff the specific hardening is not turned off.

@Ericson2314
Copy link
Member

@globin you normally advocated hardening flags going last, right? Ironically it was lowering the priority of the hardening flags that caused the build to break.

@pbogdan pbogdan deleted the hardening-unsupported-flags branch December 3, 2019 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LnL7 LnL7 LnL7 approved these changes

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@orivej orivej Awaiting requested review from orivej

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pbogdan @LnL7 @GrahamcOfBorg @cstrahan @Ericson2314 @matthewbauer @globin