Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1)

Issue 19275006: Fix a use-after-free in spellcheck client (Closed)

Created:
7 years, 5 months ago by please use gerrit instead
Modified:
7 years, 5 months ago
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch
Visibility:
Public.

Description

Fix a use-after-free condition in spellcheck request SpellCheckRequest::didCancel() and SpellCheckRequest::didSucceed() call into SpellChecker::didCheck(), which can delete the SpellCheckRequest object. After this happens, the methods write to a private member variable in the deleted instance. That's a use-after-free bug. The bug was caught with help of ASAN on content shell. Content shell does not set spellcheck client. The lack of the spellcheck client reduces the number of references to the SpellCheckRequest object by 1. When SpellChecker::didCheck() calls clear() on the ref-ptr of the SpellCheckRequest object, the object deletes itself, because there're no more references to it. The test WebFrameTest.CancelSpellingRequestCrash works by setting a NULL spellcheck client and canceling a pending spellcheck request. This setup hits the bug in SpellCheckRequest::didCancel(). It's not possible to hit the same bug in SpellCheckRequest::didSucceed(), because it happens when there's no spellcheck client, but only a spellcheck client calls didSucceed(). TEST=WebFrameTest.CancelSpellingRequestCrash TEST=LayoutTests/editing/spelling/copy-paste-crash.html BUG=259984 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154357

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+78 lines, -7 lines) Patch
A LayoutTests/editing/spelling/copy-paste-crash.html View 1 1 chunk +33 lines, -0 lines 0 comments Download
A LayoutTests/editing/spelling/copy-paste-crash-expected.txt View 1 1 chunk +10 lines, -0 lines 0 comments Download
M LayoutTests/editing/spelling/resources/util.js View 1 2 chunks +5 lines, -5 lines 0 comments Download
M Source/WebKit/chromium/tests/WebFrameTest.cpp View 1 2 chunks +26 lines, -0 lines 0 comments Download
M Source/core/editing/SpellChecker.cpp View 1 chunk +4 lines, -2 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
please use gerrit instead
Hajime: PTAL.
7 years, 5 months ago (2013-07-16 01:07:06 UTC) #1
Hajime Morrita
lgtm. Thanks for the fix!
7 years, 5 months ago (2013-07-16 05:01:31 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/19275006/4001
7 years, 5 months ago (2013-07-16 15:57:40 UTC) #3
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=3631
7 years, 5 months ago (2013-07-16 18:23:03 UTC) #4
please use gerrit instead
Adam: OWNERS PTAL.
7 years, 5 months ago (2013-07-16 18:29:30 UTC) #5
abarth-chromium
https://codereview.chromium.org/19275006/diff/4001/LayoutTests/editing/spelling/copy-paste-crash.html File LayoutTests/editing/spelling/copy-paste-crash.html (right): https://codereview.chromium.org/19275006/diff/4001/LayoutTests/editing/spelling/copy-paste-crash.html#newcode26 LayoutTests/editing/spelling/copy-paste-crash.html:26: } I'd add a "PASS: Did not crash" message ...
7 years, 5 months ago (2013-07-17 00:01:00 UTC) #6
please use gerrit instead
https://codereview.chromium.org/19275006/diff/4001/LayoutTests/editing/spelling/copy-paste-crash.html File LayoutTests/editing/spelling/copy-paste-crash.html (right): https://codereview.chromium.org/19275006/diff/4001/LayoutTests/editing/spelling/copy-paste-crash.html#newcode26 LayoutTests/editing/spelling/copy-paste-crash.html:26: } On 2013/07/17 00:01:00, abarth wrote: > I'd add ...
7 years, 5 months ago (2013-07-17 01:11:22 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/19275006/16001
7 years, 5 months ago (2013-07-17 01:14:05 UTC) #8
commit-bot: I haz the power
7 years, 5 months ago (2013-07-17 03:27:14 UTC) #9
Message was sent while issue was closed.
Change committed as 154357

Powered by Google App Engine
This is Rietveld 408576698