Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180243021:
Add google_update::GetUntrustedDataValueFromTag() (Closed)
|
Created:
6 years, 9 months ago by jackhou1 Modified:
6 years, 9 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd google_update::GetUntrustedDataValueFromTag()
This is similar to GetUntrustedDataValue() but allows
extracting a value from an arbitrary tag.
BUG=341353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256091
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Sync and rebase #Patch Set 4 : Address comments #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:135: bool ParseUntrustedData( add doc comment https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:170: if (env == NULL || !env->GetVar(kEnvVariableUntrustedData, &data_string)) i think "!env" is more idomatic for testing that a scoped ptr is NULL. this may not have been the case when the code you've moved was written. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:178: VLOG(1) << kEnvVariableUntrustedData << ": " << data_string; don't log data_string here since IsStringPrintable has yet to have been used to validate it. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:233: std::string GetUntrustedDataValueFromTag(const std::string& tag, should the size of |tag| be restricted in the same way as the env var (kEnvVariableUntrustedDataMaxLength)? if no, why not? https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:236: if (ParseUntrustedData(tag, &untrusted_data)) { without the logging calls (see next comment), i believe that: if (ParseUntrustedData(tag, &untrusted_data)) return untrusted_data[key]; return std::string(); will accomplish the same thing as lines 236-247. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:240: VLOG(1) << "Key " << key << " is " << data_it->second; coding style says "Remove most logging calls before checking in." Unless there's a strong diagnostic reason for the log statements you've added to stay, please remove them. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.h:31: // |tag|. Returns an empty string if |key| is absent or if its value please document the expected format of |tag|.
https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:135: bool ParseUntrustedData( On 2014/03/03 15:18:26, grt wrote: > add doc comment Done. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:170: if (env == NULL || !env->GetVar(kEnvVariableUntrustedData, &data_string)) On 2014/03/03 15:18:26, grt wrote: > i think "!env" is more idomatic for testing that a scoped ptr is NULL. this may > not have been the case when the code you've moved was written. Done. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:178: VLOG(1) << kEnvVariableUntrustedData << ": " << data_string; On 2014/03/03 15:18:26, grt wrote: > don't log data_string here since IsStringPrintable has yet to have been used to > validate it. Done. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:233: std::string GetUntrustedDataValueFromTag(const std::string& tag, On 2014/03/03 15:18:26, grt wrote: > should the size of |tag| be restricted in the same way as the env var > (kEnvVariableUntrustedDataMaxLength)? if no, why not? Done. I'm don't know if there is a specific reason the env var needs to be restricted to that size. But it seems reasonable that any string read from elsewhere should be restricted to a sane size. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:236: if (ParseUntrustedData(tag, &untrusted_data)) { On 2014/03/03 15:18:26, grt wrote: > without the logging calls (see next comment), i believe that: > if (ParseUntrustedData(tag, &untrusted_data)) > return untrusted_data[key]; > return std::string(); > will accomplish the same thing as lines 236-247. Done. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.cc:240: VLOG(1) << "Key " << key << " is " << data_it->second; On 2014/03/03 15:18:26, grt wrote: > coding style says "Remove most logging calls before checking in." Unless there's > a strong diagnostic reason for the log statements you've added to stay, please > remove them. Done. https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/180243021/diff/1/chrome/installer/util/google... chrome/installer/util/google_update_util.h:31: // |tag|. Returns an empty string if |key| is absent or if its value On 2014/03/03 15:18:26, grt wrote: > please document the expected format of |tag|. Done.
lgtm w/ nit https://codereview.chromium.org/180243021/diff/20001/chrome/installer/util/go... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/180243021/diff/20001/chrome/installer/util/go... chrome/installer/util/google_update_util.cc:147: LOG(ERROR) << "Invalid value in untrusted data string."; i don't think we need two log statements for the conditions on line 141 and 146. please lump them back together as they once were.
The CQ bit was checked by jackhou@chromium.org
https://codereview.chromium.org/180243021/diff/20001/chrome/installer/util/go... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/180243021/diff/20001/chrome/installer/util/go... chrome/installer/util/google_update_util.cc:147: LOG(ERROR) << "Invalid value in untrusted data string."; On 2014/03/04 15:55:59, grt wrote: > i don't think we need two log statements for the conditions on line 141 and 146. > please lump them back together as they once were. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/180243021/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/180243021/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/180243021/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/180243021/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/180243021/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/180243021/60001
Message was sent while issue was closed.
Change committed as 256091 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
