Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809293004:
Fix windows registry loader to carry over prefs from file thread correctly (Closed)
DescriptionFix windows registry loader to carry over prefs from file thread correctly
Two calls to ExternalLoader::StartLoading() will schedule two
LoadOnFileThread()s. However, because how
CompleteLoadAndStartWatchingRegistry looked at the prefs written
by LoadOnFileThread, the second LoadOnFileThread can overwrite
the first one's written prefs. This will result in nullptr access in
second CompleteLoadAndStartWatchingRegistry:
StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1]
Call again:
StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2]
[1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2]
[2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_|
BUG=709304
Review-Url: https://codereview.chromium.org/2809293004
Cr-Commit-Position: refs/heads/master@{#464182}
Committed: https://chromium.googlesource.com/chromium/src/+/57972e07b6e78e5eb245e617f814a0c8e2ea3f4b
Patch Set 1 #Patch Set 2 : add test #
Total comments: 4
Patch Set 3 : add TODO #
Messages
Total messages: 26 (21 generated)
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.
Description was changed from ========== Fix windows registry loader to carry over prefs from file thread correctly BUG=709304 ========== to ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() could schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 ==========
Description was changed from ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() could schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 ========== to ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() will schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 ==========
Description was changed from ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() will schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 ========== to ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() will schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 ==========
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@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.
lgtm, but it'd be great if we can improve this yet more in the future. https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/external_registry_loader_win.cc (right): https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/external_registry_loader_win.cc:207: LoadFinished(); I *think* this is safe - or rather, I think this won't crash. But there's still a risk of ExternalLoader passing or storing a raw ptr, which could then be unexpectedly overwritten. From ExternalLoader: // Used for passing the list of extensions from the method that loads them // to |LoadFinished|. To ensure thread safety, the rules are the following: // if this value is written on another thread than the UI, then it should // only be written in a task that was posted from |StartLoading|. After that, // this task should invoke |LoadFinished| with a PostTask. This scheme of // posting tasks will avoid concurrent access and imply the necessary memory // barriers. std::unique_ptr<base::DictionaryValue> prefs_; That's... a bit crazy. I think this would be a lot less error prone if we passed the std::unique_ptr<DictionaryValue> to LoadFinished() directly, rather than trying to set a protected member before it happens. WDYT? If that sounds good to you, can you at least add a TODO here or in external_loader.h to do so? https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/external_registry_loader_win_unittest.cc (right): https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/external_registry_loader_win_unittest.cc:46: EXPECT_TRUE(prefs_); ASSERT_TRUE
https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/external_registry_loader_win.cc (right): https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/external_registry_loader_win.cc:207: LoadFinished(); On 2017/04/12 19:32:06, Devlin wrote: > I *think* this is safe - or rather, I think this won't crash. But there's still > a risk of ExternalLoader passing or storing a raw ptr, which could then be > unexpectedly overwritten. > > From ExternalLoader: > > // Used for passing the list of extensions from the method that loads them > // to |LoadFinished|. To ensure thread safety, the rules are the following: > // if this value is written on another thread than the UI, then it should > // only be written in a task that was posted from |StartLoading|. After that, > // this task should invoke |LoadFinished| with a PostTask. This scheme of > // posting tasks will avoid concurrent access and imply the necessary memory > // barriers. > std::unique_ptr<base::DictionaryValue> prefs_; > > That's... a bit crazy. > > I think this would be a lot less error prone if we passed the > std::unique_ptr<DictionaryValue> to LoadFinished() directly, rather than trying > to set a protected member before it happens. WDYT? I was actually thinking about that while writing this CL. Given the number of usages of |prefs_|, I decided not to pursue that. If that sounds good to you, > can you at least add a TODO here or in external_loader.h to do so? Following up separately sounds good. Added TODO in external_loader.h https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/external_registry_loader_win_unittest.cc (right): https://codereview.chromium.org/2809293004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/external_registry_loader_win_unittest.cc:46: EXPECT_TRUE(prefs_); On 2017/04/12 19:32:06, Devlin wrote: > ASSERT_TRUE 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 Link to the patchset: https://codereview.chromium.org/2809293004/#ps40001 (title: "add TODO")
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": 1492033610223920,
"parent_rev": "b44ee0e7a402cbc927a59c15e2ab3fe1b9ac938a", "commit_rev":
"57972e07b6e78e5eb245e617f814a0c8e2ea3f4b"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492033610223920,
"parent_rev": "b44ee0e7a402cbc927a59c15e2ab3fe1b9ac938a", "commit_rev":
"57972e07b6e78e5eb245e617f814a0c8e2ea3f4b"}
Message was sent while issue was closed.
Description was changed from ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() will schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 ========== to ========== Fix windows registry loader to carry over prefs from file thread correctly Two calls to ExternalLoader::StartLoading() will schedule two LoadOnFileThread()s. However, because how CompleteLoadAndStartWatchingRegistry looked at the prefs written by LoadOnFileThread, the second LoadOnFileThread can overwrite the first one's written prefs. This will result in nullptr access in second CompleteLoadAndStartWatchingRegistry: StartLoading()->LoadOnFileThread()->Set prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[1] Call again: StartLoading()->LoadOnFileThread()->Overwrite prefs_, then PostTask to CompleteLoadAndStartWatchingRegistry[2] [1] CompleteLoadAndStartWatchingRegistry runs, then it consumes |prefs_| set by [2] [2] CompleteLoadAndStartWatchingRegistry runs, then it won't find |prefs_| BUG=709304 Review-Url: https://codereview.chromium.org/2809293004 Cr-Commit-Position: refs/heads/master@{#464182} Committed: https://chromium.googlesource.com/chromium/src/+/57972e07b6e78e5eb245e617f814... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/57972e07b6e78e5eb245e617f814... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
