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.

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

binutils: Apply patches from 2.30 branch #41042

Closed
wants to merge 1 commit into from

Conversation

ThomasMader
Copy link
Contributor

Motivation for this change

This fixes #40397.
Because I already had those other patches I thought it is better to commit them with the two patches which fix the strip issue.

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.

@Ericson2314
Copy link
Member

Can we fetchpatch?

@ThomasMader
Copy link
Contributor Author

ThomasMader commented May 24, 2018

Probably yes but that would also mean that we would need to get all patches in the right order.
I had errors because the original patch couldn't be applied because of the patch for the changelog file.
I would not want to add all patches because it's a lot of work and I guess 2.31 will be released soon.

So in my opinion it's better to patch concrete problems for now.

@Ericson2314
Copy link
Member

Ericson2314 commented May 24, 2018

Hmm Ok. I gues just add a note with all the code. Shouldn't this go to staging?

@ThomasMader
Copy link
Contributor Author

What do you mean with the note?

It should go everywhere where binutils 2.30 is used, why do you ask?

@GrahamcOfBorg
Copy link

Failure on x86_64-linux (full log)

Attempted: binutils

Partial log (click to expand)

cannot build derivation '/nix/store/b8i5pfq6x7p84sh06d6p6a89ysajqql5-attr-2.4.47.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/lw7f8xp04aanpm72smrn6y3h22k5q6q4-bash-4.4-p19.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/jwjj34vabcmflg251zi7q406zixw1b04-binutils-2.30.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/cfnpxjslikdrgah4iq0fljgkm42c843d-pcre-8.41.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/7a199p4pq6pig90l6wbp1igjsy1ndlph-xz-5.2.3.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/sfc6idxsy7lqx99qy5ia5sda002wr51i-acl-2.2.52.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/n24qh0i7dg58fkdfy44kqima70j0in67-gnugrep-3.1.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/0da9d3vbxnm0hxvr5slncl08jbz15vd4-coreutils-8.29.drv': 4 dependencies couldn't be built
cannot build derivation '/nix/store/il0724pq5sy9hdyp99i5r46i1h0zrdqa-binutils-wrapper-2.30.drv': 4 dependencies couldn't be built
error: build of '/nix/store/il0724pq5sy9hdyp99i5r46i1h0zrdqa-binutils-wrapper-2.30.drv' failed

@Ericson2314
Copy link
Member

@ThomasMader I mean next to the patches in default.nix, say they are a) not fetched because they are slightly rebased, and b) should be removed with 2.31.

@ThomasMader
Copy link
Contributor Author

I thought about doing b) but haven't done it because there was already a patch for 2.30 with just the link to the bug, so I figured it's normal to go through all patches on each version bump and remove the unneeded ones.
It's quite easy to see that those fixes are for 2.30 looking at the links in the comments though.

I didn't do a) because of the same reason. I just used the same style which was already used in the file.
Using fetch would needlessly introduce further changes which I wanted to prevent.
Normally I would not touch such a low level package and keeping changes at a minimum is quite a good strategy in such a case. ;-)
One could argue that I shouldn't have added the other patches then but since those are used in Archlinux I found it justified.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: binutils

Partial log (click to expand)

patching sources
updateAutotoolsGnuConfigScriptsPhase
configuring
no configure script, doing nothing
installing
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/bi40pdmac0ixg18zlp5zw8l9n0rm4i94-binutils-wrapper-2.30
checking for references to /build in /nix/store/bi40pdmac0ixg18zlp5zw8l9n0rm4i94-binutils-wrapper-2.30...
Using dynamic linker: '/nix/store/gyxzyn687xndb21wzjd2ckdlq4gw58kb-glibc-2.27/lib/ld-linux-aarch64.so.1'
/nix/store/bi40pdmac0ixg18zlp5zw8l9n0rm4i94-binutils-wrapper-2.30

@Ericson2314
Copy link
Member

Well, at least people can blame and find this PR. So that just leaves whether this should go to staging right?

@ThomasMader
Copy link
Contributor Author

We can also keep that PR unmerged and get it merged only if there are really other packages with the same problem.
It's possible that it's just happening to dmd or programs build with dmd as I suspect the binary generation of dmd to be a little bit strange.

As of now I suspect that tilix isn't building because of this problem but that can be easily verified.
I would say we keep that PR lingering for now and close it if 2.31 is available or we really see that there are many packages suffering from this problem.
If the problem really exists with tilix it can be easily worked around by putting dontStrip = true into the gtkd package.
AFAIR there are not more libs build by dmd than gtkd.
If someone strips D libs build with dmd manually we would still have a problem though but I could live with that for unstable till 2.31 is released.

@dtzWill
Copy link
Member

dtzWill commented Jun 10, 2018

I think these patches address security issues mentioned in #41748.

If you don't mind, checking and adding additional patches that address those CVEs would be great.

(we might as well fix them while we're at it!)

If merging to master it'd be good to at least have someone sanity check a few builds and monitor Hydra for the fallout. Maybe new jobset? cc @shlevy since was involved in last binutils bump.

@shlevy
Copy link
Member

shlevy commented Jun 10, 2018

I can revive the binutils-2.30 jobset if we freshen up that branch... I'd really like to see if we can use fetchpatch, though.

@dtzWill
Copy link
Member

dtzWill commented Jun 26, 2018

Ping! :)

@ThomasMader
Copy link
Contributor Author

Sorry I have only infrequent access to internet for now. I might take a look when I go back home.

@dtzWill
Copy link
Member

dtzWill commented Jun 26, 2018

Sorry I have only infrequent access to internet for now. I might take a look when I go back home.

No worries, thanks! Ping'ing the thread itself (as opposed to trying to bring it to attention elsewhere) is mostly about helping ensure this isn't left incomplete only because it was forgotten about, which is all too easy to do :). And giving a small amount of encouragement that at least someone is interested in this :).

Thanks for putting this together, and do what you can when you can. Take care!

@ThomasMader
Copy link
Contributor Author

Closing this as there is an open PR for binutils 2.31.1: #49324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

binutils: Since Version 2.30 strip is broken
6 participants