Add google_update::GetUntrustedDataValueFromTag()

chrome/installer/util/google_update_util.cc

Index: chrome/installer/util/google_update_util.cc
diff --git a/chrome/installer/util/google_update_util.cc b/chrome/installer/util/google_update_util.cc
index 748e3f6c2fe07824bf7ec6711f8c225b143aff0d..7681e03885ee4b270a6a0ad93470d913fdd0de17 100644
--- a/chrome/installer/util/google_update_util.cc
+++ b/chrome/installer/util/google_update_util.cc
@@ -132,25 +132,13 @@ bool IsUntrustedDataKeyValid(const std::string& key) {
       == key.end();
 }
 
-// Reads and parses untrusted data passed from Google Update as key-value
-// pairs, then overwrites |untrusted_data_map| with the result.
-// Returns true if data are successfully read.
-bool GetGoogleUpdateUntrustedData(
+bool ParseUntrustedData(

[grt (UTC plus 2), 2014/03/03 15:18:26]

add doc comment

[jackhou1, 2014/03/04 00:56:28]

Done.

+    const std::string& data_string,
     std::map<std::string, std::string>* untrusted_data) {
-  DCHECK(untrusted_data);
-  scoped_ptr<base::Environment> env(base::Environment::Create());
-  std::string data_string;
-  if (env == NULL || !env->GetVar(kEnvVariableUntrustedData, &data_string))
-    return false;
-
-  if (data_string.length() > kEnvVariableUntrustedDataMaxLength ||
-      !IsStringPrintable(data_string)) {
-    LOG(ERROR) << "Invalid value in " << kEnvVariableUntrustedData;
+  if (!IsStringPrintable(data_string)) {
+    LOG(ERROR) << "Invalid value in untrusted data string.";
     return false;
   }
-
-  VLOG(1) << kEnvVariableUntrustedData << ": " << data_string;
-
   std::vector<std::pair<std::string, std::string> > kv_pairs;
   if (!base::SplitStringIntoKeyValuePairs(data_string, '=', '&', &kv_pairs)) {
     LOG(ERROR) << "Failed to parse untrusted data: " << data_string;
@@ -171,6 +159,27 @@ bool GetGoogleUpdateUntrustedData(
   return true;
 }
 
+// Reads and parses untrusted data passed from Google Update as key-value
+// pairs, then overwrites |untrusted_data_map| with the result.
+// Returns true if data are successfully read.
+bool GetGoogleUpdateUntrustedData(
+    std::map<std::string, std::string>* untrusted_data) {
+  DCHECK(untrusted_data);
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  std::string data_string;
+  if (env == NULL || !env->GetVar(kEnvVariableUntrustedData, &data_string))

[grt (UTC plus 2), 2014/03/03 15:18:26]

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.

[jackhou1, 2014/03/04 00:56:28]

Done.

+    return false;
+
+  if (data_string.length() > kEnvVariableUntrustedDataMaxLength) {
+    LOG(ERROR) << kEnvVariableUntrustedData << " is too long.";
+    return false;
+  }
+
+  VLOG(1) << kEnvVariableUntrustedData << ": " << data_string;

[grt (UTC plus 2), 2014/03/03 15:18:26]

don't log data_string here since IsStringPrintable has yet to have been used to
validate it.

[jackhou1, 2014/03/04 00:56:28]

Done.

+
+  return ParseUntrustedData(data_string, untrusted_data);
+}
+
 }  // namespace
 
 bool EnsureUserLevelGoogleUpdatePresent() {
@@ -221,4 +230,21 @@ std::string GetUntrustedDataValue(const std::string& key) {
   return std::string();
 }
 
+std::string GetUntrustedDataValueFromTag(const std::string& tag,

[grt (UTC plus 2), 2014/03/03 15:18:26]

should the size of |tag| be restricted in the same way as the env var
(kEnvVariableUntrustedDataMaxLength)? if no, why not?

[jackhou1, 2014/03/04 00:56:28]

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.

+                                         const std::string& key) {
+  std::map<std::string, std::string> untrusted_data;
+  if (ParseUntrustedData(tag, &untrusted_data)) {

[grt (UTC plus 2), 2014/03/03 15:18:26]

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.

[jackhou1, 2014/03/04 00:56:28]

Done.

+    std::map<std::string, std::string>::const_iterator data_it(
+        untrusted_data.find(key));
+    if (data_it != untrusted_data.end()) {
+      VLOG(1) << "Key " << key << " is " << data_it->second;

[grt (UTC plus 2), 2014/03/03 15:18:26]

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.

[jackhou1, 2014/03/04 00:56:28]

Done.

+      return data_it->second;
+    } else {
+      VLOG(1) << "Key not found: " << key;
+    }
+  }
+
+  return std::string();
+}
+
 }  // namespace google_update