Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556523002:
Add package field to Android crash reports (Closed)
DescriptionAdd package field to Android crash reports
Required for crash server to de-obfuscate java stack traces.
BUG=620323
Review-Url: https://codereview.chromium.org/2556523002
Cr-Commit-Position: refs/heads/master@{#443305}
Committed: https://chromium.googlesource.com/chromium/src/+/11478276220ac6dc29293d2c754f8b36ec3228d7
Patch Set 1 #
Total comments: 4
Patch Set 2 : don't alloc #
Total comments: 4
Patch Set 3 : scottmg comments #
Total comments: 2
Patch Set 4 : inline "package #Messages
Total messages: 25 (10 generated)
agrieve@chromium.org changed reviewers: + mmandlis@chromium.org, thestig@chromium.org
Not sure if it's documented anywhere, but scraped the format of this field from the server-side crash code :S.
Description was changed from ========== Add package field to Android crash reports Required for crash server to de-obfuscate java stack traces. BUG=620323 ========== to ========== Add package field to Android crash reports Required for crash server to de-obfuscate java stack traces. BUG=620323 ==========
agrieve@chromium.org changed reviewers: + mcolbert@chromium.org - thestig@chromium.org
lgtm lgtm
On 2016/12/05 20:37:04, agrieve (OOO Dec 2) wrote: > Not sure if it's documented anywhere, but scraped the format of this field from > the server-side crash code :S.
On 2016/12/05 20:40:37, agrieve wrote: > On 2016/12/05 20:37:04, agrieve (OOO Dec 2) wrote: > > Not sure if it's documented anywhere, but scraped the format of this field > from > > the server-side crash code :S. Example crash report that contains the new code: https://crash.corp.google.com/browse?q=reportid=%27ff2bb13f00000000%27#2
On 2016/12/06 19:36:23, agrieve wrote: > On 2016/12/05 20:40:37, agrieve wrote: > > On 2016/12/05 20:37:04, agrieve (OOO Dec 2) wrote: > > > Not sure if it's documented anywhere, but scraped the format of this field > > from > > > the server-side crash code :S. > > Example crash report that contains the new code: > https://crash.corp.google.com/browse?q=reportid=%27ff2bb13f00000000%27#2 🎄
thestig@chromium.org changed reviewers: + thestig@chromium.org
https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... components/crash/content/app/breakpad_linux.cc:148: std::string format_android_package( nit: FormatAndroidPackage() ? https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... components/crash/content/app/breakpad_linux.cc:150: return base::StringPrintf("%s v%s (%s)", This file has "WARNING: this code runs in a compromised context. It may not call into libc nor allocate memory normally." in a few places, and it applies to this code's caller. HandleCrashDump() does not have this comment, but it declares a google_breakpad::PageAllocator which gives it away. I would try using my_strlcat() and my_strlcpy() to clobber together a buffer on the stack, or use google_breakpad::PageAllocator to allocate memory. There's a couple examples in this file. Back to winter hibernation...
New crash link with current code to show it works fine: https://crash.corp.google.com/browse?q=reportid=%272abf85e300000000%27#2 https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... components/crash/content/app/breakpad_linux.cc:148: std::string format_android_package( On 2016/12/08 06:53:50, Lei Zhang (OOO) wrote: > nit: FormatAndroidPackage() ? Done. https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... components/crash/content/app/breakpad_linux.cc:150: return base::StringPrintf("%s v%s (%s)", On 2016/12/08 06:53:50, Lei Zhang (OOO) wrote: > This file has "WARNING: this code runs in a compromised context. It may not call > into libc nor allocate memory normally." in a few places, and it applies to this > code's caller. HandleCrashDump() does not have this comment, but it declares a > google_breakpad::PageAllocator which gives it away. > > I would try using my_strlcat() and my_strlcpy() to clobber together a buffer on > the stack, or use google_breakpad::PageAllocator to allocate memory. There's a > couple examples in this file. > > Back to winter hibernation... Aha, great info! Put it on the stack.
agrieve@chromium.org changed reviewers: - thestig@chromium.org
On 2016/12/13 18:47:45, agrieve wrote: > New crash link with current code to show it works fine: > https://crash.corp.google.com/browse?q=reportid=%272abf85e300000000%27#2 > > https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... > File components/crash/content/app/breakpad_linux.cc (right): > > https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... > components/crash/content/app/breakpad_linux.cc:148: std::string > format_android_package( > On 2016/12/08 06:53:50, Lei Zhang (OOO) wrote: > > nit: FormatAndroidPackage() ? > > Done. > > https://codereview.chromium.org/2556523002/diff/1/components/crash/content/ap... > components/crash/content/app/breakpad_linux.cc:150: return > base::StringPrintf("%s v%s (%s)", > On 2016/12/08 06:53:50, Lei Zhang (OOO) wrote: > > This file has "WARNING: this code runs in a compromised context. It may not > call > > into libc nor allocate memory normally." in a few places, and it applies to > this > > code's caller. HandleCrashDump() does not have this comment, but it declares a > > google_breakpad::PageAllocator which gives it away. > > > > I would try using my_strlcat() and my_strlcpy() to clobber together a buffer > on > > the stack, or use google_breakpad::PageAllocator to allocate memory. There's a > > couple examples in this file. > > > > Back to winter hibernation... > > Aha, great info! Put it on the stack. Post-holiday bump. Moved Lei to CC since he's on leave.
agrieve@chromium.org changed reviewers: + scottmg@chromium.org
+scottmg in case he has time to look
https://codereview.chromium.org/2556523002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2556523002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:575: my_strlcpy(buf, android_build_info->package_name(), kMaxSize); Comment here why it's not just using sprintf. https://codereview.chromium.org/2556523002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:576: my_strlcat(buf, " v", kMaxSize - my_strlen(buf)); I believe strlcat and strlcpy normally take the full size of the buffer to avoid this sort of error-prone code. Is is necessary to subtract the current buffer length here each time?
https://codereview.chromium.org/2556523002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2556523002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:575: my_strlcpy(buf, android_build_info->package_name(), kMaxSize); On 2017/01/11 02:47:41, scottmg wrote: > Comment here why it's not just using sprintf. Done. https://codereview.chromium.org/2556523002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:576: my_strlcat(buf, " v", kMaxSize - my_strlen(buf)); On 2017/01/11 02:47:41, scottmg wrote: > I believe strlcat and strlcpy normally take the full size of the buffer to avoid > this sort of error-prone code. Is is necessary to subtract the current buffer > length here each time? Aha, that's much nicer :)
lgtm https://codereview.chromium.org/2556523002/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2556523002/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:561: static const char package[] = "package"; Since this is only used once, just put it into the AddPairString() call as a literal.
https://codereview.chromium.org/2556523002/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2556523002/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:561: static const char package[] = "package"; On 2017/01/12 03:30:38, scottmg wrote: > Since this is only used once, just put it into the AddPairString() call as a > literal. Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmandlis@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2556523002/#ps60001 (title: "inline "package")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484242918423450,
"parent_rev": "ab4e21b712638087bdec923f9fcccf81ae1e2154", "commit_rev":
"11478276220ac6dc29293d2c754f8b36ec3228d7"}
Message was sent while issue was closed.
Description was changed from ========== Add package field to Android crash reports Required for crash server to de-obfuscate java stack traces. BUG=620323 ========== to ========== Add package field to Android crash reports Required for crash server to de-obfuscate java stack traces. BUG=620323 Review-Url: https://codereview.chromium.org/2556523002 Cr-Commit-Position: refs/heads/master@{#443305} Committed: https://chromium.googlesource.com/chromium/src/+/11478276220ac6dc29293d2c754f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/11478276220ac6dc29293d2c754f... |
