From 31fc69f67fc49b1a08f5561ae62d098106da6565 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 20 Jun 2023 15:36:11 -0700 Subject: [PATCH] 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: https://github.com/chronotope/chrono/issues/499 Test: treehugger Change-Id: I142995a3ab4deff1073a0aa9e63ce8eac850b93d --- README.md | 4 +- docs/status.md | 2 +- libc/Android.bp | 2 +- libc/include/time.h | 49 ++++++++------- .../context_lookup_benchmark_data.h | 2 +- libc/tzcode/bionic.cpp | 63 ++++++++++++------- libc/tzcode/localtime.c | 14 ++++- tests/time_test.cpp | 52 ++++++++++++--- 8 files changed, 130 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index 5107f42a4f..113ffd75cd 100644 --- a/README.md +++ b/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. ``` diff --git a/docs/status.md b/docs/status.md index 308c35434a..1838b85f06 100644 --- a/docs/status.md +++ b/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`. diff --git a/libc/Android.bp b/libc/Android.bp index ffa82226a8..dc3c73f3d2 100644 --- a/libc/Android.bp +++ b/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\"", diff --git a/libc/include/time.h b/libc/include/time.h index 5b762880f6..08f30ef8de 100644 --- a/libc/include/time.h +++ b/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. */ @@ -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. */ diff --git a/libc/system_properties/context_lookup_benchmark_data.h b/libc/system_properties/context_lookup_benchmark_data.h index b928875dd8..1d7250c05f 100644 --- a/libc/system_properties/context_lookup_benchmark_data.h +++ b/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 diff --git a/libc/tzcode/bionic.cpp b/libc/tzcode/bionic.cpp index d2b5d80fca..7091707ddb 100644 --- a/libc/tzcode/bionic.cpp +++ b/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,26 +82,10 @@ 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"); - - 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 +211,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 +238,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. diff --git a/libc/tzcode/localtime.c b/libc/tzcode/localtime.c index 5e1181fe87..018acadd52 100644 --- a/libc/tzcode/localtime.c +++ b/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 one file per timezone on Android, +// but we do have a system property instead. +#include +#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__) diff --git a/tests/time_test.cpp b/tests/time_test.cpp index bef51a94e7..abf6ce08c7 100644 --- a/tests/time_test.cpp +++ b/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 +}