PATCH: Re: Seg fault in matcher-list matching

PATCH: Re: Seg fault in matcher-list matching

[Sven Wischnowsky]

Bart Schaefer wrote:

> I did this:
> 
> zagzig[41] /u/s/l/z/z4/s/zsh
>                      ^cursor over the 4, press TAB
> 
> The path this was intended to match was /usr/src/local/zsh/zsh-2.4/src/zsh.
> I had first tried TAB at the end of the line and gotten a feep, in case
> that matters.

I couldn't get it to seg-fault, but there was something broken. Matching 
of the suffix, for example, and that both in C and shell code.


Oliver Kiddle wrote:

> ...
> 
> It's too late too look at this in any more detail but I've just found that
> I get a seg fault when I do this:
> diff  .c<tab>
>      ^cursor here.

I couldn't reproduce this (neither before nor after this patch -- your 
match specs would have helped), but this looks suspiciously similar to 
the one above... could you try with this patch?

Bye
 Sven

Index: Completion/Core/_path_files
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Core/_path_files,v
retrieving revision 1.11
diff -u -r1.11 _path_files
--- Completion/Core/_path_files	2000/05/02 08:18:54	1.11
+++ Completion/Core/_path_files	2000/05/15 09:25:35
@@ -6,7 +6,7 @@
 local linepath realpath donepath prepath testpath exppath skips skipped
 local tmp1 tmp2 tmp3 tmp4 i orig eorig pre suf tpre tsuf opre osuf cpre
 local pats haspats ignore pfxsfx remt sopt gopt opt sdirs ignpar
-local nm=$compstate[nmatches] menu matcher mopts sort match
+local nm=$compstate[nmatches] menu matcher mopts sort match mid
 
 typeset -U prepaths exppaths
 
