Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159133002:
Shutdown StoragePartition before ProfileIOData is being shut down (Closed)
|
Created:
4 years, 5 months ago by kinuko Modified:
4 years, 4 months ago Reviewers:
michaeln, David Trainor- moved to gerrit, jam, falken, halliwell, Paweł Hajdan Jr., mmenke, alex clarke (OOO till 29th) CC:
anandc+watch-blimp_chromium.org, blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, dtrainor+watch-blimp_chromium.org, horo+watch_chromium.org, jessicag+watch-blimp_chromium.org, jochen+watch_chromium.org, jsbell+serviceworker_chromium.org, khushalsagar+watch-blimp_chromium.org, kinuko+serviceworker, kinuko+watch, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, Mike West, mlamouri+watch-content_chromium.org, nhiroki, nyquist+watch-blimp_chromium.org, Peter Beverloo, serviceworker-reviews, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, tzik, wjmaclean Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShutdown StoragePartition before ProfileIOData is being shut down
- Expose a new ShutdownStoragePartitions method on BrowserContext
- Calls the new method in the subclasses of BrowserContext before actual
destruction / shutdown starts
Content modules that are hung on StoragePartition often try to dereference
ResourceContext (which is owned by ProfileIOData in chromium cases) even after
ResourceContext is destroyed, so it could easily lead to U-A-F. This happens
because StoragePartitions are destroyed in BrowserContext's dtor, while
ProfileIOData and any other objects that are managed/owned by BrowserContext's
subclasses are usually destroyed earlier than that (i.e. their destructors run
before the base class's one).
This change tries to add a new explicit method,
BrowserContext::ShutdownStoragePartitions, and let embedders call it before
the embedders are going to destroy/shutdown their own objects (which include
ResourceContext), so that content modules can safely assume they can
dereference those objects while its StoragePartition's still there.
BUG=529896
Committed: https://crrev.com/f6ed359c6f34261f93f3ac5431c7f7526d500262
Cr-Commit-Position: refs/heads/master@{#407781}
Patch Set 1 #Patch Set 2 : dcheck #Patch Set 3 : more fix #
Total comments: 5
Patch Set 4 : Don't call profile method in ~Handle #Patch Set 5 : crash fix #Patch Set 6 : fix #
Total comments: 5
Patch Set 7 : addressed comments #Messages
Total messages: 89 (65 generated)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Description was changed from ========== Shutdown StoragePartition before ProfileIOData is being shut down BUG=529896 ========== to ========== Shutdown StoragePartition before ProfileIOData is being shut down BUG=529896 ==========
kinuko@chromium.org changed reviewers: + mmenke@chromium.org
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 kinuko@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 kinuko@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...
Description was changed from ========== Shutdown StoragePartition before ProfileIOData is being shut down BUG=529896 ========== to ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts BUG=529896 ==========
Matt- can you take a look when you have time? What do you think?
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 kinuko@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
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_...)
https://codereview.chromium.org/2159133002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2159133002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:119: profile_->ShutdownStoragePartitions(); This doesn't seem like a good place for this - this method is implicitly called during ProfileImpl's destructor, and calling a Profile method from an implicitly called destructor seems like a bad idea. I don't suppose we can just explicitly call ShutdownStoragePartitions() at the end of ProfileImpl's destructor instead? https://codereview.chromium.org/2159133002/diff/60001/content/browser/browser... File content/browser/browser_context.cc (right): https://codereview.chromium.org/2159133002/diff/60001/content/browser/browser... content/browser/browser_context.cc:500: RemoveUserData(kStoragePartitionMapKeyName); Storage partitions are stored as user data attached to the BrowserContext? I don't suppose there are plans to change that?
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by kinuko@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 kinuko@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_...)
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by kinuko@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_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 kinuko@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by kinuko@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_asan_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 kinuko@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_...)
Patchset #5 (id:140001) has been deleted
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by kinuko@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_...)
Thanks, updated. PTAL https://codereview.chromium.org/2159133002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2159133002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:119: profile_->ShutdownStoragePartitions(); On 2016/07/19 17:55:20, mmenke wrote: > This doesn't seem like a good place for this - this method is implicitly called > during ProfileImpl's destructor, and calling a Profile method from an implicitly > called destructor seems like a bad idea. > > I don't suppose we can just explicitly call ShutdownStoragePartitions() at the > end of ProfileImpl's destructor instead? I think we can, and yeah that'd be better. Done. I wanted to have some assertion to make sure we call ShutdownStoragePartitions before IOData::ShutdownOnUIThread, but don't have a good idea (other than adding a flag). Added comments instead. https://codereview.chromium.org/2159133002/diff/60001/content/browser/browser... File content/browser/browser_context.cc (right): https://codereview.chromium.org/2159133002/diff/60001/content/browser/browser... content/browser/browser_context.cc:500: RemoveUserData(kStoragePartitionMapKeyName); On 2016/07/19 17:55:20, mmenke wrote: > Storage partitions are stored as user data attached to the BrowserContext? I > don't suppose there are plans to change that? I suppose we wanted to avoid exposing internal stuff at BrowserContext's public header? I don't like UserData approach too much but I want to explorer other options separately from this change. https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc (right): https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:50: content::RunAllPendingInMessageLoop(content::BrowserThread::IO); This wasn't necessary before this CL because TestingProfile internally runs message loop in its dtor. Now we delete StoragePartition before then we need to run IO tasks a little earlier
LGTM! https://codereview.chromium.org/2159133002/diff/60001/content/browser/browser... File content/browser/browser_context.cc (right): https://codereview.chromium.org/2159133002/diff/60001/content/browser/browser... content/browser/browser_context.cc:500: RemoveUserData(kStoragePartitionMapKeyName); On 2016/07/21 03:06:25, kinuko wrote: > On 2016/07/19 17:55:20, mmenke wrote: > > Storage partitions are stored as user data attached to the BrowserContext? I > > don't suppose there are plans to change that? > > I suppose we wanted to avoid exposing internal stuff at BrowserContext's public > header? I don't like UserData approach too much but I want to explorer other > options separately from this change. I didn't mean to suggest you make that part of this change, was just hoping that since you seem to be working on the code, you might want to consider changing the ownership model. https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc (right): https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:53: content::TestBrowserThreadBundle thread_bundle_; Doesn't really matter, but these should probably be protected. https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:54: TestingProfile profile; profile_
Does this change ensure that URLRequestContexts outlive StoragePartitions?
On 2016/07/21 20:08:09, michaeln wrote: > Does this change ensure that URLRequestContexts outlive StoragePartitions? Yes it should.
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc (right): https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:53: content::TestBrowserThreadBundle thread_bundle_; On 2016/07/21 15:27:20, mmenke wrote: > Doesn't really matter, but these should probably be protected. Done. https://codereview.chromium.org/2159133002/diff/220001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc:54: TestingProfile profile; On 2016/07/21 15:27:20, mmenke wrote: > profile_ Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Adding more owners... mkwst@chromium.org: Please review changes in chrome/browser/browsing_data dtrainor@chromium.org: Please review changes in blimp/ (one-line change) halliwell@chromium.org: Please review changes in chromecast/ (one-line change) alexclarke@chromium.org: Please review changes in headless/ (one-line change) phajdan.jr@chromium.org: Please review changes in chrome/test/ (one-line change) falken@chromium.org: Please review changes in service_worker related to make sure jam@chromium.org: If you have any comments for the changes in content/
kinuko@chromium.org changed reviewers: + michaeln@chromium.org - mkwst@chromium.org
service worker lgtm. awesome!
On 2016/07/22 04:35:54, kinuko wrote: > mkwst@chromium.org: Please review changes in chrome/browser/browsing_data Looks like ooo, michaeln@: you can probably review the change? (Feel free to comment on other parts too)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
headless/ LGTM
chrome/test LGTM
On 2016/07/22 13:17:30, Paweł Hajdan Jr. wrote: > chrome/test LGTM chromecast/ lgtm
blimp/ lgtm thanks!
Can the motivation be spelled out (both for me to understand it, and for future developers)? The bug doesn't describe the issue, and just has a link to a long code review.
On 2016/07/22 16:26:22, jam wrote: > Can the motivation be spelled out (both for me to understand it, and for future > developers)? The bug doesn't describe the issue, and just has a link to a long > code review. The issue is that the StoragePartition is outliving the ProfileIOData, which StorageParition and its consumers indirectly depend on.
On 2016/07/22 17:59:43, mmenke wrote: > On 2016/07/22 16:26:22, jam wrote: > > Can the motivation be spelled out (both for me to understand it, and for > future > > developers)? The bug doesn't describe the issue, and just has a link to a long > > code review. > > The issue is that the StoragePartition is outliving the ProfileIOData, which > StorageParition and its consumers indirectly depend on. Can you expand? i.e. what are the sequence of events that lead to a UAF? In general, depending on embedders to call a method is something they can forget to do, that's why I'm trying to understand why we would add something that could be error prone.
On 2016/07/22 21:12:00, jam wrote: > On 2016/07/22 17:59:43, mmenke wrote: > > On 2016/07/22 16:26:22, jam wrote: > > > Can the motivation be spelled out (both for me to understand it, and for > > future > > > developers)? The bug doesn't describe the issue, and just has a link to a > long > > > code review. > > > > The issue is that the StoragePartition is outliving the ProfileIOData, which > > StorageParition and its consumers indirectly depend on. > > Can you expand? i.e. what are the sequence of events that lead to a UAF? > > In general, depending on embedders to call a method is something they can forget > to do, that's why I'm trying to understand why we would add something that could > be error prone. I'll defer to kinuko on that, as I don't know the specifics. I know ProfileIOData owns the ResourceContext and the URLRequestContext, and that StoragePartition and all sorts of content/ code depends on them, but content has no direct control over the lifetime of any of it (Beyond whatever path triggers BrowserContext destruction) but that's about the extent of my knowledge. The original bug is actually about dereferencing ProfileIOData itself, I believe.
Description was changed from ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts BUG=529896 ========== to ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while StoragePartition's still there. BUG=529896 ==========
Description was changed from ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while StoragePartition's still there. BUG=529896 ========== to ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while its StoragePartition's still there. BUG=529896 ==========
On 2016/07/22 21:18:50, mmenke wrote: > On 2016/07/22 21:12:00, jam wrote: > > On 2016/07/22 17:59:43, mmenke wrote: > > > On 2016/07/22 16:26:22, jam wrote: > > > > Can the motivation be spelled out (both for me to understand it, and for > > > future > > > > developers)? The bug doesn't describe the issue, and just has a link to a > > long > > > > code review. I've added a little more informative explanation in the issue description. > > > The issue is that the StoragePartition is outliving the ProfileIOData, which > > > StorageParition and its consumers indirectly depend on. > > > > Can you expand? i.e. what are the sequence of events that lead to a UAF? Yes it leads to a UAF. Basically what mmenke@ says, but let me try to expand a little further (a bit lengthy): Several content modules that are hung on StoragePartitions dereference ResourceContext (obtained via BrowserContext::ResourceContext) to access context information for resource loading, but ResourceContext's lifetime is managed by embedders and it could be destroyed at any time before the content modules can know when, which could lead to UAF. There is a known trick to avoid it that works only in chrome/chromium cases, by using URLRequestContextGetter (which is also managed by embedders and can be obtained via BrowserContext::CreateRequestContext), as it dispatches 'OnContextShuttingDown' notifications to its observers before it gets destructed and it usually means ResourceContext is going away too, but this notification is only available when embedder=chromium cases. There could be several ways to avoid this ResourceContext U-A-F situation in content (and some of these could be combined / implemented together too), to name a few: 1. implement notification mechanism similar to URLRequestContextGetterObserver (embedders need to trigger the notification) 2. make sure we shut down StoragePartition before the embedder destroys BrowserContext/Profile-owned objects (embedders need to trigger the shutdown because when StoragePartition's destroyed in BrowserContext's dtor the objects owned by its subclass are already gone) 3. implement weak-ptr like mechanism similar to URLRequestContextGetter (we need to change all the code that dereferences ResourceContext to check if it's already null) Both 1 and 2 require embedder's cooperation, and this CL tries to implement 2 as I thought it'd be probably easier / clearer than 1, i.e. requiring embedders implement various notifications for different objects. 3 could be implemented in addition to 1 and 2, but changing all the code that dereferences ResourceContext to gracefully handle the cases where ResourceContext is already null requires more cross-cutting efforts, and I wanted to first try making the lifetime contract between content and embedders clearer before trying approach 3. > > In general, depending on embedders to call a method is something they can > forget > > to do, that's why I'm trying to understand why we would add something that > could > > be error prone. Yep, that's understandable. I'm hoping this change makes the lifetime issue clearer to embedders to (and we have DCHECK so it's easier to detect when embedders forget to call the method), but I'm open to other suggestions too. > I'll defer to kinuko on that, as I don't know the specifics. I know > ProfileIOData owns the ResourceContext and the URLRequestContext, and that > StoragePartition and all sorts of content/ code depends on them, but content has > no direct control over the lifetime of any of it (Beyond whatever path triggers > BrowserContext destruction) but that's about the extent of my knowledge. The > original bug is actually about dereferencing ProfileIOData itself, I believe.
On 2016/07/25 15:05:56, kinuko wrote: > On 2016/07/22 21:18:50, mmenke wrote: > > On 2016/07/22 21:12:00, jam wrote: > > > On 2016/07/22 17:59:43, mmenke wrote: > > > > On 2016/07/22 16:26:22, jam wrote: > > > > > Can the motivation be spelled out (both for me to understand it, and for > > > > future > > > > > developers)? The bug doesn't describe the issue, and just has a link to > a > > > long > > > > > code review. > > I've added a little more informative explanation in the issue description. > > > > > The issue is that the StoragePartition is outliving the ProfileIOData, > which > > > > StorageParition and its consumers indirectly depend on. > > > > > > Can you expand? i.e. what are the sequence of events that lead to a UAF? > > Yes it leads to a UAF. Basically what mmenke@ says, but let me try to expand a > little further (a bit lengthy): > > Several content modules that are hung on StoragePartitions dereference > ResourceContext (obtained via BrowserContext::ResourceContext) to access context > information for resource loading, but ResourceContext's lifetime is managed by > embedders and it could be destroyed at any time before the content modules can > know when, which could lead to UAF. There is a known trick to avoid it that > works only in chrome/chromium cases, by using URLRequestContextGetter (which is > also managed by embedders and can be obtained via > BrowserContext::CreateRequestContext), as it dispatches 'OnContextShuttingDown' > notifications to its observers before it gets destructed and it usually means > ResourceContext is going away too, but this notification is only available when > embedder=chromium cases. > > There could be several ways to avoid this ResourceContext U-A-F situation in > content (and some of these could be combined / implemented together too), to > name a few: > 1. implement notification mechanism similar to URLRequestContextGetterObserver > (embedders need to trigger the notification) > 2. make sure we shut down StoragePartition before the embedder destroys > BrowserContext/Profile-owned objects (embedders need to trigger the shutdown > because when StoragePartition's destroyed in BrowserContext's dtor the objects > owned by its subclass are already gone) > 3. implement weak-ptr like mechanism similar to URLRequestContextGetter (we need > to change all the code that dereferences ResourceContext to check if it's > already null) > > Both 1 and 2 require embedder's cooperation, and this CL tries to implement 2 as > I thought it'd be probably easier / clearer than 1, i.e. requiring embedders > implement various notifications for different objects. > > 3 could be implemented in addition to 1 and 2, but changing all the code that > dereferences ResourceContext to gracefully handle the cases where > ResourceContext is already null requires more cross-cutting efforts, and I > wanted to first try making the lifetime contract between content and embedders > clearer before trying approach 3. > > > > In general, depending on embedders to call a method is something they can > > forget > > > to do, that's why I'm trying to understand why we would add something that > > could > > > be error prone. > > Yep, that's understandable. I'm hoping this change makes the lifetime issue > clearer to embedders to (and we have DCHECK so it's easier to detect when > embedders forget to call the method), but I'm open to other suggestions too. > > > I'll defer to kinuko on that, as I don't know the specifics. I know > > ProfileIOData owns the ResourceContext and the URLRequestContext, and that > > StoragePartition and all sorts of content/ code depends on them, but content > has > > no direct control over the lifetime of any of it (Beyond whatever path > triggers > > BrowserContext destruction) but that's about the extent of my knowledge. The > > original bug is actually about dereferencing ProfileIOData itself, I believe. Thanks for the explanation, especially the discussion about various strategies which explains why this is the best one pragmatically at this point. lgtm
lgtm 2 !
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2159133002/#ps240001 (title: "addressed comments")
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 ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while its StoragePartition's still there. BUG=529896 ========== to ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while its StoragePartition's still there. BUG=529896 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while its StoragePartition's still there. BUG=529896 ========== to ========== Shutdown StoragePartition before ProfileIOData is being shut down - Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while its StoragePartition's still there. BUG=529896 Committed: https://crrev.com/f6ed359c6f34261f93f3ac5431c7f7526d500262 Cr-Commit-Position: refs/heads/master@{#407781} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f6ed359c6f34261f93f3ac5431c7f7526d500262 Cr-Commit-Position: refs/heads/master@{#407781} |
