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

base/third_party/nspr/prtime.cc

Index: base/third_party/nspr/prtime.cc
diff --git a/base/third_party/nspr/prtime.cc b/base/third_party/nspr/prtime.cc
index 0c97bc16d8832c793da8269c01bc8d9350e1e404..e6253cbb4135cac6e769127d94735beffff031c7 100644
--- a/base/third_party/nspr/prtime.cc
+++ b/base/third_party/nspr/prtime.cc
@@ -61,6 +61,8 @@
  * 1. prtime.h
  * 2. prtypes.h
  * 3. prlong.h
+ *
+ * Unit tests are in base/time/pr_time_unittest.cc.
  */
 
 #include "base/logging.h"
@@ -505,6 +507,7 @@ typedef enum
  *   06/21/95 04:24:34 PM
  *   20/06/95 21:07
  *   95-06-08 19:32:48 EDT
+ *   1995-06-17T23:11:25.342156Z
  *
  * If the input string doesn't contain a description of the timezone,
  * we consult the `default_to_gmt' to decide whether the string should
@@ -531,6 +534,7 @@ PR_ParseTimeString(
   int hour = -1;
   int min = -1;
   int sec = -1;
+  int usec = -1;
 
   const char *rest = string;
 
@@ -774,6 +778,7 @@ PR_ParseTimeString(
                         int tmp_hour = -1;
                         int tmp_min = -1;
                         int tmp_sec = -1;
+                        int tmp_usec = -1;
                         const char *end = rest + 1;
                         while (*end >= '0' && *end <= '9')
                           end++;
@@ -833,14 +838,35 @@ PR_ParseTimeString(
                                 else
                                   tmp_sec = (rest[0]-'0');
 
-                                /* If we made it here, we've parsed hour and min,
-                                   and possibly sec, so it worked as a unit. */
-
-                                /* skip over whitespace and see if there's an AM or PM
-                                   directly following the time.
-                                 */
-                                if (tmp_hour <= 12)
+                                /* fractional second */
+                                rest = end;
+                                if (*rest == '.') {
+                                  rest++;
+                                  end++;
+                                  tmp_usec = 0;
+                                  /* use up to 6 digits, skip over the rest */
+                                  while (*end >= '0' && *end <= '9') {
+                                    if (end - rest < 6) {
+                                      tmp_usec = tmp_usec * 10 + *end - '0';
+                                    }
+                                    end++;
+                                  }
+                                  int ndigits = end - rest;
+                                  while (ndigits++ < 6)
+                                    tmp_usec *= 10;
+                                }
+
+                                if (*end == 'Z') {
+                                  zone = TT_GMT;

[wtc, 2014/05/12 19:47:15]

Should we do

    end++;

here? Otherwise 'Z' may be inspected again.

[Thiemo Nagel, 2014/05/13 14:08:19]

It's not strictly necessary because the break will skip through to the while
loop in line 1062 which skips the rest of the token.  Nevertheless I've added it
(and replaced end by rest) in the hope of making the code more readable.

+                                }

[wtc, 2014/05/12 19:47:15]

Nit: Lines 843-861 probably should follow the braces style of this file.
    if (*rest == '.')
      {
        rest++;
        end++;
        ...
      }
    if (*end == 'Z')
      {
        zone = TT_GMT;
      }
    else if (tmp_hour <= 12)

[Thiemo Nagel, 2014/05/13 14:08:19]

Thanks, I fully agree.  Done.

+                                else if (tmp_hour <= 12)
                                   {
+                                    /* If we made it here, we've parsed hour and min,
+                                       and possibly sec, so it worked as a unit. */

[wtc, 2014/05/12 19:47:15]

By "it worked as a unit", did you mean we have fully parsed the time and are
ready for the next token?

[Thiemo Nagel, 2014/05/13 14:08:19]

That's how I understand the original author.  (I've only moved the pre-existing
comment a bit.)  I'm updating the comment now to make it clearer.

+
+                                    /* skip over whitespace and see if there's an AM or PM
+                                       directly following the time.
+                                    */
                                         const char *s = end;
                                         while (*s && (*s == ' ' || *s == '\t'))
                                           s++;
@@ -858,6 +884,7 @@ PR_ParseTimeString(
                                 hour = tmp_hour;
                                 min = tmp_min;
                                 sec = tmp_sec;
+                                usec = tmp_usec;
                                 rest = end;
                                 break;
                           }
@@ -865,8 +892,7 @@ PR_ParseTimeString(
                                          end[1] >= '0' && end[1] <= '9')
                           {
                                 /* Perhaps this is 6/16/95, 16/6/95, 6-16-95, or 16-6-95
-                                   or even 95-06-05...
-                                   #### But it doesn't handle 1995-06-22.
+                                   or even 95-06-05 or 1995-06-22.
                                  */
                                 int n1, n2, n3;
                                 const char *s;
@@ -877,10 +903,18 @@ PR_ParseTimeString(
 
                                 s = rest;
 
-                                n1 = (*s++ - '0');                                /* first 1 or 2 digits */
+                                n1 = (*s++ - '0');                                /* first 1, 2 or 4 digits */
                                 if (*s >= '0' && *s <= '9')
                                   n1 = n1*10 + (*s++ - '0');
 
+                                if (*s >= '0' && *s <= '9')            /* optional digits 3 and 4 */
+                                  {
+                                        n1 = n1*10 + (*s++ - '0');
+                                        if (*s < '0' || *s > '9')
+                                          break;

[wtc, 2014/05/12 19:47:15]

This break statement means you want to disallow a three-digit year. I am fine
with that (line 942 also has a break statement). Just wanted to confirm this.

[Thiemo Nagel, 2014/05/13 14:08:19]

Yes, that's the intent.

+                                        n1 = n1*10 + (*s++ - '0');
+                                  }

[wtc, 2014/05/12 19:47:15]

Nit: it seems that this new if block (lines 910-916) should be inside the
previous if statement, i.e., we only allow digits 3 and 4 if there is digit 2:

  n1 = (*s++ - '0'); /* first 1, 2 or 4 digits */
  if (*s >= '0' && *s <= '9')
    {
      n1 = n1*10 + (*s++ - '0');
      if (*s >= '0' && *s <= '9') /* option al digits 3 and 4 */
        {
          n1 = n1*10 + (*s++ - '0');
          if (*s < '0' || *s > '9')
            break;
          n1 = n1*10 + (*s++ - '0');
        }
    }

Your current code is fine because it matches the code on lines 934-946. Note
that that code seems to allow a five-digit year. I forgot why.

[Thiemo Nagel, 2014/05/13 14:08:19]

Yes, good point, that's more readable.
I certainly would be interested to know.  Anyways, ISO 8601 does specify only
four digits, that's why I'm not supporting a fifth one.

+
                                 if (*s != '/' && *s != '-')                /* slash */
                                   break;
                                 s++;
@@ -911,17 +945,21 @@ PR_ParseTimeString(
                                           n3 = n3*10 + (*s++ - '0');
                                   }
 
-                                if ((*s >= '0' && *s <= '9') ||        /* followed by non-alphanum */
-                                        (*s >= 'A' && *s <= 'Z') ||
-                                        (*s >= 'a' && *s <= 'z'))
+                                if (*s == 'T' && s[1] >= '0' && s[1] <= '9')
+                                  /* followed by ISO 8601 T delimiter and number is ok */
+                                  ;
+                                else if ((*s >= '0' && *s <= '9') ||
+                                         (*s >= 'A' && *s <= 'Z') ||
+                                         (*s >= 'a' && *s <= 'z'))
+                                  /* but other alphanumerics are not ok */
                                   break;
 
-                                /* Ok, we parsed three 1-2 digit numbers, with / or -
+                                /* Ok, we parsed three multi-digit numbers, with / or -
                                    between them.  Now decide what the hell they are
-                                   (DD/MM/YY or MM/DD/YY or YY/MM/DD.)
+                                   (DD/MM/YY or MM/DD/YY or [YY]YY/MM/DD.)
                                  */
 
-                                if (n1 > 31 || n1 == 0)  /* must be YY/MM/DD */
+                                if (n1 > 31 || n1 == 0)  /* must be [YY]YY/MM/DD */
                                   {
                                         if (n2 > 12) break;
                                         if (n3 > 31) break;
@@ -1026,14 +1064,15 @@ PR_ParseTimeString(
                          *rest != ',' && *rest != ';' &&
                          *rest != '-' && *rest != '+' &&
                          *rest != '/' &&
-                         *rest != '(' && *rest != ')' && *rest != '[' && *rest != ']')
+                         *rest != '(' && *rest != ')' && *rest != '[' && *rest != ']' &&
+                         !(*rest == 'T' && rest[1] >= '0' && rest[1] <= '9')  /* T precedes time in ISO 8601 */

[wtc, 2014/05/12 19:47:15]

Nit: maybe this comment should be moved to the comment block at line 1060.

[Thiemo Nagel, 2014/05/13 14:08:19]

Done.  At that occasion, I've updated the pre-existing comment which was out of
sync with the code.

+                )
                 rest++;
           /* skip over uninteresting chars. */
         SKIP_MORE:
-          while (*rest &&

[Thiemo Nagel, 2014/05/09 16:19:00]

*rest is redundant here.

-                         (*rest == ' ' || *rest == '\t' ||
-                          *rest == ',' || *rest == ';' || *rest == '/' ||
-                          *rest == '(' || *rest == ')' || *rest == '[' || *rest == ']'))
+          while (*rest == ' ' || *rest == '\t' ||
+                 *rest == ',' || *rest == ';' || *rest == '/' ||
+                 *rest == '(' || *rest == ')' || *rest == '[' || *rest == ']')
                 rest++;
 
           /* "-" is ignored at the beginning of a token if we have not yet
@@ -1047,7 +1086,11 @@ PR_ParseTimeString(
                   goto SKIP_MORE;
                 }
 
-        }
+          /* Skip T that may precede ISO 8601 time. */
+          if (*rest == 'T' && rest[1] >= '0' && rest[1] <= '9')
+            rest++;
+

[wtc, 2014/05/12 19:47:15]

Nit: delete this blank line.

[Thiemo Nagel, 2014/05/13 14:08:19]

Done.

+        }  /* while */
 
   if (zone != TT_UNKNOWN && zone_offset == -1)
         {
@@ -1082,6 +1125,8 @@ PR_ParseTimeString(
       return PR_FAILURE;
 
   memset(result, 0, sizeof(*result));
+  if (usec != -1)
+        result->tm_usec = usec; 
   if (sec != -1)
         result->tm_sec = sec;
   if (min != -1)