Add: check exploded time is properly converted.
This cl introduces time checking in posix-like and mac systems.
base::Time::FromUTCExploded() and
base::Time::FromLocalExplode() can fail without returning
a proper error.
This fix does the following:
1) After calculations are done, create UTC or local time.
2) Convert UTC or local time back to exploded
3) Compare original exploded with converted one
4) If times are not equal, then return Time(0) indicating
an error.
Windows implementation already returns Time(0) on error.
BUG=601900
Review-Url: https://codereview.chromium.org/1988663002
Cr-Commit-Position: refs/heads/master@{#396638}
diff --git a/base/time/time.cc b/base/time/time.cc
index 76ffeb74..8b1a6d7 100644
--- a/base/time/time.cc
+++ b/base/time/time.cc
@@ -263,6 +263,14 @@
return true;
}
+// static
+bool Time::ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs) {
+ return lhs.year == rhs.year && lhs.month == rhs.month &&
+ lhs.day_of_month == rhs.day_of_month && lhs.hour == rhs.hour &&
+ lhs.minute == rhs.minute && lhs.second == rhs.second &&
+ lhs.millisecond == rhs.millisecond;
+}
+
std::ostream& operator<<(std::ostream& os, Time time) {
Time::Exploded exploded;
time.UTCExplode(&exploded);
diff --git a/base/time/time.h b/base/time/time.h
index 399ec826..63583aeb 100644
--- a/base/time/time.h
+++ b/base/time/time.h
@@ -56,6 +56,7 @@
#include <limits>
#include "base/base_export.h"
+#include "base/compiler_specific.h"
#include "base/numerics/safe_math.h"
#include "build/build_config.h"
@@ -519,11 +520,29 @@
// Converts an exploded structure representing either the local time or UTC
// into a Time class.
+ // TODO(maksims): Get rid of these in favor of the methods below when
+ // all the callers stop using these ones.
static Time FromUTCExploded(const Exploded& exploded) {
- return FromExploded(false, exploded);
+ base::Time time;
+ ignore_result(FromUTCExploded(exploded, &time));
+ return time;
}
static Time FromLocalExploded(const Exploded& exploded) {
- return FromExploded(true, exploded);
+ base::Time time;
+ ignore_result(FromLocalExploded(exploded, &time));
+ return time;
+ }
+
+ // Converts an exploded structure representing either the local time or UTC
+ // into a Time class. Returns false on a failure when, for example, a day of
+ // month is set to 31 on a 28-30 day month.
+ static bool FromUTCExploded(const Exploded& exploded,
+ Time* time) WARN_UNUSED_RESULT {
+ return FromExploded(false, exploded, time);
+ }
+ static bool FromLocalExploded(const Exploded& exploded,
+ Time* time) WARN_UNUSED_RESULT {
+ return FromExploded(true, exploded, time);
}
// Converts a string representation of time to a Time object.
@@ -564,8 +583,12 @@
void Explode(bool is_local, Exploded* exploded) const;
// Unexplodes a given time assuming the source is either local time
- // |is_local = true| or UTC |is_local = false|.
- static Time FromExploded(bool is_local, const Exploded& exploded);
+ // |is_local = true| or UTC |is_local = false|. Function returns false on
+ // failure and sets |time| to Time(0). Otherwise returns true and sets |time|
+ // to non-exploded time.
+ static bool FromExploded(bool is_local,
+ const Exploded& exploded,
+ Time* time) WARN_UNUSED_RESULT;
// Converts a string representation of time to a Time object.
// An example of a time string which is converted is as below:-
@@ -577,6 +600,9 @@
static bool FromStringInternal(const char* time_string,
bool is_local,
Time* parsed_time);
+
+ // Comparison does not consider |day_of_week| when doing the operation.
+ static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs);
};
// static
diff --git a/base/time/time_mac.cc b/base/time/time_mac.cc
index c23c491..373ec3a 100644
--- a/base/time/time_mac.cc
+++ b/base/time/time_mac.cc
@@ -34,7 +34,7 @@
struct timeval boottime;
int mib[2] = {CTL_KERN, KERN_BOOTTIME};
size_t size = sizeof(boottime);
- int kr = sysctl(mib, arraysize(mib), &boottime, &size, NULL, 0);
+ int kr = sysctl(mib, arraysize(mib), &boottime, &size, nullptr, 0);
DCHECK_EQ(KERN_SUCCESS, kr);
base::TimeDelta time_difference = base::Time::Now() -
(base::Time::FromTimeT(boottime.tv_sec) +
@@ -168,7 +168,7 @@
}
// static
-Time Time::FromExploded(bool is_local, const Exploded& exploded) {
+bool Time::FromExploded(bool is_local, const Exploded& exploded, Time* time) {
base::ScopedCFTypeRef<CFTimeZoneRef> time_zone(
is_local
? CFTimeZoneCopySystem()
@@ -184,8 +184,28 @@
exploded.day_of_month, exploded.hour, exploded.minute, exploded.second,
exploded.millisecond);
CFAbsoluteTime seconds = absolute_time + kCFAbsoluteTimeIntervalSince1970;
- return Time(static_cast<int64_t>(seconds * kMicrosecondsPerSecond) +
- kWindowsEpochDeltaMicroseconds);
+
+ base::Time converted_time =
+ Time(static_cast<int64_t>(seconds * kMicrosecondsPerSecond) +
+ kWindowsEpochDeltaMicroseconds);
+
+ // If |exploded.day_of_month| is set to 31
+ // on a 28-30 day month, it will return the first day of the next month.
+ // Thus round-trip the time and compare the initial |exploded| with
+ // |utc_to_exploded| time.
+ base::Time::Exploded to_exploded;
+ if (!is_local)
+ converted_time.UTCExplode(&to_exploded);
+ else
+ converted_time.LocalExplode(&to_exploded);
+
+ if (ExplodedMostlyEquals(to_exploded, exploded)) {
+ *time = converted_time;
+ return true;
+ }
+
+ *time = Time(0);
+ return false;
}
void Time::Explode(bool is_local, Exploded* exploded) const {
diff --git a/base/time/time_posix.cc b/base/time/time_posix.cc
index 32614bc..495e249 100644
--- a/base/time/time_posix.cc
+++ b/base/time/time_posix.cc
@@ -211,7 +211,7 @@
}
// static
-Time Time::FromExploded(bool is_local, const Exploded& exploded) {
+bool Time::FromExploded(bool is_local, const Exploded& exploded, Time* time) {
struct tm timestruct;
timestruct.tm_sec = exploded.second;
timestruct.tm_min = exploded.minute;
@@ -301,8 +301,26 @@
}
// Adjust from Unix (1970) to Windows (1601) epoch.
- return Time((milliseconds * kMicrosecondsPerMillisecond) +
- kWindowsEpochDeltaMicroseconds);
+ base::Time converted_time =
+ Time((milliseconds * kMicrosecondsPerMillisecond) +
+ kWindowsEpochDeltaMicroseconds);
+
+ // If |exploded.day_of_month| is set to 31 on a 28-30 day month, it will
+ // return the first day of the next month. Thus round-trip the time and
+ // compare the initial |exploded| with |utc_to_exploded| time.
+ base::Time::Exploded to_exploded;
+ if (!is_local)
+ converted_time.UTCExplode(&to_exploded);
+ else
+ converted_time.LocalExplode(&to_exploded);
+
+ if (ExplodedMostlyEquals(to_exploded, exploded)) {
+ *time = converted_time;
+ return true;
+ }
+
+ *time = Time(0);
+ return false;
}
// TimeTicks ------------------------------------------------------------------
diff --git a/base/time/time_unittest.cc b/base/time/time_unittest.cc
index 25c6ca594..ce68b042 100644
--- a/base/time/time_unittest.cc
+++ b/base/time/time_unittest.cc
@@ -21,6 +21,52 @@
namespace {
+TEST(TimeTestOutOfBounds, FromExplodedOutOfBoundsTime) {
+ // FromUTCExploded must set time to Time(0) and failure, if the day is set to
+ // 31 on a 28-30 day month. Test |exploded| returns Time(0) on 31st of
+ // February and 31st of April. New implementation handles this.
+
+ const struct DateTestData {
+ Time::Exploded explode;
+ bool is_valid;
+ } kDateTestData[] = {
+ // 31st of February
+ {{2016, 2, 0, 31, 12, 30, 0, 0}, true},
+ // 31st of April
+ {{2016, 4, 0, 31, 8, 43, 0, 0}, true},
+ // Negative month
+ {{2016, -5, 0, 2, 4, 10, 0, 0}, false},
+ // Negative date of month
+ {{2016, 6, 0, -15, 2, 50, 0, 0}, false},
+ // Negative hours
+ {{2016, 7, 0, 10, -11, 29, 0, 0}, false},
+ // Negative minutes
+ {{2016, 3, 0, 14, 10, -29, 0, 0}, false},
+ // Negative seconds
+ {{2016, 10, 0, 25, 7, 47, -30, 0}, false},
+ // Negative milliseconds
+ {{2016, 10, 0, 25, 7, 47, 20, -500}, false},
+ // Hours are too large
+ {{2016, 7, 0, 10, 26, 29, 0, 0}, false},
+ // Minutes are too large
+ {{2016, 3, 0, 14, 10, 78, 0, 0}, false},
+ // Seconds are too large
+ {{2016, 10, 0, 25, 7, 47, 234, 0}, false},
+ // Milliseconds are too large
+ {{2016, 10, 0, 25, 6, 31, 23, 1643}, false},
+ };
+
+ for (const auto& test : kDateTestData) {
+ EXPECT_EQ(test.explode.HasValidValues(), test.is_valid);
+
+ base::Time result;
+ EXPECT_FALSE(base::Time::FromUTCExploded(test.explode, &result));
+ EXPECT_TRUE(result.is_null());
+ EXPECT_FALSE(base::Time::FromLocalExploded(test.explode, &result));
+ EXPECT_TRUE(result.is_null());
+ }
+}
+
// Specialized test fixture allowing time strings without timezones to be
// tested by comparing them to a known time in the local zone.
// See also pr_time_unittests.cc
diff --git a/base/time/time_win.cc b/base/time/time_win.cc
index ac3197a..8708eb2 100644
--- a/base/time/time_win.cc
+++ b/base/time/time_win.cc
@@ -235,7 +235,7 @@
}
// static
-Time Time::FromExploded(bool is_local, const Exploded& exploded) {
+bool Time::FromExploded(bool is_local, const Exploded& exploded, Time* time) {
// Create the system struct representing our exploded time. It will either be
// in local time or UTC.
SYSTEMTIME st;
@@ -253,17 +253,19 @@
// Ensure that it's in UTC.
if (is_local) {
SYSTEMTIME utc_st;
- success = TzSpecificLocalTimeToSystemTime(NULL, &st, &utc_st) &&
+ success = TzSpecificLocalTimeToSystemTime(nullptr, &st, &utc_st) &&
SystemTimeToFileTime(&utc_st, &ft);
} else {
success = !!SystemTimeToFileTime(&st, &ft);
}
if (!success) {
- NOTREACHED() << "Unable to convert time";
- return Time(0);
+ *time = Time(0);
+ return false;
}
- return Time(FileTimeToMicroseconds(ft));
+
+ *time = Time(FileTimeToMicroseconds(ft));
+ return true;
}
void Time::Explode(bool is_local, Exploded* exploded) const {
@@ -288,7 +290,7 @@
// daylight saving time, it will take daylight saving time into account,
// even if the time you are converting is in standard time.
success = FileTimeToSystemTime(&utc_ft, &utc_st) &&
- SystemTimeToTzSpecificLocalTime(NULL, &utc_st, &st);
+ SystemTimeToTzSpecificLocalTime(nullptr, &utc_st, &st);
} else {
success = !!FileTimeToSystemTime(&utc_ft, &st);
}
