Improve time handling and error checking

The database settings object’s last_upload_attempt_time (time_t) field
is switched from uint64_t to int64_t, for better compatibility with
time_t, which is normally a signed type. This change should be
transparent, as there should be no valid high-bit-set 64-bit timestamps
in this field in the wild.

A number of improvements are made to crashpad_database_util’s time
handling. Errors are checked during time conversion.
--set-last-upload-attempt-time=now is a new supported (and documented)
option.

A StringToNumber() overload for int64_t, along with a test, is added to
aid in crashpad_database_util’s time conversions from numeric strings. A
test is also added for the previously-untested uint64_t implementation.

TEST=crashpad_util_test StringNumberConversion.*

Change-Id: I089c4bf7b95f5df0982bdbb3c27b4f6a89db966e
Reviewed-on: https://chromium-review.googlesource.com/410068
Reviewed-by: Robert Sesek <rsesek@chromium.org>

client/settings.cc

@@ -55,7 +55,7 @@ struct Settings::Data {
   uint32_t version;
   uint32_t options;
   uint32_t padding_0;
-  uint64_t last_upload_attempt_time;  // time_t
+  int64_t last_upload_attempt_time;  // time_t
   UUID client_id;
 };
 
@@ -136,7 +136,7 @@ bool Settings::SetLastUploadAttemptTime(time_t time) {
   if (!handle.is_valid())
     return false;
 
-  settings.last_upload_attempt_time = InRangeCast<uint64_t>(time, 0);
+  settings.last_upload_attempt_time = InRangeCast<int64_t>(time, 0);
 
   return WriteSettings(handle.get(), settings);
 }

tools/crashpad_database_util.cc

@@ -12,12 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <errno.h>
 #include <getopt.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <strings.h>
+#include <sys/time.h>
 #include <sys/types.h>
 #include <time.h>
 
@@ -38,6 +38,7 @@
 #include "util/file/file_io.h"
 #include "util/file/file_reader.h"
 #include "util/misc/uuid.h"
