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

R: disable stack protector on aarch64-darwin #158992

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

collares
Copy link
Member

@collares collares commented Feb 10, 2022

Motivation for this change

An attempt at fixing #158730, copied from #128606. I don't have an aarch64-darwin machine to test it on, but ofBorg seems happy.

cc @ConnorBaker

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:
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@collares collares requested a review from jbedo as a code owner February 10, 2022 17:03
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Feb 10, 2022
@ofborg ofborg bot requested review from timokau, 7c6f434c and omasanori February 10, 2022 17:17
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Feb 10, 2022
@collares
Copy link
Member Author

The correct fix for this is #151983, since it is a gfortran problem. The present PR should be an acceptable stopgap, though, since the workaround is already used in a few places in nixpkgs.

@ConnorBaker
Copy link
Contributor

ConnorBaker commented Feb 10, 2022

@collares this does fixes the issue I was running into -- what are the possible repercussions of disabling stack protector hardening?

@collares
Copy link
Member Author

collares commented Feb 10, 2022

Disabling the stack protector is a bad idea If you run R on malicious inputs that could cause buffer/stack overflows through exploiting R security bugs. If you're not worried about R security bugs (because you only run code you trust, say), you should be good. See https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html for more details.

@7c6f434c
Copy link
Member

Isn't malicious R code a problem on its own without sack smashing? I guess there could be about maliciously crafted input data to attack the standard library functions or something like that.

@smaret smaret self-requested a review February 12, 2022 10:02
@smaret
Copy link
Member

smaret commented Feb 12, 2022

This builds and runs fine on x86_darwin.

@7c6f434c 7c6f434c merged commit bff729e into NixOS:master Feb 12, 2022
@collares collares deleted the R-stackprotector branch March 22, 2022 19:01
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Mar 25, 2022
Some derivations, like `rPackages.KernSmooth`, fail to build on `aarch64-darwin` because `stackprotector` is enabled.

This is similar to the fix required to get R itself working on `aarch64-darwin`: NixOS#158992.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants