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

pkgsMusl.ocamlPackages.ocaml: Disable pie hardening on musl #124498

Closed
wants to merge 4 commits into from

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented May 26, 2021

This was broken since 4e9dc46, and fixes #124476

@nomeata
Copy link
Contributor Author

nomeata commented May 26, 2021

Hmm, this might not be good enough; it seems that now other ocaml packages fails to build. Maybe this needs to be disabled for ocaml on musl in more central way.

@nomeata
Copy link
Contributor Author

nomeata commented May 26, 2021

Indeed, it seems that this affects all executables that are ocaml build. I fixed a few manually, but is there a way to fix this more widely?

@veprbl
Copy link
Member

veprbl commented May 26, 2021

FYI We don't enable pie on non-musl

supportedHardeningFlags = [ "fortify" "stackprotector" "pie" "pic" "strictoverflow" "format" "relro" "bindnow" ];
# Musl-based platforms will keep "pie", other platforms will not.
defaultHardeningFlags = if stdenv.hostPlatform.isMusl &&
# Except when:
# - static aarch64, where compilation works, but produces segfaulting dynamically linked binaries.
# - static armv7l, where compilation fails.
!((stdenv.hostPlatform.isAarch64 || stdenv.hostPlatform.isAarch32) && stdenv.hostPlatform.isStatic)
then supportedHardeningFlags
else lib.remove "pie" supportedHardeningFlags;

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

stdenv.lib is deprecated. Please replace it with lib.

@nomeata
Copy link
Contributor Author

nomeata commented May 27, 2021

FYI We don't enable pie on non-musl

So you are saying pie is enabled only on musl, and then disabled again in GHC and glibc:

~/build/nixpkgs/pkgs $ git grep 'isMusl "pie"'
development/compilers/ghc/8.10.4.nix:  hardeningDisable = [ "format" ] ++ lib.optional stdenv.targetPlatform.isMusl "pie";
development/compilers/ghc/8.8.4.nix:  hardeningDisable = [ "format" ] ++ lib.optional stdenv.targetPlatform.isMusl "pie";
development/compilers/ghc/9.0.1.nix:  hardeningDisable = [ "format" ] ++ lib.optional stdenv.targetPlatform.isMusl "pie";
development/compilers/ghc/head.nix:  hardeningDisable = [ "format" ] ++ lib.optional stdenv.targetPlatform.isMusl "pie";
development/libraries/glibc/default.nix:      ++ lib.optional stdenv.hostPlatform.isMusl "pie";

stdenv.lib is deprecated. Please replace it with lib.

Sorry, of course. I cargo-culted from elsewhere. Fixed.

