Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Clint Adams]

> > Okay, if you use a different prompt (walters, for example), it won't
> > segfault.  Correct?
> 
> Yep.
> #0  0x080a5cdf in ztrftime ()
> #1  0x08098fc4 in promptexpand ()
> #2  0x08098330 in promptexpand ()
> #3  0x402e5b8f in zleread () from /usr/lib/zsh/4.2.0/zsh/zle.so

Seems that when LC_TIME (or LC_ALL or LANG) is set to a locale such as
da_DK or de_DE, wherein am_pm is set to null strings, zsh will segfault
upon prompt-expanding %p or %P.  Seems that the first argument to
ztrftime() is often either NULL or 0x51, though I can't figure out why.

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Clint Adams]

> Seems that when LC_TIME (or LC_ALL or LANG) is set to a locale such as
> da_DK or de_DE, wherein am_pm is set to null strings, zsh will segfault
> upon prompt-expanding %p or %P.  Seems that the first argument to

So, when am_pm is set to null strings, strftime() with format "%p" or
"%P" will return 0, which zsh is ill-equipped to handle.
The following patch avoids the segfault.  I hope there's a better way to
do this.

Index: Src/prompt.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/prompt.c,v
retrieving revision 1.15
diff -u -r1.15 prompt.c
--- Src/prompt.c	21 Apr 2004 08:33:37 -0000	1.15
+++ Src/prompt.c	4 May 2004 04:08:54 -0000
@@ -528,7 +528,9 @@
 		    tm = localtime(&timet);
 		    for(t0=80; ; t0*=2) {
 			addbufspc(t0);
-			if (ztrftime(bp, t0, tmfmt, tm))
+			if (ztrftime(bp, t0, tmfmt, tm) ||
+			    !strcmp("%P", tmfmt) ||
+			    !strcmp("%p", tmfmt))
 			    break;
 		    }
 		    bp += strlen(bp);
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.58
diff -u -r1.58 utils.c
--- Src/utils.c	25 Mar 2004 12:32:11 -0000	1.58
+++ Src/utils.c	4 May 2004 04:08:55 -0000
@@ -1831,7 +1831,8 @@
 		 */
 		*buf = '\0';
 		tmp[1] = fmt[-1];
-		if (!strftime(buf, bufsize + 2, tmp, tm))
+		if (!strftime(buf, bufsize + 2, tmp, tm) &&
+		    tmp[1]!='p' && tmp[1]!='P')
 		    return 0;
 		decr = strlen(buf);
 		buf += decr;

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Peter Stephenson]

Clint Adams wrote:
> > Seems that when LC_TIME (or LC_ALL or LANG) is set to a locale such as
> > da_DK or de_DE, wherein am_pm is set to null strings, zsh will segfault
> > upon prompt-expanding %p or %P.  Seems that the first argument to
> 
> So, when am_pm is set to null strings, strftime() with format "%p" or
> "%P" will return 0, which zsh is ill-equipped to handle.
> The following patch avoids the segfault.  I hope there's a better way to
> do this.

Yes, the right way to do it is to handle `0' properly, which says the
array is in an indeterminate state and hence the shell should tidy up.
The current patch is not the right way to do it.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Oliver Kiddle]

Clint Adams wrote:
> 
> So, when am_pm is set to null strings, strftime() with format "%p" or
> "%P" will return 0, which zsh is ill-equipped to handle.

I get a seg fault for %t too which is perhaps related.

Oliver

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Clint Adams]

> I get a seg fault for %t too which is perhaps related.

Yes, %t is the same as %D{%l:%M%p}, and 19869 avoids the segfault in
that case as well.

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Peter Stephenson]

Peter Stephenson wrote:
> Clint Adams wrote:
> > > Seems that when LC_TIME (or LC_ALL or LANG) is set to a locale such as
> > > da_DK or de_DE, wherein am_pm is set to null strings, zsh will segfault
> > > upon prompt-expanding %p or %P.  Seems that the first argument to
> > 
> > So, when am_pm is set to null strings, strftime() with format "%p" or
> > "%P" will return 0, which zsh is ill-equipped to handle.
> > The following patch avoids the segfault.  I hope there's a better way to
> > do this.
> 
> Yes, the right way to do it is to handle `0' properly, which says the
> array is in an indeterminate state and hence the shell should tidy up.
> The current patch is not the right way to do it.

