Fix tzalloc(nullptr) and add a test.

This works (by reading /etc/localtime) on NetBSD, but not on Android
since we have no such file. Fix that by using our equivalent system
property instead.

Also s/time zone/timezone/ in documentation and comments. We've always
been inconsistent about this (as is upstream in code comments and
documentation) but it seems especially odd now we expose a _type_ that
spells it "timezone" to talk of "time zone" even as we're describing
that type and its associated functions.

Bug: chronotope/chrono#499
Test: treehugger
Change-Id: I142995a3ab4deff1073a0aa9e63ce8eac850b93d

README.md

@@ -134,9 +134,9 @@ libc/
   tzcode/
     # A modified superset of the IANA tzcode. Most of the modifications relate
     # to Android's use of a single file (with corresponding index) to contain
-    # time zone data.
+    # timezone data.
   zoneinfo/
-    # Android-format time zone data.
+    # Android-format timezone data.
     # See 'Updating tzdata' later.
 ```
 

docs/status.md

@@ -59,7 +59,7 @@ New libc functions in V (API level 35):
   * `timespec_getres` (C23 addition).
   * `localtime_rz`, `mktime_z`, `tzalloc`, and `tzfree` (NetBSD
     extensions implemented in tzcode, and the "least non-standard"
-    functions for avoiding $TZ if you need to use multiple time zones in
+    functions for avoiding $TZ if you need to use multiple timezones in
     multi-threaded C).
   * `mbsrtowcs_l` and `wcsrtombs_l` aliases for `mbsrtowcs` and `wcsrtombs`.
 

libc/Android.bp

@@ -269,7 +269,7 @@ cc_library_static {
         "-DTHREAD_SAFE=1",
         // The name of the tm_gmtoff field in our struct tm.
         "-DTM_GMTOFF=tm_gmtoff",
-        // TZDEFAULT is not applicable to Android as there is no one file per time zone mapping
+        // Android uses a system property instead of /etc/localtime, so make callers crash.
         "-DTZDEFAULT=NULL",
         // Where we store our tzdata.
         "-DTZDIR=\"/system/usr/share/zoneinfo\"",

libc/include/time.h

@@ -42,22 +42,22 @@ __BEGIN_DECLS
 /* If we just use void* in the typedef, the compiler exposes that in error messages. */
 struct __timezone_t;
 
-/** The `timezone_t` type that represents a time zone. */
+/** The `timezone_t` type that represents a timezone. */
 typedef struct __timezone_t* timezone_t;
 
 /** Divisor to compute seconds from the result of a call to clock(). */
 #define CLOCKS_PER_SEC 1000000
 
 /**
- * The name of the current time zone's non-daylight savings (`tzname[0]`) and
+ * The name of the current timezone's non-daylight savings (`tzname[0]`) and
  * daylight savings (`tzname[1]`) variants. See tzset().
  */
 extern char* _Nonnull tzname[];
 
-/** Whether the current time zone ever uses daylight savings time. See tzset(). */
+/** Whether the current timezone ever uses daylight savings time. See tzset(). */
 extern int daylight;
 
-/** The difference in seconds between UTC and the current time zone. See tzset(). */
+/** The difference in seconds between UTC and the current timezone. See tzset(). */
 extern long int timezone;
 
 struct sigevent;
@@ -86,7 +86,7 @@ struct tm {
   int tm_isdst;
   /** Offset from UTC (GMT) in seconds for this time. */
   long int tm_gmtoff;
-  /** Name of the time zone for this time. */
+  /** Name of the timezone for this time. */
   const char* _Nullable tm_zone;
 };
 
@@ -145,7 +145,7 @@ double difftime(time_t __lhs, time_t __rhs);
  * [mktime(3)](http://man7.org/linux/man-pages/man3/mktime.3p.html) converts
  * broken-down time `tm` into the number of seconds since the Unix epoch.
  *
- * See tzset() for details of how the time zone is set, and mktime_rz()
+ * See tzset() for details of how the timezone is set, and mktime_rz()
  * for an alternative.
  *
  * Returns the time in seconds on success, and returns -1 and sets `errno` on failure.
@@ -154,7 +154,7 @@ time_t mktime(struct tm* _Nonnull __tm);
 
 /**
  * mktime_z(3) converts broken-down time `tm` into the number of seconds
- * since the Unix epoch, assuming the given time zone.
+ * since the Unix epoch, assuming the given timezone.
  *
  * Returns the time in seconds on success, and returns -1 and sets `errno` on failure.
  *
@@ -178,7 +178,7 @@ struct tm* _Nullable localtime(const time_t* _Nonnull __t);
  * the number of seconds since the Unix epoch in `t` to a broken-down time.
  * That broken-down time will be written to the given struct `tm`.
  *
- * See tzset() for details of how the time zone is set, and localtime_rz()
+ * See tzset() for details of how the timezone is set, and localtime_rz()
  * for an alternative.
  *
  * Returns a pointer to a broken-down time on success, and returns null and sets `errno` on failure.
@@ -187,7 +187,7 @@ struct tm* _Nullable localtime_r(const time_t* _Nonnull __t, struct tm* _Nonnull
 
 /**
  * localtime_rz(3) converts the number of seconds since the Unix epoch in
- * `t` to a broken-down time, assuming the given time zone. That broken-down
+ * `t` to a broken-down time, assuming the given timezone. That broken-down
  * time will be written to the given struct `tm`.
  *
  * Returns a pointer to a broken-down time on success, and returns null and sets `errno` on failure.
@@ -278,29 +278,36 @@ char* _Nullable ctime_r(const time_t* _Nonnull __t, char* _Nonnull __buf);
 
 /**
  * [tzset(3)](http://man7.org/linux/man-pages/man3/tzset.3.html) tells
- * libc that the time zone has changed.
+ * libc that the timezone has changed.
  *
- * Android looks at both the system property `persist.sys.timezone` and the
- * environment variable `TZ`. The former is the device's current time zone
- * as shown in Settings, while the latter is usually unset but can be used
- * to override the global setting. This is a bad idea outside of unit tests
- * or single-threaded programs because it's inherently thread-unsafe.
- * See tzalloc(), localtime_rz(), mktime_z(), and tzfree() for an
- * alternative.
+ * tzset() on Android looks at both the system property
+ * `persist.sys.timezone` and the environment variable `TZ`. The former is
+ * the device's current timezone as shown in Settings, while the latter is
+ * usually unset but can be used to override the global setting. This is a
+ * bad idea outside of unit tests or single-threaded programs because it's
+ * inherently thread-unsafe. See tzalloc(), localtime_rz(), mktime_z(),
+ * and tzfree() for an alternative.
  */
 void tzset(void);
 
 /**
- * tzalloc(3) allocates a time zone corresponding to the given Olson id.
+ * tzalloc(3) allocates a timezone corresponding to the given Olson id.
  *
- * Returns a time zone object on success, and returns NULL and sets `errno` on failure.
+ * A null `id` returns the system timezone (as seen in Settings) from
+ * the system property `persist.sys.timezone`, ignoring `$TZ`. Although
+ * tzset() honors `$TZ`, callers of tzalloc() can use `$TZ` themselves if
+ * that's the (thread-unsafe) behavior they want, but by ignoring `$TZ`
+ * tzalloc() is thread safe (though obviously the system timezone can
+ * change, especially if your mobile device is actually mobile!).
+ *
+ * Returns a timezone object on success, and returns NULL and sets `errno` on failure.
  *
  * Available since API level 35.
  */
 timezone_t _Nullable tzalloc(const char* _Nullable __id) __INTRODUCED_IN(35);
 
 /**
- * tzfree(3) frees a time zone object returned by tzalloc().
+ * tzfree(3) frees a timezone object returned by tzalloc().
  *
  * Available since API level 35.
  */

libc/system_properties/context_lookup_benchmark_data.h

@@ -321,7 +321,7 @@ persist.odm.            u:object_r:vendor_default_prop:s0
 persist.vendor.         u:object_r:vendor_default_prop:s0
 vendor.                 u:object_r:vendor_default_prop:s0
 
-# Properties that relate to time / time zone detection behavior.
+# Properties that relate to time / timezone detection behavior.
 persist.time.           u:object_r:time_prop:s0
 
 # Properties that relate to server configurable flags

libc/tzcode/bionic.cpp

@@ -36,37 +36,44 @@
 #include "private/CachedProperty.h"
 
 extern "C" void tzset_unlocked(void);
+extern "C" void __bionic_get_system_tz(char* buf, size_t n);
 extern "C" int __bionic_open_tzdata(const char*, int32_t*);
 
 extern "C" void tzsetlcl(char const*);
 
+void __bionic_get_system_tz(char* buf, size_t n) {
+  static CachedProperty persist_sys_timezone("persist.sys.timezone");
+  const char* name = persist_sys_timezone.Get();
+
+  // If the system property is not available (because you're running AOSP on a WiFi-only
+  // device, say), fall back to GMT.
+  if (name == nullptr) name = "GMT";
+
+  strlcpy(buf, name, n);
+
+  if (!strcmp(buf, "GMT")) {
+    // POSIX and Java disagree about the sign in a timezone string. For POSIX, "GMT+3" means
+    // "3 hours west/behind", but for Java it means "3 hours east/ahead". Since (a) Java is
+    // the one that matches human expectations and (b) this system property is used directly
+    // by Java, we flip the sign here to translate from Java to POSIX. http://b/25463955.
+    char sign = buf[3];
+    if (sign == '-' || sign == '+') {
+      buf[3] = (sign == '-') ? '+' : '-';
+    }
+  }
+}
+
 void tzset_unlocked() {
   // The TZ environment variable is meant to override the system-wide setting.
   const char* name = getenv("TZ");
   char buf[PROP_VALUE_MAX];
 
   // If that's not set, look at the "persist.sys.timezone" system property.
   if (name == nullptr) {
-    static CachedProperty persist_sys_timezone("persist.sys.timezone");
-
-    if ((name = persist_sys_timezone.Get()) != nullptr && strlen(name) > 3) {
-      // POSIX and Java disagree about the sign in a timezone string. For POSIX, "GMT+3" means
-      // "3 hours west/behind", but for Java it means "3 hours east/ahead". Since (a) Java is
-      // the one that matches human expectations and (b) this system property is used directly
-      // by Java, we flip the sign here to translate from Java to POSIX. http://b/25463955.
-      char sign = name[3];
-      if (sign == '-' || sign == '+') {
-        strlcpy(buf, name, sizeof(buf));
-        buf[3] = (sign == '-') ? '+' : '-';
-        name = buf;
-      }
-    }
+    __bionic_get_system_tz(buf, sizeof(buf));
+    name = buf;
   }
 
-  // If the system property is also not available (because you're running AOSP on a WiFi-only
-  // device, say), fall back to GMT.
-  if (name == nullptr) name = "GMT";
-
   tzsetlcl(name);
 }
 
@@ -192,14 +199,14 @@ static int __bionic_open_tzdata_path(const char* path,
     close(fd);
     // This file descriptor (-1) is passed to localtime.c. In invalid fd case
     // upstream passes errno value around methods and having 0 there will
-    // indicate that time zone was found and read successfully and localtime's
+    // indicate that timezone was found and read successfully and localtime's
     // internal state was properly initialized (which wasn't as we couldn't find
-    // requested time zone in the tzdata file).
+    // requested timezone in the tzdata file).
     // If we reached this point errno is unlikely to be touched. It is only
     // close(fd) which can do it, but that is very unlikely to happen. And
     // even if it happens we can't extract any useful insights from it.
     // We are overriding it to ENOENT as it matches upstream expectations -
-    // time zone is absent in the tzdata file == there is no TZif file in
+    // timezone is absent in the tzdata file == there is no TZif file in
     // /usr/share/zoneinfo.
     errno = ENOENT;
     return -1;
@@ -219,7 +226,7 @@ int __bionic_open_tzdata(const char* olson_id, int32_t* entry_length) {
   int fd;
 
   // Try the two locations for the tzdata file in a strict order:
-  // 1: The time zone data module which contains the main copy. This is the
+  // 1: The timezone data module which contains the main copy. This is the
   //    common case for current devices.
   // 2: The ultimate fallback: the non-updatable copy in /system.
 

libc/tzcode/localtime.c

@@ -374,8 +374,11 @@ union input_buffer {
      + 4 * TZ_MAX_TIMES];
 };
 
-// Android-removed: There is no directory with file-per-time zone on Android.
-#ifndef __BIONIC__
+#if defined(__BIONIC__)
+// Android: there is no directory with file-per-time zone on Android,
+// but we do have a system property instead.
+#include <sys/system_properties.h>
+#else
 /* TZDIR with a trailing '/' rather than a trailing '\0'.  */
 static char const tzdirslash[sizeof TZDIR] = TZDIR "/";
 #endif
@@ -415,13 +418,20 @@ tzloadbody(char const *name, struct state *sp, bool doextend,
 #endif
 	register union input_buffer *up = &lsp->u.u;
 	register int tzheadsize = sizeof(struct tzhead);
+	char system_tz_name[PROP_VALUE_MAX];
 
 	sp->goback = sp->goahead = false;
 
 	if (! name) {
+#if defined(__BIONIC__)
+		extern void __bionic_get_system_tz(char* , size_t);
+		__bionic_get_system_tz(system_tz_name, sizeof(system_tz_name));
+		name = system_tz_name;
+#else
 		name = TZDEFAULT;
 		if (! name)
 		  return EINVAL;
+#endif
 	}
 
 #if defined(__BIONIC__)

tests/time_test.cpp

@@ -97,7 +97,7 @@ TEST(time, mktime_TZ_as_UTC_and_offset) {
 
 static void* gmtime_no_stack_overflow_14313703_fn(void*) {
   const char* original_tz = getenv("TZ");
-  // Ensure we'll actually have to enter tzload by using a time zone that doesn't exist.
+  // Ensure we'll actually have to enter tzload by using a timezone that doesn't exist.
   setenv("TZ", "gmtime_stack_overflow_14313703", 1);
   tzset();
   if (original_tz != nullptr) {
@@ -324,7 +324,7 @@ TEST(time, strftime_Z_null_tm_zone) {
 
 // According to C language specification the only tm struct field needed to
 // find out replacement for %z and %Z in strftime is tm_isdst. Which is
-// wrong, as time zones change their standard offset and even DST savings.
+// wrong, as timezones change their standard offset and even DST savings.
 // tzcode deviates from C language specification and requires tm struct either
 // to be output of localtime-like functions or to be modified by mktime call
 // before passing to strftime. See tz mailing discussion for more details
@@ -560,7 +560,7 @@ TEST(time, strptime_Z) {
   EXPECT_EQ(1, tm.tm_isdst);
   EXPECT_EQ(3600, tm.tm_gmtoff);
 
-  // And as long as we're in Europe/Berlin, those are the only time zone
+  // And as long as we're in Europe/Berlin, those are the only timezone
   // abbreviations that are recognized.
   tm = {};
   ASSERT_TRUE(strptime("PDT", "%Z", &tm) == nullptr);
@@ -1118,7 +1118,7 @@ TEST(time, bug_31938693) {
   // Actual underlying bug (the code change, not the tzdata upgrade that first exposed the bug):
   // http://b/31848040
 
-  // This isn't a great test, because very few time zones were actually affected, and there's
+  // This isn't a great test, because very few timezones were actually affected, and there's
   // no real logic to which ones were affected: it was just a coincidence of the data that came
   // after them in the tzdata file.
 
@@ -1159,10 +1159,10 @@ TEST(time, bug_31938693) {
 TEST(time, bug_31339449) {
   // POSIX says localtime acts as if it calls tzset.
   // tzset does two things:
-  //  1. it sets the time zone ctime/localtime/mktime/strftime will use.
+  //  1. it sets the timezone ctime/localtime/mktime/strftime will use.
   //  2. it sets the global `tzname`.
   // POSIX says localtime_r need not set `tzname` (2).
-  // Q: should localtime_r set the time zone (1)?
+  // Q: should localtime_r set the timezone (1)?
   // Upstream tzcode (and glibc) answer "no", everyone else answers "yes".
 
   // Pick a time, any time...
@@ -1361,7 +1361,7 @@ TEST(time, localtime_rz) {
   ASSERT_EQ(&tm, localtime_rz(seoul, &t, &tm));
   AssertTmEq(tm, 17);
 
-  // Just check that mktime()'s time zone didn't change.
+  // Just check that mktime()'s timezone didn't change.
   tm = {};
   ASSERT_EQ(&tm, localtime_r(&t, &tm));
   ASSERT_EQ(0, tm.tm_hour);
@@ -1403,7 +1403,7 @@ TEST(time, mktime_z) {
   tm = {.tm_year = 93, .tm_mday = 1};
   ASSERT_EQ(725814000, mktime_z(seoul, &tm));
 
-  // Just check that mktime()'s time zone didn't change.
+  // Just check that mktime()'s timezone didn't change.
   tm = {.tm_year = 93, .tm_mday = 1};
   ASSERT_EQ(725875200, mktime(&tm));
 
@@ -1416,3 +1416,39 @@ TEST(time, mktime_z) {
   GTEST_SKIP() << "glibc doesn't have timezone_t";
 #endif
 }
+
+TEST(time, tzalloc_nullptr) {
+#if __BIONIC__
+  // tzalloc(nullptr) returns the system timezone.
+  timezone_t default_tz = tzalloc(nullptr);
+  ASSERT_NE(nullptr, default_tz);
+
+  // Check that mktime_z() with the default timezone matches mktime().
+  // This assumes that the system timezone doesn't change during the test,
+  // but that should be unlikely, and we don't have much choice if we
+  // want to write a test at all.
+  // We unset $TZ before calling mktime() because mktime() honors $TZ.
+  unsetenv("TZ");
+  struct tm tm = {.tm_year = 93, .tm_mday = 1};
+  time_t t = mktime(&tm);
+  ASSERT_EQ(t, mktime_z(default_tz, &tm));
+
+  // Check that changing $TZ doesn't affect the tzalloc() default in
+  // the same way it would the mktime() default.
+  setenv("TZ", "America/Los_Angeles", 1);
+  tzset();
+  ASSERT_EQ(t, mktime_z(default_tz, &tm));
+
+  setenv("TZ", "Europe/London", 1);
+  tzset();
+  ASSERT_EQ(t, mktime_z(default_tz, &tm));
+
+  setenv("TZ", "Asia/Seoul", 1);
+  tzset();
+  ASSERT_EQ(t, mktime_z(default_tz, &tm));
+
+  tzfree(default_tz);
+#else
+  GTEST_SKIP() << "glibc doesn't have timezone_t";
+#endif
+}