@@ -529,6 +529,7 @@
       cpre="${cpre}${tpre%%/*}/"
       tpre="${tpre#*/}"
     elif [[ "$tsuf" = */* ]]; then
+      mid="$testpath"
       cpre="${cpre}${tpre}/"
       tpre="${tsuf#*/}"
       tsuf=
@@ -539,17 +540,32 @@
   done
 
   if [[ -z "$tmp4" ]]; then
-    if [[ "$osuf" = */* ]]; then
-      PREFIX="${opre}${osuf}"
-      SUFFIX=
-    else
+    if [[ "$mid" = */ ]]; then
       PREFIX="${opre}"
       SUFFIX="${osuf}"
+
+      tmp4="${testpath#${mid}}"
+      tmp3="${mid%/*/}"
+      tmp2="${${mid%/}##*/}"
+      compquote tmp4 tmp3 tmp2 tmp1
+      for i in "$tmp1[@]"; do
+        compadd -Qf "$mopts[@]" -p "$linepath$tmp3/" -s "/$tmp4$i" \
+                -W "$prepath$realpath${mid%/*/}/" \
+	        "$pfxsfx[@]" -M "r:|/=* r:|=*" - "$tmp2"
+      done
+    else
+      if [[ "$osuf" = */* ]]; then
+        PREFIX="${opre}${osuf}"
+        SUFFIX=
+      else
+        PREFIX="${opre}"
+        SUFFIX="${osuf}"
+      fi
+      tmp4="$testpath"
+      compquote tmp4 tmp1
+      compadd -Qf "$mopts[@]" -p "$linepath$tmp4" -W "$prepath$realpath$testpath" \
+	      "$pfxsfx[@]" -M "r:|/=* r:|=*" - "$tmp1[@]"
     fi
-    tmp4="$testpath"
-    compquote tmp4 tmp1
-    compadd -Qf "$mopts[@]" -p "$linepath$tmp4" -W "$prepath$realpath$testpath" \
-	    "$pfxsfx[@]" -M "r:|/=* r:|=*" - "$tmp1[@]"
   fi
 done
 
Index: Src/Zle/compmatch.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compmatch.c,v
retrieving revision 1.12
diff -u -r1.12 compmatch.c
--- Src/Zle/compmatch.c	2000/05/12 11:52:30	1.12
+++ Src/Zle/compmatch.c	2000/05/15 09:25:37
@@ -482,9 +482,10 @@
 	 */
 
 	bslash = 0;
-	if (test && (l[ind] == w[ind] ||
-		     (bslash = (lw > 1 && w[ind] == '\\' &&
-				(ind ? (w[0] == l[0]) : (w[1] == l[0])))))) {
+	if (test && !sfx &&
+	    (l[ind] == w[ind] ||
+	     (bslash = (lw > 1 && w[ind] == '\\' &&
+			(ind ? (w[0] == l[0]) : (w[1] == l[0])))))) {
 	    /* No matcher could be used, but the strings have the same
 	     * character here, skip over it. */
 	    l += add; w += (bslash ? (add + add ) : add);
@@ -562,10 +563,8 @@
 
 		    /* Fine, now we call ourselves recursively to find the
 		     * string matched by the `*'. */
-		    if (sfx) {
-			savl = l[-(llen + zoff)];
+		    if (sfx && (savl = l[-(llen + zoff)]))
 			l[-(llen + zoff)] = '\0';
-		    }
 		    for (t = 0, tp = w, ct = 0, ict = lw - alen + 1;
 			 ict;
 			 tp += add, ct++, ict--) {
@@ -585,11 +584,12 @@
 				!match_parts(l + aoff , tp - moff, alen, part))
 				break;
 			    if (sfx) {
-				savw = tp[-zoff];
-				tp[-zoff] = '\0';
+				if ((savw = tp[-zoff]))
+				    tp[-zoff] = '\0';
 				t = match_str(l - ll, w - lw,
 					      NULL, 0, NULL, 1, 2, part);
-				tp[-zoff] = savw;
+				if (savw)
+				    tp[-zoff] = savw;
 			    } else
 				t = match_str(l + llen + moff, tp + moff,
 					      NULL, 0, NULL, 0, 1, part);
@@ -598,7 +598,7 @@
 			}
 		    }
 		    ict = ct;
-		    if (sfx)
+		    if (sfx && savl)
 			l[-(llen + zoff)] = savl;
 
 		    /* Have we found a position in w where the rest of l
@@ -794,9 +794,10 @@
 	/* Same code as at the beginning, used in top-level calls. */
 
 	bslash = 0;
-	if (!test && (l[ind] == w[ind] ||
-		      (bslash = (lw > 1 && w[ind] == '\\' &&
-				 (ind ? (w[0] == l[0]) : (w[1] == l[0])))))) {
+	if ((!test || sfx) &&
+	    (l[ind] == w[ind] ||
+	     (bslash = (lw > 1 && w[ind] == '\\' &&
+			(ind ? (w[0] == l[0]) : (w[1] == l[0])))))) {
 	    /* No matcher could be used, but the strings have the same
 	     * character here, skip over it. */
 	    l += add; w += (bslash ? (add + add ) : add);
@@ -811,6 +812,8 @@
 	    lm = NULL;
 	    he = 0;
 	} else {
+	    if (!lw)
+		break;
 	    /* No matcher and different characters: l does not match w. */
 	    if (test)
 		return 0;
@@ -873,10 +876,15 @@
     char lsav = l[n], wsav = w[n];
     int ret;
 
-    l[n] = w[n] = '\0';
+    if (lsav)
+	l[n] = '\0';
+    if (wsav)
+	w[n] = '\0';
     ret = match_str(l, w, NULL, 0, NULL, 0, 1, part);
-    l[n] = lsav;
-    w[n] = wsav;
+    if (lsav)
+	l[n] = lsav;
+    if (wsav)
+	w[n] = wsav;
 
     return ret;
 }

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: Seg fault in matcher-list matching

[Bart Schaefer]

On May 15, 11:26am, Sven Wischnowsky wrote:
} Subject: PATCH: Re: Seg fault in matcher-list matching
}
} 
} Bart Schaefer wrote:
} 
} > I did this:
} > 
} > zagzig[41] /u/s/l/z/z4/s/zsh
} >                      ^cursor over the 4, press TAB
} > 
} > The path this was intended to match was /usr/src/local/zsh/zsh-2.4/src/zsh.
} > I had first tried TAB at the end of the line and gotten a feep, in case
} > that matters.
} 
} I couldn't get it to seg-fault, but there was something broken. Matching 
} of the suffix, for example, and that both in C and shell code.

It doesn't dump for me any more, but I'm still nervous about line 1767 of
compcore.c:

#1  0x80bd798 in addmatches (dat=0xbfffa854, argv=0xbfffa8d8)
    at ../../../zsh-3.1.6/Src/Zle/compcore.c:1768
