2.6b11-t10: -fwritable-strings (and another completion bug)

2.6b11-t10: -fwritable-strings (and another completion bug)

[Thorsten Meinecke]

Hi,

here's a patch for two bugs that were lurking in zle_tricky.c for quite
a while now. Both bugs caused SEGV core dumps occasionally when the
unsuspecting user requested completion.

1) COMPLETE_IN_WORD dumps core when completing reserved names
   Fix from P.Stephenson, in: archive/latest/293

   Peter had pointed out that the (gcc) flag `-fwritable-strings' isn't
   the right approach and he had fixed the code instead.

   If you don't want my second patch to go into baseline zsh, then at
   least put his patch in, which still applies (with a little fuzz).
   And please, remove `-fwritable-strings' from CFLAGS.

2) The code that detects if a completed filename is a directory, or
   if a completed parameter's content refers to a directory (with
   AUTO_PARAM_SLASH/GLOB_SUBST set), has arbitrary limits. Try comple-
   tion on a parameter name with more than PATH_MAX chars in length.
   Or try it (with AUTO_PARAM_SLASH on) when the parameter's content
   is more than PATH_MAX in length: the buffer holding only PATH_MAX
   chars will overflow and corrupt the stack.

   I'm fixing that by ncalloc()'ing the buffer with a size sufficient
   to hold the parameter, at least PATH_MAX chars. The expanded string,
   although it may be longer, will then be truncated at PATH_MAX-1
   chars. That shouldn't make any difference to stat().

Regards,
  Thorsten


rcsdiff -qu4 -kk -r1.59 -r1.61 Src/zle_tricky.c
--- 1.59	1995/10/20 03:07:44
+++ Src/zle_tricky.c	1995/10/22 20:40:17
@@ -1391,9 +1391,9 @@
 void
 addmatch(char *s, char *t)
 {
     int test = 0, sl = strlen(s), pl = rpl, cc = 0, *bp, *ep;
-    char sav = 0, *e = NULL, *tt, *te, *fc, **fm;
+    char *e = NULL, *tt, *te, *fc, **fm;
     Comp cp = patcomp;
     HashNode hn;
     Param pm;
     LinkList l = matches;
@@ -1522,18 +1522,16 @@
     }
     if (!test)
 	return;
 
