aliases+=(foo 'echo bar') crash

aliases+=(foo 'echo bar') crash

[Stephane Chazelas]

$ zsh --version
zsh 5.0.5 (x86_64-pc-linux-gnu)
$ zsh -c 'aliases+=(a b)'
*** Error in `zsh': double free or corruption (out): 0x00007f55e838f7e0 ***
zsh: abort      zsh -c 'aliases+=(a b)'

(gdb) bt
#0  0x00007ffff7135407 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff71367e8 in __GI_abort () at abort.c:89
#2  0x00007ffff7173394 in __libc_message (do_abort=do_abort@entry=1, fmt=fmt@entry=0x7ffff7266010 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff7178b6e in malloc_printerr (action=1, str=0x7ffff7266160 "double free or corruption (out)", ptr=<optimized out>) at malloc.c:4996
#4  0x00007ffff7179876 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3840
#5  0x0000000000455305 in strsetfn ()
#6  0x000000000045a2f2 in ?? ()
#7  0x000000000045a7cc in arrhashsetfn ()
#8  0x000000000045b263 in assignaparam ()
#9  0x00000000004226cc in ?? ()
#10 0x00000000004272a5 in ?? ()
#11 0x0000000000428896 in ?? ()
#12 0x0000000000428c0e in ?? ()
#13 0x0000000000429db1 in execlist ()
#14 0x000000000042a030 in execode ()
#15 0x000000000042a102 in execstring ()
#16 0x000000000043bde2 in init_misc ()
#17 0x000000000043d29e in zsh_main ()
#18 0x00007ffff7121b45 in __libc_start_main (main=0x40f010 <main>, argc=3, argv=0x7fffffffdd98, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>,
    stack_end=0x7fffffffdd88) at libc-start.c:287
#19 0x000000000040f03e in _start ()

Re: aliases+=(foo 'echo bar') crash

[Peter Stephenson]

On Wed, 23 Jul 2014 17:09:35 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> $ zsh -c 'aliases+=(a b)'
> *** Error in `zsh': double free or corruption (out): 0x00007f55e838f7e0 ***
> zsh: abort      zsh -c 'aliases+=(a b)'

I haven't got very far with this, but I'm suspicious of this blithe
assumption in arrhashsetfn()...

	/* ...but we can use the value without copying. */
	setstrvalue(v, *aptr++);

The crash is down there (it comes up as just a debug warning for me, I
assume it's the same thing).

pws

Re: aliases+=(foo 'echo bar') crash

[Bart Schaefer]

On Jul 23,  5:52pm, Peter Stephenson wrote:
}
} I haven't got very far with this, but I'm suspicious of this blithe
} assumption in arrhashsetfn()...
} 
} 	/* ...but we can use the value without copying. */
} 	setstrvalue(v, *aptr++);

That does appear to be related; valgrind complains about it:

==5082== Invalid free() / delete / delete[]
==5082==    at 0x4004EFA: free (vg_replace_malloc.c:235)
==5082==    by 0x8091F38: zsfree (mem.c:1727)
==5082==    by 0x80A0572: strsetfn (params.c:3148)
==5082==    by 0x809DA30: setstrvalue (params.c:2297)
==5082==    by 0x80A07FE: arrhashsetfn (params.c:3247)
==5082==    by 0x809E234: setarrvalue (params.c:2472)
==5082==    by 0x809F724: assignaparam (params.c:2829)
==5082==    by 0x80650B7: addvars (exec.c:2304)
==5082==    by 0x8066030: execcmd (exec.c:2677)
==5082==    by 0x8063A59: execpline2 (exec.c:1691)
==5082==    by 0x8062DFE: execpline (exec.c:1478)
==5082==    by 0x80626D6: execlist (exec.c:1261)
==5082==  Address 0x43C16A8 is not stack'd, malloc'd or (recently) free'd

However, if we look at addvars (exec.c:2304):

   2286         if (vl) {
   2287             ptr = arr = (char **) zalloc(sizeof(char **) *
   2288                                          (countlinknodes(vl) + 1));
   2289 
   2290             while (nonempty(vl))
   2291                 *ptr++ = ztrdup((char *) ugetnode(vl));
   2292         } else
   2293             ptr = arr = (char **) zalloc(sizeof(char **));
   2294 
   2295         *ptr = NULL;
   2296         if (xtr) {
   2297             fprintf(xtrerr, "( ");
   2298             for (ptr = arr; *ptr; ptr++) {
   2299                 quotedzputs(*ptr, xtrerr);
   2300                 fputc(' ', xtrerr);
   2301             }
   2302             fprintf(xtrerr, ") ");
   2303         }
   2304         assignaparam(name, arr, myflags);

The "arr" pointer is zalloc'd and every value in it is ztrdup'd, so the
basic assumption seems to be good.

The real problem seems to be here:

   3224     /* Best not to shortcut this by using the existing hash table,   *
   3225      * since that could cause trouble for special hashes.  This way, *
   3226      * it's up to pm->gsu.h->setfn() what to do.                     */
   3227     int alen = arrlen(val);
   3228     HashTable opmtab = paramtab, ht = 0;
   3229     char **aptr = val;
   3230     Value v = (Value) hcalloc(sizeof *v);
   3231     v->end = -1;
...
   3242         /* The parameter name is ztrdup'd... */
   3243         v->pm = createparam(*aptr, PM_SCALAR|PM_UNSET);