This is a better patch, although it's not ideal, owing to the interface
of strftime which doesn't signal success or failure properly.

In general, please don't commit gross hacks to the archive unless they
are strictly necessary, since it just makes extra work later.  In this
case, the new hack was just disguising the fact that the previous hack
was allocating infinite amounts of memory without an exit test.  It's
not surprising the shell code is less than transparent in a lot of
places.

The second hunk is a bit imperfect, too, for the same reason --- well,
actually we could be smarter in returns from ztrftime than from
strftime, but at the moment we aren't --- but will at least ensure the
buffer is correctly terminated in cases like the present one.

Index: Src/prompt.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/prompt.c,v
retrieving revision 1.16
diff -u -r1.16 prompt.c
--- Src/prompt.c	4 May 2004 04:17:29 -0000	1.16
+++ Src/prompt.c	4 May 2004 14:34:47 -0000
@@ -526,11 +526,16 @@
 		    }
 		    timet = time(NULL);
 		    tm = localtime(&timet);
-		    for(t0=80; ; t0*=2) {
+		    /*
+		     * Ghastly hack because strftime won't say how
+		     * much space it actually needs.  Try to add it
+		     * a few times until it works.  Some formats don't
+		     * actually have a length, so we could go on for
+		     * ever.
+		     */
+		    for(j = 0, t0 = strlen(tmfmt)*8; j < 3; j++, t0*=2) {
 			addbufspc(t0);
-			if (ztrftime(bp, t0, tmfmt, tm) ||
-			    !strcmp("%P", tmfmt) ||
-			    !strcmp("%p", tmfmt))
+			if (ztrftime(bp, t0, tmfmt, tm))
 			    break;
 		    }
 		    bp += strlen(bp);
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.59
diff -u -r1.59 utils.c
--- Src/utils.c	4 May 2004 04:17:29 -0000	1.59
+++ Src/utils.c	4 May 2004 14:34:48 -0000
@@ -1831,9 +1831,11 @@
 		 */
 		*buf = '\0';
 		tmp[1] = fmt[-1];
-		if (!strftime(buf, bufsize + 2, tmp, tm) &&
-		    tmp[1]!='p' && tmp[1]!='P')
+		if (!strftime(buf, bufsize + 2, tmp, tm))
+		{
+		    buf[0] = '\0';
 		    return 0;
+		}
 		decr = strlen(buf);
 		buf += decr;
 		bufsize -= decr - 2;

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Clint Adams]

> This is a better patch, although it's not ideal, owing to the interface
> of strftime which doesn't signal success or failure properly.

glibc suggests this method

          buf[0] = '\1';
          len = strftime (buf, bufsize, format, tp);
          if (len == 0 && buf[0] != '\0')
            {
              /* Something went wrong in the strftime call.  */
              ...
            }

If it's portable, it would seem a better hack.

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Peter Stephenson]

Clint Adams wrote:
> > This is a better patch, although it's not ideal, owing to the interface
> > of strftime which doesn't signal success or failure properly.
> 
> glibc suggests this method
> 
>           buf[0] = '\1';
>           len = strftime (buf, bufsize, format, tp);
>           if (len == 0 && buf[0] != '\0')
>             {
>               /* Something went wrong in the strftime call.  */
>               ...
>             }
> 
> If it's portable, it would seem a better hack.

I agree, although I don't know if it's portable... the following allows
ztrftime to return -1 for an error.  If the fix isn't portable, it means
we won't handle long strings properly.  I've still limited the size of
the loop in the prompt, since I'm paranoid.  I think making the size
of the buffer depend on the length of the format should also improve
matters there.