-    t = s += (ispattern ? 0 : pl);
-    e += t - s;
-    s = t;
-
-    if (ispattern)
-	e = NULL, sav = '\0';
-    else {
-	if ((sav = *e)) {
-	    *e = '\0';
-	    t = dupstring(t);
+    if (ispattern) {
+	t = s;
+    } else {
+	t = s += pl;
+	if (*e) {
+	    sl = e - s;
+	    t = s = dupstring(t);
+	    s[sl] = '\0';
 	}
     }
 
     if (l == fmatches) {
@@ -1570,10 +1568,8 @@
 	if (l == fmatches)
 	    fshortl = sl, fshortest = t;
 	else
 	    shortl = sl, shortest = t;
-    if (sav)
-	*e = sav;
 }
 
 #ifdef HAVE_NIS
 static int
@@ -3270,12 +3266,22 @@
 	/* There is no suffix, so we may add one. */
 	if (!(haswhat & HAS_MISC) || (parampre && isset(AUTOPARAMSLASH))) {
 	    /* If we have only filenames or we completed a parameter name
 	       and auto_param_slash is set, lets see if it is a directory. */
-	    char p[PATH_MAX], *ss;
+	    char *p;
 	    struct stat buf;
+	    int len = strlen (str);
 
 	    /* Build the path name. */
+	    if (!ispattern || ic || parampre)
+		len += parampre ?
+		    strlen (parampre) + strlen (lpre) + strlen (lsuf) :
+		    strlen (fpre) + strlen (fsuf) + strlen (psuf) +
+		    (ic ? 1 + strlen (ppre) :
+			(prpre && *prpre) ? strlen (prpre) : 2);
+
+	    p = (char *) ncalloc (len >= PATH_MAX ? len + 1 : PATH_MAX);
+
 	    if (ispattern || ic || parampre) {
 		int ne = noerrs;
 
 		noerrs = 1;
@@ -3291,18 +3297,19 @@
 			    ppre, fpre, str, fsuf, psuf);
 		}
 		else
 		    strcpy(p, str);
-		ss = dupstring(p);
-		tokenize(ss);
-		singsub(&ss);
-		strcpy(p, ss);
+		tokenize(p);
+		singsub(&p);
 
 		noerrs = ne;
 	    } else
 		sprintf(p, "%s%s%s%s%s",
 			(prpre && *prpre) ? prpre : "./", fpre, str,
 			fsuf, psuf);
+
+	    p[PATH_MAX-1] = '\0';
+
 	    /* And do the stat. */
 	    if (!ztat(p, &buf, 0) && (buf.st_mode & S_IFMT) == S_IFDIR) {
 		/* It is a directory, so prepare to add the slash and set
 		   addedsuffix. */

Re: 2.6b11-t10: -fwritable-strings (and another completion bug)

[Zoltan Hidvegi]

Thorsten Meinecke wrote:
> 2) The code that detects if a completed filename is a directory, or
>    if a completed parameter's content refers to a directory (with
>    AUTO_PARAM_SLASH/GLOB_SUBST set), has arbitrary limits. Try comple-
>    tion on a parameter name with more than PATH_MAX chars in length.
>    Or try it (with AUTO_PARAM_SLASH on) when the parameter's content
>    is more than PATH_MAX in length: the buffer holding only PATH_MAX
>    chars will overflow and corrupt the stack.
> 
>    I'm fixing that by ncalloc()'ing the buffer with a size sufficient
>    to hold the parameter, at least PATH_MAX chars. The expanded string,
>    although it may be longer, will then be truncated at PATH_MAX-1
>    chars. That shouldn't make any difference to stat().

Here is an other version of this fix.  It is a bit more readable, and also
fixes a similar problem in an other piece of code.

This patch overrides Thorsten's patch in art. 489, but do not forget, that
this also containd patch in art. 293 from Peter which is still necessary.

The patch applies to vanilla beta11-test10.

Cheers,

   Zoltan

