time overflow

base/time_posix.cc

Index: base/time_posix.cc
===================================================================
--- base/time_posix.cc	(revision 4280)
+++ base/time_posix.cc	(working copy)
@@ -10,6 +10,8 @@
 #include <sys/time.h>
 #include <time.h>
 
+#include <limits>
+
 #include "base/basictypes.h"
 #include "base/logging.h"
 
@@ -58,16 +60,34 @@
     seconds = mktime(&timestruct);
   else
     seconds = timegm(&timestruct);
-  DCHECK(seconds >= 0) << "mktime/timegm could not convert from exploded";
 
-  uint64 milliseconds = seconds * kMillisecondsPerSecond + exploded.millisecond;
+  int64 milliseconds;
+  // Handle overflow.
+  if (seconds == -1 &&
+      (exploded.year < 1969 || exploded.year > 1970)) {
+    // If exploded.year is 1969 or 1970, take -1 as correct, with the
+    // time indicating 1 second prior to the epoch.  (1970 is allowed to handle
+    // time zone and DST offsets.)  Otherwise, return the most future or past
+    // time representable.  Assumes the time_t epoch is 1970-01-01 00:00:00 UTC.
+    if (exploded.year < 1969) {
+      milliseconds = std::numeric_limits<time_t>::min() *

[wtc, 2008/10/31 20:52:01]

Some points to ponder:

- What if time_t is uint32?  Then the min of time_t is 0.
  I think this is fine for our purposes.
- What if time_t is double?  Is the range of double contained
  in the range of int64?

[Mark Mentovai, 2008/10/31 21:21:01]

wtc wrote:
Me too.  But we can add this to check our assumption:

COMPILE_ASSERT(std::numeric_limits<time_t>::min() < 0);
No, it's not, but double's precision is worse than 1 unit per step at the
extremities of the range supported by int64, so for us to get into this case,
we'd be dealing with a weird clock that can't accurately represent seconds
anyway.

Do you want to do this?

COMPILE_ASSERT(std::numeric_limits<int64>::min() <=
               std::numeric_limits<time_t>::min() * kMicrosecondPerSecond &&
               std::numeric_limits<int64>::max() >=
               std::numeric_limits<time_t>::max() * kMicrosecondPerSecond);

And possibly even add an extra 999ms into the last clause?

With these assertions, I think the only not-unreasonable assumption made by the
code that's unchecked is whether the system treats the epoch as 1970-01-01
00:00:00 UTC.

+                     kMillisecondsPerSecond;
+    } else {
+      milliseconds = (std::numeric_limits<time_t>::max() *
+                      kMillisecondsPerSecond) +
+                     kMillisecondsPerSecond - 1;

[wtc, 2008/10/31 20:52:01]

The need to add 999 milliseconds is still not clear to me.
If you want to return a number greater than the max of time_t,
why don't you just add 1 millisecond or 1 second?

And why don't you do this for the min case?  Why don't you
subtract 999 milliseconds from the min of time_t?

[Mark Mentovai, 2008/10/31 21:21:01]

wtc wrote:
Because in the non-overflow case, we never subtract milliseconds from the result
of mktime/timegm, we only add.  There can never be a time earlier than
1901-12-13 20:45:52.000 UTC, so the correct time to return in the case of
negative overflow must be <= 1901-12-13 20:45:52.000 UTC.  I chose = rather than
<= so that if someone tries to convert it back into a standard time_t by
truncating the fractional portion, it round-trips properly.  That is, if the
time is re-exploded, it can be imploded again without hitting the overflow
condition.  In a perfect world where everyone comes through the same API, we
could overflow on the low end with -1ms (instead of 0) and on the high end with
+1000ms (instead of 999), but realistically, it's probably too tempting for
people to try using native time routines by marshalling things back into a
time_t in non-cross-platform code, and in those cases, -1ms and +1000ms will not
necessarily be interpreted consistently as we intend.  Consider someone dividing
the time by kMicrosecondsPerSecond to feed a time_t into some interface that
requires one, like localtime+strftime or ctime, for example.  If we overflow to
something that will wind up outside of the range of time_t, this will break.

The same applies to my choice of 999ms on top of the maximum range of time_t,
2038-01-19 03:14:07.999 UTC.

To better understand the +999ms, assume a bunch of time samples taken near this
maximum at sub-second intervals.  If you didn't add 999ms, your values would
look like:

2038-01-19 03:14:07.994 UTC
2038-01-19 03:14:07.996 UTC
2038-01-19 03:14:07.998 UTC  // overflow occurs next
2038-01-19 03:14:07.000 UTC  // time went backwards! bad!
2038-01-19 03:14:07.000 UTC
2038-01-19 03:14:07.000 UTC

It's preferable to clamp things at the maximum value:

2038-01-19 03:14:07.994 UTC
2038-01-19 03:14:07.996 UTC
2038-01-19 03:14:07.998 UTC  // overflow occurs next
2038-01-19 03:14:07.999 UTC
2038-01-19 03:14:07.999 UTC
2038-01-19 03:14:07.999 UTC

In the latter case, you can't disambiguate the last three times from each other
as "real" 07.999 or "overflow" 07.999, but I think that's OK given the
round-tripping and interfacing with native time_t described above.

+    }
+  } else {
+    milliseconds = seconds * kMillisecondsPerSecond + exploded.millisecond;
+  }
+
   return Time(milliseconds * kMicrosecondsPerMillisecond);
 }
 
 void Time::Explode(bool is_local, Exploded* exploded) const {
   // Time stores times with microsecond resolution, but Exploded only carries
   // millisecond resolution, so begin by being lossy.
-  uint64 milliseconds = us_ / kMicrosecondsPerMillisecond;
+  int64 milliseconds = us_ / kMicrosecondsPerMillisecond;
   time_t seconds = milliseconds / kMillisecondsPerSecond;
 
   struct tm timestruct;