@ofborg ofborg bot requested a review from vbgl May 27, 2021 07:42
@nomeata nomeata mentioned this pull request May 27, 2021
9 tasks
mergify bot pushed a commit to dfinity/motoko that referenced this pull request May 28, 2021
to see what breaks, and also to get
NixOS/nixpkgs@5a923e5
which may allow us to use `rustc` from `nixpkgs` again (see #2519)

Issues encountered:

 * [x] in nixpks, various `cargoSHA256` were wrong. (NixOS/nixpkgs#123522, NixOS/nixpkgs#123348, NixOS/nixpkgs#123349)

 * [x] Ocaml built with musl, used for static builds, is currently broken. 
        
    Reported at NixOS/nixpkgs#124476.

    Found a possible fix, submitted at NixOS/nixpkgs#124498 and currently included in this branch.
 * [x] New ocamlformat version
 * [x] Running new formatter on these files
 * [x] The RTS fails to build or link the C files (`stddef.h` missing)
    This stuff was very hairy before, and that doesn’t get better :-(

    If I use the wrapped `wasm32-unknown-wasi-clang`, the build makes some progress, but then fails with https://bugs.llvm.org/show_bug.cgi?id=43393 (as it used to before, and then somehow we fixed that)

    In the end, surgically adding an include to `CLANG_FLAGS` works, it seems.
 * [x] Some upgraded tool produces new `names` subsections, presumably from this Wasm proposal: https://github.com/WebAssembly/extended-name-section/blob/master/proposals/extended-name-section/Overview.md
   Worked around by jumping over them, but otherwise ignoring them. Should be refined.
 * [x] Haskell code ought to be adjusted to latest GHC
 * [x] Bump the `niv` that we install into the shell (also because of Haskell changes)
 * [x] The darwin rebuild of `rustc` and `git` keeps timing out on hydra.
@SuperSandro2000 SuperSandro2000 requested review from Mic92 and sternenseemann and removed request for vbgl May 29, 2021 00:34
@SuperSandro2000
Copy link
Member

Because @vbgl blocked me he got removed while requesting a review from other people. Sorry, not sorry.

@veprbl veprbl requested a review from vbgl May 29, 2021 02:25
@Mic92
Copy link
Member

Mic92 commented May 29, 2021

I have not understand the issue yet. Is the issue that ocaml build static libraries, which do not work with pie?

@nomeata
Copy link
Contributor Author

nomeata commented May 29, 2021

The linked issue has the error messages: it seems that without this (or something like this), ocaml programs won't build in pkgsMusl.

@veprbl veprbl added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 15, 2021
…m/ocaml-musl

deleted pkgs/development/ocaml-modules/menhir/generic.nix, not sure if I
need to update pkgs/development/ocaml-modules/menhir/default.nix
@nomeata
Copy link
Contributor Author

nomeata commented Aug 25, 2021

May be superceded by #135619

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 25, 2021
nomeata added a commit to dfinity/motoko that referenced this pull request Aug 25, 2021
in #2532 we added a patch related to static building of ocaml packages,
submitted to nixpkgs as NixOS/nixpkgs#124498,
but it was never merged upstream.

Supposedly a patch from NixOS/nixpkgs#135619
fixes it as well (and maybe more properly). So let’s try that!
mergify bot pushed a commit to dfinity/motoko that referenced this pull request Sep 1, 2021
* Use nixpkgs master

to see what breaks, and also to get
NixOS/nixpkgs@5a923e5
which may allow us to use `rustc` from `nixpkgs` again (see #2519)

* Use buildDunePackage

* Less stdenv.lib

* Update generated nix files

* Bump to get fixes

* Bump nixpkgs

* Bump ocamlformat

* Add nixpkgs patch

* More patches

* Try building vlq differently

* Fix vlq

* Build vlq with buildDunePackage and dune2

* Revert "Build vlq with buildDunePackage and dune2"

This reverts commit d5e3399.

* Disable static ocaml build

* Allow requisites for now

* Quench warning

* More warnings

* Better error message

* Try GHC-8.8.4

* Actually bump to GHC-8.10.4

* Bump niv

* Fix parsing global names

* Call bindgen directly

* Bump nixpkgs, patches have been applied

* Run formatter

* Does this work?

* More comments

* Try static biuld again

* Revert "Try static biuld again"

This reverts commit ab44bbf.

* Actually follow the release branch

* Include patch from NixOS/nixpkgs#124498

* More patches

* Set allowedRequisites again

* Remove references to menhir

* Refresh patch

* Upgrade LLVM to version 12

* Apply suggestions from code review

Co-authored-by: Claudio Russo <[email protected]>

* Start decoding extended name sections

* Export apply_global_relocs in RTS module, call it before RTS init

* Make __wasm_apply_data_relocs optional in the linker (but not in codegen!), update linker test outputs

* Call wasm_apply_global_relocs from link_start

* Put the comment back

* Add patch from https://github.com/NixOS/nixpkgs/pull/125472/files

* Patch now upstream

* Linker: remove special case around __wasm_apply_global_relocs, add RTS start to merged module start

* Prepend m2 start in join_modules

Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Ömer Sinan Ağacan <[email protected]>
@r-burns
Copy link
Contributor

r-burns commented Oct 17, 2021

Closing as the PIE fixes have hit master and seem to be working well. Feel free to reopen and/or ping me if not

@r-burns r-burns closed this Oct 17, 2021
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.

pkgsMusl.ocamlPackages.ocaml: -r and -pie may not be used together
5 participants