Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2504773002:
Have crashpad call ASan's crash handler if present (Closed)
|
Created:
4 years, 1 month ago by Reid Kleckner Modified:
4 years ago CC:
chromium-reviews, pennymac+watch_chromium.org, caitkp+watch_chromium.org, Mark Mentovai, inferno Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHave crashpad call ASan's crash handler if present
This ensures that ClusterFuzz will see an ASan report for null
dereferences.
We can also remove chrome_elf's ifdefs after this change, since
we don't need ASan's call to SetUnhandledExceptionFilter to succeed.
R=mark@chromium.org
TBR=pennymac@chromium.org
BUG=661209
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/c9175dab5210cb05151ee27dc3e9bd54439650e2
Cr-Commit-Position: refs/heads/master@{#435147}
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #Patch Set 3 : rebase #
Messages
Total messages: 53 (25 generated)
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. DO NOT SUBMIT until we roll clang past LLVM r287040 R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. DO NOT SUBMIT until we roll clang past LLVM r287040 R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
This change will also need to be made to upstream Crashpad. https://codereview.chromium.org/2504773002/diff/1/third_party/crashpad/crashp... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2504773002/diff/1/third_party/crashpad/crashp... third_party/crashpad/crashpad/client/crashpad_client_win.cc:110: #if defined(ADDRESS_SANITIZER) Can the ASan handler be out-of-process? Or does it only work in process? (We don't really want any more "stuff" in unhandled exception filter insofar as possible.)
What does ASAN’s UEF do? Does it need to be ordered before Crashpad’s handling?
https://codereview.chromium.org/2504773002/diff/1/third_party/crashpad/crashp... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2504773002/diff/1/third_party/crashpad/crashp... third_party/crashpad/crashpad/client/crashpad_client_win.cc:112: __asan_unhandled_exception_filter(EXCEPTION_POINTERS* info); No reason to have wrapped this. I’d move it just below the #includes, too. Even though it’s extern "C", it’s a little weird (in our local style at least) to be declared in crashpad::(unnamed namespace).
On 2016/11/15 22:46:04, Mark Mentovai wrote: > What does ASAN’s UEF do? Does it need to be ordered before Crashpad’s handling? It prints a stack trace and exits. That more or less disables crashpad, so I'm sure this is going to break some test somewhere. I'll need to do a dry run and track down those tests.
https://codereview.chromium.org/2504773002/diff/1/third_party/crashpad/crashp... File third_party/crashpad/crashpad/client/crashpad_client_win.cc (right): https://codereview.chromium.org/2504773002/diff/1/third_party/crashpad/crashp... third_party/crashpad/crashpad/client/crashpad_client_win.cc:110: #if defined(ADDRESS_SANITIZER) On 2016/11/15 22:28:02, scottmg wrote: > Can the ASan handler be out-of-process? Or does it only work in process? (We > don't really want any more "stuff" in unhandled exception filter insofar as > possible.) Not really. This particular callback is really boring. The only thing interesting about it is that the stack trace comes out in the same format as an ASan UAF report, which is useful to ClusterFuzz. It doesn't add any extra metadata that a minidump wouldn't contain. We'll need to disable this logic when we ship ASan to users so that we get an actual minidump. I don't think we have a macro for that not-yet-existent configuration right now.
Tests wouldn’t be the primary problem, but you’re basically talking about defeating Crashpad for any ASan build, and that would be a problem.
On 2016/11/15 23:03:11, Mark Mentovai wrote: > Tests wouldn’t be the primary problem, but you’re basically talking about > defeating Crashpad for any ASan build, and that would be a problem. Right, but based on https://bugs.chromium.org/p/chromium/issues/detail?id=661209#c15, I thought "disabling crashpad in an asan build" was the desired behavior.
On 2016/11/16 01:51:15, Reid Kleckner wrote: > On 2016/11/15 23:03:11, Mark Mentovai wrote: > > Tests wouldn’t be the primary problem, but you’re basically talking about > > defeating Crashpad for any ASan build, and that would be a problem. > > Right, but based on > https://bugs.chromium.org/p/chromium/issues/detail?id=661209#c15, I thought > "disabling crashpad in an asan build" was the desired behavior. Mark, friendly ping for code review. This is blocking our fuzzing with windows asan builds and regressions are not getting caught.
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. DO NOT SUBMIT until we roll clang past LLVM r287040 R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. DO NOT SUBMIT until we roll clang past LLVM r287040 R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
mark@chromium.org changed reviewers: + mark@chromium.org
LGTM. I thought that we were already periodically shipping ASan canaries to users. We’ll obviously need to not do this thing in order to do that thing. Is anyone tracking what needs to be done to ship an ASan canary?
On 2016/11/20 18:42:04, Mark Mentovai wrote: > LGTM. I thought that we were already periodically shipping ASan canaries to > users. We’ll obviously need to not do this thing in order to do that thing. Is > anyone tracking what needs to be done to ship an ASan canary? I filed a tracking bug https://bugs.chromium.org/p/chromium/issues/detail?id=667428, but i don't think anyone will get to it anytime soon. Reid, any idea when the clang roll is planned ? Current revision seems too old.
The CQ bit was checked by rnk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/21 19:24:46, inferno wrote: > On 2016/11/20 18:42:04, Mark Mentovai wrote: > > LGTM. I thought that we were already periodically shipping ASan canaries to > > users. We’ll obviously need to not do this thing in order to do that thing. Is > > anyone tracking what needs to be done to ship an ASan canary? Those are SyzyASan builds, and they don't define ADDRESS_SANITIZER. Eventually the plan is to transition from SyzyASan instrumentation to LLVM ASan instrumentation, and yes, we'll have to do more work here to make sure that both crashpad and clusterfuzz can get crash reports in the right way. > Reid, any idea when the clang roll is planned ? Current revision seems too old. We rolled last week, but it got reverted due to a linker warning on Mac. Hopefully we can sort that out this week.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
rebase
On 2016/11/22 20:47:55, Reid Kleckner wrote: > rebase Looks like clang rolled to 287685. if this roll stick for another 2-3 days, can you please cq.
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. DO NOT SUBMIT until we roll clang past LLVM r287040 R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by rnk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2504773002/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rnk@chromium.org changed reviewers: + robertshield@google.com
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=pennymac@chromium.org,scottmg@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=pennymac@chromium.org,mark@chromium.org,robertsheild@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
It needs an owner for chrome_elf. Can I get a stamp from Penny or Robert? Thanks!
inferno@chromium.org changed reviewers: + inferno@chromium.org
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=pennymac@chromium.org,mark@chromium.org,robertsheild@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=mark@chromium.org TBR=pennymac@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Adding Penny as TBR for chrome_elf. I'm removing some ifdefs, so I'm assuming that's good.
The CQ bit was checked by rnk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
rebase
The CQ bit was checked by rnk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2504773002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robertshield@chromium.org changed reviewers: + robertshield@chromium.org
Belated LGTM, sorry for the delay.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by inferno@chromium.org
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": 40001, "attempt_start_ts": 1480476476993780,
"parent_rev": "98c3f4f10e7d190d149e9a2fb84a089e8666b523", "commit_rev":
"2aca7827c57b66c99946de396cd5664260f83397"}
Message was sent while issue was closed.
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=mark@chromium.org TBR=pennymac@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=mark@chromium.org TBR=pennymac@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=mark@chromium.org TBR=pennymac@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Have crashpad call ASan's crash handler if present This ensures that ClusterFuzz will see an ASan report for null dereferences. We can also remove chrome_elf's ifdefs after this change, since we don't need ASan's call to SetUnhandledExceptionFilter to succeed. R=mark@chromium.org TBR=pennymac@chromium.org BUG=661209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/c9175dab5210cb05151ee27dc3e9bd54439650e2 Cr-Commit-Position: refs/heads/master@{#435147} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c9175dab5210cb05151ee27dc3e9bd54439650e2 Cr-Commit-Position: refs/heads/master@{#435147} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
