Added UMA measurement for how often we see control characters during cookie parsing.

net/cookies/parsed_cookie.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[erikwright (departed), 2013/05/24 14:15:56]

Was this removed intentionally?

[jww, 2013/05/24 17:56:33]

Nope, not at all. Fixed.
On 2013/05/24 14:15:56, erikwright wrote:

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
@@ skipping 26 matching lines @@
  * decision by deleting the provisions above and replace them with the notice
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "net/cookies/parsed_cookie.h"
 
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/string_util.h"
 
 namespace {
 
 const char kPathTokenName[] = "path";
 const char kDomainTokenName[] = "domain";
 const char kExpiresTokenName[] = "expires";
 const char kMaxAgeTokenName[] = "max-age";
 const char kSecureTokenName[] = "secure";
 const char kHttpOnlyTokenName[] = "httponly";
@@ skipping 65 matching lines @@
   if (value.size() >= 2 && *value.begin() == '"' && *(value.end()-1) == '"')
     skip = 1;
   for (std::string::const_iterator i = value.begin() + skip;
        i != value.end() - skip; ++i) {
     bool valid_octet =
         (*i == 0x21 ||
          (*i >= 0x23 && *i <= 0x2B) ||
          (*i >= 0x2D && *i <= 0x3A) ||
          (*i >= 0x3C && *i <= 0x5B) ||
          (*i >= 0x5D && *i <= 0x7E));
-    if (!valid_octet)
+    if (!valid_octet) {
+      UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieValue", false);

[erikwright (departed), 2013/05/24 14:15:56]

This will be called for changing the value but not changing the name via the
setters.

Consider moving it to SetName and SetValue instead.

[jww, 2013/05/24 17:56:33]

Done.

       return false;
+    }
   }
+  UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieValue", true);
   return true;
 }
 
 bool IsValidCookieAttributeValue(const std::string& value) {
   // The greatest common denominator of cookie attribute values is
   // <any CHAR except CTLs or ";"> according to RFC 6265.
   for (std::string::const_iterator i = value.begin(); i != value.end(); ++i) {
-    if ((*i >= 0 && *i <= 31) || *i == ';')
+    if ((*i >= 0 && *i <= 31) || *i == ';') {
+      UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", false);

[erikwright (departed), 2013/05/24 14:15:56]

These metrics will be clouded because they cover both changes to attributes via
the accessors and the parsing of a cookie-line (both value and attributes)
during the constructor.

Consider moving them to SetAttributePair instead.

[jww, 2013/05/24 17:56:33]

The goal of this measurement is to get an overall view of how often there are
cookie values that are rejected because of control characters

On a related note, in SetAttributePair, it seems like the IsValidAttrbitueValue
is a redundant call because IsValidToken includes a check for control
characters. Am I misunderstanding something there? I'm actually going to add a
check in the cookie line parser to check how often parsed cookies are not valid
tokens either.
On 2013/05/24 14:15:56, erikwright wrote:

[erikwright (departed), 2013/05/27 19:19:41]

The key is checked with IsValidToken and the value is checked against
IsValidAttributeValue.

       return false;
+    }
   }
+  UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", true);
   return true;
 }
 
 }  // namespace
 
 namespace net {
 
 ParsedCookie::ParsedCookie(const std::string& cookie_line)
     : path_index_(0),
       domain_index_(0),
       expires_index_(0),
       maxage_index_(0),
       secure_index_(0),
       httponly_index_(0),
-      priority_index_(0) {
+      priority_index_(0),
+      parsed_invalid_control_char_(false) {
 
   if (cookie_line.size() > kMaxCookieSize) {
     VLOG(1) << "Not parsing cookie, too large: " << cookie_line.size();
     return;
   }
 
   ParseTokenValuePairs(cookie_line);
+  UMA_HISTOGRAM_BOOLEAN("Cookies.ParsedInvalidControlCharacter",
+      parsed_invalid_control_char_);
   if (!pairs_.empty())
     SetupAttributes();
 }
 
 ParsedCookie::~ParsedCookie() {
 }
 
 bool ParsedCookie::IsValid() const {
   return !pairs_.empty();
 }
@@ skipping 196 matching lines @@
       }
     } else {
       // We have a TOKEN=VALUE.
       pair.first = std::string(token_start, token_end);
       ++it;  // Skip past the '='.
     }
 
     // OK, now try to parse a value.
     std::string::const_iterator value_start, value_end;
     ParseValue(&it, end, &value_start, &value_end);
+
+    std::string value(value_start, value_end);
+
+    if (!IsValidCookieAttributeValue(value))

[erikwright (departed), 2013/05/24 14:15:56]

Why not just put this after the assignment to pair.second and do
IsValidCookieAttributeValue(pair.second)?

Saves a copy, but also makes it easier to just rip this code out later once you
have your data.

[erikwright (departed), 2013/05/24 14:15:56]

Note that the call to IsValidCookieAttributeValue also sends its own histogram.

[jww, 2013/05/24 17:56:33]

Done.

[jww, 2013/05/24 17:56:33]

See my other responses. We want to measure a couple of different things.
On 2013/05/24 14:15:56, erikwright wrote:

+      parsed_invalid_control_char_ = true;

[erikwright (departed), 2013/05/24 14:15:56]

I suggest converting this to a local boolean and doing the UMA collection at the
end of this method rather than adding member state.

[palmer, 2013/05/24 17:38:43]

I'll second that.

[jww, 2013/05/24 17:56:33]

Done.

[jww, 2013/05/24 17:56:33]

Done.

+
     // OK, we're finished with a Token/Value.
-    pair.second = std::string(value_start, value_end);
+    pair.second = value;
 
     // From RFC2109: "Attributes (names) (attr) are case-insensitive."
     if (pair_num != 0)
       StringToLowerASCII(&pair.first);
     pairs_.push_back(pair);
 
     // We've processed a token/value pair, we're either at the end of
     // the string or a ValueSeparator like ';', which we want to skip.
     if (it != end)
       ++it;
@@ skipping 74 matching lines @@
   for (size_t i = 0; i < arraysize(indexes); ++i) {
     if (*indexes[i] == index)
       *indexes[i] = 0;
     else if (*indexes[i] > index)
       --*indexes[i];
   }
   pairs_.erase(pairs_.begin() + index);
 }
 
 }  // namespace