+#include "util/stdlib/string_number_conversion.h"
 
 namespace crashpad {
 namespace {
@@ -130,16 +131,24 @@ std::string BoolToString(bool boolean) {
   return std::string(boolean ? "true" : "false");
 }
 
-// Converts |string| to |time|, returning true if a conversion could be
+// Converts |string| to |out_time|, returning true if a conversion could be
 // performed, and false without setting |boolean| if no conversion could be
 // performed. Various time formats are recognized, including several string
-// representations and a numeric time_t representation. The special string
-// "never" is recognized as |string| and converts to a |time| value of 0. |utc|,
-// when true, causes |string| to be interpreted as a UTC time rather than a
-// local time when the time zone is ambiguous.
-bool StringToTime(const char* string, time_t* time, bool utc) {
+// representations and a numeric time_t representation. The special |string|
+// "never" is recognized as converted to a |out_time| value of 0; "now" is
+// converted to the current time. |utc|, when true, causes |string| to be
+// interpreted as a UTC time rather than a local time when the time zone is
+// ambiguous.
+bool StringToTime(const char* string, time_t* out_time, bool utc) {
   if (strcasecmp(string, "never") == 0) {
-    *time = 0;
+    *out_time = 0;
+    return true;
+  }
+
+  if (strcasecmp(string, "now") == 0) {
+    timeval tv;
+    PCHECK(gettimeofday(&tv, nullptr) == 0);
+    *out_time = tv.tv_sec;
     return true;
   }
 
@@ -155,41 +164,53 @@ bool StringToTime(const char* string, time_t* time, bool utc) {
     tm time_tm;
     const char* strptime_result = strptime(string, kFormats[index], &time_tm);
     if (strptime_result == end) {
+      time_t test_out_time;
       if (utc) {
-        *time = timegm(&time_tm);
+        test_out_time = timegm(&time_tm);
       } else {
-        *time = mktime(&time_tm);
+        test_out_time = mktime(&time_tm);
       }
 
-      return true;
+      // mktime() is supposed to set errno in the event of an error, but support
+      // for this is spotty, so there’s no way to distinguish between a true
+      // time_t of -1 (1969-12-31 23:59:59 UTC) and an error. Assume error.
+      //
+      // See 10.11.5 Libc-1082.50.1/stdtime/FreeBSD/localtime.c and
+      // glibc-2.24/time/mktime.c, which don’t set errno or save and restore
+      // errno. Post-Android 7.1.0 Bionic is even more hopeless, setting errno
+      // whenever the time conversion returns -1, even for valid input. See
+      // libc/tzcode/localtime.c mktime(). Windows seems to get it right: see
+      // 10.0.14393 SDK Source/ucrt/time/mktime.cpp.
+      if (test_out_time != -1) {
+        *out_time = test_out_time;
+        return true;
+      }
     }
   }
 
-  char* end_result;
-  errno = 0;
-  long long strtoll_result = strtoll(string, &end_result, 0);
-  if (end_result == end && errno == 0 &&
-      base::IsValueInRangeForNumericType<time_t>(strtoll_result)) {
-    *time = strtoll_result;
+  int64_t int64_result;
+  if (StringToNumber(string, &int64_result) &&
+      base::IsValueInRangeForNumericType<time_t>(int64_result)) {
+    *out_time = int64_result;
     return true;
   }
 
   return false;
 }
 
-// Converts |time_tt| to a string, and returns it. |utc| determines whether the
-// converted time will reference local time or UTC. If |time_tt| is 0, the
+// Converts |out_time| to a string, and returns it. |utc| determines whether the
+// converted time will reference local time or UTC. If |out_time| is 0, the
 // string "never" will be returned as a special case.
-std::string TimeToString(time_t time_tt, bool utc) {
-  if (time_tt == 0) {
+std::string TimeToString(time_t out_time, bool utc) {
+  if (out_time == 0) {
     return std::string("never");
   }
 
   tm time_tm;
   if (utc) {
-    gmtime_r(&time_tt, &time_tm);
+    PCHECK(gmtime_r(&out_time, &time_tm));
   } else {
-    localtime_r(&time_tt, &time_tm);
+    PCHECK(localtime_r(&out_time, &time_tm));
   }
 
   char string[64];

tools/crashpad_database_util.md

@@ -118,7 +118,8 @@ database.
 
    Set the last-upload-attempt time in the database’s settings. _TIME_ is a
    string representation of a time, which may be in _yyyy-mm-dd hh:mm:ss_
-   format, a numeric `time_t` value, or the special string `"never"`.
+   format, a numeric `time_t` value, or the special strings `"never"` or
+   `"now"`.
 
    See also **--show-last-upload-attempt-time**.
 

util/stdlib/string_number_conversion.cc

@@ -110,6 +110,13 @@ struct StringToUnsignedIntTraits
   }
 };
 
+struct StringToInt64Traits
+    : public StringToSignedIntegerTraits<int64_t, int64_t> {
+  static LongType Convert(const char* str, char** end, int base) {
+    return strtoll(str, end, base);
+  }
+};
+
 struct StringToUnsignedInt64Traits
     : public StringToUnsignedIntegerTraits<uint64_t, uint64_t> {
   static LongType Convert(const char* str, char** end, int base) {
@@ -166,6 +173,10 @@ bool StringToNumber(const base::StringPiece& string, unsigned int* number) {
   return StringToIntegerInternal<StringToUnsignedIntTraits>(string, number);
 }
 
+bool StringToNumber(const base::StringPiece& string, int64_t* number) {
+  return StringToIntegerInternal<StringToInt64Traits>(string, number);
+}
+
 bool StringToNumber(const base::StringPiece& string, uint64_t* number) {
   return StringToIntegerInternal<StringToUnsignedInt64Traits>(string, number);
 }

util/stdlib/string_number_conversion.h

@@ -56,6 +56,7 @@ namespace crashpad {
 //!     where such prefix recognition is desirable.
 bool StringToNumber(const base::StringPiece& string, int* number);
 bool StringToNumber(const base::StringPiece& string, unsigned int* number);
+bool StringToNumber(const base::StringPiece& string, int64_t* number);
 bool StringToNumber(const base::StringPiece& string, uint64_t* number);
 //! \}
 

util/stdlib/string_number_conversion_test.cc

@@ -221,6 +221,106 @@ TEST(StringNumberConversion, StringToUnsignedInt) {
   EXPECT_EQ(6u, output);
 }
 
+TEST(StringNumberConversion, StringToInt64) {
+  const struct {
+    const char* string;
+    bool valid;
+    int64_t value;
+  } kTestData[] = {
+      {"", false, 0},
+      {"0", true, 0},
+      {"1", true, 1},
+      {"2147483647", true, 2147483647},
+      {"2147483648", true, 2147483648},
+      {"4294967295", true, 4294967295},
+      {"4294967296", true, 4294967296},
+      {"9223372036854775807", true, std::numeric_limits<int64_t>::max()},
+      {"9223372036854775808", false, 0},
+      {"18446744073709551615", false, 0},
+      {"18446744073709551616", false, 0},
+      {"-1", true, -1},
+      {"-2147483648", true, -2147483648},
+      {"-2147483649", true, -2147483649},
+      {"-2147483648", true, -2147483648},
+      {"-9223372036854775808", true, std::numeric_limits<int64_t>::min()},
+      {"-9223372036854775809", false, 0},
+      {"0x7fffffffffffffff", true, std::numeric_limits<int64_t>::max()},
+      {"0x8000000000000000", false, 0},
+      {"0xffffffffffffffff", false, 0},
+      {"0x10000000000000000", false, 0},
+      {"-0x7fffffffffffffff", true, -9223372036854775807},
+      {"-0x8000000000000000", true, std::numeric_limits<int64_t>::min()},
+      {"-0x8000000000000001", false, 0},
+      {"0x7Fffffffffffffff", true, std::numeric_limits<int64_t>::max()},
+  };
+
+  for (size_t index = 0; index < arraysize(kTestData); ++index) {
+    int64_t value;
+    bool valid = StringToNumber(kTestData[index].string, &value);
+    if (kTestData[index].valid) {
+      EXPECT_TRUE(valid) << "index " << index << ", string "
+                         << kTestData[index].string;
+      if (valid) {
+        EXPECT_EQ(kTestData[index].value, value)
+            << "index " << index << ", string " << kTestData[index].string;
+      }
+    } else {
+      EXPECT_FALSE(valid) << "index " << index << ", string "
+                          << kTestData[index].string << ", value " << value;
+    }
+  }
+}
+
+TEST(StringNumberConversion, StringToUnsignedInt64) {
+  const struct {
+    const char* string;
+    bool valid;
+    uint64_t value;
+  } kTestData[] = {
+      {"", false, 0},
+      {"0", true, 0},
+      {"1", true, 1},
+      {"2147483647", true, 2147483647},
+      {"2147483648", true, 2147483648},
+      {"4294967295", true, 4294967295},
+      {"4294967296", true, 4294967296},
+      {"9223372036854775807", true, 9223372036854775807},
+      {"9223372036854775808", true, 9223372036854775808u},
+      {"18446744073709551615", true, std::numeric_limits<uint64_t>::max()},
+      {"18446744073709551616", false, 0},
+      {"-1", false, 0},
+      {"-2147483648", false, 0},
+      {"-2147483649", false, 0},
+      {"-2147483648", false, 0},
+      {"-9223372036854775808", false, 0},
+      {"-9223372036854775809", false, 0},
+      {"0x7fffffffffffffff", true, 9223372036854775807},
+      {"0x8000000000000000", true, 9223372036854775808u},
+      {"0xffffffffffffffff", true, std::numeric_limits<uint64_t>::max()},
+      {"0x10000000000000000", false, 0},
+      {"-0x7fffffffffffffff", false, 0},
+      {"-0x8000000000000000", false, 0},
+      {"-0x8000000000000001", false, 0},
+      {"0xFfffffffffffffff", true, std::numeric_limits<uint64_t>::max()},
+  };
+
+  for (size_t index = 0; index < arraysize(kTestData); ++index) {
+    uint64_t value;
+    bool valid = StringToNumber(kTestData[index].string, &value);
+    if (kTestData[index].valid) {
+      EXPECT_TRUE(valid) << "index " << index << ", string "
+                         << kTestData[index].string;
+      if (valid) {
+        EXPECT_EQ(kTestData[index].value, value)
+            << "index " << index << ", string " << kTestData[index].string;
+      }
+    } else {
+      EXPECT_FALSE(valid) << "index " << index << ", string "
+                          << kTestData[index].string << ", value " << value;
+    }
+  }
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad