Fix parsing of max-age in SdchManager.

net/base/sdch_manager.cc

 // Copyright (c) 2012 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 "net/base/sdch_manager.h"
 
 #include <limits.h>
 
 #include <utility>
 
@@ skipping 358 matching lines @@
       std::string value(dictionary_text, value_start, line_end - value_start);
       name = base::ToLowerASCII(name);
       if (name == "domain") {
         domain = value;
       } else if (name == "path") {
         path = value;
       } else if (name == "format-version") {
         if (value != "1.0")
           return SDCH_DICTIONARY_UNSUPPORTED_VERSION;
       } else if (name == "max-age") {
-        int64_t seconds;
-        // TODO(eroman): crbug.com/596541 -- should not accept a leading +.
-        base::StringToInt64(value, &seconds);
-        expiration = base::Time::Now() + base::TimeDelta::FromSeconds(seconds);
+        // max-age must be a non-negative number. If it is very large saturate

[eroman, 2016/04/11 19:36:37]

Randy, what is the expected behavior here?

I wrote some code as a proof of concept, but don't expect to actually leave it
as-written.

  * What is the normative reference for the SDCH spec? I found some stuff
online, but it didn't clarify parsing strictness, nor the full production for
max-age (other than it is delta-seconds, which it leaves undefined. The HTTP
spec defines delta-seconds as 1*DIGIT though, so it does not include negative
numbers).

  * At least one test relies on max-age allowing negative numbers. This seems
like a violation. The existing behavior has it successfully parse it and infer
an expired time.

  * What is the rationale for which things in this function hard fail, vs those
that do optimistic parsing? For instance the parsing of "port" below is
best-effort and if given a bad number simply pretends it doesn't exist. (I
previously ported this to use net::ParseUint32, but preserved its existing
semantics).

  * The default expiration if max-age is unspecified is Now() + 30days. The code
I wrote here as a proof of concept uses the default in the case too where
max-age was specified but was invalid. This seems wrong, and does cause a test
to fail. A better approach would be to either default to an expired date, or to
just fail the parsing outright.

What do you think?

+        // to 2^32 - 1.
+        uint32_t seconds = std::numeric_limits<uint32_t>::max();
+        ParseIntError parse_int_error;
+        if (ParseUint32(value, &seconds, &parse_int_error) ||
+            parse_int_error == ParseIntError::FAILED_OVERFLOW) {
+          expiration =
+              base::Time::Now() + base::TimeDelta::FromSeconds(seconds);
+        }
       } else if (name == "port") {
         int port;
         if (ParseInt32(value, ParseIntFormat::NON_NEGATIVE, &port))
           ports.insert(port);
       }
     }
 
     if (line_end >= header_end)
       break;
     line_start = line_end + 1;
@@ skipping 81 matching lines @@
       entry_dict->SetInteger("tries", it->second.count);
     entry_dict->SetInteger("reason", it->second.reason);
     entry_list->Append(std::move(entry_dict));
   }
   value->Set("blacklisted", std::move(entry_list));
 
   return std::move(value);
 }
 
 }  // namespace net