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

http://crrev.com/988863002/ triggered use-after-free in ASAN because it passes
  ("#" + color_str).c_str()
to FindColor() (which takes char*), then dereferences the returned pointer, which
has been deallocated.  This is a redo of the CL that fixes the problem.

BUG=455063

Review URL: https://codereview.chromium.org/989313002

Cr-Commit-Position: refs/heads/master@{#319875}

chrome/browser/ui/webui/fallback_icon_source.h

@@ -20,11 +20,11 @@
 //  'size'
 //    Positive integer to specify the fallback icon's size in pixels.
 //  'bc'
-//    Fallback icon's background color, as named CSS color, or #RGB / #RRGGBB /
-//    #AARRGGBB hex formats.
+//    Fallback icon's background color, as named CSS color, or RGB / ARGB /
+//    RRGGBB / AARRGGBB hex formats (no leading "#").
 //  'tc'
-//    Fallback icon text color, as named CSS color, or #RGB / #RRGGBB /
-//    #AARRGGBB hex formats.
+//    Fallback icon text color, as named CSS color, or RGB / ARGB / RRGGBB /
+//    AARRGGBB hex formats (no leading "#").
 //  'fsr'
 //    Number in [0.0, 1.0] to specify the fallback icon's font size (pixels)
 //    as a ratio to the icon's size.

chrome/common/favicon/fallback_icon_url_parser.cc

@@ -4,12 +4,36 @@
 
 #include "chrome/common/favicon/fallback_icon_url_parser.h"
 
+#include <algorithm>
+
 #include "base/logging.h"
+#include "base/macros.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 {
+
+// List of sizes corresponding to RGB, ARGB, RRGGBB, AARRGGBB.
+const size_t kValidHexColorSizes[] = {3, 4, 6, 8};
+
+// 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();
+  const size_t* end = kValidHexColorSizes + arraysize(kValidHexColorSizes);
+  if (std::find(kValidHexColorSizes, end, len) == end)
+    return false;
+  for (auto ch : color_str)
+    if (!IsHexDigit(ch))
+      return false;
+  return true;
+}
+
+}  // namespace
+
 namespace chrome {
 
 ParsedFallbackIconPath::ParsedFallbackIconPath()
@@ -76,9 +100,32 @@ bool ParsedFallbackIconPath::ParseSpecs(
 // 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, but in URL '#' is used 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)) {
+    // Default alpha to 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);  // Call should succeed and consume string.
+    return true;
+  }
+
+  // Default alpha to 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

chrome/common/favicon/fallback_icon_url_parser.h

@@ -38,8 +38,8 @@ class ParsedFallbackIconPath {
                          int *size,
                          favicon_base::FallbackIconStyle* style);
 
-  // Helper to parse color string (e.g., "red", "#f00", "#fF0000"). Returns true
-  // on success.
+  // Helper to parse color string (e.g., "red", "#f00", "#aB0137"). On success,
+  // returns true and writes te result to |*color|.
   static bool ParseColor(const std::string& color_str, SkColor* color);
 
   friend class FallbackIconUrlParserTest;

chrome/common/favicon/fallback_icon_url_parser_unittest.cc

@@ -42,13 +42,7 @@ class FallbackIconUrlParserTest : public testing::Test {
   }
 
   bool ParseColor(const std::string& color_str, SkColor* color) {
-    int size_dummy;
-    favicon_base::FallbackIconStyle style;
-    std::string spec_str = "16," + color_str + ",,,";
-    if (!ParseSpecs(spec_str, &size_dummy, &style))
-      return false;
-    *color = style.background_color;
-    return true;
+    return ParsedFallbackIconPath::ParseColor(color_str, color);
   }
 
  private:
@@ -57,13 +51,15 @@ class FallbackIconUrlParserTest : public testing::Test {
 
 TEST_F(FallbackIconUrlParserTest, ParseColorSuccess) {
   SkColor c;
-  EXPECT_TRUE(ParseColor("#01aBf0f4", &c));
-  EXPECT_EQ(SkColorSetARGB(0x01, 0xAB, 0xF0, 0xF4), c);
-  EXPECT_TRUE(ParseColor("#01aBf0", &c));
+  EXPECT_TRUE(ParseColor("31aBf0f4", &c));
+  EXPECT_EQ(SkColorSetARGB(0x31, 0xAB, 0xF0, 0xF4), c);
+  EXPECT_TRUE(ParseColor("01aBf0", &c));
   EXPECT_EQ(SkColorSetRGB(0x01, 0xAB, 0xF0), c);
-  EXPECT_TRUE(ParseColor("#01a", &c));
+  EXPECT_TRUE(ParseColor("501a", &c));
+  EXPECT_EQ(SkColorSetARGB(0x55, 0x00, 0x11, 0xAA), c);
+  EXPECT_TRUE(ParseColor("01a", &c));
   EXPECT_EQ(SkColorSetRGB(0x00, 0x11, 0xAA), c);
-  EXPECT_TRUE(ParseColor("#000000", &c));
+  EXPECT_TRUE(ParseColor("000000", &c));
   EXPECT_EQ(SkColorSetARGB(0xFF, 0x00, 0x00, 0x00), c);
   EXPECT_TRUE(ParseColor("red", &c));
   EXPECT_EQ(SkColorSetARGB(0xFF, 0xFF, 0x00, 0x00), c);
@@ -71,12 +67,16 @@ TEST_F(FallbackIconUrlParserTest, ParseColorSuccess) {
 
 TEST_F(FallbackIconUrlParserTest, ParseColorFailure) {
   const char* test_cases[] = {
-    "#00000",
-    "#000000000",
-    " #000000",
-    "#ABCDEFG",
-    "000000",
-    "#000000 ",
+    "",
+    "00000",
+    "000000000",
+    " 000000",
+    "ABCDEFG",
+    "#000",
+    "#000000",
+    "000000 ",
+    "ABCDEFH",
+    "#ABCDEF",
   };
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
     SkColor c;
@@ -99,7 +99,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsEmpty) {
 TEST_F(FallbackIconUrlParserTest, ParseSpecsPartial) {
   int size;
   FallbackIconStyle style;
-  EXPECT_TRUE(ParseSpecs(",,#aCE,,0.1", &size, &style));
+  EXPECT_TRUE(ParseSpecs(",,aCE,,0.1", &size, &style));
   EXPECT_EQ(16, size);
   EXPECT_EQ(kDefaultBackgroundColor, style.background_color);
   EXPECT_EQ(SkColorSetRGB(0xAA, 0xCC, 0xEE), style.text_color);
@@ -112,7 +112,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFull) {
 
   {
     FallbackIconStyle style;
-    EXPECT_TRUE(ParseSpecs("16,#000,#f01,0.75,0.25", &size, &style));
+    EXPECT_TRUE(ParseSpecs("16,000,f01,0.75,0.25", &size, &style));
     EXPECT_EQ(16, size);
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
     EXPECT_EQ(SkColorSetRGB(0xff, 0x00, 0x11), style.text_color);
@@ -122,7 +122,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFull) {
 
   {
     FallbackIconStyle style;
-    EXPECT_TRUE(ParseSpecs("48,black,#123456,0.5,0.3", &size, &style));
+    EXPECT_TRUE(ParseSpecs("48,black,123456,0.5,0.3", &size, &style));
     EXPECT_EQ(48, size);
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
     EXPECT_EQ(SkColorSetRGB(0x12, 0x34, 0x56), style.text_color);
@@ -132,7 +132,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFull) {
 
   {
     FallbackIconStyle style;
-    EXPECT_TRUE(ParseSpecs("1,#000,red,0,0", &size, &style));
+    EXPECT_TRUE(ParseSpecs("1,000,red,0,0", &size, &style));
     EXPECT_EQ(1, size);
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
     EXPECT_EQ(SkColorSetRGB(0xFF, 0x00, 0x00), style.text_color);
@@ -147,21 +147,21 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsDefaultTextColor) {
   {
     // Dark background -> Light text.
     FallbackIconStyle style;
-    EXPECT_TRUE(ParseSpecs(",#000,,,", &size, &style));
+    EXPECT_TRUE(ParseSpecs(",000,,,", &size, &style));
     EXPECT_EQ(kDefaultTextColorLight, style.text_color);
   }
 
   {
     // Light background -> Dark text.
     FallbackIconStyle style;
-    EXPECT_TRUE(ParseSpecs(",#fff,,,", &size, &style));
+    EXPECT_TRUE(ParseSpecs(",fff,,,", &size, &style));
     EXPECT_EQ(kDefaultTextColorDark, style.text_color);
   }
 
   {
     // Light background -> Dark text, more params don't matter.
     FallbackIconStyle style;
-    EXPECT_TRUE(ParseSpecs("107,#fff,,0.3,0.5", &size, &style));
+    EXPECT_TRUE(ParseSpecs("107,fff,,0.3,0.5", &size, &style));
     EXPECT_EQ(kDefaultTextColorDark, style.text_color);
   }
 }
@@ -172,30 +172,30 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFailure) {
     "",
     "16",
     "16,black",
-    "16,black,#fff",
-    "16,black,#fff,0.75",
+    "16,black,fff",
+    "16,black,fff,0.75",
     ",,,"
     ",,,,,",
-    "16,black,#fff,0.75,0.25,junk",
+    "16,black,fff,0.75,0.25,junk",
     // Don't allow any space.
-    "16,black,#fff, 0.75,0.25",
-    "16,black ,#fff,0.75,0.25",
-    "16,black,#fff,0.75,0.25 ",
+    "16,black,fff, 0.75,0.25",
+    "16,black ,fff,0.75,0.25",
+    "16,black,fff,0.75,0.25 ",
     // Adding junk text.
-    "16,black,#fff,0.75,0.25junk",
-    "junk,black,#fff,0.75,0.25",
-    "16,#junk,#fff,0.75,0.25",
-    "16,black,#junk,0.75,0.25",
-    "16,black,#fff,junk,0.25",
-    "16,black,#fff,0.75,junk",
+    "16,black,fff,0.75,0.25junk",
+    "junk,black,fff,0.75,0.25",
+    "16,#junk,fff,0.75,0.25",
+    "16,black,junk,0.75,0.25",
+    "16,black,fff,junk,0.25",
+    "16,black,fff,0.75,junk",
     // Out of bound.
-    "0,black,#fff,0.75,0.25",  // size.
-    "4294967296,black,#fff,0.75,0.25",  // size.
-    "-1,black,#fff,0.75,0.25",  // size.
-    "16,black,#fff,-0.1,0.25",  // font_size_ratio.
-    "16,black,#fff,1.1,0.25",  // font_size_ratio.
-    "16,black,#fff,0.75,-0.1",  // roundness.
-    "16,black,#fff,0.75,1.1",  // roundness.
+    "0,black,fff,0.75,0.25",  // size.
+    "4294967296,black,fff,0.75,0.25",  // size.
+    "-1,black,fff,0.75,0.25",  // size.
+    "16,black,fff,-0.1,0.25",  // font_size_ratio.
+    "16,black,fff,1.1,0.25",  // font_size_ratio.
+    "16,black,fff,0.75,-0.1",  // roundness.
+    "16,black,fff,0.75,1.1",  // roundness.
   };
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
     int size;
@@ -207,7 +207,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFailure) {
 }
 
 TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathSuccess) {
-  const std::string specs = "31,black,#fff,0.75,0.25";
+  const std::string specs = "31,black,fff,0.75,0.25";
 
   // Everything populated.
   {
@@ -252,11 +252,11 @@ TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathSuccess) {
 TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathFailure) {
   const char* test_cases[] = {
     // Bad size.
-    "-1,#000,#fff,0.75,0.25/http://www.google.com/",
+    "-1,000,fff,0.75,0.25/http://www.google.com/",
     // Bad specs.
-    "32,#junk,#fff,0.75,0.25/http://www.google.com/",
+    "32,#junk,fff,0.75,0.25/http://www.google.com/",
     // Bad URL.
-    "32,#000,#fff,0.75,0.25/NOT A VALID URL",
+    "32,000,fff,0.75,0.25/NOT A VALID URL",
   };
   for (size_t i = 0; i < arraysize(test_cases); ++i) {
     chrome::ParsedFallbackIconPath parsed;