Index: Src/prompt.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/prompt.c,v
retrieving revision 1.16
diff -u -r1.16 prompt.c
--- Src/prompt.c	4 May 2004 04:17:29 -0000	1.16
+++ Src/prompt.c	4 May 2004 15:25:22 -0000
@@ -526,11 +526,16 @@
 		    }
 		    timet = time(NULL);
 		    tm = localtime(&timet);
-		    for(t0=80; ; t0*=2) {
+		    /*
+		     * Hack because strftime won't say how
+		     * much space it actually needs.  Try to add it
+		     * a few times until it works.  Some formats don't
+		     * actually have a length, so we could go on for
+		     * ever.
+		     */
+		    for(j = 0, t0 = strlen(tmfmt)*8; j < 3; j++, t0*=2) {
 			addbufspc(t0);
-			if (ztrftime(bp, t0, tmfmt, tm) ||
-			    !strcmp("%P", tmfmt) ||
-			    !strcmp("%p", tmfmt))
+			if (ztrftime(bp, t0, tmfmt, tm) >= 0)
 			    break;
 		    }
 		    bp += strlen(bp);
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.59
diff -u -r1.59 utils.c
--- Src/utils.c	4 May 2004 04:17:29 -0000	1.59
+++ Src/utils.c	4 May 2004 15:25:31 -0000
@@ -1719,6 +1719,11 @@
  * Like the system function, this returns the number of characters
  * copied, not including the terminating NUL.  This may be zero
  * if the string didn't fit.
+ *
+ * As an extension, try to detect an error in strftime --- typically
+ * not enough memory --- and return -1.  Not guaranteed to be portable,
+ * since the strftime() interface doesn't make any guarantees about
+ * the state of the buffer if it returns zero.
  */
 
 /**/
@@ -1831,9 +1836,14 @@
 		 */
 		*buf = '\0';
 		tmp[1] = fmt[-1];
-		if (!strftime(buf, bufsize + 2, tmp, tm) &&
-		    tmp[1]!='p' && tmp[1]!='P')
+		if (!strftime(buf, bufsize + 2, tmp, tm))
+		{
+		    if (*buf) {
+			buf[0] = '\0';
+			return -1;
+		    }
 		    return 0;
+		}
 		decr = strlen(buf);
 		buf += decr;
 		bufsize -= decr - 2;
Index: Src/Modules/datetime.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/datetime.c,v
retrieving revision 1.7
diff -u -r1.7 datetime.c
--- Src/Modules/datetime.c	22 Jan 2004 17:51:06 -0000	1.7
+++ Src/Modules/datetime.c	4 May 2004 15:25:31 -0000
@@ -61,7 +61,7 @@
     buffer = zalloc(bufsize);
 
     for (x=0; x < 4; x++) {
-        if (ztrftime(buffer, bufsize, argv[0], t))
+        if (ztrftime(buffer, bufsize, argv[0], t) >= 0)
 	    break;
 	buffer = zrealloc(buffer, bufsize *= 2);
     }

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Wayne Davison]

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

Looks to me like you failed to set the first character in buf to a
non-zero value.  Here's a patch.

..wayne..

[-- Attachment #2: strftime.patch --]
[-- Type: text/plain, Size: 373 bytes --]

--- Src/utils.c	4 May 2004 16:43:43 -0000	1.60
+++ Src/utils.c	5 May 2004 04:41:31 -0000
@@ -1834,7 +1834,7 @@ ztrftime(char *buf, int bufsize, char *f
 		 * Remember we've already allowed for two characters
 		 * in the accounting in bufsize (but nowhere else).
 		 */
-		*buf = '\0';
+		*buf = '\1';
 		tmp[1] = fmt[-1];
 		if (!strftime(buf, bufsize + 2, tmp, tm))
 		{

Re: Bug#245974: zsh: export LC_ALL=da_DK causes segfault

[Peter Stephenson]

Wayne Davison wrote:
> Looks to me like you failed to set the first character in buf to a
> non-zero value.  Here's a patch.

Yes, I read it that other way: if there's an error, strftime() would
have inserted something and not nulled it, but that doesn't cover
the case of an empty prompt with a failure (if that can happen).
Thanks.

pws


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************