[core] Fix overflow when using util::Timestamp

Detected by the undefined behavior sanitizer:
https://circleci.com/gh/mapbox/mapbox-gl-native/318016

8 files changed, 19 insertions, 19 deletions

diff --git a/include/mbgl/util/chrono.hpp b/include/mbgl/util/chrono.hpp
index 723cd131e3..63033627a1 100644
--- a/include/mbgl/util/chrono.hpp
+++ b/include/mbgl/util/chrono.hpp
@@ -12,14 +12,12 @@ using Milliseconds = std::chrono::milliseconds;
 
 using TimePoint = Clock::time_point;
 using Duration  = Clock::duration;
-
-// Used to measure second-precision times, such as times gathered from HTTP responses.
-using Timestamp = std::chrono::time_point<std::chrono::system_clock, Seconds>;
+using Timestamp = std::chrono::time_point<std::chrono::system_clock>;
 
 namespace util {
 
 inline Timestamp now() {
-    return std::chrono::time_point_cast<Seconds>(std::chrono::system_clock::now());
+    return std::chrono::system_clock::now();
 }
 
 // Returns the RFC1123 formatted date. E.g. "Tue, 04 Nov 2014 02:13:24 GMT"
diff --git a/platform/default/src/mbgl/storage/online_file_source.cpp b/platform/default/src/mbgl/storage/online_file_source.cpp
index 4e19d48a15..87874f0c59 100644
--- a/platform/default/src/mbgl/storage/online_file_source.cpp
+++ b/platform/default/src/mbgl/storage/online_file_source.cpp
@@ -356,7 +356,7 @@ Timestamp interpolateExpiration(const Timestamp& current,
         return current;
     }
 
-    auto delta = current - *prior;
+    auto delta = std::chrono::duration_cast<Seconds>(current - *prior);
 
     // Server is serving the same expired resource
     // over and over, fallback to exponential backoff.
diff --git a/platform/default/src/mbgl/storage/sqlite3.cpp b/platform/default/src/mbgl/storage/sqlite3.cpp
index 522210f88f..31d68ac168 100644
--- a/platform/default/src/mbgl/storage/sqlite3.cpp
+++ b/platform/default/src/mbgl/storage/sqlite3.cpp
@@ -301,7 +301,7 @@ void Query::bindBlob(int offset, const std::vector<uint8_t>& value, bool retain)
 
 template <>
 void Query::bind(
-        int offset, std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds> value) {
+        int offset, std::chrono::time_point<std::chrono::system_clock> value) {
     assert(stmt.impl);
     stmt.impl->check(sqlite3_bind_int64(stmt.impl->stmt, offset, std::chrono::system_clock::to_time_t(value)));
 }
@@ -317,7 +317,7 @@ template <> void Query::bind(int offset, mbgl::optional<std::string> value) {
 template <>
 void Query::bind(
     int offset,
-    mbgl::optional<std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>> value) {
+    mbgl::optional<std::chrono::time_point<std::chrono::system_clock>> value) {
     if (!value) {
         bind(offset, nullptr);
     } else {
@@ -377,7 +377,7 @@ template <> std::vector<uint8_t> Query::get(int offset) {
 }
 
 template <>
-std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
+std::chrono::time_point<std::chrono::system_clock>
 Query::get(int offset) {
     assert(stmt.impl);
     return std::chrono::time_point_cast<std::chrono::seconds>(
@@ -412,13 +412,13 @@ template <> mbgl::optional<std::string> Query::get(int offset) {
 }
 
 template <>
-mbgl::optional<std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>>
+mbgl::optional<std::chrono::time_point<std::chrono::system_clock>>
 Query::get(int offset) {
     assert(stmt.impl);
     if (sqlite3_column_type(stmt.impl->stmt, offset) == SQLITE_NULL) {
         return mbgl::nullopt;
     } else {
-        return get<std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>>(
+        return get<std::chrono::time_point<std::chrono::system_clock>>(
             offset);
     }
 }
diff --git a/src/mbgl/util/chrono.cpp b/src/mbgl/util/chrono.cpp
index 6674e73c42..0cb402a3e6 100644
--- a/src/mbgl/util/chrono.cpp
+++ b/src/mbgl/util/chrono.cpp
@@ -38,11 +38,11 @@ std::string iso8601(Timestamp timestamp) {
 }
 
 Timestamp parseTimestamp(const char* timestamp) {
-    return std::chrono::time_point_cast<Seconds>(std::chrono::system_clock::from_time_t(parse_date(timestamp)));
+    return std::chrono::system_clock::from_time_t(parse_date(timestamp));
 }
 
 Timestamp parseTimestamp(const int32_t timestamp) {
-    return std::chrono::time_point_cast<Seconds>(std::chrono::system_clock::from_time_t(timestamp));
+    return std::chrono::system_clock::from_time_t(timestamp);
 }
 
 } // namespace util
diff --git a/src/mbgl/util/http_header.cpp b/src/mbgl/util/http_header.cpp
index cdb4759d51..b318e979fa 100644
--- a/src/mbgl/util/http_header.cpp
+++ b/src/mbgl/util/http_header.cpp
@@ -31,7 +31,7 @@ optional<Timestamp> parseRetryHeaders(const optional<std::string>& retryAfter,
     if (retryAfter) {
         try {
             auto secs = std::chrono::seconds(std::stoi(*retryAfter));
-            return std::chrono::time_point_cast<Seconds>(std::chrono::system_clock::now() + secs);
+            return std::chrono::system_clock::now() + secs;
         } catch (...) {
             return util::parseTimestamp((*retryAfter).c_str());
         }
diff --git a/src/mbgl/util/http_timeout.cpp b/src/mbgl/util/http_timeout.cpp
index 04842e48be..18d53e237c 100644
--- a/src/mbgl/util/http_timeout.cpp
+++ b/src/mbgl/util/http_timeout.cpp
@@ -33,7 +33,7 @@ Duration expirationTimeout(optional<Timestamp> expires, uint32_t expiredRequests
     if (expiredRequests) {
         return Seconds(1u << std::min(expiredRequests - 1, 31u));
     } else if (expires) {
-        return std::max(Seconds::zero(), *expires - util::now());
+        return std::max(std::chrono::system_clock::duration::zero(), *expires - util::now());
     } else {
         return Duration::max();
     }
diff --git a/test/storage/default_file_source.test.cpp b/test/storage/default_file_source.test.cpp
index 8853b3dd13..38c7abb039 100644
--- a/test/storage/default_file_source.test.cpp
+++ b/test/storage/default_file_source.test.cpp
@@ -5,6 +5,7 @@
 #include <mbgl/util/run_loop.hpp>
 
 using namespace mbgl;
+using namespace std::chrono;
 
 TEST(DefaultFileSource, TEST_REQUIRES_SERVER(CacheResponse)) {
     util::RunLoop loop;
@@ -34,7 +35,7 @@ TEST(DefaultFileSource, TEST_REQUIRES_SERVER(CacheResponse)) {
             EXPECT_EQ(response.error, res2.error);
             ASSERT_TRUE(res2.data.get());
             EXPECT_EQ(*response.data, *res2.data);
-            EXPECT_EQ(response.expires, res2.expires);
+            EXPECT_EQ(system_clock::to_time_t(*response.expires), system_clock::to_time_t(*res2.expires));
             EXPECT_EQ(response.mustRevalidate, res2.mustRevalidate);
             EXPECT_EQ(response.modified, res2.modified);
             EXPECT_EQ(response.etag, res2.etag);
@@ -267,7 +268,7 @@ TEST(DefaultFileSource, OptionalNonExpired) {
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Cached value", *res.data);
         ASSERT_TRUE(bool(res.expires));
-        EXPECT_EQ(*response.expires, *res.expires);
+        EXPECT_EQ(system_clock::to_time_t(*response.expires), system_clock::to_time_t(*res.expires));
         EXPECT_FALSE(res.mustRevalidate);
         EXPECT_FALSE(bool(res.modified));
         EXPECT_FALSE(bool(res.etag));
@@ -297,7 +298,7 @@ TEST(DefaultFileSource, OptionalExpired) {
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Cached value", *res.data);
         ASSERT_TRUE(bool(res.expires));
-        EXPECT_EQ(*response.expires, *res.expires);
+        EXPECT_EQ(system_clock::to_time_t(*response.expires), system_clock::to_time_t(*res.expires));
         EXPECT_FALSE(res.mustRevalidate);
         EXPECT_FALSE(bool(res.modified));
         EXPECT_FALSE(bool(res.etag));
diff --git a/test/util/http_timeout.test.cpp b/test/util/http_timeout.test.cpp
index c9373d955d..e696cd1c9c 100644
--- a/test/util/http_timeout.test.cpp
+++ b/test/util/http_timeout.test.cpp
@@ -41,8 +41,9 @@ TEST(HttpRetry, RateLimit) {
 }
 
 TEST(HttpRetry, ExpiredInitial) {
-    // 1 sec timeout
-    ASSERT_EQ(Seconds(1), expirationTimeout({ util::now() + Seconds(1) }, 0));
+    // 1 sec timeout, will not be exactly 1 because of precision/rounding
+    ASSERT_TRUE(Milliseconds(1000) >= expirationTimeout({ util::now() + Seconds(1) }, 0));
+    ASSERT_TRUE(Milliseconds(990) < expirationTimeout({ util::now() + Seconds(1) }, 0));
 }
 
 TEST(HttpRetry, ExpiredSubsequent) {