Re: PATCH: Small memory leak and doc fix

Re: PATCH: Small memory leak and doc fix

[Sven Wischnowsky]

Felix Rosencrantz wrote:

> ...
> 
> There is only one set of leaks that the tests still find.
> These are found by the 08traps.ztst.  (Sorry, I missed sending these before.)
> 
>         zcalloc        [mem.c:469]
>         dupeprog       [parse.c:2003]
>         dosavetrap     [signals.c:675]
>         removetrap     [signals.c:756]
>         unsettrap      [signals.c:733]
>         ------------------------------
>         zalloc         [mem.c:453]
>         dupeprog       [parse.c:1998]
>         dosavetrap     [signals.c:675]
>         removetrap     [signals.c:756]
>         unsettrap      [signals.c:733]

Seems to be an unbalanced dupeprog()/freeeprog() pair. I don't dare to 
touch that code, in case it might explode.

> >> There seems to be some situations when the following code will see
> >> uninitialized memory reads from the following stack:
> >>       pattern_match  [compmatch.c:1035]
> >>         match_str      [compmatch.c:577]
> >>         comp_match     [compmatch.c:941]
> >>         addmatches     [compcore.c:1954]
> >>         bin_compadd    [complete.c:595]
> >> I haven't looked into this, so if more details are needed let me know.
> >
> >Haven't had the time to look at this, but knowing the match specs used
> >would help.
> 
> I think this comes up while running _path_files.  The match specs used:
> 	m:{a-zA-Z}={A-Za-z}  r:|[.,_-]=** r:[^0-9]||[0-9]=**

The patch below seems to make sense, but I'm not sure if that was the
problem. Felix, could you try, please?

Bye
 Sven

Index: Src/Zle/compmatch.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compmatch.c,v
retrieving revision 1.16
diff -u -r1.16 compmatch.c
--- Src/Zle/compmatch.c	2000/05/29 12:42:59	1.16
+++ Src/Zle/compmatch.c	2000/06/07 13:21:13
@@ -535,7 +535,8 @@
 		    }
 		    /* Give up if we don't have enough characters for the
 		     * line-string and the anchor. */
