[Fallback icons] Redoing http://crrev.com/988863002/, fixing use-after-free bug.

chrome/common/favicon/fallback_icon_url_parser.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/favicon/fallback_icon_url_parser.h"
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "third_party/skia/include/utils/SkParse.h"
 #include "ui/gfx/favicon_size.h"
 
+namespace {
+
+// Returns whether |color_str| is a valid CSS color in hex format if we prepend
+// '#', i.e., whether |color_str| matches /^[0-9A-Fa-f]{3,4,6,8}$/.
+bool IsHexColorString(const std::string& color_str) {
+  size_t len = color_str.length();
+  if (len != 3 && len != 4 && len != 6 && len != 8)

[James Hawkins, 2015/03/09 20:27:38]

This could be cleaner as:

std::vector<int> lengths = {3, 4, 6, 8};
if (std::find(lengths.begin(), lengths.end(), len) != lengths.end()) {
  return false;
}

What do you think?

[huangs, 2015/03/09 20:44:56]

I prefer to list all the numbers, but I guess having the numbers together
improves readability.  Compromising by unsigned int array with std::find() so we
won't have to involves std::vector and memory allocation.

+    return false;
+  for (auto ch : color_str)
+    if (!IsHexDigit(ch))
+      return false;
+  return true;
+}
+
+}  // namespace
+
 namespace chrome {
 
 ParsedFallbackIconPath::ParsedFallbackIconPath()
     : size_in_pixels_(gfx::kFaviconSize) {
 }
 
 ParsedFallbackIconPath::~ParsedFallbackIconPath() {
 }
 
 bool ParsedFallbackIconPath::Parse(const std::string& path) {
@@ skipping 46 matching lines @@
 
   if (!tokens[4].empty() && !base::StringToDouble(tokens[4], &style->roundness))
     return false;
 
   return favicon_base::ValidateFallbackIconStyle(*style);
 }
 
 // static
 bool ParsedFallbackIconPath::ParseColor(const std::string& color_str,
                                         SkColor* color) {
-  const char* end = SkParse::FindColor(color_str.c_str(), color);
-  // Return true if FindColor() succeeds and |color_str| is entirely consumed.
-  return end && !*end;
+  DCHECK(color);
+  // Exclude the empty case. Also disallow the '#' prefix, since we want color
+  // to be part of an URL, where '#' is reserved for ref fragment.
+  if (color_str.empty() || color_str[0] == '#')
+    return false;
+
+  // If a valid color hex string is given, prepend '#' and parse (always works).
+  // This is unambiguous since named color never only use leters 'a' to 'f'.
+  if (IsHexColorString(color_str)) {
+    // Force alpha = 0xFF since FindColor() preserves unspecified alpha.
+    *color = SK_ColorWHITE;
+    // Need temp variable to avoid use-after-free of returned pointer.
+    std::string color_str_with_hash = "#" + color_str;
+    const char* end = SkParse::FindColor(color_str_with_hash.c_str(), color);
+    DCHECK(end && !*end);  // Ensure call succees and string is consumed.

[James Hawkins, 2015/03/09 20:27:38]

nit: s/succees/succeeds/

[huangs, 2015/03/09 20:44:56]

Done, also updated comments.

+    return true;
+  }
+
+  // Force alpha = 0xFF.
+  SkColor temp_color = SK_ColorWHITE;
+  const char* end = SkParse::FindColor(color_str.c_str(), &temp_color);
+  if (end && !*end) {  // Successful if call succeeds and string is consumed.
+    *color = temp_color;
+    return true;
+  }
+  return false;
 }
 
 }  // namespace chrome