Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469673002:
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown (Closed)
DescriptionInvalidate WeakPtrs of ResourceMessageFilter on channel shutdown
ResourceMessageFilter is a RefCountedThreadSafe object with WeakPtrFactory.
This kind of object tends to have potential UAF flaw as:
1. The refcount gets to zero on UI thread.
2. ResourceMessageFilter::OnDestruct posts a deletion task to IO thread.
3. Someone creates a scoped_refptr from a WeakPtr on IO thread, then
increments the refcount to 1.
4. The task posted at (2) arrives at IO thread and deletes ResourceMessageFilter.
5. The scoped_refptr at (3) goes away and decrease the refcount to 0 again.
This CL invalidates all WeakPtr to ResourceMessageFilter on the channel shutdown, and
prevents further WeakPtr creation, so that the scenario above doesn't happen.
Committed: https://crrev.com/5c734abad3035cfcb14b6510a152cd945146e6bd
Cr-Commit-Position: refs/heads/master@{#432746}
Patch Set 1 #Patch Set 2 : +test #Patch Set 3 : fix #Patch Set 4 : fix #Patch Set 5 : rebase #Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : fix #Patch Set 9 : rebase #
Total comments: 8
Patch Set 10 : +override. +comment. #
Total comments: 7
Patch Set 11 : remove test #
Messages
Total messages: 65 (49 generated)
Description was changed from
==========
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown
BUG=
==========
to
==========
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown
ResourceMessageFilter is a RefCountedThreadSafe object with WeakPtrFactory.
This kind of object tends to have potential UAF flaw as:
1. The refcount gets to zero on UI thread.
2. ResourceMessageFilter::OnDestruct posts a deletion task to IO thread.
3. Someone creates a scoped_refptr from a WeakPtr on IO thread, then
increments the refcount to 1.
4. The task posted at (2) arrives at IO thread and deletes
ResourceMessageFilter.
5. The scoped_refptr at (3) goes away and decrease the refcount to 0 again.
This CL invalidates all WeakPtr to ResourceMessageFilter on the channel
shutdown, and
prevents further WeakPtr creation, so that the scenario above doesn't happen.
==========
The CQ bit was checked by tzik@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tzik@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...
tzik@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tzik@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tzik@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@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 checked by tzik@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tzik@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...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tzik@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.
lgtm
tzik@chromium.org changed reviewers: + mmenke@chromium.org
Adding mmenke as a //content/browser/loader owner, PTAL. CC: rockot, horo. JFYI.
On 2016/11/07 05:03:38, tzik wrote: > Adding mmenke as a //content/browser/loader owner, PTAL. > > CC: rockot, horo. JFYI. I'll try to do this today, but may not get to it until tomorrow. I don't know ResourceMessageFilter that well, so need to do a bit of homework first.
On 2016/11/07 15:33:20, mmenke wrote: > On 2016/11/07 05:03:38, tzik wrote: > > Adding mmenke as a //content/browser/loader owner, PTAL. > > > > CC: rockot, horo. JFYI. > > I'll try to do this today, but may not get to it until tomorrow. I don't know > ResourceMessageFilter that well, so need to do a bit of homework first. Sorry for the delay, looked at it, but then forgot to follow up. Is there a bug associated with this issue, if not, could you create one? Also, is this CL meant to fix a particular crash? Most of the crashes I've seen, the profile has been destroyed, but the RDH has not, which this CL won't fix.
https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:863: ~ResourceDispatcherHostTest() { override? https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... File content/browser/loader/resource_message_filter_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:36: EXPECT_FALSE(strong_reference); Is this materially different from EXPECT_FALSE(weak_ptr.get())? https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:83: resource_dispatcher_host.Shutdown(); Why do we need a resource_dispatcher_host? I'm really not following what this test is doing, or what resource_message_filter has to do with the RDH in this test. https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:84: RunOnIOThread(base::Bind(&base::DoNothing)); Why is this needed?
The CQ bit was checked by tzik@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...
https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:863: ~ResourceDispatcherHostTest() { On 2016/11/09 18:34:19, mmenke wrote: > override? Done. https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... File content/browser/loader/resource_message_filter_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:36: EXPECT_FALSE(strong_reference); On 2016/11/09 18:34:19, mmenke wrote: > Is this materially different from EXPECT_FALSE(weak_ptr.get())? Added a comment here. No, they are equivalent here. But, since my purpose is to ensure the strong reference doesn't cause a crash, EXPECT_FALSE(strong_reference); is probably more straightforward. https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:83: resource_dispatcher_host.Shutdown(); On 2016/11/09 18:34:19, mmenke wrote: > Why do we need a resource_dispatcher_host? I'm really not following what this > test is doing, or what resource_message_filter has to do with the RDH in this > test. This is used by the impl of ResourceMessageFilter::OnChannelClosed, it touchs the ResourceDispatcherHost instance taken from a global variable. https://codereview.chromium.org/2469673002/diff/160001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:84: RunOnIOThread(base::Bind(&base::DoNothing)); On 2016/11/09 18:34:19, mmenke wrote: > Why is this needed? This is to wait for ResourceMessageFilter destruction on IO thread. I think ResourceDispatcherHost should conceptually outlive ResourceMessageFilter, and without this RunOnIOThread, the destruction order of ResourceDispatcherHost and ResourceMessageFilter is undefined.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay - yesterday really wasn't my day. I plan to get back to this after lunch.
https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... File content/browser/loader/resource_message_filter_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:38: // ResourceMessageFilter here doesn't causes a crash. I'm still really not following why you want to check that trying to create a strong reference doesn't cause a crash. https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:73: &waitable_event, weak_ptr)); Check TakeWeakPtrOnIOThread returns NULL after OnChannelClosing. https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:73: &waitable_event, weak_ptr)); Why is this even needed? We have a weak_ptr, and the previous RunOnIOThread verified it's a nullptr. That will continue to be true after the resource_message_filter is destroyed. This seems to do nothing but verify that weakptrs work. https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:86: resource_dispatcher_host.Shutdown(); On 2016/11/10 06:08:14, tzik wrote: > On 2016/11/09 18:34:19, mmenke wrote: > > Why do we need a resource_dispatcher_host? I'm really not following what this > > test is doing, or what resource_message_filter has to do with the RDH in this > > test. > > This is used by the impl of ResourceMessageFilter::OnChannelClosed, it touchs > the ResourceDispatcherHost instance taken from a global variable. I think that's worth mentioning where the RDH is declared, it's not exactly obvious. https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:87: RunOnIOThread(base::Bind(&base::DoNothing)); On 2016/11/10 06:08:14, tzik wrote: > On 2016/11/09 18:34:19, mmenke wrote: > > Why is this needed? > > This is to wait for ResourceMessageFilter destruction on IO thread. I think > ResourceDispatcherHost should conceptually outlive ResourceMessageFilter, and > without this RunOnIOThread, the destruction order of ResourceDispatcherHost and > ResourceMessageFilter is undefined. Actually, ResourceMessageFilters are scoped to RendererProcessHosts, so I assume most are torn down before the RDH is. And I'm not seeing why this is needed to destroy the filter - what's still holding on to a reference to it? I thought that's why the whole waitable_event thing was for, in fact - waiting until the message filter was destroyed before trying to dereference a weak reference. This also seems like it needs some comments.
https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... File content/browser/loader/resource_message_filter_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:38: // ResourceMessageFilter here doesn't causes a crash. On 2016/11/11 19:20:52, mmenke wrote: > I'm still really not following why you want to check that trying to create a > strong reference doesn't cause a crash. Without the fix in this CL, ResourceMessageFilter has a potential UAF bug, and this test should reveal the crash. In the previous buggy code, |weak_ptr| was non-null here, but the scoped_refptr created from |weak_ptr| is in an invalid state. So, I think creating a scoped_refptr was important to reveal the bug.
https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... File content/browser/loader/resource_message_filter_unittest.cc (right): https://codereview.chromium.org/2469673002/diff/180001/content/browser/loader... content/browser/loader/resource_message_filter_unittest.cc:38: // ResourceMessageFilter here doesn't causes a crash. On 2016/11/15 09:42:39, tzik wrote: > On 2016/11/11 19:20:52, mmenke wrote: > > I'm still really not following why you want to check that trying to create a > > strong reference doesn't cause a crash. > > Without the fix in this CL, ResourceMessageFilter has a potential UAF bug, and > this test should reveal the crash. > In the previous buggy code, |weak_ptr| was non-null here, but the scoped_refptr > created from |weak_ptr| is in an invalid state. So, I think creating a > scoped_refptr was important to reveal the bug. I'm still lost here. EXPECT_FALSE(weak_ptr.get()) would cause a test failure if the weak_ptr was unexpected non-NULL.
The CQ bit was checked by tzik@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 test is to reproduce the crash and to prove it's fixed in this CL. If it does not make sense to you to leave that kind of tests in the tree, can we just remove the test? I think it's not useful just to verify the weak ptr invalidation in OnChannelClosed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/16 11:41:42, tzik wrote: > The test is to reproduce the crash and to prove it's fixed in this CL. > If it does not make sense to you to leave that kind of tests in the tree, can we > just remove the test? I think it's not useful just to verify the weak ptr > invalidation in OnChannelClosed. LGTM. If it weren't for the (eventual) mojo transition, and removal of this class I'd push for a test.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2469673002/#ps200001 (title: "remove test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown
ResourceMessageFilter is a RefCountedThreadSafe object with WeakPtrFactory.
This kind of object tends to have potential UAF flaw as:
1. The refcount gets to zero on UI thread.
2. ResourceMessageFilter::OnDestruct posts a deletion task to IO thread.
3. Someone creates a scoped_refptr from a WeakPtr on IO thread, then
increments the refcount to 1.
4. The task posted at (2) arrives at IO thread and deletes
ResourceMessageFilter.
5. The scoped_refptr at (3) goes away and decrease the refcount to 0 again.
This CL invalidates all WeakPtr to ResourceMessageFilter on the channel
shutdown, and
prevents further WeakPtr creation, so that the scenario above doesn't happen.
==========
to
==========
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown
ResourceMessageFilter is a RefCountedThreadSafe object with WeakPtrFactory.
This kind of object tends to have potential UAF flaw as:
1. The refcount gets to zero on UI thread.
2. ResourceMessageFilter::OnDestruct posts a deletion task to IO thread.
3. Someone creates a scoped_refptr from a WeakPtr on IO thread, then
increments the refcount to 1.
4. The task posted at (2) arrives at IO thread and deletes
ResourceMessageFilter.
5. The scoped_refptr at (3) goes away and decrease the refcount to 0 again.
This CL invalidates all WeakPtr to ResourceMessageFilter on the channel
shutdown, and
prevents further WeakPtr creation, so that the scenario above doesn't happen.
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from
==========
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown
ResourceMessageFilter is a RefCountedThreadSafe object with WeakPtrFactory.
This kind of object tends to have potential UAF flaw as:
1. The refcount gets to zero on UI thread.
2. ResourceMessageFilter::OnDestruct posts a deletion task to IO thread.
3. Someone creates a scoped_refptr from a WeakPtr on IO thread, then
increments the refcount to 1.
4. The task posted at (2) arrives at IO thread and deletes
ResourceMessageFilter.
5. The scoped_refptr at (3) goes away and decrease the refcount to 0 again.
This CL invalidates all WeakPtr to ResourceMessageFilter on the channel
shutdown, and
prevents further WeakPtr creation, so that the scenario above doesn't happen.
==========
to
==========
Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown
ResourceMessageFilter is a RefCountedThreadSafe object with WeakPtrFactory.
This kind of object tends to have potential UAF flaw as:
1. The refcount gets to zero on UI thread.
2. ResourceMessageFilter::OnDestruct posts a deletion task to IO thread.
3. Someone creates a scoped_refptr from a WeakPtr on IO thread, then
increments the refcount to 1.
4. The task posted at (2) arrives at IO thread and deletes
ResourceMessageFilter.
5. The scoped_refptr at (3) goes away and decrease the refcount to 0 again.
This CL invalidates all WeakPtr to ResourceMessageFilter on the channel
shutdown, and
prevents further WeakPtr creation, so that the scenario above doesn't happen.
Committed: https://crrev.com/5c734abad3035cfcb14b6510a152cd945146e6bd
Cr-Commit-Position: refs/heads/master@{#432746}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5c734abad3035cfcb14b6510a152cd945146e6bd Cr-Commit-Position: refs/heads/master@{#432746} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
