Fix multiple minor tzset glitches [BZ #24004]

Message ID 20190211163728.31655-1-eggert@cs.ucla.edu
State New
Headers show
Series
  • Fix multiple minor tzset glitches [BZ #24004]
Related show

Commit Message

Paul Eggert Feb. 11, 2019, 4:37 p.m.
Avoid polling if TZ is unset.
Consistently ignore leading ":" in TZ as per documentation.
Treat TZ="" as Universal Time without leap seconds,
for consistency with upstream.
* time/tzset.c (tzset_internal): Check for null TZ first.
Do not substitute "Universal" for ""; this is __tzfile_read’s
job and we were doing it incorrectly as we treated TZ=""
differently from TZ=":".
* time/tzfile.c (__tzfile_read): Simplify, as FILE cannot be null.
---
 ChangeLog     | 13 +++++++++++++
 NEWS          |  7 +++++++
 time/tzfile.c |  5 +----
 time/tzset.c  | 15 ++++++---------
 4 files changed, 27 insertions(+), 13 deletions(-)

Comments

Florian Weimer Feb. 18, 2019, 9:37 a.m. | #1
* Paul Eggert:

> Avoid polling if TZ is unset.

I thought this polling (for reloading /etc/localtime if it has changed)
was a feature.  I'm not sure if we can remove it.

Thanks,
Florian
Paul Eggert Feb. 20, 2019, 7:29 p.m. | #2
On 2/18/19 1:37 AM, Florian Weimer wrote:
> I thought this polling (for reloading /etc/localtime if it has changed)
> was a feature.  I'm not sure if we can remove it.

No such polling is documented. On the contrary, the glibc manual says 
that an unset TZ is treated as if it were set to ':/etc/localtime'. 
There is no polling when you set TZ=':/etc/localtime' (or to 
TZ='/etc/localtime', or to any other value). So the code and the 
documentation disagree, and one or the other needs to be fixed.

Neither Solaris nor OpenBSD poll, so portable programs cannot assume 
that polling occurs. (These are the only two other systems I checked.)

Commentary like this:

https://blog.packagecloud.io/eng/2017/02/21/set-environment-variable-save-thousands-of-system-calls/

says that polling is a bad idea, and gives TZ=':/etc/localtime' as a 
workaround for glibc. I'm sympathetic, as the polling makes glibc look 
bad compared to the competition.

Patch

diff --git a/ChangeLog b/ChangeLog
index 6f1d967f62..831d89ba8f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@ 
+2019-02-11  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix multiple minor tzset glitches [BZ #24004]
+	Avoid polling if TZ is unset.
+	Consistently ignore leading ":" in TZ as per documentation.
+	Treat TZ="" as Universal Time without leap seconds,
+	for consistency with upstream.
+	* time/tzset.c (tzset_internal): Check for null TZ first.
+	Do not substitute "Universal" for ""; this is __tzfile_read’s
+	job and we were doing it incorrectly as we treated TZ=""
+	differently from TZ=":".
+	* time/tzfile.c (__tzfile_read): Simplify, as FILE cannot be null.
+
 2019-02-11  Paul A. Clarke  <pc@us.ibm.com>
 
 	* sysdeps/powerpc/fpu/e_sqrt.c (__slow_ieee754_sqrtf):
diff --git a/NEWS b/NEWS
index 0a3b6c7a5a..87de5030c1 100644
--- a/NEWS
+++ b/NEWS
@@ -22,6 +22,13 @@  Deprecated and removed features, and other changes affecting compatibility:
   definitions in libc will be used automatically, which have been available
   since glibc 2.17.
 
+* If the TZ environment variable is unset, functions like localtime no
+  longer stat the file /etc/localtime if the file has already been
+  consulted.  Instead, they reuse the cached timezone info, just as they
+  would with TZ="/etc/localtime".  If TZ="", these functions now omit leap
+  seconds even on unusual installations where TZ="Universal" identifies a
+  TZif file with leap seconds; this is for consistency with tzcode and BSD.
+
 Changes to build and runtime requirements:
 
 * GCC 6.2 or later is required to build the GNU C Library.
diff --git a/time/tzfile.c b/time/tzfile.c
index 7229ed93b7..db66f22a4e 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -115,10 +115,7 @@  __tzfile_read (const char *file, size_t extra, char **extrap)
 
   __use_tzfile = 0;
 
-  if (file == NULL)
-    /* No user specification; use the site-wide default.  */
-    file = TZDEFAULT;
-  else if (*file == '\0')
+  if (*file == '\0')
     /* User specified the empty string; use UTC with no leap seconds.  */
     goto ret_free_transitions;
   else
diff --git a/time/tzset.c b/time/tzset.c
index 307eb651ad..1903ef864d 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -375,25 +375,22 @@  tzset_internal (int always)
 
   /* Examine the TZ environment variable.  */
   tz = getenv ("TZ");
-  if (tz && *tz == '\0')
-    /* User specified the empty string; use UTC explicitly.  */
-    tz = "Universal";
+
+  if (tz == NULL)
+    /* No user specification; use the site-wide default.  */
+    tz = TZDEFAULT;
 
   /* A leading colon means "implementation defined syntax".
      We ignore the colon and always use the same algorithm:
      try a data file, and if none exists parse the 1003.1 syntax.  */
-  if (tz && *tz == ':')
+  if (*tz == ':')
     ++tz;
 
   /* Check whether the value changed since the last run.  */
-  if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
+  if (old_tz != NULL && strcmp (tz, old_tz) == 0)
     /* No change, simply return.  */
     return;
 
-  if (tz == NULL)
-    /* No user specification; use the site-wide default.  */
-    tz = TZDEFAULT;
-
   tz_rules[0].name = NULL;
   tz_rules[1].name = NULL;