Extend PR_ParseTimeString() to accept some ISO 8601 formats to fix timezone parsing in SyslogParser.

chrome/browser/extensions/api/log_private/syslog_parser.cc

Index: chrome/browser/extensions/api/log_private/syslog_parser.cc
diff --git a/chrome/browser/extensions/api/log_private/syslog_parser.cc b/chrome/browser/extensions/api/log_private/syslog_parser.cc
index 5479886c4ca2324ee6c9f8cdd968091986e53f42..32e1ce7d626af7f6bc0bf2b05bbcaa106bfae8f8 100644
--- a/chrome/browser/extensions/api/log_private/syslog_parser.cc
+++ b/chrome/browser/extensions/api/log_private/syslog_parser.cc
@@ -23,8 +23,8 @@ namespace extensions {
 
 namespace {
 
-const int kExpectedTimeTokenNum = 7;
-const char kLogEntryDelimiters[] = "-:T";
+const int kExpectedTimeTokenNum = 17;
+const char kLogEntryDelimiters[] = "-+.:T";
 const char kProcessInfoDelimiters[] = "[]";
 
 }  // namespace
@@ -71,27 +71,38 @@ SyslogParser::Error SyslogParser::ParseEntry(
   return SyslogParser::SUCCESS;
 }
 
-SyslogParser::Error ParseTimeHelper(base::StringTokenizer* tokenizer,
-                                    std::string* output) {
-  if (!tokenizer->GetNext()) {
+SyslogParser::Error ParseTimeHelper(base::StringTokenizer& tokenizer,
+                                    std::string& output) {

[not at google - send to devlin, 2014/05/05 19:36:24]

if these are refs they need to be const.

but given you call GetNext() the tokenizer can't be const.

and given you write to output, it can't be const either.

so these need to be pointers.

[Thiemo Nagel, 2014/05/06 18:15:49]

I wasn't aware of that convention.  Thanks a lot!

+  if (!tokenizer.GetNext()) {
     LOG(ERROR) << "Error when parsing time";
     return SyslogParser::PARSE_ERROR;
   }
-  *output = tokenizer->token();
+  output = tokenizer.token();
   return SyslogParser::SUCCESS;
 }
 
 SyslogParser::Error SyslogParser::ParseTime(const std::string& input,
                                             double* output) const {
   base::StringTokenizer tokenizer(input, kLogEntryDelimiters);
+  tokenizer.set_options(base::StringTokenizer::RETURN_DELIMS);
   std::string tokens[kExpectedTimeTokenNum];
   for (int i = 0; i < kExpectedTimeTokenNum; i++) {
-    if (ParseTimeHelper(&tokenizer, &(tokens[i])) != SyslogParser::SUCCESS)
+    if (ParseTimeHelper(tokenizer, tokens[i]) != SyslogParser::SUCCESS)
       return SyslogParser::PARSE_ERROR;
   }
 
-  std::string buffer = tokens[1] + '-' + tokens[2] + '-' + tokens[0] + ' ' +
-                       tokens[3] + ':' + tokens[4] + ":00";
+  DCHECK(tokens[1] == "-");
+  DCHECK(tokens[3] == "-");
+  DCHECK(tokens[5] == "T");
+  DCHECK(tokens[7] == ":");
+  DCHECK(tokens[9] == ":");
+  DCHECK(tokens[11] == ".");
+  DCHECK(tokens[15] == ":");

[not at google - send to devlin, 2014/05/05 19:36:24]

DCHECKs here seem wrong especially since we can return an Error from here. maybe
there should be a NOTREACHED but better still would be to return a
SyslogParser::Error if any of these are wrong, and then write a test which
exercises that. since writing a test is likely to be really tricky (you'd need
to generate real system logs, ugh) NOTREACHED is fine.

[Thiemo Nagel, 2014/05/06 18:15:49]

OK.  Done.

+
+  std::string buffer =
+      tokens[2] + "/" + tokens[4] + "/" + tokens[0] + " " +   // month/day/year
+      tokens[6] + ":" + tokens[8] + ":" + tokens[10] + " " +  // hour:min:sec
+      tokens[13] + tokens[14] + tokens[16];                   // +/- tz offset

[not at google - send to devlin, 2014/05/05 19:36:24]

maybe have a look at sscanf[_s] here? surely there is an easier way than this.

[Thiemo Nagel, 2014/05/06 18:15:49]

Could you please explain what specifically you'd suggest to change?  I see
nothing wrong with the original author's approach of re-shuffling the time
string so that it can be parsed by base::Time::FromString().

[not at google - send to devlin, 2014/05/06 18:24:51]

The code is very hard to read. De-formatting it using sscanf would be much
nicer, like:

char month, day, year, hour, min, sec;
sscanf(tokens, "%c/%c/%c/ etc", &month, &day, &year, &hour, &min, &sec);

plus whatever the required buffer safety etc mechanisms are.

 
   base::Time parsed_time;
   if (!base::Time::FromString(buffer.c_str(), &parsed_time)) {
@@ -100,7 +111,7 @@ SyslogParser::Error SyslogParser::ParseTime(const std::string& input,
   }
 
   double seconds;
-  base::StringToDouble(tokens[5], &seconds);
+  base::StringToDouble("." + tokens[12], &seconds);
   *output = parsed_time.ToJsTime() +
             (seconds * base::Time::kMillisecondsPerSecond);