The bad free that's being complained about is v->pm->u.str, which either
came from hcalloc() for v or from somewhere in createparam().  The crash
is actually here at ->setfn():

   2298     switch (PM_TYPE(v->pm->node.flags)) {
   2299     case PM_SCALAR:
   2300         if (v->start == 0 && v->end == -1) {
   2301             v->pm->gsu.s->setfn(v->pm, val);

The bad values in *pm come from here:

    857         oldpm = (Param) (paramtab == realparamtab ?
    858                          gethashnode2(paramtab, name) :
    859                          paramtab->getnode(paramtab, name));

(where paramtab != realparamtab).  That's as far as I've gotten.

Re: aliases+=(foo 'echo bar') crash

[Bart Schaefer]

On Jul 23,  6:37pm, Bart Schaefer wrote:
}
} The bad values in *pm come from here:
} 
}     857         oldpm = (Param) (paramtab == realparamtab ?
}     858                          gethashnode2(paramtab, name) :
}     859                          paramtab->getnode(paramtab, name));
} 
} (where paramtab != realparamtab).  That's as far as I've gotten.

So ->getnode() there is getpmralias() which calls getalias() which
allocates everything on the heap.

When arrhashsetfn() tries to replace the value returned from getalias(),
kaboom.

The problem may be that the aliases hash has been autoloaded from the
zsh/parameter module AFTER the flow of control has already passed the
point of deciding what function to call to perform the assignment, and
so is using the generic hash function instead of the special functions
for setting aliases.

[PATCH] Re: aliases+=(foo 'echo bar') crash

[Bart Schaefer]

On Jul 23,  8:04pm, Bart Schaefer wrote:
}
} The problem may be that the aliases hash has been autoloaded from the
} zsh/parameter module AFTER the flow of control has already passed the
} point of deciding what function to call to perform the assignment, and
} so is using the generic hash function instead of the special functions
} for setting aliases.

It's both better and worse than that.  When createparam() is called for
a value that doesn't already exist in the aliases hash table, a new
unset value is correctly created, but then assigngetset() is called on
it, which clobbers the special gsu structure with the default setfn.

I believe the following fixes it for the zsh/parameter module, but there
may be other modules where the same problem arises.


diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index 22148f9..0385a70 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -106,7 +106,7 @@ getpmparameter(UNUSED(HashTable ht), const char *name)
 	pm->u.str = paramtypestr(rpm);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -224,7 +224,7 @@ getpmcommand(UNUSED(HashTable ht), const char *name)
 	}
     } else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -410,7 +410,7 @@ getfunction(UNUSED(HashTable ht), const char *name, int dis)
 	}
     } else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -661,7 +661,7 @@ getbuiltin(UNUSED(HashTable ht), const char *name, int dis)
 	pm->u.str = dupstring(t);
     } else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -876,7 +876,7 @@ getpmoption(UNUSED(HashTable ht), const char *name)
     }
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -934,7 +934,7 @@ getpmmodule(UNUSED(HashTable ht), const char *name)
 	pm->u.str = dupstring(type);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1048,7 +1048,7 @@ getpmhistory(UNUSED(HashTable ht), const char *name)
 	pm->u.str = dupstring(he->node.nam);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1158,7 +1158,7 @@ getpmjobtext(UNUSED(HashTable ht), const char *name)
 	pm->u.str = pmjobtext(job);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1259,7 +1259,7 @@ getpmjobstate(UNUSED(HashTable ht), const char *name)
 	pm->u.str = pmjobstate(job);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1325,7 +1325,7 @@ getpmjobdir(UNUSED(HashTable ht), const char *name)
 	pm->u.str = pmjobdir(job);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1451,7 +1451,7 @@ getpmnameddir(UNUSED(HashTable ht), const char *name)
 	pm->u.str = dupstring(nd->dir);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1502,7 +1502,7 @@ getpmuserdir(UNUSED(HashTable ht), const char *name)
 	pm->u.str = dupstring(nd->dir);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1754,7 +1754,7 @@ getalias(HashTable alht, UNUSED(HashTable ht), const char *name, int flags)
 	pm->u.str = dupstring(al->text);
     else {
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     }
     return &pm->node;
 }
@@ -1950,7 +1950,7 @@ getpmusergroups(UNUSED(HashTable ht), const char *name)
     if (!gs) {
 	zerr("failed to retrieve groups for user: %e", errno);
 	pm->u.str = dupstring("");
-	pm->node.flags |= PM_UNSET;
+	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
 	return &pm->node;
     }
 
@@ -1965,7 +1965,7 @@ getpmusergroups(UNUSED(HashTable ht), const char *name)
     }
 
     pm->u.str = dupstring("");
-    pm->node.flags |= PM_UNSET;
+    pm->node.flags |= (PM_UNSET|PM_SPECIAL);
     return &pm->node;
 }
 

Re: [PATCH] Re: aliases+=(foo 'echo bar') crash

[Peter Stephenson]

On Wed, 23 Jul 2014 22:45:35 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> -	pm->node.flags |= PM_UNSET;
> +	pm->node.flags |= (PM_UNSET|PM_SPECIAL);

It's a bit surprising this hasn't caused mayhem before now, but I suppose
the damage is limited or non-existent until you try to set the
parameters, and mostly that just gives you a non-standard interface to
something that's usually done another way.

pws

Re: [PATCH] Re: aliases+=(foo 'echo bar') crash

[Bart Schaefer]

On Jul 24, 10:47am, Peter Stephenson wrote:
} Subject: Re: [PATCH] Re: aliases+=(foo 'echo bar') crash
}
} On Wed, 23 Jul 2014 22:45:35 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > -	pm->node.flags |= PM_UNSET;
} > +	pm->node.flags |= (PM_UNSET|PM_SPECIAL);
} 
} It's a bit surprising this hasn't caused mayhem before now, but I
} suppose the damage is limited or non-existent until you try to set
} the parameters

Simple assignment via either aliases=(a b) or aliases[a]=b doesn't go
through the same code path, so there was no issue until the += syntax
was added ... and it's pretty unusual to append something to a hash
table that way, so I'm not that surprised that no one encountered it.