Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1189823002:
Update breakpad for Android packed relocations. (Closed)
|
Created:
5 years, 6 months ago by simonb (inactive) Modified:
5 years, 5 months ago Base URL:
https://chromium.googlesource.com/external/google-breakpad/src.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionUpdate breakpad for Android packed relocations.
Shared libraries containing Android packed relocations have a load
bias that differs from the start address in /proc/$$/maps. Current
breakpad assumes that the load bias and mapping start address are
the same.
Fixed by changing the client to detect the presence of Android packed
relocations in the address space of a loaded library, and adjusting the
stored mapping start address of any that are packed so that it contains
the linker's load bias.
For this to work properly, it is important that the non-packed library
is symbolized for breakpad. Either packed or non-packed libraries may
be run on the device; the client detects which has been loaded by the
linker.
BUG=499747
Patch Set 1 #Patch Set 2 : Improved readability, check for packing in dump_symbols #Patch Set 3 : memcmp -> my_memcmp #
Total comments: 30
Patch Set 4 : Update for review comments #
Total comments: 2
Patch Set 5 : Update for more review comments #Patch Set 6 : Remove assertion. #
Total comments: 20
Patch Set 7 : Update for more review comments. #
Messages
Total messages: 19 (2 generated)
simonb@chromium.org changed reviewers: + primiano@chromium.org, rmcilroy@chromium.org
Initial review, for style, viability, and general breakpaddiness. Thanks.
https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:466: assert(min_vaddr != max_addr); Do we want to be a bit more permissive to failures (e.g., just ignoring libraries which for some reason have invalid elf program headers)? Just wondering if we could end up crashing during a dump in situations where previously we wouldn't have? https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:473: // Retrieve and check dynamic tags, checking for the telltale that marks nit - "Retrieve and check dynamic task, returning true if tags indicating Android packed relocations are present." https://codereview.chromium.org/1189823002/diff/40001/common/linux/dump_symbo... File common/linux/dump_symbols.cc (right): https://codereview.chromium.org/1189823002/diff/40001/common/linux/dump_symbo... common/linux/dump_symbols.cc:643: "may not be symbolized.\n", obj_file.c_str()); nit - could we just include the error about not being symbolized in the above error messages and just return at the first packed relocation section we find? https://codereview.chromium.org/1189823002/diff/40001/common/linux/dump_symbo... common/linux/dump_symbols.cc:645: } Should this be #ifdef (__ANDROID__) ? Maybe SHT_LOOS + 1 is something different on other OS platforms which is OK. https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc File processor/microdump.cc (right): https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc#... processor/microdump.cc:196: while (GetLine(&stream, &line)) { Is there a reason you needed to write your own GetLine rather than using std::getline (it doesn't seem to be mentioned in the description or comments).
Thanks for comments. Updates in patch set 4. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:466: assert(min_vaddr != max_addr); On 2015/06/18 09:42:10, rmcilroy wrote: > Do we want to be a bit more permissive to failures (e.g., just ignoring > libraries which for some reason have invalid elf program headers)? Just > wondering if we could end up crashing during a dump in situations where > previously we wouldn't have? I don't think so. We strongly assume that a library has at least one LOAD segment and that we can find it here. If that assumption fails to hold I'd rather see a crash so that we can fix it, rather than silently produce an incorrect/unusable stack trace. Note that we *do* fail if ELFMAG isn't found by GetLoadedElfHeader. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:473: // Retrieve and check dynamic tags, checking for the telltale that marks On 2015/06/18 09:42:10, rmcilroy wrote: > nit - "Retrieve and check dynamic task, returning true if tags indicating > Android packed relocations are present." Done. https://codereview.chromium.org/1189823002/diff/40001/common/linux/dump_symbo... File common/linux/dump_symbols.cc (right): https://codereview.chromium.org/1189823002/diff/40001/common/linux/dump_symbo... common/linux/dump_symbols.cc:643: "may not be symbolized.\n", obj_file.c_str()); On 2015/06/18 09:42:11, rmcilroy wrote: > nit - could we just include the error about not being symbolized in the above > error messages and just return at the first packed relocation section we find? Done. https://codereview.chromium.org/1189823002/diff/40001/common/linux/dump_symbo... common/linux/dump_symbols.cc:645: } On 2015/06/18 09:42:11, rmcilroy wrote: > Should this be #ifdef (__ANDROID__) ? Maybe SHT_LOOS + 1 is something different > on other OS platforms which is OK. __ANDROID__ is not defined/relevant for host tools. Checking the file's ELF header ELFOSABI would be redundant since this is explicitly Linux's dump_symbols.cc module. https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc File processor/microdump.cc (right): https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc#... processor/microdump.cc:196: while (GetLine(&stream, &line)) { On 2015/06/18 09:42:11, rmcilroy wrote: > Is there a reason you needed to write your own GetLine rather than using > std::getline (it doesn't seem to be mentioned in the description or comments). This is the diff from a separate code review that has sneaked in. I'll remove it from here in a future patch set. https://codereview.chromium.org/1181113004/
Many thanks for splitting the code into their own functions, that makes it **much** more readable and reviewable. I have only one major doubt in the outer loop (LatePostprocessMappings()) which ironically is the simplest one. Maybe let's discuss this offline The rest is just minor nits. P.S: we'll need some test coverage here. We can do in a separate CL but let's figure out a way for covering this. I'm going to forget the rationale of all this in less than a week :) https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:447: const uintptr_t max_addr = ~static_cast<uintptr_t>(0); Isn't this just UINTPTR_MAX from stdint.h? Or is that not available on Android? https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:457: if (phdr.p_type == PT_LOAD && phdr.p_vaddr < min_vaddr) { Just doublechecking. I thought you meant to look for the smallest non-zero vaddr, not just the min(vaddr). Or probably I'm just remembering wrong. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:479: uint8_t* dyn_addr = reinterpret_cast<uint8_t*>(load_bias) + dyn_vaddr; I think (dyn_addr) this can stay an uintptr_t without the extra cast here, so you can do the math. I think you just need the cast below on line 484 to const void* https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:507: const uintptr_t load_bias = start_addr - min_vaddr; should you also check here that min_vadr < start_addr in order to avoid overflowing in the load_bias if something wrong happens in ParseLoadedElfProgramHeaders? https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:524: if (!(mapping->exec && mapping->name && mapping->name[0] == '/')) { I think you don't really need the middle check (mapping->name) here as name is a static array in the MappingInfo struct. i.e. just if(!(mapping->exec && mapping->name[0] == '/')) https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:529: return false; Hmm should you really return here or just continue? By reading this loop sounds like that if you encounter a mmap containing JiT-ted code * (either by V8 or the Android Runtime) before your elf you'll bail out without adjusting your library. * That would look like a r-x or rwx mapping to /dev/ashmem/something or /path/to/class.dex https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.h (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.h:42: #include <link.h> Since you need this only Android move it in a #if... __ANDROID__ guarded section. I run in the past in very odd situations where a header ends up including other headers that define conflicting types w.r.t. the ones being pulled by the other headers, but only on some platforms. #ifdef-fing this might reduce the chance that this gets reverted because broke a who-knows-which-weird-platform :) https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.h:157: #if defined(__ANDROID__) Shouldn't all these be private and non-virtual? They seem shared by both the ptrace and the core dumper https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc File processor/microdump.cc (right): https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc#... processor/microdump.cc:79: bool GetLine(std::istringstream* istream, string* str) { Maybe add a comment explaining that this removes Win-style LF produced by adb and/or rename this to GetLineWithUnixEndings
https://codereview.chromium.org/1189823002/diff/60001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/60001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:529: return false; As discussed offline, let's make this a continue instead.
Thanks for review feedback. Addressed in patch set 5. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:447: const uintptr_t max_addr = ~static_cast<uintptr_t>(0); On 2015/06/18 10:59:38, Primiano Tucci wrote: > Isn't this just UINTPTR_MAX from stdint.h? Or is that not available on Android? Done. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:457: if (phdr.p_type == PT_LOAD && phdr.p_vaddr < min_vaddr) { On 2015/06/18 10:59:38, Primiano Tucci wrote: > Just doublechecking. I thought you meant to look for the smallest non-zero > vaddr, not just the min(vaddr). Or probably I'm just remembering wrong. I do want the min(vaddr). Typically this will be zero for non-packed libraries, but we should always find one LOAD segment. Initializing min_vaddr to UINTPTR_MAX helps catch any case where we find none. Finding none breaks assumptions on which determining the real load_bias rests. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:479: uint8_t* dyn_addr = reinterpret_cast<uint8_t*>(load_bias) + dyn_vaddr; On 2015/06/18 10:59:38, Primiano Tucci wrote: > I think (dyn_addr) this can stay an uintptr_t without the extra cast here, so > you can do the math. I think you just need the cast below on line 484 to const > void* Done. Also phdr_addr at 445 above. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:507: const uintptr_t load_bias = start_addr - min_vaddr; On 2015/06/18 10:59:38, Primiano Tucci wrote: > should you also check here that min_vadr < start_addr in order to avoid > overflowing in the load_bias if something wrong happens in > ParseLoadedElfProgramHeaders? Not sure this would offer much protection. If the ELF program headers are corrupted there are many other ways to get the wrong answers here (including min_vaddr being less than start_addr but still not correct). https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:524: if (!(mapping->exec && mapping->name && mapping->name[0] == '/')) { On 2015/06/18 10:59:38, Primiano Tucci wrote: > I think you don't really need the middle check (mapping->name) here as name is a > static array in the MappingInfo struct. i.e. just > if(!(mapping->exec && mapping->name[0] == '/')) Done (and thanks, I know from annoying prior experience that clang will complain about this). https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:529: return false; On 2015/06/18 10:59:38, Primiano Tucci wrote: > Hmm should you really return here or just continue? > By reading this loop sounds like that if you encounter a mmap containing JiT-ted > code * (either by V8 or the Android Runtime) before your elf you'll bail out > without adjusting your library. > > * That would look like a r-x or rwx mapping to /dev/ashmem/something or > /path/to/class.dex Done. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.h (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.h:42: #include <link.h> On 2015/06/18 10:59:38, Primiano Tucci wrote: > Since you need this only Android move it in a #if... __ANDROID__ guarded > section. I run in the past in very odd situations where a header ends up > including other headers that define conflicting types w.r.t. the ones being > pulled by the other headers, but only on some platforms. > #ifdef-fing this might reduce the chance that this gets reverted because broke a > who-knows-which-weird-platform :) Done. https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.h:157: #if defined(__ANDROID__) On 2015/06/18 10:59:38, Primiano Tucci wrote: > Shouldn't all these be private and non-virtual? > They seem shared by both the ptrace and the core dumper Done. https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc File processor/microdump.cc (right): https://codereview.chromium.org/1189823002/diff/40001/processor/microdump.cc#... processor/microdump.cc:79: bool GetLine(std::istringstream* istream, string* str) { On 2015/06/18 10:59:39, Primiano Tucci wrote: > Maybe add a comment explaining that this removes Win-style LF produced by adb > and/or rename this to GetLineWithUnixEndings Removed from this review in later patch sets, since this is its own change: https://codereview.chromium.org/1181113004/ https://codereview.chromium.org/1189823002/diff/60001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/60001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:529: return false; On 2015/06/18 15:08:13, rmcilroy wrote: > As discussed offline, let's make this a continue instead. Done.
lgtm, but please get a breakpad owner's review (thestig@?) https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:466: assert(min_vaddr != max_addr); On 2015/06/18 10:46:39, simonb wrote: > On 2015/06/18 09:42:10, rmcilroy wrote: > > Do we want to be a bit more permissive to failures (e.g., just ignoring > > libraries which for some reason have invalid elf program headers)? Just > > wondering if we could end up crashing during a dump in situations where > > previously we wouldn't have? > > I don't think so. We strongly assume that a library has at least one LOAD > segment and that we can find it here. If that assumption fails to hold I'd > rather see a crash so that we can fix it, rather than silently produce an > incorrect/unusable stack trace. Sure, but we wouldn't see a crash since this would kill the crash reporting tool... ;) At least if we get incorrect / unusable stack traces we would see them in the crash tool and try and figure out what is going wrong. Silent crashing will just hide any issues in the wild. > Note that we *do* fail if ELFMAG isn't found by GetLoadedElfHeader. I think you've changed this now to continue, right?
simonb@chromium.org changed reviewers: + thestig@chromium.org
+thestig for any further comments, thanks. [ I'm aware this change cannot be landed from this repo, but needs to be patched/cherrypicked across to the main breakpad dev tree and landed there, then DEPS rolled back to here. ]
LGTM, make sure an owner stamps this. Note: before landing make sure that: - "make check" from the upstream breakpad tree still works - building this within a chrome for android tree works on both arm,arm64,x86,x86_64. It is a bit of a local pain, but it's much better than figuring that out after your breakpad roll lands and gets reverted :) (if it sounds like I talk by pain of experience... I do :P)
https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/40001/client/linux/minidump_w... client/linux/minidump_writer/linux_dumper.cc:466: assert(min_vaddr != max_addr); On 2015/06/18 17:13:36, rmcilroy wrote: > On 2015/06/18 10:46:39, simonb wrote: > > On 2015/06/18 09:42:10, rmcilroy wrote: > > > Do we want to be a bit more permissive to failures (e.g., just ignoring > > > libraries which for some reason have invalid elf program headers)? Just > > > wondering if we could end up crashing during a dump in situations where > > > previously we wouldn't have? > > > > I don't think so. We strongly assume that a library has at least one LOAD > > segment and that we can find it here. If that assumption fails to hold I'd > > rather see a crash so that we can fix it, rather than silently produce an > > incorrect/unusable stack trace. > > Sure, but we wouldn't see a crash since this would kill the crash reporting > tool... ;) At least if we get incorrect / unusable stack traces we would see > them in the crash tool and try and figure out what is going wrong. Silent > crashing will just hide any issues in the wild. Okay, I've removed the assert. If we fail (inexplicably!) to find a single LOAD segment, min_vaddr returns max_addr. This will cause the check later for DT_ANDROID_REL[A] to not find those tags (more accurately, it is *extremely unlikely* to find them because it will not be looking in the right memory location for dynamic tags). The result is no start_addr adjustment, for better or worse. > > > Note that we *do* fail if ELFMAG isn't found by GetLoadedElfHeader. > > I think you've changed this now to continue, right? Right.
Mostly nits and some question regarding {over,under}flows.
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
File client/linux/minidump_writer/linux_dumper.cc (right):
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:424: // Read the memory at
start_addr, expecting to find an ELF header. The first
Comments should go in the header. Ditto below.
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:431: if (my_memcmp(&ehdr->e_ident,
ELFMAG, SELFMAG) != 0) {
Just: return (my_memcmp(...) != 0); ?
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:445: uintptr_t phdr_addr =
start_addr + ehdr->e_phoff;
Can this or the addition to |phdr_addr| below overflow?
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:478: uintptr_t dyn_addr = load_bias
+ dyn_vaddr;
Can this or the addition to |dyn_addr| below overflow?
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:503: // If min vaddr is non-zero
and we find Android packed relocation tags,
min vaddr -> |min_vaddr|
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:506: const uintptr_t load_bias =
start_addr - min_vaddr;
Is |min_vaddr| guaranteeded to be less than |start_addr| ? i.e. this won't
underflow.
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:516: // Iterate mappings_, and
adjust any start_addr fields that are for mapped
|mappings_|
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.cc:538: return true;
This function can't return anything but true as is.
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
File client/linux/minidump_writer/linux_dumper.h (right):
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_...
client/linux/minidump_writer/linux_dumper.h:79: // Parse the data for |threads|
and |mappings|. LateInit() should be
nit: Can you separate the LateInit() comment from the Init() comment?
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:478: uintptr_t dyn_addr = load_bias + dyn_vaddr; On 2015/06/18 21:45:37, Lei Zhang wrote: > Can this or the addition to |dyn_addr| below overflow? but in that case CopyFromProcess will just fill up any failed ptrace(PEEKDATA) with zeros, which is the all point of using CopyFromProcess rather than trying to dereference stuff yourself in the forked process image.
Thanks, comments addressed in patch set 7. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:424: // Read the memory at start_addr, expecting to find an ELF header. The first On 2015/06/18 21:45:37, Lei Zhang wrote: > Comments should go in the header. Ditto below. Done. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:431: if (my_memcmp(&ehdr->e_ident, ELFMAG, SELFMAG) != 0) { On 2015/06/18 21:45:37, Lei Zhang wrote: > Just: return (my_memcmp(...) != 0); ? Done. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:445: uintptr_t phdr_addr = start_addr + ehdr->e_phoff; On 2015/06/18 21:45:37, Lei Zhang wrote: > Can this or the addition to |phdr_addr| below overflow? To somewhat amplify Primiano's comment below... not unless memory here is corrupted, in which case all bets are off anyway. The dynamic linker cannot map a segment of size N at an address greater than 0xfff..f - N. And static linker cannot create a library where e_phoff is greater than N. If we encounter conditions that apparently violate this, we have either a corrupted .so file or corrupted (readonly-mapped!) memory. In the unlikely case we encounter this, we either inspect mapped memory that is not actually the program headers, dynamic tags, and so on, or we wander outside mapped memory entirely, where CopyFromProcess returns all-zeros for attempts to read unmapped addresses. Either way we will almost certainly fail to find DT_ANDROID_REL[A] in HasAndroidPackedRelocations, not adjust start_addr, and get the same results out of breakpad as if this attempt to correct start_addr were not present. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:503: // If min vaddr is non-zero and we find Android packed relocation tags, On 2015/06/18 21:45:37, Lei Zhang wrote: > min vaddr -> |min_vaddr| Done. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:506: const uintptr_t load_bias = start_addr - min_vaddr; On 2015/06/18 21:45:37, Lei Zhang wrote: > Is |min_vaddr| guaranteeded to be less than |start_addr| ? i.e. this won't > underflow. Done. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:516: // Iterate mappings_, and adjust any start_addr fields that are for mapped On 2015/06/18 21:45:38, Lei Zhang wrote: > |mappings_| Done. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:538: return true; On 2015/06/18 21:45:37, Lei Zhang wrote: > This function can't return anything but true as is. Done. I have left LateInit() returning bool for now, even though the only thing currently done by LateInit() is to call this function, which 'cannot' fail. LateInit() seems more in the way of a framework extension than underlying implementation. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... File client/linux/minidump_writer/linux_dumper.h (right): https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.h:79: // Parse the data for |threads| and |mappings|. LateInit() should be On 2015/06/18 21:45:38, Lei Zhang wrote: > nit: Can you separate the LateInit() comment from the Init() comment? Done.
thestig@: We would like to get this in before the weekend, and since you seemed only to have nits I'm going to land it for Simon now. If you have any other comments Simon will follow up with a new CL.
Just the question about underflows. Otherwise lgtm. https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:506: const uintptr_t load_bias = start_addr - min_vaddr; On 2015/06/19 12:19:27, simonb wrote: > On 2015/06/18 21:45:37, Lei Zhang wrote: > > Is |min_vaddr| guaranteeded to be less than |start_addr| ? i.e. this won't > > underflow. > > Done. Can I get a yes/no answer on this question?
Just for history, this landed as r1459 (Simon could you answer Lei's question and then close the review manually please?).
https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... File client/linux/minidump_writer/linux_dumper.cc (right): https://codereview.chromium.org/1189823002/diff/100001/client/linux/minidump_... client/linux/minidump_writer/linux_dumper.cc:506: const uintptr_t load_bias = start_addr - min_vaddr; On 2015/06/19 18:59:28, Lei Zhang wrote: > On 2015/06/19 12:19:27, simonb wrote: > > On 2015/06/18 21:45:37, Lei Zhang wrote: > > > Is |min_vaddr| guaranteeded to be less than |start_addr| ? i.e. this won't > > > underflow. > > > > Done. > > Can I get a yes/no answer on this question? Sorry, mis-clicked Done here. What all of this code is doing is re-calculating the value of load_bias that the linker used internally, and in the same way as the linker. Line 346 of: https://android.googlesource.com/platform/bionic/+/android-m-preview/linker/l... Or in the case of the chromium crazy linker: https://cs.corp.google.com/#clankium/src/third_party/android_crazy_linker/src... The linker selects a (somewhat) random address for start_addr, in Android's case with ASLR. min_vaddr is equal to the space saved by relocation packing, so around 0x200000 (32-bit) to 0x600000 (64-bit). ASLR from mmap in the linker would have to return an address lower than this for min_vaddr to be greater than start_addr. If it does, the linker's load_bias is 'negative' through underflow, so our's must be also. load_bias is an 'effective' value, and addresses between it and start_addr are never accessed. When added back in inside the linker, the symmetrical overflow gets the right values. The probability of load_bias becoming negative is low, but perhaps non-zero. If it occurs, the right actions for breakpad to take are to wrap it on use, using the the target's overflow/underflow, so 32/64 bit depending on the target's type, to arrive at the right addresses. Having said that, it appears that breakpad non-client code may not wrap as it should for this case. For example, the microdump reader casts to uint64_t before use, so this would not wrap the adjustment in the way the target did. I'm not sure about other parts of breakpad at the moment. Any ideas for the best fix for this? Unstated here is that the load_bias and the start_addr for a library are not necessarily the same, but breakpad assumes that they always are because they have been up to now. Relocation packing uncouples them by introducing a non-zero vaddr to the first LOAD segment. Breakpad reads start_addr from /proc/self/maps, but maps does not expose load_bias, and that is what breakpad really requires. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