-		    if (ll < llen + alen || lw < alen + aol)
+		    if (ll < llen + alen ||
+			(sfx ? (lw < alen + aol) : (lw < alen || iw < aol)))
 			continue;
 
 		    if (mp->flags & CMF_LEFT) {
@@ -571,7 +572,8 @@
 		     * string matched by the `*'. */
 		    if (sfx && (savl = l[-(llen + zoff)]))
 			l[-(llen + zoff)] = '\0';
-		    for (t = 0, tp = w, ct = 0, ict = lw - alen + 1;
+		    for (t = 0, tp = w, ct = 0,
+			     ict = lw - alen + 1 - (sfx ? aol : 0);
 			 ict;
 			 tp += add, ct++, ict--) {
 			if ((both &&

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

Re: PATCH: Small memory leak and doc fix

[Felix Rosencrantz]

>Ok. Could you try again with this patch? It should make things even
>safer.

Seems to work ok.  And I'm not seeing the error I reported.  Though, I didn't
get a chance to use the shell as much today.  The sort of completions that
would cause the problem seem not to cause that error.

Thanks Sven.
-FR





__________________________________________________
Do You Yahoo!?
Yahoo! Photos -- now, 100 FREE prints!
http://photos.yahoo.com

Re: PATCH: Small memory leak and doc fix

[Sven Wischnowsky]

Felix Rosencrantz wrote:

> >The patch below seems to make sense, but I'm not sure if that was the
> >problem. Felix, could you try, please?
> 
> Sven, I built a cvs pull with your changes, and noticed that compmatch test
> failure that Vin is also seeing.

Yes, a thinko in the patch. Sorry.

> I used the shell during the day, and didn't see the memory problem I reported.
> However, some of completions I tried that I think will cause the problem also
> are failing.  So, the patch took care of the memory problem, but probably
> caused this new matching problem.

Ok. Could you try again with this patch? It should make things even
safer.

And I forgot to say: this might also have to do with the build.out
test failure some people saw (11653) and which couldn't
reproduce. Could someone who can reproduce that reliably (Andrej?
Felix?) try it again?


Bye
 Sven

Index: Src/Zle/compmatch.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compmatch.c,v
retrieving revision 1.17
diff -u -r1.17 compmatch.c
--- Src/Zle/compmatch.c	2000/06/07 13:25:32	1.17
+++ Src/Zle/compmatch.c	2000/06/08 09:18:18
@@ -535,8 +535,7 @@
 		    }
 		    /* Give up if we don't have enough characters for the
 		     * line-string and the anchor. */
-		    if (ll < llen + alen ||
-			(sfx ? (lw < alen + aol) : (lw < alen || iw < aol)))
+		    if (ll < llen + alen || lw < alen)
 			continue;
 
 		    if (mp->flags & CMF_LEFT) {
@@ -561,8 +560,9 @@
 			if (!pattern_match(ap, l + aoff, NULL, NULL) ||
 			    (both &&
 			     (!pattern_match(ap, w + aoff, NULL, NULL) ||
-			      (aol && !pattern_match(aop, w + aoff - aol,
-						     NULL, NULL)) ||
+			      (aol && aol <= aoff + iw &&
+			       !pattern_match(aop, w + aoff - aol,
+					      NULL, NULL)) ||
 			      !match_parts(l + aoff, w + aoff, alen, part))))
 				continue;
 		    } else if (!both || il || iw)
@@ -572,19 +572,20 @@
 		     * string matched by the `*'. */
 		    if (sfx && (savl = l[-(llen + zoff)]))
 			l[-(llen + zoff)] = '\0';
-		    for (t = 0, tp = w, ct = 0,
-			     ict = lw - alen + 1 - (sfx ? aol : 0);
+		    for (t = 0, tp = w, ct = 0, ict = lw - alen + 1;
 			 ict;
 			 tp += add, ct++, ict--) {
 			if ((both &&
 			     (!ap || !test ||
 			      !pattern_match(ap, tp + aoff, NULL, NULL) ||
-			      (aol && !pattern_match(aop, tp + aoff - aol,
-						     NULL, NULL)))) ||
+			      (aol && aol <= aoff + ct + iw &&
+			       !pattern_match(aop, tp + aoff - aol,
+					      NULL, NULL)))) ||
 			    (!both &&
 			     pattern_match(ap, tp - moff, NULL, NULL) &&
-			     (!aol || pattern_match(aop, tp - moff - aol,
-						    NULL, NULL)) &&
+			     (!aol || (aol <= iw + ct - moff &&
+				       pattern_match(aop, tp - moff - aol,
+						     NULL, NULL))) &&
 			     (mp->wlen == -1 ||
 			      match_parts(l + aoff , tp - moff,
 						      alen, part)))) {

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

Re: PATCH: Small memory leak and doc fix

[Felix Rosencrantz]

>The patch below seems to make sense, but I'm not sure if that was the
>problem. Felix, could you try, please?

Sven, I built a cvs pull with your changes, and noticed that compmatch test
failure that Vin is also seeing.

I used the shell during the day, and didn't see the memory problem I reported.
However, some of completions I tried that I think will cause the problem also
are failing.  So, the patch took care of the memory problem, but probably
caused this new matching problem.

-FR.



__________________________________________________
Do You Yahoo!?
Yahoo! Photos -- now, 100 FREE prints!
http://photos.yahoo.com

Re: PATCH: Small memory leak and doc fix

[Felix Rosencrantz]

That last set of patches cleaned up most of the memory leaks found in the
tests, plus all the ones I see from just using the shell.   Thanks, Sven.

Glad they made it into 3.1.9.  Thanks, Peter.

There is only one set of leaks that the tests still find.
These are found by the 08traps.ztst.  (Sorry, I missed sending these before.)

        zcalloc        [mem.c:469]
        dupeprog       [parse.c:2003]
        dosavetrap     [signals.c:675]
        removetrap     [signals.c:756]
        unsettrap      [signals.c:733]
        ------------------------------
        zalloc         [mem.c:453]
        dupeprog       [parse.c:1998]
        dosavetrap     [signals.c:675]
        removetrap     [signals.c:756]
        unsettrap      [signals.c:733]




>> There seems to be some situations when the following code will see
>> uninitialized memory reads from the following stack:
>>       pattern_match  [compmatch.c:1035]
>>         match_str      [compmatch.c:577]
>>         comp_match     [compmatch.c:941]
>>         addmatches     [compcore.c:1954]
>>         bin_compadd    [complete.c:595]
>> I haven't looked into this, so if more details are needed let me know.
>
>Haven't had the time to look at this, but knowing the match specs used
>would help.

I think this comes up while running _path_files.  The match specs used:
	m:{a-zA-Z}={A-Za-z}  r:|[.,_-]=** r:[^0-9]||[0-9]=**


-FR.

__________________________________________________
Do You Yahoo!?
Yahoo! Photos -- now, 100 FREE prints!
http://photos.yahoo.com

Re: PATCH: Small memory leak and doc fix

[Sven Wischnowsky]

Felix Rosencrantz wrote:

> Here's another memory leak fix along with a small doc fix.
> 
> There seems to be some situations when the following code will see
> uninitialized memory reads from the following stack:
> 	pattern_match  [compmatch.c:1035]
>         match_str      [compmatch.c:577]
>         comp_match     [compmatch.c:941]
>         addmatches     [compcore.c:1954]
>         bin_compadd    [complete.c:595]
> I haven't looked into this, so if more details are needed let me know.

Haven't had the time to look at this, but knowing the match specs used
would help.

> --- Sven Wischnowsky <wischnow@informatik.hu-berlin.de> wrote:
> >Hmm. Maybe we get this because some parameter setfn() neither uses nor 
> >frees the string it gets.
> I took a look at some of the memory that wasn't freed, and found one of the
> strings had a value that only came out of the value of complist
> ("ambiguous packed rows").  So something that touches that value has a problem.
> Maybe a parameter operator.

I found that at the weekend already: it's $compstate[list]. And then
there was another one in $commands.

Does that fix the problems with addvars()?

> >I can't see where this comes from. mkautofn() creates the
> >autofn-program wich is then freed in loadautofn() (or
> >freeshfuncnode(), with ksh-autoloading).
> >
> >All these autofn-progs won't be freed at the end, though (together
> >with many other things).
> 
> I can see these errors even before the shell exits.  The memory checker
> will only list a memory leak if there are no pointers in memory to a memory
> block.  We are probably losing the pointer to the memory from buitlin.c:2162.

Ouch. Right. `pats' should point to the same memory as `prog' because
that's what's used for freeing.

Bye
 Sven

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.20
diff -u -r1.20 builtin.c
--- Src/builtin.c	2000/05/27 08:31:32	1.20
+++ Src/builtin.c	2000/06/05 07:55:01
@@ -2162,7 +2162,7 @@
     p->strs = NULL;
     p->shf = shf;
     p->npats = 0;
-    p->pats = NULL;
+    p->pats = (Patprog *) p->prog;
     p->flags = EF_REAL;
     p->dump = NULL;
 
Index: Src/Modules/parameter.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/parameter.c,v
retrieving revision 1.8
diff -u -r1.8 parameter.c
--- Src/Modules/parameter.c	2000/06/02 01:54:16	1.8
+++ Src/Modules/parameter.c	2000/06/05 07:55:01
@@ -192,9 +192,10 @@
 static void
 setpmcommand(Param pm, char *value)
 {
-    if (isset(RESTRICTED))
+    if (isset(RESTRICTED)) {
 	zwarn("restricted: %s", value, 0);
-    else {
+	zsfree(value);
+    } else {
 	Cmdnam cn = zcalloc(sizeof(*cn));
 
 	cn->flags = HASHED;
Index: Src/Zle/compresult.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compresult.c,v
retrieving revision 1.18
diff -u -r1.18 compresult.c
--- Src/Zle/compresult.c	2000/05/31 09:56:12	1.18
+++ Src/Zle/compresult.c	2000/06/05 07:55:02
@@ -1168,7 +1168,7 @@
 comp_list(char *v)
 {
     zsfree(complist);
-    complist = ztrdup(v);
+    complist = v;
 
     onlyexpl = (v ? ((strstr(v, "expl") ? 1 : 0) |
 		     (strstr(v, "messages") ? 2 : 0)) : 0);

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

PATCH: Small memory leak and doc fix

[Felix Rosencrantz]

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

Here's another memory leak fix along with a small doc fix.

There seems to be some situations when the following code will see
uninitialized memory reads from the following stack:
	pattern_match  [compmatch.c:1035]
        match_str      [compmatch.c:577]
        comp_match     [compmatch.c:941]
        addmatches     [compcore.c:1954]
        bin_compadd    [complete.c:595]
I haven't looked into this, so if more details are needed let me know.

--- Sven Wischnowsky <wischnow@informatik.hu-berlin.de> wrote:
>Hmm. Maybe we get this because some parameter setfn() neither uses nor 
>frees the string it gets.
I took a look at some of the memory that wasn't freed, and found one of the
strings had a value that only came out of the value of complist
("ambiguous packed rows").  So something that touches that value has a problem.
Maybe a parameter operator.

>I can't see where this comes from. mkautofn() creates the
>autofn-program wich is then freed in loadautofn() (or
>freeshfuncnode(), with ksh-autoloading).
>
>All these autofn-progs won't be freed at the end, though (together
>with many other things).

I can see these errors even before the shell exits.  The memory checker
will only list a memory leak if there are no pointers in memory to a memory
block.  We are probably losing the pointer to the memory from buitlin.c:2162.

-FR


__________________________________________________
Do You Yahoo!?
Yahoo! Photos -- now, 100 FREE prints!
http://photos.yahoo.com

[-- Attachment #2: patch7.txt --]
[-- Type: text/plain, Size: 1145 bytes --]

Index: Src/Zle/compcore.c
===================================================================
--- zsh/Src/Zle/ocompcore.c	Fri Jun  2 01:09:56 2000
+++ zsh/Src/Zle/compcore.c	Fri Jun  2 19:16:33 2000
@@ -733,6 +733,7 @@
 	}
 	compinsert = (useline < 0 ? tricat("tab ", "", compinsert) :
 		      ztrdup(compinsert));
+	zsfree(compexact);
 	if (useexact)
 	    compexact = ztrdup("accept");
 	else {

Index: Doc/Zsh/mod_complist.yo
===================================================================
--- zsh/Doc/Zsh/omod_complist.yo	Sun May 21 11:27:36 2000
+++ zsh/Doc/Zsh/mod_complist.yo	Fri Jun  2 19:33:20 2000
@@ -144,7 +144,7 @@
 letter will be replaced with a string of fixed width, padded to the
 right with spaces, while the lowercase form will not be padded.
 
-If the option att(LISTPROMPT) is set, the completion code will not ask if
+If the parameter tt(LISTPROMPT) is set, the completion code will not ask if
 the list should be shown.  Instead it immediately starts displaying the
 list, stopping after the first screenful, showing the prompt at the bottom,
 waiting for a keypress after temporarily switching to the tt(listscroll)