Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478503002:
Enable non-sequential entries when loading registry lists. (Closed)
|
Created:
4 years, 1 month ago by Georges Khalil Modified:
4 years, 1 month ago Reviewers:
pastarmovj CC:
chromium-reviews, chrome-enterprise-team_google.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable non-sequential entries when loading registry lists.
This CL makes the loading much more permissive. The names still have to be integers, but need not be sequential.
BUG=655774
Committed: https://crrev.com/1a839edc0bc326d05f368ff00ce5a9e3b10d3041
Cr-Commit-Position: refs/heads/master@{#429870}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Validate that the key is numerical. #
Messages
Total messages: 28 (20 generated)
The CQ bit was checked by georgesak@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...
georgesak@chromium.org changed reviewers: + pastarmovj@chromium.org
PTAL. https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... File components/policy/core/common/registry_dict_win_unittest.cc (right): https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... components/policy/core/common/registry_dict_win_unittest.cc:226: &error); Result of git cl format.
https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... File components/policy/core/common/registry_dict_win.cc (right): https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... components/policy/core/common/registry_dict_win.cc:106: for (int i = 1; ; ++i) { Don't you need to fix this part too? https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... components/policy/core/common/registry_dict_win.cc:326: entry != keys_.end(); ++entry) { I think it still makes sense to only parse numbered keys. The code as it is now will parse a key with arbitrary names. Sometimes while testing polivies admins tend to rename them to switch them off e.g. "HomePageUrl" -> "_HomePageUrl" one can imagine they would do this to the indexed entries too so skipping anything that is not a number sounds like a good idea to me. Wdyt? If you agree about this change please augment the test to capture this too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... File components/policy/core/common/registry_dict_win.cc (right): https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... components/policy/core/common/registry_dict_win.cc:106: for (int i = 1; ; ++i) { On 2016/11/03 16:27:47, pastarmovj wrote: > Don't you need to fix this part too? Oops, will fix it. https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... components/policy/core/common/registry_dict_win.cc:326: entry != keys_.end(); ++entry) { On 2016/11/03 16:27:47, pastarmovj wrote: > I think it still makes sense to only parse numbered keys. The code as it is now > will parse a key with arbitrary names. Sometimes while testing polivies admins > tend to rename them to switch them off e.g. "HomePageUrl" -> "_HomePageUrl" one > can imagine they would do this to the indexed entries too so skipping anything > that is not a number sounds like a good idea to me. Wdyt? > > If you agree about this change please augment the test to capture this too. Skipping over non-integer names makes sense. We won't enforce that integers have to be increasing, however. I don't see any issues with that, WDYT? ie. 1, 5, 7, 3, 6 would be valid.
Description was changed from ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more persmissive. Basically, the names of the entries is ignored, we only retain the values. BUG=655774 ========== to ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more persmissive. The names still have to be integers, but need not be sequential. BUG=655774 ==========
Description was changed from ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more persmissive. The names still have to be integers, but need not be sequential. BUG=655774 ========== to ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more permissive. The names still have to be integers, but need not be sequential. BUG=655774 ==========
The CQ bit was checked by georgesak@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 georgesak@chromium.org to run a CQ dry run
Patchset #2 (id:20001) 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 checked by georgesak@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...
Patchset #2 (id:40001) has been deleted
PTAnL. Added numeric check. https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... File components/policy/core/common/registry_dict_win.cc (right): https://codereview.chromium.org/2478503002/diff/1/components/policy/core/comm... components/policy/core/common/registry_dict_win.cc:326: entry != keys_.end(); ++entry) { On 2016/11/03 18:16:16, Georges Khalil wrote: > On 2016/11/03 16:27:47, pastarmovj wrote: > > I think it still makes sense to only parse numbered keys. The code as it is > now > > will parse a key with arbitrary names. Sometimes while testing polivies admins > > tend to rename them to switch them off e.g. "HomePageUrl" -> "_HomePageUrl" > one > > can imagine they would do this to the indexed entries too so skipping anything > > that is not a number sounds like a good idea to me. Wdyt? > > > > If you agree about this change please augment the test to capture this too. > > Skipping over non-integer names makes sense. > > We won't enforce that integers have to be increasing, however. I don't see any > issues with that, WDYT? ie. 1, 5, 7, 3, 6 would be valid. I just realized that a dictionary uses a map instead of an unordered_map, so that's irrelevant, the number will always be increasing. I'm adding just the numerical check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by georgesak@chromium.org
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 ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more permissive. The names still have to be integers, but need not be sequential. BUG=655774 ========== to ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more permissive. The names still have to be integers, but need not be sequential. BUG=655774 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more permissive. The names still have to be integers, but need not be sequential. BUG=655774 ========== to ========== Enable non-sequential entries when loading registry lists. This CL makes the loading much more permissive. The names still have to be integers, but need not be sequential. BUG=655774 Committed: https://crrev.com/1a839edc0bc326d05f368ff00ce5a9e3b10d3041 Cr-Commit-Position: refs/heads/master@{#429870} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1a839edc0bc326d05f368ff00ce5a9e3b10d3041 Cr-Commit-Position: refs/heads/master@{#429870} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