1768                if ((ml = match_str(lsuf, s, &bsl, 0, NULL, 1, 0, 1)) >= 0) {
(gdb) l 
1763                    else
1764                        *argv = NULL;
1765                    bcp = lpl;
1766                }
1767                s = dat->psuf ? dat->psuf : "";
1768                if ((ml = match_str(lsuf, s, &bsl, 0, NULL, 1, 0, 1)) >= 0) {
1769                    if (matchsubs) {
1770                        Cline tmp = get_cline(NULL, 0, NULL, 0, NULL, 0, CLF_SUF);
1771
1772                        tmp->suffix = matchsubs;

The reported core dump was caused because match_str() wrote a '\0' byte into
the string pointed to by its second argument [`s' above, `w' in match_str()]
which is being passed as a string constant when dat->psuf == 0.  Is that a
potential bug, still?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

seg fault fixed and Makefile change

[Oliver Kiddle]

Sven Wischnowsky wrote:
> 
> I couldn't reproduce this (neither before nor after this patch -- your
> match specs would have helped), but this looks suspiciously similar to
> the one above... could you try with this patch?

That's fixed it: it now works fine, thanks.

This small patch to the Makefile causes it to install the functions
before the man pages. This is because the man page installation fails
if yodl isn't installed meaning that I have to do a separate
make install.fns. Of course this is only an issue if someone compiles
from CVS because the proper releases include pre-built man pages.

Completion for netscape remote commands doesn't seem to work properly
anymore:
netscape -remote <tab>
Basically, it is a bit too eager to put the \( suffix in. This did
actually work before but I've always felt that this part of _netscape
(which I am to blame for) is a bit of a nasty hack. Basically, the
'(' suffix needs to be quoted unless it already is and that was the
way I found which worked. 

Oliver

Index: Makefile.in
===================================================================
RCS file: /cvsroot/zsh/zsh/Makefile.in,v
retrieving revision 1.3
diff -u -r1.3 Makefile.in
--- Makefile.in 2000/04/06 11:47:46     1.3
+++ Makefile.in 2000/05/15 11:57:53
@@ -60,8 +60,8 @@
        $(MAKE) install STRIPFLAGS="-s"

 # install/uninstall most things
-install: install.bin install.modules install.man install.fns
-uninstall: uninstall.bin uninstall.modules uninstall.man uninstall.fns
+install: install.bin install.modules install.fns install.man
+uninstall: uninstall.bin uninstall.modules uninstall.fns uninstall.man

 # install/uninstall just the binary
 install.bin uninstall.bin:

Re: PATCH: Re: Seg fault in matcher-list matching

[Tanaka Akira]

In article <200005150926.LAA17371@beta.informatik.hu-berlin.de>,
  Sven Wischnowsky <wischnow@informatik.hu-berlin.de> writes:

> I couldn't get it to seg-fault, but there was something broken. Matching 
> of the suffix, for example, and that both in C and shell code.

I found another problem about matching control.

Z(4):akr@serein% Src/zsh -f
serein% bindkey -e; autoload -U compinit; compinit -D; compdef _tst tst
serein% _tst () { compadd -M 'r:|[:.]=* r:|=*' a.b:0 c.d:1 }
serein% tst :<TAB>

This inserts nothing now.

But 3.1.7-pre-2 inserts `.' as follows (and place the cursor on the
`.').

% tst .:
-- 
Tanaka Akira

Re: PATCH: Re: Seg fault in matcher-list matching

[Tanaka Akira]

In article <200005161048.MAA26740@beta.informatik.hu-berlin.de>,
  Sven Wischnowsky <wischnow@informatik.hu-berlin.de> writes:

> The new behaviour is the correct one: with `*', the `*' must not match 
> anchors and that's what it does now (it would have to match the `.',
> of course). With `**' you get the old behaviour, which is correct, too.

I see.  

This is a patch for _cvs to follow the new behaviour.

Index: Completion/User/_cvs
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/User/_cvs,v
retrieving revision 1.7
diff -u -r1.7 _cvs
--- Completion/User/_cvs	2000/05/16 00:07:06	1.7
+++ Completion/User/_cvs	2000/05/16 15:59:19
@@ -584,7 +584,7 @@
   fi
 
   _tags files && {
-    compadd -M 'r:|[:@./]=* r:|=*' "$@" $_cvs_roots || _files "$@" -/
+    compadd -M 'r:|[:@./]=** r:|=**' "$@" $_cvs_roots || _files "$@" -/
   }
 }
 
-- 
Tanaka Akira

Re: PATCH: Re: Seg fault in matcher-list matching

[Sven Wischnowsky]

Tanaka Akira wrote:

> In article <200005150926.LAA17371@beta.informatik.hu-berlin.de>,
>   Sven Wischnowsky <wischnow@informatik.hu-berlin.de> writes:
> 
> > I couldn't get it to seg-fault, but there was something broken. Matching 
> > of the suffix, for example, and that both in C and shell code.
> 
> I found another problem about matching control.
> 
> Z(4):akr@serein% Src/zsh -f
> serein% bindkey -e; autoload -U compinit; compinit -D; compdef _tst tst
> serein% _tst () { compadd -M 'r:|[:.]=* r:|=*' a.b:0 c.d:1 }
> serein% tst :<TAB>
> 
> This inserts nothing now.
> 
> But 3.1.7-pre-2 inserts `.' as follows (and place the cursor on the
> `.').
> 
> % tst .:

The new behaviour is the correct one: with `*', the `*' must not match 
anchors and that's what it does now (it would have to match the `.',
of course). With `**' you get the old behaviour, which is correct, too.

;-)

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: Seg fault in matcher-list matching

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> It doesn't dump for me any more, but I'm still nervous about line 1767 of
> compcore.c:
> 
> #1  0x80bd798 in addmatches (dat=0xbfffa854, argv=0xbfffa8d8)
>     at ../../../zsh-3.1.6/Src/Zle/compcore.c:1768
> 1768                if ((ml = match_str(lsuf, s, &bsl, 0, NULL, 1, 0, 1)) >= 0) {
> (gdb) l 
> 1763                    else
> 1764                        *argv = NULL;
> 1765                    bcp = lpl;
> 1766                }
> 1767                s = dat->psuf ? dat->psuf : "";
> 1768                if ((ml = match_str(lsuf, s, &bsl, 0, NULL, 1, 0, 1)) >= 0) {
> 1769                    if (matchsubs) {
> 1770                        Cline tmp = get_cline(NULL, 0, NULL, 0, NULL, 0, CLF_SUF);
> 1771
> 1772                        tmp->suffix = matchsubs;
> 
> The reported core dump was caused because match_str() wrote a '\0' byte into
> the string pointed to by its second argument [`s' above, `w' in match_str()]
> which is being passed as a string constant when dat->psuf == 0.

