bug#23600: 25.1.50; encode-time returns wrong result

bug#23600: 25.1.50; encode-time returns wrong result

[Kazuhiro Ito]

When I evaluate the below code on Emacs (trunk, MSYS2), it returns
wrong result.

(list
 ;; local time-zone is JST-9 in my environment.
 (encode-time 0 02 23  22 5 2016 nil)
 (encode-time 0 02 23  22 5 2016 '(32400 "JST"))
 (encode-time 0 02 23  22 5 2016 32400))

-> ((22337 48088) (22338 11352) (22338 43752))

The first element of the result depends on local time-zone.  But the
second and the last element should be (22337 48088).

-- 
Kazuhiro Ito

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

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

Thanks for the bug report. This appears to be due to an incompatibility between 
MS-Windows and POSIX that I didn't know about. Please try the attached patch. I 
have not tested or installed this (I don't use MS-Windows).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Port-angle-bracket-TZ-settings-to-MS-Windows.patch --]
[-- Type: text/x-diff; name="0001-Port-angle-bracket-TZ-settings-to-MS-Windows.patch", Size: 2442 bytes --]

From fef3119fc136889673a1a032ee0a5a47584a03fe Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 1 Jun 2016 01:09:42 -0700
Subject: [PATCH] Port angle-bracket TZ settings to MS-Windows

* doc/lispref/os.texi (Time Zone Rules): Document MS-Windows
lack of support for numeric time zone abbreviations.
* src/w32.c (sys_putenv): Convert angle-bracket TZ syntax
to MS-compatible syntax if possible, and to "ZZZ" otherwise.
Problem reported by Kazuhiro Ito (Bug#23600).
---
 doc/lispref/os.texi |  3 ++-
 src/w32.c           | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index becb691..38dde26 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -1327,7 +1327,8 @@ Time Zone Rules
 a string, the conversion uses the time zone rule equivalent to setting
 @env{TZ} to that string.  If it is an integer @var{offset}, the
 conversion uses a fixed time zone with the given offset and a numeric
-abbreviation.  If it is a list (@var{offset} @var{abbr}), where
+abbreviation on POSIX-compatible platforms and an unspecified abbreviation
+on MS-Windows.  If it is a list (@var{offset} @var{abbr}), where
 @var{offset} is an integer number of seconds east of Universal Time
 and @var{abbr} is a string, the conversion uses a fixed time zone with
 the given offset and abbreviation.
diff --git a/src/w32.c b/src/w32.c
index 442ce79..71a38b9 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -2505,6 +2505,35 @@ sys_putenv (char *str)
       return unsetenv (str);
     }
 
+  if (strncmp (str, "TZ=<", 4) == 0)
+    {
+      /* MS-Windows does not support POSIX.1-2001 angle-bracket TZ
+	 abbreviation syntax.  Convert to POSIX.1-1988 syntax if possible,
+	 and to the undocumented placeholder "ZZZ" otherwise.  */
+      bool supported_abbr = true;
+      for (char *p = str + 4; *p; p++)
+	{
+	  if (('0' <= *p && *p <= '9') || *p == '-' || *p == '+')
+	    supported_abbr = false;
+	  else if (*p == '>')
+	    {
+	      ptrdiff_t abbrlen;
+	      if (supported_abbr)
+		{
+		  abbrlen = p - (str + 4);
+		  memmove (str + 3, str + 4, abbrlen);
+		}
+	      else
+		{
+		  abbrlen = 3;
+		  memset (str + 3, 'Z', abbrlen);
+		}
+	      memmove (str + 3 + abbrlen, p + 1, strlen (p));
+	      break;
+	    }
+	}
+    }
+
   return _putenv (str);
 }
 
-- 
2.7.4

bug#23600: 25.1.50; encode-time returns wrong result

[Kazuhiro Ito]

> Thanks for the bug report. This appears to be due to an incompatibility
> between MS-Windows and POSIX that I didn't know about. Please try the
> attached patch. I have not tested or installed this (I don't use
> MS-Windows).

Thank you for the fix.  The problem I showed in the bug report seems
to be resolved.  But there still be a problem related timezone (I
dont' know whether it is the same problem).  With your patch, the
below code returns unexpected result.

(list (progn (set-time-zone-rule 0)
	     (current-time-zone))
      (progn (set-time-zone-rule "JST-9")
	     (current-time-zone))
      (progn (set-time-zone-rule "<JST>-9")
	     (current-time-zone)))

-> ((0 "ZZZ") (0 "ZZZ") (32400 "JST"))

I want it to return '((0 "ZZZ") (32400 "JST") (32400 "JST"))'.

-- 
Kazuhiro Ito

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

Kazuhiro Ito wrote:

> Thank you for the fix.  The problem I showed in the bug report seems
> to be resolved.

Thanks, I installed the fix in master.

> But there still be a problem related timezone (I
> dont' know whether it is the same problem).  With your patch, the
> below code returns unexpected result.
>
> (list (progn (set-time-zone-rule 0)
> 	     (current-time-zone))
>        (progn (set-time-zone-rule "JST-9")
> 	     (current-time-zone))
>        (progn (set-time-zone-rule "<JST>-9")
> 	     (current-time-zone)))
>
> -> ((0 "ZZZ") (0 "ZZZ") (32400 "JST"))
>
> I want it to return '((0 "ZZZ") (32400 "JST") (32400 "JST"))'.

Yes, that's the correct result and it's what I observe on Ubuntu 16.04 x86-64. I 
don't offhand see how the just-installed patch would cause the wrong answer for 
the "JST-9" case, as the patch cannot make a difference unless TZ's value starts 
with "<".

Do you see the same (wrong) behavior for "JST-9" in the emacs-25 branch? In 
Emacs 24.5?

bug#23600: 25.1.50; encode-time returns wrong result

[Eli Zaretskii]

> Cc: 23600@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 1 Jun 2016 01:19:50 -0700
> 
> +  if (strncmp (str, "TZ=<", 4) == 0)
> +    {
> +      /* MS-Windows does not support POSIX.1-2001 angle-bracket TZ
> +	 abbreviation syntax.  Convert to POSIX.1-1988 syntax if possible,
> +	 and to the undocumented placeholder "ZZZ" otherwise.  */
> +      bool supported_abbr = true;
> +      for (char *p = str + 4; *p; p++)
> +	{
> +	  if (('0' <= *p && *p <= '9') || *p == '-' || *p == '+')
> +	    supported_abbr = false;
> +	  else if (*p == '>')
> +	    {
> +	      ptrdiff_t abbrlen;
> +	      if (supported_abbr)
> +		{
> +		  abbrlen = p - (str + 4);
> +		  memmove (str + 3, str + 4, abbrlen);
> +		}
> +	      else
> +		{
> +		  abbrlen = 3;
> +		  memset (str + 3, 'Z', abbrlen);
> +		}
> +	      memmove (str + 3 + abbrlen, p + 1, strlen (p));
> +	      break;
> +	    }

Do callers of putenv expect the argument to be destroyed?

Thanks.

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

Eli Zaretskii wrote:

> Do callers of putenv expect the argument to be destroyed?

Yes and no. POSIX says that the environment can be modified in-place, so a 
caller of putenv (S) should be prepared for *S to be modified (which is what I 
assume you mean by "destroyed") because the string will be put into the 
environment. Also, POSIX allows putenv to modify *S, as S is of type char * and 
there is no prohibition in the standard against putenv modifying the pointed-to 
storage. That being said, I don't know of any POSIXish implementation of putenv 
(S) that modifies *S and I doubt whether any mainstream implementation would do 
that.

Although the code you mention is stretching things a bit, it's not stretching 
them beyond recognition: the intent of putenv ("TZ=<JST>-9") is not really "set 
the 'TZ' value to the byte-string '<JST>-9' in the environment array", it's more 
"set the time zone to 9 hours ahead of UTC and with abbreviation 'JST'", and on 
MS-Windows the code implements this intent more faithfully than doing nothing would.

bug#23600: 25.1.50; encode-time returns wrong result

[Eli Zaretskii]

> Cc: kzhr@d1.dion.ne.jp, 23600@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 4 Jun 2016 10:15:04 -0700
> 
> > Do callers of putenv expect the argument to be destroyed?
> 
> Yes and no. POSIX says that the environment can be modified in-place, so a 
> caller of putenv (S) should be prepared for *S to be modified (which is what I 
> assume you mean by "destroyed") because the string will be put into the 
> environment. Also, POSIX allows putenv to modify *S, as S is of type char * and 
> there is no prohibition in the standard against putenv modifying the pointed-to 
> storage. That being said, I don't know of any POSIXish implementation of putenv 
> (S) that modifies *S and I doubt whether any mainstream implementation would do 
> that.

Right, that's what I thought.

> Although the code you mention is stretching things a bit, it's not stretching 
> them beyond recognition: the intent of putenv ("TZ=<JST>-9") is not really "set 
> the 'TZ' value to the byte-string '<JST>-9' in the environment array", it's more 
> "set the time zone to 9 hours ahead of UTC and with abbreviation 'JST'", and on 
> MS-Windows the code implements this intent more faithfully than doing nothing would.

I think it would be cleaner if we copied the string before modifying
it.  I will do that when I have time.

Thanks.

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

Eli Zaretskii wrote:
> I think it would be cleaner if we copied the string before modifying
> it.

Other parts of Emacs modify the string in place and expect the modifications to 
affect the time zone setting, so in general it won't work to copy the string, 
modify the copy, and pass the copy's address to putenv.

bug#23600: 25.1.50; encode-time returns wrong result

[Eli Zaretskii]

> Cc: kzhr@d1.dion.ne.jp, 23600@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 4 Jun 2016 15:18:43 -0700
> 
> Eli Zaretskii wrote:
> > I think it would be cleaner if we copied the string before modifying
> > it.
> 
> Other parts of Emacs modify the string in place and expect the modifications to 
> affect the time zone setting, so in general it won't work to copy the string, 
> modify the copy, and pass the copy's address to putenv.

Too bad.

bug#23600: 25.1.50; encode-time returns wrong result

[Kazuhiro Ito]

> > Thank you for the fix.  The problem I showed in the bug report seems
> > to be resolved.
> Thanks, I installed the fix in master.

Thank you.

> > But there still be a problem related timezone (I
> > dont' know whether it is the same problem).  With your patch, the
> > below code returns unexpected result.
> > 
> > (list (progn (set-time-zone-rule 0)
> > 	     (current-time-zone))
> >        (progn (set-time-zone-rule "JST-9")
> > 	     (current-time-zone))
> >        (progn (set-time-zone-rule "<JST>-9")
> > 	     (current-time-zone)))
> > 
> > -> ((0 "ZZZ") (0 "ZZZ") (32400 "JST"))
> > 
> > I want it to return '((0 "ZZZ") (32400 "JST") (32400 "JST"))'.
> Yes, that's the correct result and it's what I observe on Ubuntu 16.04
> x86-64. I don't offhand see how the just-installed patch would cause the
> wrong answer for the "JST-9" case, as the patch cannot make a difference
> unless TZ's value starts with "<".
> 
> Do you see the same (wrong) behavior for "JST-9" in the emacs-25 branch?
> In Emacs 24.5?

Emacs 24.5 can treat "JST-9" correctly but not for "<JST-9>",
emacs-25 shows the same result as trunk.

(list
  (progn (set-time-zone-rule "<AAA>-1")
	 (current-time-zone))
  (progn (set-time-zone-rule t)
	 (current-time-zone))
  (progn (set-time-zone-rule "BBB-2")
	 (current-time-zone))
  (progn (set-time-zone-rule 36000)
	 (current-time-zone))
  (progn (set-time-zone-rule "<JST>-9")
	 (current-time-zone)))

emacs-25 branch
-> ((32400 "JST") (0 "GMT") (32400 "JST") (32400 "JST") (32400 "JST"))

emacs-25 branch with sys_putenv patch and trunk
-> ((3600 "AAA") (0 "GMT") (3600 "AAA") (36000 "ZZZ") (32400 "JST"))


(list (progn (set-time-zone-rule "GMT0")
 	     (current-time-zone))
      (progn (set-time-zone-rule "BBB-2")
 	     (current-time-zone))
      (progn (set-time-zone-rule "<JST>-9")
 	     (current-time-zone)))

24.5 pre-built binary
-> ((0 "GMT") (7200 "BBB") (3600 "T>-"))

-- 
Kazuhiro Ito

bug#23600: 25.1.50; encode-time returns wrong result

[Eli Zaretskii]

> Date: Sun, 05 Jun 2016 05:47:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: kzhr@d1.dion.ne.jp, 23600@debbugs.gnu.org
> 
> > Cc: kzhr@d1.dion.ne.jp, 23600@debbugs.gnu.org
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Sat, 4 Jun 2016 15:18:43 -0700
> > 
> > Eli Zaretskii wrote:
> > > I think it would be cleaner if we copied the string before modifying
> > > it.
> > 
> > Other parts of Emacs modify the string in place and expect the modifications to 
> > affect the time zone setting, so in general it won't work to copy the string, 
> > modify the copy, and pass the copy's address to putenv.
> 
> Too bad.

Actually, could you point me to those places?  Because if the code
which expects that is not already ifdef'ed out/around for Windows,
it's a problem waiting to be discovered, since MS _putenv accepts a
'const char *' argument, and my references indicate that the
implementation indeed copies the input string.  So those other parts
of the code expect something that cannot work on Windows.

Thanks.

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

Eli Zaretskii wrote:
> Actually, could you point me to those places?

It's emacs_setenv_TZ, in editfns.c. Ah, now I see that there's an ifdef 
WINDOWSNT there that sidesteps the issue on MS-Windows, so you should be OK.

bug#23600: 25.1.50; encode-time returns wrong result

[Kazuhiro Ito]

I noticed that calling format-time-string (or encode-time) with
time-zone string fixes the problem.  But I can't see why.

(list
 (progn (set-time-zone-rule "<AAA>-1")
	(current-time-zone))
 (progn (set-time-zone-rule "BBB-2")
	(current-time-zone))
 (progn (format-time-string "" nil "UTC0")
	(current-time-zone)))

-> ((3600 "AAA") (3600 "AAA") (7200 "BBB"))

-- 
Kazuhiro Ito

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

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

On 06/12/2016 03:45 AM, Kazuhiro Ito wrote:
> I noticed that calling format-time-string (or encode-time) with
> time-zone string fixes the problem.  But I can't see why.

I think I see the problem, at least on GNU/Linux: localtime_r needs 
someone else to call tzset. I installed the attached patch into the 
master branch, to do that.


[-- Attachment #2: 0001-Call-tzset-after-setting-TZ.patch --]
[-- Type: application/x-patch, Size: 2341 bytes --]

bug#23600: 25.1.50; encode-time returns wrong result

[Kazuhiro Ito]

> > I noticed that calling format-time-string (or encode-time) with
> > time-zone string fixes the problem.  But I can't see why.
>
> I think I see the problem, at least on GNU/Linux: localtime_r needs
> someone else to call tzset. I installed the attached patch into the
> master branch, to do that.

I confirmed the patch resolves the problem.  Thank you for the fix.

-- 
Kazuhiro Ito

bug#23600: 25.1.50; encode-time returns wrong result

[Paul Eggert]

Thanks for checking; closing the bug.