bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Valery Ushakov]

Emacs 25 treats tzalloc(3) failure as out-of-memory condition.
E.g. when an invalid timezone is specified, it fails to start with:

$ TZ=FOOBAR emacs -nw
emacs: Memory exhausted--use M-x save-some-buffers then exit and restart Emacs


The code in tzlookup() also assumes that tzalloc(3) understands the
direct zone specification in the name/offset format.  I haven't
checked tzcode history, but this support is only a few years old
(around 2014, I'd estimate).  E.g. NetBSD-6 has older tzcode(3) that
doesn't support this feature.  This leads to a lot of "Memory
exhausted" errors when trying to use e.g. vc.el

  "encode-time" (0xffffa2d4)
  "apply" (0xffffa3fc)
  "vc-cvs-parse-entry" (0xffffa718)
  "vc-cvs-registered" (0xffffa9b0)
  "progn" (0xffffab44)
  "if" (0xffffac24)
  "vc-cvs-registered" (0xffffae7c)
  "apply" (0xffffae78)
  "vc-call-backend" (0xffffb188)
  0x1c65360 PVEC_COMPILED
  "mapc" (0xffffb5c8)
  "vc-registered" (0xffffb8d8)
  "vc-backend" (0xffffbbe8)
  "vc-refresh-state" (0xffffbfa0)
  "run-hooks" (0xffffc08c)
  "after-find-file" (0xffffc3b4)
  ...

-uwe

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 264 bytes --]

[resending, I hope to the right place this time]

