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.

Sign up for GitHub

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

Jump to bottom

Compute base addresses from program headers while reading /proc/self/… #261

Merged
merged 1 commit into from
Nov 10, 2017
Merged

Compute base addresses from program headers while reading /proc/self/… #261

merged 1 commit into from
Nov 10, 2017

Conversation

pcc
Copy link
Contributor

@pcc pcc commented Nov 3, 2017

…maps.

We previously had logic to compute the base address from program
headers as part of symbolization. The problem is that we need a correct
base address earlier in order to adjust a PC into the image's address
space, as these addresses can appear in unsymbolized output.

There was previously an assumption that only the mapping that
was lowest in the address space did not need to be adjusted. This
assumption is not guaranteed (for example, the kernel may choose to
map an ET_DYN lowest) and in fact turned out to be wrong in binaries
linked with lld because the first mapping is read-only.

The solution is to move the program header reading logic into the
code that reads /proc/self/maps.

@pcc
Copy link
Contributor Author

pcc commented Nov 3, 2017

Addressed comments from Chromium review (https://chromium-review.googlesource.com/752753).

MXEBot pushed a commit to mirror/chromium that referenced this pull request Nov 4, 2017
Compute base addresses from program headers while reading /proc/self/…
6f903f3 
…maps.

This cherry picks this glog change:
google/glog#261
with a reimplementation of the program header reading logic for the
sandboxed symbolizer.

This should cause unsymbolized stack traces to contain the correct
addresses for binaries linked with lld.

Bug: 772559
Change-Id: Ief9cbb463b3b4c32149da893c89c2eefd76b05d4
Reviewed-on: https://chromium-review.googlesource.com/752753
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513956}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Nov 4, 2017
Revert "Compute base addresses from program headers while reading /pr…
65c5338 
…oc/self/maps."

This reverts commit 6f903f3.

Reason for revert: https://build.chromium.org/p/chromium.linux/builders/Android%20Clang%20Builder%20%28dbg%29/builds/104887

Original change's description:
> Compute base addresses from program headers while reading /proc/self/maps.
> 
> This cherry picks this glog change:
> google/glog#261
> with a reimplementation of the program header reading logic for the
> sandboxed symbolizer.
> 
> This should cause unsymbolized stack traces to contain the correct
> addresses for binaries linked with lld.
> 
> Bug: 772559
> Change-Id: Ief9cbb463b3b4c32149da893c89c2eefd76b05d4
> Reviewed-on: https://chromium-review.googlesource.com/752753
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513956}

TBR=ukai@chromium.org,pcc@chromium.org,mark@chromium.org

Change-Id: Ie02672bcfb2d28729530d61975083ba2187efd4e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 772559
Reviewed-on: https://chromium-review.googlesource.com/753864
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513978}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Nov 5, 2017
Reland "Compute base addresses from program headers while reading /pr…
1a3ed31 
…oc/self/maps."

This is a reland of 6f903f3

Fixed Android build issue.

Original change's description:
> Compute base addresses from program headers while reading /proc/self/maps.
>
> This cherry picks this glog change:
> google/glog#261
> with a reimplementation of the program header reading logic for the
> sandboxed symbolizer.
>
> This should cause unsymbolized stack traces to contain the correct
> addresses for binaries linked with lld.
>
> Bug: 772559
> Change-Id: Ief9cbb463b3b4c32149da893c89c2eefd76b05d4
> Reviewed-on: https://chromium-review.googlesource.com/752753
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513956}

Bug: 772559
Change-Id: Id6a8c62c0b05a7d12f817e389c7b96223fec8073
Reviewed-on: https://chromium-review.googlesource.com/754232
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514018}
@ukai
Copy link
Contributor

ukai commented Nov 6, 2017

could we have test for this?

I tried CC=/path/to/clang CXX=/path/to/clang++ ./configure && make && make check-TESTS
all tests passed without this CL.