Yes, I know.

>  Is that a
> potential bug, still?

I'm pretty sure I made sure that we don't try to write into strings we 
can't write into with the patch I sent. But I'll also commit the one
below for some extra savety.

Bye
 Sven

Index: Src/Zle/compcore.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compcore.c,v
retrieving revision 1.19
diff -u -r1.19 compcore.c
--- Src/Zle/compcore.c	2000/05/12 07:03:41	1.19
+++ Src/Zle/compcore.c	2000/05/15 11:52:03
@@ -1739,7 +1739,7 @@
 		    llpl -= gfl;
 		}
 	    }
-	    s = dat->ppre ? dat->ppre : "";
+	    s = dat->ppre ? dat->ppre : dupstring("");
 	    if ((ml = match_str(lpre, s, &bpl, 0, NULL, 0, 0, 1)) >= 0) {
 		if (matchsubs) {
 		    Cline tmp = get_cline(NULL, 0, NULL, 0, NULL, 0, 0);
@@ -1757,14 +1757,14 @@
 		bpadd = strlen(s) - ml;
 	    } else {
 		if (llpl <= lpl && strpfx(lpre, s))
-		    lpre = "";
+		    lpre = dupstring("");
 		else if (llpl > lpl && strpfx(s, lpre))
 		    lpre += lpl;
 		else
 		    *argv = NULL;
 		bcp = lpl;
 	    }
-	    s = dat->psuf ? dat->psuf : "";
+	    s = dat->psuf ? dat->psuf : dupstring("");
 	    if ((ml = match_str(lsuf, s, &bsl, 0, NULL, 1, 0, 1)) >= 0) {
 		if (matchsubs) {
 		    Cline tmp = get_cline(NULL, 0, NULL, 0, NULL, 0, CLF_SUF);
@@ -1782,7 +1782,7 @@
 		bsadd = strlen(s) - ml;
 	    } else {
 		if (llsl <= lsl && strsfx(lsuf, s))
-		    lsuf = "";
+		    lsuf = dupstring("");
 		else if (llsl > lsl && strsfx(s, lsuf))
 		    lsuf[llsl - lsl] = '\0';
 		else

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: Seg fault in matcher-list matching

[Sven Wischnowsky]

I wrote:

> Bart Schaefer wrote:
> 
> > I did this:
> > 
> > zagzig[41] /u/s/l/z/z4/s/zsh
> >                      ^cursor over the 4, press TAB
> > 
> > The path this was intended to match was /usr/src/local/zsh/zsh-2.4/src/zsh.
> > I had first tried TAB at the end of the line and gotten a feep, in case
> > that matters.
> 
> I couldn't get it to seg-fault, but there was something broken. Matching 
> of the suffix, for example, and that both in C and shell code.

Small followup... let's try to do that as seldom as possible.

Bye
 Sven

Index: Completion/Core/_path_files
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Core/_path_files,v
retrieving revision 1.12
diff -u -r1.12 _path_files
--- Completion/Core/_path_files	2000/05/15 09:34:12	1.12
+++ Completion/Core/_path_files	2000/05/15 10:41:52
@@ -529,7 +529,7 @@
       cpre="${cpre}${tpre%%/*}/"
       tpre="${tpre#*/}"
     elif [[ "$tsuf" = */* ]]; then
-      mid="$testpath"
+      [[ "$tsuf" != /* ]] && mid="$testpath"
       cpre="${cpre}${tpre}/"
       tpre="${tsuf#*/}"
       tsuf=

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de