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

build: unbreak -prof, disable PIE on OS X #6453

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 28, 2016

Commit 204f3a8 ("build: Bump MACOSX_DEPLOYMENT_TARGET to 10.7")
unwittingly turned on new ASLR features that make -prof unusable for
profiling C++ code, breaking test/parallel/test-tick-processor.js in
the process. Build with -Wl,-no_pie for now.

Fixes: #5903

CI: https://ci.nodejs.org/job/node-test-pull-request/2420/

Commit 204f3a8 ("build: Bump MACOSX_DEPLOYMENT_TARGET to 10.7")
unwittingly turned on new ASLR features that make `-prof` unusable for
profiling C++ code, breaking `test/parallel/test-tick-processor.js` in
the process.  Build with `-Wl,-no_pie` for now.

Fixes: nodejs#5903
@bnoordhuis bnoordhuis added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. macos Issues and PRs related to the macOS platform / OSX. labels Apr 28, 2016
@indutny
Copy link
Member

indutny commented Apr 28, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 28, 2016

LGTM pending CI. Confirmed locally that it addresses the issue.

@Trott
Copy link
Member

Trott commented Apr 29, 2016

CI is green! 🎉

I know we just had a conversation about not rushing PRs and stuff, but to the extent that my opinion matters, I think this one warrants expediting. Broken CI means people mistrust it even after it's fixed and then ignore additional failures, and this seems like a small and easy-to-back-out change.

@addaleax
Copy link
Member

LGTM and go ahead if you want to land this.

jasnell pushed a commit that referenced this pull request Apr 29, 2016
Commit 204f3a8 ("build: Bump MACOSX_DEPLOYMENT_TARGET to 10.7")
unwittingly turned on new ASLR features that make `-prof` unusable for
profiling C++ code, breaking `test/parallel/test-tick-processor.js` in
the process.  Build with `-Wl,-no_pie` for now.

Fixes: #5903
PR-URL: #6453
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Landed in a5012a0 to get back to green CI. @bnoordhuis ... I've personally had to set this for a couple of other projects. What are the particular drawbacks for leaving this on indefinitely? (sorry, not that familiar with ASLR) Should be open a tracking issue to pull this back out at a later date?

@jasnell jasnell closed this Apr 29, 2016
@bnoordhuis bnoordhuis deleted the fix5903 branch April 29, 2016 08:27
@bnoordhuis
Copy link
Member Author

@jasnell ASLR is a security feature that makes it more difficult to (for example) execute return-to-libc attacks because the address of the system() function won't be at a fixed address. I've filed #6466 for investigating how we can enable it again.

Fishrock123 pushed a commit that referenced this pull request May 4, 2016
Commit 204f3a8 ("build: Bump MACOSX_DEPLOYMENT_TARGET to 10.7")
unwittingly turned on new ASLR features that make `-prof` unusable for
profiling C++ code, breaking `test/parallel/test-tick-processor.js` in
the process.  Build with `-Wl,-no_pie` for now.

Fixes: #5903
PR-URL: #6453
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants