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

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

 // Copyright 2013 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/browser/extensions/api/log_private/syslog_parser.h"
 
 #include <string>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/time/time.h"
 #include "chrome/browser/extensions/api/log_private/filter_handler.h"
 #include "chrome/browser/extensions/api/log_private/log_parser.h"
 #include "chrome/browser/extensions/api/log_private/log_private_api.h"
 #include "chrome/common/extensions/api/log_private.h"
 
 namespace extensions {
 
 namespace {
 
-const int kExpectedTimeTokenNum = 7;
-const char kLogEntryDelimiters[] = "-:T";
+const int kExpectedTimeTokenNum = 17;
+const char kLogEntryDelimiters[] = "-+.:T";
 const char kProcessInfoDelimiters[] = "[]";
 
 }  // namespace
 
 SyslogParser::SyslogParser() {}
 
 SyslogParser::~SyslogParser() {}
 
 SyslogParser::Error SyslogParser::ParseEntry(
     const std::string& input,
@@ skipping 26 matching lines @@
   ParseLevel(input, entry.get());
   entry->full_entry = input;
 
   if (filter_handler->IsValidLogEntry(*(entry.get()))) {
     output->push_back(entry);
   }
 
   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)) {
     LOG(ERROR) << "Error when parsing time";
     return SyslogParser::PARSE_ERROR;
   }
 
   double seconds;
-  base::StringToDouble(tokens[5], &seconds);
+  base::StringToDouble("." + tokens[12], &seconds);
   *output = parsed_time.ToJsTime() +
             (seconds * base::Time::kMillisecondsPerSecond);
 
   return SyslogParser::SUCCESS;
 }
 
 SyslogParser::Error SyslogParser::ParseProcess(
     const std::string& input,
     api::log_private::LogEntry* entry) const {
   base::StringTokenizer tokenizer(input, kProcessInfoDelimiters);
@@ skipping 21 matching lines @@
   } else if (input.find("WARN") != std::string::npos) {
     entry->level = "warning";
   } else if (input.find("INFO") != std::string::npos) {
     entry->level = "info";
   } else {
     entry->level = "unknown";
   }
 }
 
 }  // namespace extensions