Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629783002:
Upload crash reports when ExtensionFunctions generate bad messages. (Closed)
DescriptionUpload crash reports when ExtensionFunctions generate bad messages.
ExtensionFunctionDispatcher used to have its own implementation of
bad message handling, which missed calling DumpWithoutCrashing().
Use extensions::bad_message namespace's functions instead, with
little unfortunate deviation in logging to be consistent with the
existing UMA.
Note that this is not a fix for issue 642794. This CL will help
tracking 642794.
BUG=642794
Test=Note that we should start seeing crash reports for bad
message termination of ExtensionFunctions. This should give
us a clue narrow down reasons that cause crbug 642794.
Review-Url: https://codereview.chromium.org/2629783002
Cr-Commit-Position: refs/heads/master@{#444183}
Committed: https://chromium.googlesource.com/chromium/src/+/0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828
Patch Set 1 #
Total comments: 5
Patch Set 2 : address comments #
Total comments: 11
Patch Set 3 : address comments #Patch Set 4 : sync #
Messages
Total messages: 20 (10 generated)
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+Charlie for a TODO in bad_message.cc +Devlin for the rest, if you think this fix deserves its own bug than 642794, then let me know. https://codereview.chromium.org/2629783002/diff/1/extensions/browser/bad_mess... File extensions/browser/bad_message.cc (right): https://codereview.chromium.org/2629783002/diff/1/extensions/browser/bad_mess... extensions/browser/bad_message.cc:23: // TODO(creis): We should call SetCrashKeyValue("bad_message_reason") similar @creis: As you've said: we use string version of |reason| , maybe we can prefix the int with "e::" or some other char/string that makes sense? Or does that have further complication? Either way, I wanted to add this TODO to keep a track.
lazyboy@chromium.org changed reviewers: + creis@chromium.org
seems reasonable to me; a couple small comments. https://codereview.chromium.org/2629783002/diff/1/extensions/browser/extensio... File extensions/browser/extension_function_dispatcher.cc (right): https://codereview.chromium.org/2629783002/diff/1/extensions/browser/extensio... extensions/browser/extension_function_dispatcher.cc:86: // bug in that API's implementation on the renderer. s/on the renderer/. We flag something as a bad message on the browser, so that could be the result of a bad message from the renderer or a bad indication on the browser (e.g. liberal use of EXTENSION_FUNCTION_VALIDATE). https://codereview.chromium.org/2629783002/diff/1/extensions/browser/extensio... extensions/browser/extension_function_dispatcher.cc:93: void CommonResponseCallback(IPC::Sender* ipc_sender, instead of BadMessageReceivedFunc, could we just have something like template <class T> CommonResponseCallback(..., T sender) { ... if (bad message) { LogBadMessage(histogram_value); bad_message::ReceivedBadMessage(sender, EFD_BAD_MESSAGE); } ... } ?
https://codereview.chromium.org/2629783002/diff/1/extensions/browser/extensio... File extensions/browser/extension_function_dispatcher.cc (right): https://codereview.chromium.org/2629783002/diff/1/extensions/browser/extensio... extensions/browser/extension_function_dispatcher.cc:86: // bug in that API's implementation on the renderer. On 2017/01/13 02:45:50, Devlin wrote: > s/on the renderer/. We flag something as a bad message on the browser, so that > could be the result of a bad message from the renderer or a bad indication on > the browser (e.g. liberal use of EXTENSION_FUNCTION_VALIDATE). Done. https://codereview.chromium.org/2629783002/diff/1/extensions/browser/extensio... extensions/browser/extension_function_dispatcher.cc:93: void CommonResponseCallback(IPC::Sender* ipc_sender, On 2017/01/13 02:45:50, Devlin wrote: > instead of BadMessageReceivedFunc, could we just have something like > template <class T> > CommonResponseCallback(..., T sender) { > ... > if (bad message) { > LogBadMessage(histogram_value); > bad_message::ReceivedBadMessage(sender, EFD_BAD_MESSAGE); > } > ... > } > ? Ty, Done.
lgtm https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... File extensions/browser/extension_function_dispatcher.cc (right): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... extensions/browser/extension_function_dispatcher.cc:94: T bad_message_sender, nitty nit: Let's use T* (so that T is RenderProcessHost or BrowserMessageFilter, rather than RenderProcessHost* or BrowserMessageFilter*)
Thanks for converting this over! I like having fewer distinct ways to kill renderers. A few minor thoughts below. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... File extensions/browser/bad_message.cc (right): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... extensions/browser/bad_message.cc:20: if (reason == EFD_BAD_MESSAGE) It feels unfortunate to bake knowledge about particular BadMessageReasons into this class. Maybe we can live with it in this extensions/ version of BadMessage, but would it be a problem to just let both log/uma types happen on EFD_BAD_MESSAGE? That seems simpler to me. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... extensions/browser/bad_message.cc:27: // content::bad_message::BadMessageReason enum values overlap. The crash key only helps if there are multiple ReceivedBadMessage calls from the same method, so that we can offer the same magic signature for them. I don't think you're adding a case of that in this CL, but I suppose there is a pre-existing case in AppViewGuest::CompletePendingRequest... Ok, let's rephrase the TODO: TODO(creis): We should add a crash key similar to the "bad_message_reason" key logged in content::bad_message::ReceivedBadMessage, for disambiguating multiple kills in the same method. It's important not to overlap with the content::bad_message::BadMessageReason enum values. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... File extensions/browser/extension_function_dispatcher.cc (left): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... extensions/browser/extension_function_dispatcher.cc:90: // attacker trying to exploit the browser, so we crash the renderer instead. Is there somewhere you can preserve this comment? I think it's useful for understanding which kinds of validation we kill the renderer for. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... File extensions/browser/extension_function_dispatcher.cc (right): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... extensions/browser/extension_function_dispatcher.cc:241: bad_message::ReceivedBadMessage(sender, bad_message::EFD_BAD_MESSAGE); Please use a unique BadMessageReason in each ReceivedBadMessage callsite. Otherwise you can't tell which kill we're hitting in the wild without looking at the crash stack of each crash.
https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... File extensions/browser/bad_message.cc (right): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... extensions/browser/bad_message.cc:20: if (reason == EFD_BAD_MESSAGE) On 2017/01/13 21:22:25, Charlie Reis wrote: > It feels unfortunate to bake knowledge about particular BadMessageReasons into > this class. Maybe we can live with it in this extensions/ version of > BadMessage, but would it be a problem to just let both log/uma types happen on > EFD_BAD_MESSAGE? That seems simpler to me. SG, Done. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... extensions/browser/bad_message.cc:27: // content::bad_message::BadMessageReason enum values overlap. On 2017/01/13 21:22:25, Charlie Reis wrote: > The crash key only helps if there are multiple ReceivedBadMessage calls from the > same method, so that we can offer the same magic signature for them. Interesting. Although I feel you meant different signatures? > think you're adding a case of that in this CL, Got it. but I suppose there is a > pre-existing case in AppViewGuest::CompletePendingRequest... > > Ok, let's rephrase the TODO: > TODO(creis): We should add a crash key similar to the "bad_message_reason" key > logged in content::bad_message::ReceivedBadMessage, for disambiguating multiple > kills in the same method. It's important not to overlap with the > content::bad_message::BadMessageReason enum values. Done. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... File extensions/browser/extension_function_dispatcher.cc (left): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... extensions/browser/extension_function_dispatcher.cc:90: // attacker trying to exploit the browser, so we crash the renderer instead. On 2017/01/13 21:22:25, Charlie Reis wrote: > Is there somewhere you can preserve this comment? I think it's useful for > understanding which kinds of validation we kill the renderer for. Restored the comment. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... File extensions/browser/extension_function_dispatcher.cc (right): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... extensions/browser/extension_function_dispatcher.cc:94: T bad_message_sender, On 2017/01/13 16:16:17, Devlin wrote: > nitty nit: Let's use T* (so that T is RenderProcessHost or BrowserMessageFilter, > rather than RenderProcessHost* or BrowserMessageFilter*) Done. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/exte... extensions/browser/extension_function_dispatcher.cc:241: bad_message::ReceivedBadMessage(sender, bad_message::EFD_BAD_MESSAGE); On 2017/01/13 21:22:25, Charlie Reis wrote: > Please use a unique BadMessageReason in each ReceivedBadMessage callsite. > Otherwise you can't tell which kill we're hitting in the wild without looking at > the crash stack of each crash. Done.
Thanks! LGTM. https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... File extensions/browser/bad_message.cc (right): https://codereview.chromium.org/2629783002/diff/20001/extensions/browser/bad_... extensions/browser/bad_message.cc:27: // content::bad_message::BadMessageReason enum values overlap. On 2017/01/14 01:04:43, lazyboy wrote: > On 2017/01/13 21:22:25, Charlie Reis wrote: > > The crash key only helps if there are multiple ReceivedBadMessage calls from > the > > same method, so that we can offer the same magic signature for them. > Interesting. > Although I feel you meant different signatures? Right! Sorry about the typo. :) > > think you're adding a case of that in this CL, > Got it. > but I suppose there is a > > pre-existing case in AppViewGuest::CompletePendingRequest... > > > > Ok, let's rephrase the TODO: > > TODO(creis): We should add a crash key similar to the "bad_message_reason" key > > logged in content::bad_message::ReceivedBadMessage, for disambiguating > multiple > > kills in the same method. It's important not to overlap with the > > content::bad_message::BadMessageReason enum values. > > Done.
The CQ bit was checked by lazyboy@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lazyboy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2629783002/#ps50001 (title: "sync")
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": 50001, "attempt_start_ts": 1484695113443330,
"parent_rev": "1ce0d75da469537fd542b02ed0cf36bd9d95ecb7", "commit_rev":
"0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828"}
Message was sent while issue was closed.
Description was changed from ========== Upload crash reports when ExtensionFunctions generate bad messages. ExtensionFunctionDispatcher used to have its own implementation of bad message handling, which missed calling DumpWithoutCrashing(). Use extensions::bad_message namespace's functions instead, with little unfortunate deviation in logging to be consistent with the existing UMA. Note that this is not a fix for issue 642794. This CL will help tracking 642794. BUG=642794 Test=Note that we should start seeing crash reports for bad message termination of ExtensionFunctions. This should give us a clue narrow down reasons that cause crbug 642794. ========== to ========== Upload crash reports when ExtensionFunctions generate bad messages. ExtensionFunctionDispatcher used to have its own implementation of bad message handling, which missed calling DumpWithoutCrashing(). Use extensions::bad_message namespace's functions instead, with little unfortunate deviation in logging to be consistent with the existing UMA. Note that this is not a fix for issue 642794. This CL will help tracking 642794. BUG=642794 Test=Note that we should start seeing crash reports for bad message termination of ExtensionFunctions. This should give us a clue narrow down reasons that cause crbug 642794. Review-Url: https://codereview.chromium.org/2629783002 Cr-Commit-Position: refs/heads/master@{#444183} Committed: https://chromium.googlesource.com/chromium/src/+/0605ec9d2876c3c58b3f91ba5e82... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/0605ec9d2876c3c58b3f91ba5e82... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