shinh
shinh reviewed Nov 6, 2017
View reviewed changes
src/symbolize.cc Outdated
ElfW(Ehdr) ehdr;
if (flags_start[0] == 'r' &&
ReadFromOffsetExact(mem_fd, &ehdr, sizeof(ElfW(Ehdr)), start_address) &&
memcmp(ehdr.e_ident, ELFMAG, SELFMAG) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment for this? I think this is skipping non-text maps for usual cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in fact skipping non-readable maps. I've added a comment.

@shinh
Copy link
Collaborator

shinh commented Nov 6, 2017

This PR and https://chromium-review.googlesource.com/c/chromium/src/+/752753 look great to me. I hope no project except Chromium is using InstallSymbolizeCallback. Quick google search seems to suggest there's no other code which uses InstallSymbolizeCallback. However, in case there're hidden users of this APIs, could you clearly mention this changes the semantics of InstallSymbolizeCallback? I'd like to send the message to glog's list after we merge this PR.

@pcc
Compute base addresses from program headers while reading /proc/self/…
c4d37a7 
…maps.

We previously had logic to compute the base address from program
headers as part of symbolization. The problem is that we need a correct
base address earlier in order to adjust a PC into the image's address
space, as these addresses can appear in unsymbolized output.

There was previously an assumption that only the mapping that
was lowest in the address space did not need to be adjusted. This
assumption is not guaranteed (for example, the kernel may choose to
map an ET_DYN lowest) and in fact turned out to be wrong in binaries
linked with lld because the first mapping is read-only.

The solution is to move the program header reading logic into the
code that reads /proc/self/maps.

There is a change in semantics for clients that install a callback
using the InstallSymbolizeOpenObjectFileCallback function. Any such
clients will need to return a correct base address from the callback
by reading program headers using code similar to that in the function
OpenObjectFileContainingPcAndGetStartAddress.
@pcc
Copy link
Contributor Author

pcc commented Nov 8, 2017

could we have test for this?

I tried CC=/path/to/clang CXX=/path/to/clang++ ./configure && make && make check-TESTS
all tests passed without this CL.

Done. The test makes sure that PIEs mapped at low addresses can be symbolized correctly.

However, in case there're hidden users of this APIs, could you clearly mention this changes the semantics of InstallSymbolizeCallback?

I've added a note to the commit message.

@ukai
Copy link
Contributor

ukai commented Nov 10, 2017

Thanks!
confirmed.

@ukai ukai merged commit 2063b38 into google:master Nov 10, 2017
robin-raymond pushed a commit to webrtc-uwp/chromium-base that referenced this pull request Sep 20, 2018
Compute base addresses from program headers while reading /proc/self/…
6860328 
…maps.

This cherry picks this glog change:
google/glog#261
with a reimplementation of the program header reading logic for the
sandboxed symbolizer.

This should cause unsymbolized stack traces to contain the correct
addresses for binaries linked with lld.

Bug: 772559
Change-Id: Ief9cbb463b3b4c32149da893c89c2eefd76b05d4
Reviewed-on: https://chromium-review.googlesource.com/752753
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513956}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 6f903f3a228bc8e10cd7bda76342954feb3000c5
robin-raymond pushed a commit to webrtc-uwp/chromium-base that referenced this pull request Sep 20, 2018
Revert "Compute base addresses from program headers while reading /pr…
06a9a3e 
…oc/self/maps."

This reverts commit 6f903f3a228bc8e10cd7bda76342954feb3000c5.

Reason for revert: https://build.chromium.org/p/chromium.linux/builders/Android%20Clang%20Builder%20%28dbg%29/builds/104887

Original change's description:
> Compute base addresses from program headers while reading /proc/self/maps.
> 
> This cherry picks this glog change:
> google/glog#261
> with a reimplementation of the program header reading logic for the
> sandboxed symbolizer.
> 
> This should cause unsymbolized stack traces to contain the correct
> addresses for binaries linked with lld.
> 
> Bug: 772559
> Change-Id: Ief9cbb463b3b4c32149da893c89c2eefd76b05d4
> Reviewed-on: https://chromium-review.googlesource.com/752753
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513956}

TBR=ukai@chromium.org,pcc@chromium.org,mark@chromium.org

Change-Id: Ie02672bcfb2d28729530d61975083ba2187efd4e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 772559
Reviewed-on: https://chromium-review.googlesource.com/753864
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513978}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 65c533858f1d0c507e91b34f64fce93f5d1cf595
robin-raymond pushed a commit to webrtc-uwp/chromium-base that referenced this pull request Sep 20, 2018
Reland "Compute base addresses from program headers while reading /pr…
debb89a 
…oc/self/maps."

This is a reland of 6f903f3a228bc8e10cd7bda76342954feb3000c5

Fixed Android build issue.

Original change's description:
> Compute base addresses from program headers while reading /proc/self/maps.
>
> This cherry picks this glog change:
> google/glog#261
> with a reimplementation of the program header reading logic for the
> sandboxed symbolizer.
>
> This should cause unsymbolized stack traces to contain the correct
> addresses for binaries linked with lld.
>
> Bug: 772559
> Change-Id: Ief9cbb463b3b4c32149da893c89c2eefd76b05d4
> Reviewed-on: https://chromium-review.googlesource.com/752753
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513956}

Bug: 772559
Change-Id: Id6a8c62c0b05a7d12f817e389c7b96223fec8073
Reviewed-on: https://chromium-review.googlesource.com/754232
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#514018}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 1a3ed318cbbe2b53d9d6410a376e75439b9e6007
durswd pushed a commit to durswd/glog that referenced this pull request Sep 2, 2019
@ukai
Merge pull request google#261 from pcc/fix3
d41c2a1 
Compute base addresses from program headers while reading /proc/self/maps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shinh shinh shinh left review comments

Assignees
No one assigned
Labels
cla: yes enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pcc @ukai @shinh @sergiud @googlebot