Compute base addresses from program headers while reading /proc/self/… #261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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
…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 <[email protected]> Reviewed-by: Mark Mentovai <[email protected]> Cr-Commit-Position: refs/heads/master@{#513956}
MXEBot
pushed a commit
to mirror/chromium
that referenced
this pull request
Nov 4, 2017
…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 <[email protected]> > Reviewed-by: Mark Mentovai <[email protected]> > Cr-Commit-Position: refs/heads/master@{#513956} [email protected],[email protected],[email protected] 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 <[email protected]> Commit-Queue: Sky Malice <[email protected]> Cr-Commit-Position: refs/heads/master@{#513978}
MXEBot
pushed a commit
to mirror/chromium
that referenced
this pull request
Nov 5, 2017
…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 <[email protected]> > Reviewed-by: Mark Mentovai <[email protected]> > Cr-Commit-Position: refs/heads/master@{#513956} Bug: 772559 Change-Id: Id6a8c62c0b05a7d12f817e389c7b96223fec8073 Reviewed-on: https://chromium-review.googlesource.com/754232 Reviewed-by: Mark Mentovai <[email protected]> Commit-Queue: Peter Collingbourne <[email protected]> Cr-Commit-Position: refs/heads/master@{#514018}
|
could we have test for this? I tried CC=/path/to/clang CXX=/path/to/clang++ ./configure && make && make check-TESTS |
shinh
reviewed
Nov 6, 2017
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) { |
There was a problem hiding this comment.
Could you add a comment for this? I think this is skipping non-text maps for usual cases.
There was a problem hiding this comment.
It is in fact skipping non-readable maps. I've added a comment.
|
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. |
…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.
Done. The test makes sure that PIEs mapped at low addresses can be symbolized correctly.
I've added a note to the commit message. |
|
Thanks! |
robin-raymond
pushed a commit
to webrtc-uwp/chromium-base
that referenced
this pull request
Sep 20, 2018
…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 <[email protected]> Reviewed-by: Mark Mentovai <[email protected]> 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
…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 <[email protected]> > Reviewed-by: Mark Mentovai <[email protected]> > Cr-Commit-Position: refs/heads/master@{#513956} [email protected],[email protected],[email protected] 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 <[email protected]> Commit-Queue: Sky Malice <[email protected]> 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
…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 <[email protected]> > Reviewed-by: Mark Mentovai <[email protected]> > Cr-Commit-Position: refs/heads/master@{#513956} Bug: 772559 Change-Id: Id6a8c62c0b05a7d12f817e389c7b96223fec8073 Reviewed-on: https://chromium-review.googlesource.com/754232 Reviewed-by: Mark Mentovai <[email protected]> Commit-Queue: Peter Collingbourne <[email protected]> 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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…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.