|
|
Descriptioncrash: Compress crash report with gzip on Linux
BUG=495563
TEST=Open chrome://crash to upload a crash report and confirm chrome://crashes contains a new crash report.
Committed: https://crrev.com/9e4fec0f333f834c1d745d509d04037fd4d4dfe2
Cr-Commit-Position: refs/heads/master@{#333012}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments #Patch Set 3 : Add "-f" to avoid getting caught by prompt #
Total comments: 2
Patch Set 4 : sizeof(kGzipExtension) #Messages
Total messages: 15 (3 generated)
hashimoto@chromium.org changed reviewers: + thestig@chromium.org
https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... File components/crash/app/breakpad_linux.cc (right): https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1116: if (sys_waitpid(gzip_child, nullptr, 0) != gzip_child) { Don't we want to pass in a status variable, so we know if the gzip child succeeded or not? https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1119: sys_kill(gzip_child, SIGKILL); Do you need to bail out after this? Isn't the .gz file non-existant or garbage in this case? https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1123: strlen(dumpfile) + 4)); Rather than using 4 here and 3 below, can we have a constant for strlen(".gz") and use that? https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1124: memcpy(gzip_file, dumpfile, strlen(dumpfile)); We should be using my_memcpy(), and also on line 1144/1145. :\ There's also my_strncpy() and my_strncat() if you'd like to use them.
https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... File components/crash/app/breakpad_linux.cc (right): https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1116: if (sys_waitpid(gzip_child, nullptr, 0) != gzip_child) { On 2015/06/03 00:54:54, Lei Zhang wrote: > Don't we want to pass in a status variable, so we know if the gzip child > succeeded or not? Good point. Added status check. https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1119: sys_kill(gzip_child, SIGKILL); On 2015/06/03 00:54:54, Lei Zhang wrote: > Do you need to bail out after this? Isn't the .gz file non-existant or garbage > in this case? You're right, we should exit when gzip fails. Fixed. https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1123: strlen(dumpfile) + 4)); On 2015/06/03 00:54:54, Lei Zhang wrote: > Rather than using 4 here and 3 below, can we have a constant for strlen(".gz") > and use that? Done. https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1124: memcpy(gzip_file, dumpfile, strlen(dumpfile)); On 2015/06/03 00:54:54, Lei Zhang wrote: > We should be using my_memcpy(), and also on line 1144/1145. :\ > > There's also my_strncpy() and my_strncat() if you'd like to use them. Replaced all memcpy and strlen in this file which can be called in a compromised context with my_memcpy and my_strlen. Also now using my_strlcpy() and my_strlcat() to construct gzip file path. (is there any other libc functions we have to be aware of?) BTW, why are we maintaining my_strncpy() and my_strncat() in this file, when my_strlcpy() and my_strlcat() are provided by breakpad/src/common/linux/linux_libc_support.h?
lgtm https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... File components/crash/app/breakpad_linux.cc (right): https://codereview.chromium.org/1166743002/diff/1/components/crash/app/breakp... components/crash/app/breakpad_linux.cc:1124: memcpy(gzip_file, dumpfile, strlen(dumpfile)); On 2015/06/03 08:04:41, hashimoto wrote: > On 2015/06/03 00:54:54, Lei Zhang wrote: > > We should be using my_memcpy(), and also on line 1144/1145. :\ > > > > There's also my_strncpy() and my_strncat() if you'd like to use them. > > Replaced all memcpy and strlen in this file which can be called in a compromised > context with my_memcpy and my_strlen. > Also now using my_strlcpy() and my_strlcat() to construct gzip file path. > (is there any other libc functions we have to be aware of?) > > BTW, why are we maintaining my_strncpy() and my_strncat() in this file, when > my_strlcpy() and my_strlcat() are provided by > breakpad/src/common/linux/linux_libc_support.h? Not sure. Maybe these existed before linux_libc_support.h? https://codereview.chromium.org/1166743002/diff/40001/components/crash/app/br... File components/crash/app/breakpad_linux.cc (right): https://codereview.chromium.org/1166743002/diff/40001/components/crash/app/br... components/crash/app/breakpad_linux.cc:1128: my_strlen(dumpfile) + my_strlen(kGzipExtension) + 1; sizeof(kGzipExtension) also works in place of my_strlen(kGzipExtension) + 1
https://codereview.chromium.org/1166743002/diff/40001/components/crash/app/br... File components/crash/app/breakpad_linux.cc (right): https://codereview.chromium.org/1166743002/diff/40001/components/crash/app/br... components/crash/app/breakpad_linux.cc:1128: my_strlen(dumpfile) + my_strlen(kGzipExtension) + 1; On 2015/06/05 03:03:26, Lei Zhang wrote: > sizeof(kGzipExtension) also works in place of my_strlen(kGzipExtension) + 1 Done.
The CQ bit was checked by hashimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1166743002/#ps60001 (title: "sizeof(kGzipExtension)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1166743002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9e4fec0f333f834c1d745d509d04037fd4d4dfe2 Cr-Commit-Position: refs/heads/master@{#333012}
Message was sent while issue was closed.
Can you check and make sure this is working? I found that the OOM crash in https://code.google.com/p/chromium/issues/detail?id=524254 does not upload a crash report in Chrome 46, but does in Chrome 44. I bisected it down to: 45.0.2423.0 good, and 45.0.2424.0 bad. Unless I bisected wrong, this seems like the culprit from the changelogs. https://chromium.googlesource.com/chromium/src/+log/45.0.2423.0..45.0.2424.0?... From the crash server, I see: HTTP request sent, awaiting response... 400 Bad Request 2015-08-24 21:03:50 ERROR 400: Bad Request.
Message was sent while issue was closed.
On 2015/08/25 04:28:55, Lei Zhang wrote: > Can you check and make sure this is working? I found that the OOM crash in > https://code.google.com/p/chromium/issues/detail?id=524254 does not upload a > crash report in Chrome 46, but does in Chrome 44. I bisected it down to: > > 45.0.2423.0 good, and 45.0.2424.0 bad. Unless I bisected wrong, this seems like > the culprit from the changelogs. > https://chromium.googlesource.com/chromium/src/+log/45.0.2423.0..45.0.2424.0?... > > From the crash server, I see: > > HTTP request sent, awaiting response... 400 Bad Request > 2015-08-24 21:03:50 ERROR 400: Bad Request. On 45.0.2454.46, it seems just working fine as opening chrome://crash and chrome://inducebrowsercrashforrealz either results in a new crash report in chrome://crashes. Now trying to induce the said renderer OOM. Probably something special happening when handling renderer OOM?
Message was sent while issue was closed.
On 2015/08/25 04:41:19, hashimoto wrote: > On 2015/08/25 04:28:55, Lei Zhang wrote: > > Can you check and make sure this is working? I found that the OOM crash in > > https://code.google.com/p/chromium/issues/detail?id=524254 does not upload a > > crash report in Chrome 46, but does in Chrome 44. I bisected it down to: > > > > 45.0.2423.0 good, and 45.0.2424.0 bad. Unless I bisected wrong, this seems > like > > the culprit from the changelogs. > > > https://chromium.googlesource.com/chromium/src/+log/45.0.2423.0..45.0.2424.0?... > > > > From the crash server, I see: > > > > HTTP request sent, awaiting response... 400 Bad Request > > 2015-08-24 21:03:50 ERROR 400: Bad Request. > > On 45.0.2454.46, it seems just working fine as opening chrome://crash and > chrome://inducebrowsercrashforrealz either results in a new crash report in > chrome://crashes. > Now trying to induce the said renderer OOM. > Probably something special happening when handling renderer OOM? On 45.0.2454.46 (Official Build) beta (64-bit), repeatedly calling insertAdjacentText() as described in crbug.com/524254 results in a renderer crash and it got uploaded to the server (crash id = 7db5652f07aad5b5). Could let me know the exact steps you followed to cause the crash when bisecting?
Message was sent while issue was closed.
On 2015/08/25 05:04:13, hashimoto wrote: > On 45.0.2454.46 (Official Build) beta (64-bit), repeatedly calling > insertAdjacentText() as described in crbug.com/524254 > results in a renderer crash and it got uploaded to the server (crash id = > 7db5652f07aad5b5). > > Could let me know the exact steps you followed to cause the crash when > bisecting? Nothing special, just visit the webpage. Thanks for trying to reproduce the bug. I'll look into this. |