Merge "Fix tzalloc(nullptr) and add a test." am: 0e437c00e0 am: 8e1ea43c79 am: 667d82ead1 am: 027f108b2e am: 16ad0c41f2

Original change: https://android-review.googlesource.com/c/platform/bionic/+/2630990

Change-Id: I15554d1527351fadca0f29ce9a5fb32a639454ab
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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

@@ -280,18 +280,25 @@ 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 timezone has changed.
  *
- * Android looks at both the system property `persist.sys.timezone` and the
+ * tzset() on Android looks at both the system property
- * environment variable `TZ`. The former is the device's current time zone
+ * `persist.sys.timezone` and the environment variable `TZ`. The former is
- * as shown in Settings, while the latter is usually unset but can be used
+ * the device's current timezone as shown in Settings, while the latter is
- * to override the global setting. This is a bad idea outside of unit tests
+ * usually unset but can be used to override the global setting. This is a
- * or single-threaded programs because it's inherently thread-unsafe.
+ * bad idea outside of unit tests or single-threaded programs because it's
- * See tzalloc(), localtime_rz(), mktime_z(), and tzfree() for an
+ * inherently thread-unsafe. See tzalloc(), localtime_rz(), mktime_z(),
- * alternative.
+ * 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.
+ *
+ * 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.
  *
@@ -320,7 +327,7 @@ clock_t clock(void);
 
 /**
  * [clock_getcpuclockid(3)](http://man7.org/linux/man-pages/man3/clock_getcpuclockid.3.html)
- * gets the clock id of the cpu-time clock for the given `pid`.
+ * gets the clock ID of the cpu-time clock for the given `pid`.
  *
  * Returns 0 on success, and returns -1 and returns an error number on failure.
  */

libc/tzcode/bionic.cpp

@@ -36,10 +36,45 @@
 #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 set, perhaps because this is called
+  // before the default value has been set (the recovery image being a
+  // classic example), fall back to GMT.
+  if (name == nullptr) name = "GMT";
+
+  strlcpy(buf, name, n);
+
+  if (!strcmp(buf, "GMT")) {
+    // Typically we'll set the system property to an Olson ID, but
+    // java.util.TimeZone also supports the "GMT+xxxx" style, and at
+    // least historically (see http://b/25463955) some Android-based set
+    // top boxes would get the timezone from the TV network in this format
+    // and use it directly in the system property. This caused trouble
+    // for native code because 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. We only need to worry about the "GMT+xxxx" case because
+    // the expectation is that these are valid java.util.TimeZone ids,
+    // not general POSIX custom timezone specifications (which is why this
+    // code only applies to the system property, and not to the environment
+    // variable).
+    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");
@@ -47,25 +82,9 @@ void tzset_unlocked() {
 
   // If that's not set, look at the "persist.sys.timezone" system property.
   if (name == nullptr) {
-    static CachedProperty persist_sys_timezone("persist.sys.timezone");
+    __bionic_get_system_tz(buf, sizeof(buf));
-
-    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;
   }
-    }
-  }
-
-  // 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);
 }

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.
+#if defined(__BIONIC__)
-#ifndef __BIONIC__
+// Android: there is no directory with one file per timezone 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

@@ -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
+}