Thanks for reporting the problem. I have installed the attached two patches,
which I think should fix the problem so I'm closing the bug report. Please give
them a try on NetBSD (as I typically don't use NetBSD).


[-- Attachment #2: 0001-Fix-minor-timezone-memory-leak.patch --]
[-- Type: text/x-patch, Size: 1219 bytes --]

From f7c07930b581b1bcfdfb1874b6883233516bdf11 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 16 May 2017 14:19:36 -0700
Subject: [PATCH] Fix minor timezone memory leak

* src/editfns.c (wall_clock_tz): Remove; unused.
---
 src/editfns.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/editfns.c b/src/editfns.c
index ecb8e3f083..75eb75a729 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -81,10 +81,8 @@ static Lisp_Object styled_format (ptrdiff_t, Lisp_Object *, bool);
 
 enum { tzeqlen = sizeof "TZ=" - 1 };
 
-/* Time zones equivalent to current local time, to wall clock time,
-   and to UTC, respectively.  */
+/* Time zones equivalent to current local time and to UTC, respectively.  */
 static timezone_t local_tz;
-static timezone_t wall_clock_tz;
 static timezone_t const utc_tz = 0;
 
 /* The cached value of Vsystem_name.  This is used only to compare it
@@ -269,7 +267,6 @@ init_editfns (bool dumping)
 
   /* Set the time zone rule now, so that the call to putenv is done
      before multiple threads are active.  */
-  wall_clock_tz = xtzalloc (0);
   tzlookup (tz ? build_string (tz) : Qwall, true);
 
   pw = getpwuid (getuid ());
-- 
2.14.3


[-- Attachment #3: 0001-Port-to-NetBSD-tzalloc.patch --]
[-- Type: text/x-patch, Size: 1590 bytes --]

From 46844ad78968cd804d0e5fc98ce49b46b3d8a53d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Mar 2018 00:18:34 -0800
Subject: [PATCH] Port to NetBSD tzalloc

Problem reported by Valery Ushakov (Bug#30738).
* src/editfns.c (xtzalloc): Remove.
(invalid_time_zone_specification): New function.
(tzlookup): Port to NetBSD, where tzalloc can fail when the TZ
string has an invalid value.
---
 src/editfns.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/editfns.c b/src/editfns.c
index 3a34dd0980..debe10572d 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -117,14 +117,10 @@ emacs_mktime_z (timezone_t tz, struct tm *tm)
   return t;
 }
 
-/* Allocate a timezone, signaling on failure.  */
-static timezone_t
-xtzalloc (char const *name)
+static _Noreturn void
+invalid_time_zone_specification (Lisp_Object zone)
 {
-  timezone_t tz = tzalloc (name);
-  if (!tz)
-    memory_full (SIZE_MAX);
-  return tz;
+  xsignal2 (Qerror, build_string ("Invalid time zone specification"), zone);
 }
 
 /* Free a timezone, except do not free the time zone for local time.
@@ -205,9 +201,15 @@ tzlookup (Lisp_Object zone, bool settz)
 	    }
 	}
       else
-	xsignal2 (Qerror, build_string ("Invalid time zone specification"),
-		  zone);
-      new_tz = xtzalloc (zone_string);
+	invalid_time_zone_specification (zone);
+
+      new_tz = tzalloc (zone_string);
+      if (!new_tz)
+	{
+	  if (errno == ENOMEM)
+	    memory_full (SIZE_MAX);
+	  invalid_time_zone_specification (zone);
+	}
     }
 
   if (settz)
-- 
2.14.3

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Valery Ushakov]

On Sun, Mar 11, 2018 at 00:33:03 -0800, Paul Eggert wrote:

> Thanks for reporting the problem.  I have installed the attached two
> patches, which I think should fix the problem so I'm closing the bug
> report.  Please give them a try on NetBSD (as I typically don't use
> NetBSD).

Thank you for your prompt response.  This helps but only partially.

Now emacs doesn't freak out with a scary message when tzalloc()
returns NULL, but tzlookup() still calls tzalloc("XXXhh:mm:ss") for
integer "zone".  Unfortunately, as I mentioned in the original
submission, tzcode in NetBSD-6 doesn't grok that (support for that
appeared in tzcode only around 2014 I think), so each time you open a
file under CVS control you get the "Invalid time zone" error.

Perhaps in the long term configure should try to detect if host's
tzalloc() understands that kind of direct timezone specification.

However in the case of CVS the offending zone value is zero that comes
from parsed-time-string, so the following quick hack can be used

--- editfns.c.dist	2017-04-14 18:02:47.000000000 +0300
+++ editfns.c	2018-03-12 21:46:58.000000000 +0300
@@ -153,7 +147,8 @@
 
   if (NILP (zone))
     return local_tz;
-  else if (EQ (zone, Qt))
+  else if (EQ (zone, Qt)
+	   || (INTEGERP (zone) && XINT (zone) == 0))
     {
       zone_string = "UTC0";
       new_tz = utc_tz;

I don't know if CVS always uses UTC or it's just because the CVS repos
I work with all use UTC as an adminsitrative decision.  Though I guess
it can be argued that special-casing zone 0 like that to return utc_tz
is the right thing to do regardless.

Thanks.

-uwe

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On 03/12/2018 12:10 PM, Valery Ushakov wrote:
> -  else if (EQ (zone, Qt))
> +  else if (EQ (zone, Qt)
> +	   || (INTEGERP (zone) && XINT (zone) == 0))

Thanks for diagnosing the problem. If I understand things correctly, we 
can do a more-general fix, which should work for any used-in-practice 
time zone that is an integer hour offset from UTC. I installed the 
attached patch into master; please give it a try. I don't have easy 
access to NetBSD and would appreciate your testing it on NetBSD 6 and 7. 
I think this problem is limited to older NetBSD and so is not worth 
testing for in configure.ac (as 'configure' is pretty slow already).


[-- Attachment #2: 0001-Improve-port-to-NetBSD-tzalloc.txt --]
[-- Type: text/plain, Size: 1365 bytes --]

From 64a1da4989b0551481600facb1781ac92089d182 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 15 Mar 2018 09:35:33 -0700
Subject: [PATCH] Improve port to NetBSD tzalloc
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Valery Ushakov (Bug#30738#13).
* src/editfns.c (tzlookup) [__NetBSD_Version__ < 700000000]:
If tzalloc fails for any reason other than memory exhaustion,
assume it’s because NetBSD 6 does not support tzalloc on
POSIX-format TZ strings, and fall back on tzdb if possible.
---
 src/editfns.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/editfns.c b/src/editfns.c
index 6ecc83fc30..d26319441b 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -204,6 +204,18 @@ tzlookup (Lisp_Object zone, bool settz)
 	invalid_time_zone_specification (zone);
 
       new_tz = tzalloc (zone_string);
+
+#if defined __NetBSD_Version__ && __NetBSD_Version__ < 700000000
+      /* NetBSD 6 tzalloc mishandles POSIX TZ strings (Bug#30738).
+	 If possible, fall back on tzdb.  */
+      if (!new_tz && errno != ENOMEM && plain_integer
+	  && XINT (zone) % (60 * 60) == 0)
+	{
+	  sprintf (tzbuf, "Etc/GMT%+"pI"d", - (XINT (zone) / (60 * 60)));
+	  new_tz = tzalloc (zone_string);
+	}
+#endif
+
       if (!new_tz)
 	{
 	  if (errno == ENOMEM)
-- 
2.14.3

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Valery Ushakov]

On Thu, Mar 15, 2018 at 09:43:20 -0700, Paul Eggert wrote:

> On 03/12/2018 12:10 PM, Valery Ushakov wrote:
> > -  else if (EQ (zone, Qt))
> > +  else if (EQ (zone, Qt)
> > +	   || (INTEGERP (zone) && XINT (zone) == 0))
> 
> Thanks for diagnosing the problem. If I understand things correctly, we can
> do a more-general fix, which should work for any used-in-practice time zone
> that is an integer hour offset from UTC. I installed the attached patch into
> master; please give it a try.

Sorry, I still haven't got around to test it, but I used exactly the
same approach with Etc/GMT* zones as a kludge when I first ran into
this problem and needed a working emacs asap.  I'll try to actually
test your patch this week.

I'd say special casing 0 is still a good idea as it saves a call to
tzalloc().  I also suspect that zone == 0 from parsed-time-string is
the most common case.

-uwe

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On 03/19/2018 09:29 AM, Valery Ushakov wrote:
> special casing 0 is still a good idea as it saves a call to
> tzalloc().

Fair enough; I installed the first attached patch.


> this patch also needs to #include <sys/param.h>
> where __NetBSD_Version__ is defined.

Thanks, I installed the second attached patch to do that.


> tzbuf_format can
> probably drop that artisanal &"-"[...] and its corresponding %s, and
> just pass -hour instead.

That would mishandle time zones like -00:30:00 (i.e., 30 minutes behind 
UTC). That is, the sign is a property of the minutes and the seconds 
too, not just the hours.


[-- Attachment #2: 0001-Tune-time-zone-0.patch --]
[-- Type: text/x-patch, Size: 745 bytes --]

From 7a7ee53dbdcf489050653f04ba3d0c491245c5fa Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 19 Mar 2018 13:29:22 -0700
Subject: [PATCH] Tune time zone 0

* src/editfns.c (tzlookup): Treat time zone 0 like t, for speed.
Suggested by Valery Ushakov (Bug#30738#19).
---
 src/editfns.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/editfns.c b/src/editfns.c
index d26319441b..cb7353a48c 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -147,7 +147,7 @@ tzlookup (Lisp_Object zone, bool settz)
 
   if (NILP (zone))
     return local_tz;
-  else if (EQ (zone, Qt))
+  else if (EQ (zone, Qt) || EQ (zone, make_number (0)))
     {
       zone_string = "UTC0";
       new_tz = utc_tz;
-- 
2.14.3


[-- Attachment #3: 0001-Improve-port-to-NetBSD-tzalloc.patch --]
[-- Type: text/x-patch, Size: 1708 bytes --]

From 6d12e7af88b8287b8dd520aa3a3470f7f112cfe0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 19 Mar 2018 16:49:09 -0700
Subject: [PATCH] Improve port to NetBSD tzalloc

Problem reported by Valery Ushakov (Bug#30738#22).
* src/editfns.c (HAVE_TZALLOC_BUG): New macro.
(tzlookup): Use it.  Compile on all platforms, not just on NetBSD.
---
 src/editfns.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/editfns.c b/src/editfns.c
index cb7353a48c..7e35fe8797 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -48,6 +48,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <float.h>
 #include <limits.h>
 
+#ifdef HAVE_TIMEZONE_T
+# include <sys/param.h>
+# if defined __NetBSD_Version__ && __NetBSD_Version__ < 700000000
+#  define HAVE_TZALLOC_BUG true
+# endif
+#endif
+#ifndef HAVE_TZALLOC_BUG
+# define HAVE_TZALLOC_BUG false
+#endif
+
 #include <c-ctype.h>
 #include <intprops.h>
 #include <stdlib.h>
@@ -205,16 +215,14 @@ tzlookup (Lisp_Object zone, bool settz)
 
       new_tz = tzalloc (zone_string);
 
-#if defined __NetBSD_Version__ && __NetBSD_Version__ < 700000000
-      /* NetBSD 6 tzalloc mishandles POSIX TZ strings (Bug#30738).
-	 If possible, fall back on tzdb.  */
-      if (!new_tz && errno != ENOMEM && plain_integer
+      if (HAVE_TZALLOC_BUG && !new_tz && errno != ENOMEM && plain_integer
 	  && XINT (zone) % (60 * 60) == 0)
 	{
+	  /* tzalloc mishandles POSIX strings; fall back on tzdb if
+	     possible (Bug#30738).  */
 	  sprintf (tzbuf, "Etc/GMT%+"pI"d", - (XINT (zone) / (60 * 60)));
 	  new_tz = tzalloc (zone_string);
 	}
-#endif
 
       if (!new_tz)
 	{
-- 
2.14.3

bug#30738: Invalid timezone (tzalloc failure) treated as out-of-memory

[Valery Ushakov]

On Thu, Mar 15, 2018 at 09:43:20 -0700, Paul Eggert wrote:

> On 03/12/2018 12:10 PM, Valery Ushakov wrote:
> > -  else if (EQ (zone, Qt))
> > +  else if (EQ (zone, Qt)
> > +	   || (INTEGERP (zone) && XINT (zone) == 0))
> 
> Thanks for diagnosing the problem. If I understand things correctly, we can
> do a more-general fix, which should work for any used-in-practice time zone
> that is an integer hour offset from UTC. I installed the attached patch into
> master; please give it a try. I don't have easy access to NetBSD and would
> appreciate your testing it on NetBSD 6 and 7.

I've tested it and this patch also needs to #include <sys/param.h>
where __NetBSD_Version__ is defined.

PS: As an aside, that %+ made me realize that tzbuf_format can
probably drop that artisanal &"-"[...] and its corresponding %s, and
just pass -hour instead.

-uwe