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

os x: re-enable PIE (ASLR) #6466

Closed
bnoordhuis opened this issue Apr 29, 2016 · 13 comments
Closed

os x: re-enable PIE (ASLR) #6466

bnoordhuis opened this issue Apr 29, 2016 · 13 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. macos Issues and PRs related to the macOS platform / OSX. security Issues and PRs related to security.

Comments

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 29, 2016

Commit a5012a0 disables PIE (and therefore ASLR) on OS X because it breaks profiling of C++ code. Ideally, we'd figure out a way to keep it turned on except when -prof is specified on the command line.

I believe the only way to do that (except for having two separate binaries, which I don't think we want) is to re-exec the process with the _POSIX_SPAWN_DISABLE_ASLR (256) flag set. The flag is ignored for setuid/setgid binaries so in that respect -Wl,-no_pie is superior.

@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. macos Issues and PRs related to the macOS platform / OSX. labels Apr 29, 2016
@ChALkeR ChALkeR added the security Issues and PRs related to security. label Apr 29, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Apr 29, 2016
This is a follow-up to commit a5012a0 ("build: unbreak -prof, disable
PIE on OS X") that disabled PIE (and therefore ASLR) completely on OS X.

We now scan the command line options and re-exec the process with the
_POSIX_SPAWN_DISABLE_ASLR flag set when `-prof` is specified.

The options parser is smart enough to understand any combination of
`-prof` and `-noprof`, including alternative spellings like `--no_prof`.

Fixes: nodejs#6466
@bnoordhuis
Copy link
Member Author

#6468

@indutny
Copy link
Member

indutny commented Apr 29, 2016

@bnoordhuis I wonder how lldb starts the process in such way that it is still able to resolve symbols.

@bnoordhuis
Copy link
Member Author

bnoordhuis commented Apr 29, 2016

@indutny In exactly the same way as the PR. :-)

@indutny
Copy link
Member

indutny commented Apr 29, 2016

While this is simple enough, lldb also works when attaching to existing process. I wonder if the image ranges reported by _dyld_get_image_header() may be used to determine ASLR shift in profiler.

@bnoordhuis
Copy link
Member Author

I've been investigating that but it's not exactly trivial: we'd first need to teach the profiler about the VM slide for each shared object and every third-party profiling tool would have to duplicate the effort.

@indutny
Copy link
Member

indutny commented Apr 29, 2016

@bnoordhuis the reason why I ask this, is that with your patch one won't be able to start the profiler at runtime, and this is certainly desirable in some environments.

If the tooling needs to break, then we probably need to keep it -no_pie for v6, and break the things in v7. Sliding all symbols by fixed value in tools/tickprocessor.js doesn't seem to be complicated at all.

Will open an alternative PR in a bit.

@indutny indutny mentioned this issue Apr 29, 2016
4 tasks
indutny added a commit to indutny/io.js that referenced this issue Apr 29, 2016
@indutny
Copy link
Member

indutny commented Apr 29, 2016

Here is my proposal: #6475

@indutny
Copy link
Member

indutny commented Apr 29, 2016

It seems that #6475 works too, so we have to decide what is the best for us. Would love to hear @nodejs/ctc opinion on this.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@indutny ... to be honest, I'd trust you and @bnoordhuis most to make the decision on this. Whichever approach you each feel is right, works for me.

@indutny
Copy link
Member

indutny commented Apr 29, 2016

@bnoordhuis how do you feel about Sumo fight?

kisg pushed a commit to paul99/v8mips that referenced this issue May 2, 2016
When exporting `shared-library` in profile log, additionally export a
slide offset. This is required to parse profile logs generated on
systems with ASLR (OS X), otherwise it is impossible to assign C++
symbol names to their addresses in the log.

See: nodejs/node#6466

BUG=

Review-Url: https://codereview.chromium.org/1934453003
Cr-Commit-Position: refs/heads/master@{#35921}
indutny added a commit to indutny/io.js that referenced this issue May 4, 2016
Original commit message:

    [prof] export slide offset in profile log

    When exporting `shared-library` in profile log, additionally export a
    slide offset. This is required to parse profile logs generated on
    systems with ASLR (OS X), otherwise it is impossible to assign C++
    symbol names to their addresses in the log.

    See: nodejs#6466

    BUG=

    Review-Url: https://codereview.chromium.org/1934453003
    Cr-Commit-Position: refs/heads/master@{nodejs#35921}
@jasnell
Copy link
Member

jasnell commented Jul 6, 2016

@indutny @bnoordhuis ... ping

@indutny
Copy link
Member

indutny commented Jul 6, 2016

pong

@bnoordhuis
Copy link
Member Author

This PR can be closed (and that's what I'll do.) I'll post a follow-up question in #6558.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. macos Issues and PRs related to the macOS platform / OSX. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants