[PATCH] Re: dirstack history: loving zsh, crashing zsh...

[Andrey Borzenkov <arvidjaar@newmail.ru>]

[-- Attachment #1.1: Type: text/plain, Size: 1659 bytes --]

On Saturday 04 March 2006 11:04, Andrey Borzenkov wrote:
> [moved to workers]
>
> On Thursday 02 March 2006 20:52, Francisco Borges wrote:
> > % typeset -U dirstack
> >
> > and the shell crashed.
>
> The problem is rather non-trivial. dirsgetfn returns array built on-the-fly
> in heap, while typeset -U calls uniqarray() that tries to zfree array
> elements. There are at least two problems here:
>
> - typeset -U is not prepared to deal with "pseudo" parameters at all. It
> assumes a->getfn() returns pointer to real parameter value. So it would
> have not worked for dirstack anyway
>
> - I was about to change typeset -U to pm->gsu.a->setfn(pm,
> pm->gsu.a->getfn(pm)) (basically doing foo=($foo)) and adding uniqarray
> call to dirssetfn() when I realized that it would not help at all in this
> case as dirssetfn() tries to free passed value too; so it would have
> crashed just the same.
>
> Apparently to solve it in general we need one of
>
> - per-parameter type ->uniq function (is it an overkill?) Possibly
> generalized to per-parameter ->setflags function.
>
> - some way to know if passed pointer was allocated from heap or not. I
> guess it should be possible; something like isheap(p)?
>

Assuming the issue with freeing heap memory is resolved, the patch adds 
support for -U to dirstack. It also adds framework for adding uniqueness 
support to any parameter by changing typeset to simply do equiv. of 
'foo=($foo)'. It also uncovered one case of using free() instead of zfree(); 
assuming previous patch is accepted it is necessary to audit code and replace 
all plain free() with zfree().

-andrey

[-- Attachment #1.2: dirstack_-U_support.patch --]
[-- Type: text/x-diff, Size: 2681 bytes --]

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.151
diff -u -p -r1.151 builtin.c
--- Src/builtin.c	7 Nov 2005 09:37:34 -0000	1.151
+++ Src/builtin.c	4 Mar 2006 09:19:18 -0000
@@ -1920,24 +1920,17 @@ typeset_single(char *cname, char *pname,
 	    zerrnam(cname, "%s: restricted", pname, 0);
 	    return pm;
 	}
+	pm->flags = (pm->flags | (on & ~PM_READONLY)) & ~(off | PM_UNSET);
 	if ((on & PM_UNIQUE) && !(pm->flags & PM_READONLY & ~off)) {
 	    Param apm;
-	    char **x;
-	    if (PM_TYPE(pm->flags) == PM_ARRAY) {
-		x = (*pm->gsu.a->getfn)(pm);
-		uniqarray(x);
-		if (pm->ename && x)
-		    arrfixenv(pm->ename, x);
-	    } else if (PM_TYPE(pm->flags) == PM_SCALAR && pm->ename &&
-		       (apm =
-			(Param) paramtab->getnode(paramtab, pm->ename))) {
-		x = (*apm->gsu.a->getfn)(apm);
-		uniqarray(x);
-		if (x)
-		    arrfixenv(pm->nam, x);
-	    }
+
+	    if (PM_TYPE(pm->flags) == PM_ARRAY)
+		apm = pm;
+	    else if (PM_TYPE(pm->flags) == PM_SCALAR && pm->ename)
+		apm = (Param) paramtab->getnode(paramtab, pm->ename);
+	    if (apm)
+		apm->gsu.a->setfn(pm, apm->gsu.a->getfn(apm));
 	}
-	pm->flags = (pm->flags | (on & ~PM_READONLY)) & ~(off | PM_UNSET);
 	if (on & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) {
 	    if (typeset_setwidth(cname, pm, ops, on, 0))
 		return NULL;
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.118
diff -u -p -r1.118 utils.c
--- Src/utils.c	1 Mar 2006 14:50:52 -0000	1.118
+++ Src/utils.c	4 Mar 2006 09:19:21 -0000
@@ -2499,7 +2499,7 @@ freearray(char **s)
 
     while (*s)
 	zsfree(*s++);
-    free(t);
+    zfree(t, 0);
 }
 
 /**/
Index: Src/Modules/parameter.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/parameter.c,v
retrieving revision 1.35
diff -u -p -r1.35 parameter.c
--- Src/Modules/parameter.c	10 Mar 2005 17:56:05 -0000	1.35
+++ Src/Modules/parameter.c	4 Mar 2006 09:19:21 -0000
@@ -929,15 +929,19 @@ scanpmmodules(UNUSED(HashTable ht), Scan
 
 /**/
 static void
-dirssetfn(UNUSED(Param pm), char **x)
+dirssetfn(Param pm, char **x)
 {
     char **ox = x;
 
     if (!incleanup) {
 	freelinklist(dirstack, freestr);
 	dirstack = znewlinklist();
-	while (x && *x)
-	    zaddlinknode(dirstack, ztrdup(*x++));
+	if (x) {
+	    if (pm->flags & PM_UNIQUE)
+		uniqarray(x);
+	    while (*x)
+		zaddlinknode(dirstack, ztrdup(*x++));
+	}
     }
     if (ox)
 	freearray(ox);

[-- Attachment #2: Type: application/pgp-signature, Size: 191 bytes --]