*** 1.2	1995/10/23 22:29:11
--- Src/zle_tricky.c	1995/10/23 22:35:51
***************
*** 3179,3185 ****
  {
      char b[PATH_MAX], *p;
  
!     for (p = b; *nam; nam++)
  	if (*nam == '\\' && nam[1])
  	    *p++ = *++nam;
  	else
--- 3179,3185 ----
  {
      char b[PATH_MAX], *p;
  
!     for (p = b; p < b + sizeof(b) - 1 && *nam; nam++)
  	if (*nam == '\\' && nam[1])
  	    *p++ = *++nam;
  	else
***************
*** 3266,3272 ****
  	if (!(haswhat & HAS_MISC) || (parampre && isset(AUTOPARAMSLASH))) {
  	    /* If we have only filenames or we completed a parameter name
  	       and auto_param_slash is set, lets see if it is a directory. */
! 	    char p[PATH_MAX], *ss;
  	    struct stat buf;
  
  	    /* Build the path name. */
--- 3266,3272 ----
  	if (!(haswhat & HAS_MISC) || (parampre && isset(AUTOPARAMSLASH))) {
  	    /* If we have only filenames or we completed a parameter name
  	       and auto_param_slash is set, lets see if it is a directory. */
! 	    char *p;
  	    struct stat buf;
  
  	    /* Build the path name. */
***************
*** 3277,3302 ****
  
  		if (parampre) {
  		    int pl = strlen(parampre);
  		    sprintf(p, "%s%s%s%s", parampre, lpre, str, lsuf);
  		    if (pl && p[pl-1] == Inbrace)
  			strcpy(p+pl-1, p+pl);
  		}
  		else if (ic) {
  		    sprintf(p, "%c%s%s%s%s%s", ic,
  			    ppre, fpre, str, fsuf, psuf);
  		}
  		else
! 		    strcpy(p, str);
! 		ss = dupstring(p);
! 		tokenize(ss);
! 		singsub(&ss);
! 		strcpy(p, ss);
  
  		noerrs = ne;
! 	    } else
  		sprintf(p, "%s%s%s%s%s",
  			(prpre && *prpre) ? prpre : "./", fpre, str,
  			fsuf, psuf);
  	    /* And do the stat. */
  	    if (!ztat(p, &buf, 0) && (buf.st_mode & S_IFMT) == S_IFDIR) {
  		/* It is a directory, so prepare to add the slash and set
--- 3277,3307 ----
  
  		if (parampre) {
  		    int pl = strlen(parampre);
+ 		    p = (char *) ncalloc(pl + strlen(lpre) + strlen(str) +
+ 					 strlen(lsuf) + 1);
  		    sprintf(p, "%s%s%s%s", parampre, lpre, str, lsuf);
  		    if (pl && p[pl-1] == Inbrace)
  			strcpy(p+pl-1, p+pl);
  		}
  		else if (ic) {
+ 		    p = (char *) ncalloc(strlen(ppre) + strlen(fpre) + strlen(str) +
+ 					 strlen(fsuf) + strlen(psuf) + 2);
  		    sprintf(p, "%c%s%s%s%s%s", ic,
  			    ppre, fpre, str, fsuf, psuf);
  		}
  		else
! 		    p = dupstring(str);
! 		tokenize(p);
! 		singsub(&p);
  
  		noerrs = ne;
! 	    } else {
! 		p = (char *) ncalloc((prpre ? strlen(prpre) : 0) + strlen(fpre) +
! 				     strlen(str) + strlen(fsuf) + strlen(psuf) + 3);
  		sprintf(p, "%s%s%s%s%s",
  			(prpre && *prpre) ? prpre : "./", fpre, str,
  			fsuf, psuf);
+ 	    }
  	    /* And do the stat. */
  	    if (!ztat(p, &buf, 0) && (buf.st_mode & S_IFMT) == S_IFDIR) {
  		/* It is a directory, so prepare to add the slash and set
***************
*** 3549,3555 ****
  	    while (*ap) {
  		int t2 = ispattern ? strlen(*ap) :
  		strlen(*ap + off) - boff + 1 + fpl + fsl;
! 		char pbuf[PATH_MAX], *pb;
  		struct stat buf;
  
  		/* Build the path name for the stat. */
--- 3554,3560 ----
  	    while (*ap) {
  		int t2 = ispattern ? strlen(*ap) :
  		strlen(*ap + off) - boff + 1 + fpl + fsl;
! 		char *pb;
  		struct stat buf;
  
  		/* Build the path name for the stat. */
***************
*** 3562,3568 ****
  		    t2 -= off + boff - 1;
  		} else {
  		    fprintf(shout, "%s%s%s", fpre, *ap, fsuf);
! 		    sprintf(pb = pbuf, "%s%s%s%s",
  			    (prpre && *prpre) ? prpre : "./", fpre, *ap, fsuf);
  		}
  		if (ztat(pb, &buf, 1))
--- 3567,3575 ----
  		    t2 -= off + boff - 1;
  		} else {
  		    fprintf(shout, "%s%s%s", fpre, *ap, fsuf);
! 		    pb = (char *) ncalloc((prpre ? strlen(prpre) : 0) + 3 +
! 					  strlen(fpre) + strlen(*ap) + strlen(fsuf));
! 		    sprintf(pb, "%s%s%s%s",
  			    (prpre && *prpre) ? prpre : "./", fpre, *ap, fsuf);
  		}
  		if (ztat(pb, &buf, 1))