Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13441008:
Add parser for PReg files. (Closed)
|
Created:
7 years, 8 months ago by Mattias Nissler (ping if slow) Modified:
7 years, 8 months ago CC:
chromium-reviews, Joao da Silva Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd parser for PReg files.
These files are used on Windows to store applied GPO settings. The parser will
be used to read PReg files directly to obtain Chrome policy if possible.
BUG=chromium:186445
TEST=unit tests
TBR=ben@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193437
Patch Set 1 #
Total comments: 34
Patch Set 2 : Address comments. #
Total comments: 10
Patch Set 3 : Address comments. #Patch Set 4 : One last small fix. #Patch Set 5 : Break out binary test file. #
Messages
Total messages: 16 (0 generated)
Here's the first piece for reading GPO directly.
Almost stamped but I'd like to hear why am i wrong here : chrome/browser/policy/preg_parser_win.cc:178 :) And also please verify that the binary file is actually properly uploaded or dcommit the patch in the end. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win.cc (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:178: value_name.substr(arraysize(kActionTriggerPrefix) - 1 + Isn't here one -1 too much? I suspect this worked since the last char is a . maybe? https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:213: // http://msdn.microsoft.com/en-us/library/windows/desktop/aa374407(v=vs.85).aspx you can skip microsoft. and the address is still valid - msdn.com exists. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:266: break; Why not directly return true here? https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:270: if (parse_error) { ...then you can simply assume you encountered an error if you get here at all.
Looks good but needs some fixes, see inline. The pol file is also empty, I guess that's why the tests fail in the bots. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win.cc (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:62: *cursor = reinterpret_cast<const char16*>(end_bytes); Unfortunately the documentation doesn't mention what happens wrt alignment here, when size is odd; I guess this is fine. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:92: const size_t len = std::max(0U, (data.size() / sizeof(char16)) - 1); If data.size() is 0 then the 2nd arg to max() becomes SIZE_MAX. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:127: default: Why have some cases and a default, and not just default? https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:169: StartsWithASCII(kActionTriggerDeleteKeys, action_trigger, true)) { The first 2 args to StartsWithASCII are swapped https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:182: dict->Clear(); The documentation of DelVals says that it deletes all the values, but this also deletes all the keys. Keys in |dict| mapping to another DictionaryValue should be kept. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:184: StartsWithASCII(action_trigger, kActionTriggerSoft, true)) { Where did you find about "Soft"? The linked document doesn't describe that. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:213: // http://msdn.microsoft.com/en-us/library/windows/desktop/aa374407(v=vs.85).aspx Suggestion: point to the documentation at the top of the file https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:217: mapped_file.data() + mapped_file.length()); What happens if the file ends in a byte i.e. half a char16? I guess if the alignment of |end| isn't right then the file should be rejected. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:253: data.resize(size); We should impose a size limit, otherwise borked files will prevent chrome from running. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win.h (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.h:6: #define CHROME_BROWSER_POLICY_PREG_PARSER_WIN_H nit: the header guard macros usually end in _H_ https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.h:12: #include "chrome/browser/policy/policy_types.h" Not needed in the header https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.h:23: base::DictionaryValue* dict); Forward decl? https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win_unittest.cc (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win_unittest.cc:14: #include "base/logging.h" List all the #includes together?
Thanks for your valuable comments, new version is up for review. Anyone know how to run tryjobs with binary files? If not, I'll commit the file first, run a try job with the code to verify and then commit the code. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win.cc (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:62: *cursor = reinterpret_cast<const char16*>(end_bytes); On 2013/04/05 12:51:33, Joao da Silva wrote: > Unfortunately the documentation doesn't mention what happens wrt alignment here, > when size is odd; I guess this is fine. Standard says this is implementation-defined. I've changed the parsing to work with uint8 now to be on the safe side. Who knows, at some point this will run on ARM? ;) https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:92: const size_t len = std::max(0U, (data.size() / sizeof(char16)) - 1); On 2013/04/05 12:51:33, Joao da Silva wrote: > If data.size() is 0 then the 2nd arg to max() becomes SIZE_MAX. Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:127: default: On 2013/04/05 12:51:33, Joao da Silva wrote: > Why have some cases and a default, and not just default? To document that we're purposely not implementing those, and that this is the list that I have considered. If some windows header has additional ones, we'll know that I didn't have them on the radar when writing this. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:169: StartsWithASCII(kActionTriggerDeleteKeys, action_trigger, true)) { On 2013/04/05 12:51:33, Joao da Silva wrote: > The first 2 args to StartsWithASCII are swapped Good catch. Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:178: value_name.substr(arraysize(kActionTriggerPrefix) - 1 + On 2013/04/05 08:41:43, pastarmovj wrote: > Isn't here one -1 too much? I suspect this worked since the last char is a . > maybe? Note that the -1 here is just for taking the terminating '\0' character into account. But let's see, the trigger might be: **del.foobar where: arraysize(kActionTriggerPrefix) = 3 arraysize(kActionTriggerDel) = 5 so we call value_name.substr(3 - 1 + 5 - 1), i.e. value.substr(6), which is "foobar". Seems correct to me? https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:182: dict->Clear(); On 2013/04/05 12:51:33, Joao da Silva wrote: > The documentation of DelVals says that it deletes all the values, but this also > deletes all the keys. > > Keys in |dict| mapping to another DictionaryValue should be kept. Interesting edge case :) Fixed. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:184: StartsWithASCII(action_trigger, kActionTriggerSoft, true)) { On 2013/04/05 12:51:33, Joao da Silva wrote: > Where did you find about "Soft"? The linked document doesn't describe that. Apparently Microsoft used this at some point. I found code on the net that checks for this, so I figured I'd be liberal in what I expect. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:213: // http://msdn.microsoft.com/en-us/library/windows/desktop/aa374407(v=vs.85).aspx On 2013/04/05 08:41:43, pastarmovj wrote: > you can skip microsoft. and the address is still valid - http://msdn.com exists. That redirects though, and the URL I have seems to be the canonical one, so I'd rather keep it. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:213: // http://msdn.microsoft.com/en-us/library/windows/desktop/aa374407(v=vs.85).aspx On 2013/04/05 12:51:33, Joao da Silva wrote: > Suggestion: point to the documentation at the top of the file Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:217: mapped_file.data() + mapped_file.length()); On 2013/04/05 12:51:33, Joao da Silva wrote: > What happens if the file ends in a byte i.e. half a char16? > > I guess if the alignment of |end| isn't right then the file should be rejected. Fixed. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:253: data.resize(size); On 2013/04/05 12:51:33, Joao da Silva wrote: > We should impose a size limit, otherwise borked files will prevent chrome from > running. Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:266: break; On 2013/04/05 08:41:43, pastarmovj wrote: > Why not directly return true here? Good idea, done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.cc:270: if (parse_error) { On 2013/04/05 08:41:43, pastarmovj wrote: > ...then you can simply assume you encountered an error if you get here at all. Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win.h (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.h:6: #define CHROME_BROWSER_POLICY_PREG_PARSER_WIN_H On 2013/04/05 12:51:33, Joao da Silva wrote: > nit: the header guard macros usually end in _H_ Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.h:12: #include "chrome/browser/policy/policy_types.h" On 2013/04/05 12:51:33, Joao da Silva wrote: > Not needed in the header Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win.h:23: base::DictionaryValue* dict); On 2013/04/05 12:51:33, Joao da Silva wrote: > Forward decl? Done. https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... File chrome/browser/policy/preg_parser_win_unittest.cc (right): https://codereview.chromium.org/13441008/diff/1/chrome/browser/policy/preg_pa... chrome/browser/policy/preg_parser_win_unittest.cc:14: #include "base/logging.h" On 2013/04/05 12:51:33, Joao da Silva wrote: > List all the #includes together? Done.
lgtm after maybe fixing the case-sensitivity issue. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... File chrome/browser/policy/preg_parser_win.cc (right): https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:53: int result = **cursor | (*(*cursor + 1) << 8); I suppose the file format always uses LE. But if you think this doesn't work on BE archs then add a compile-time assertion that ARCH_CPU_LITTLE_ENDIAN is defined here. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:187: // Delete a values, i.e. keep dictionary entries. Delete all values https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:218: if (mapped_file.length() > kMaxPRegFileSize) { Is 16MB a reasonable size? Let's gather UMA stats on file sizes found in the wild. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:268: if (size > kMaxPRegFileSize) UMA sample? https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:280: if (StartsWith(key_name, root, true) && Registry keys and values are case-insensitive, shouldn't the 3rd arg be false?
The registry.pol file is still empty, and that's likely the reason that the test fails on the bots.
Updated, PTAL. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... File chrome/browser/policy/preg_parser_win.cc (right): https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:53: int result = **cursor | (*(*cursor + 1) << 8); On 2013/04/08 12:31:25, Joao da Silva wrote: > I suppose the file format always uses LE. But if you think this doesn't work on > BE archs then add a compile-time assertion that ARCH_CPU_LITTLE_ENDIAN is > defined here. We're reading byte-wise here, so this will work also on big-endian machines. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:187: // Delete a values, i.e. keep dictionary entries. On 2013/04/08 12:31:25, Joao da Silva wrote: > Delete all values Done. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:218: if (mapped_file.length() > kMaxPRegFileSize) { On 2013/04/08 12:31:25, Joao da Silva wrote: > Is 16MB a reasonable size? Let's gather UMA stats on file sizes found in the > wild. We probably want UMA on this, but probably at a different level, i.e. gathering data on how often we fall back to the registry and for what reason. I'll add that in a later CL. https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:268: if (size > kMaxPRegFileSize) On 2013/04/08 12:31:25, Joao da Silva wrote: > UMA sample? See above, the error return is good enough for now (the fallback will trigger). https://codereview.chromium.org/13441008/diff/11001/chrome/browser/policy/pre... chrome/browser/policy/preg_parser_win.cc:280: if (StartsWith(key_name, root, true) && On 2013/04/08 12:31:25, Joao da Silva wrote: > Registry keys and values are case-insensitive, shouldn't the 3rd arg be false? Done.
lgtm from my side
Alright, I'm going to break out the binary files and get this landed if there are no further concerns.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/13441008/33002
Presubmit check for 13441008-33002 failed and returned exit status 1.
INFO:root:Found 5 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/chrome_browser.gypi
TBR ben@chromium.org for the gyp changes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/13441008/33002
TBR Ben for realz.
gyp lgtm
Message was sent while issue was closed.
Change committed as 193437 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
