D04parameter.ztst crashes if USE_MMAP is not defined

D04parameter.ztst crashes if USE_MMAP is not defined

[Jun. T]

If I use autoconf-2.72 to create configure on Cygwin, and build
zsh, then test D04parameter crashes. The crash can be reproduced
on Linux or macOS by, after ./configure, manually removing the
following lines from zsh.h,
#define HAVE_MMAP 1
#define HAVE_MSYNC 1
#define HAVE_MUNMAP 1
and "make; make TESTNUM=D04 check".

[1] AC_FUNC_MMAP in autoconf-2.72 checks functionalities of
mmap() more strictly, and Cygwin's mmap() can't pass this
test, and HAVE_MMAP is not defined in zsh.h.

# The new AC_FUNC_MMAP checks whether MAP_FIXED works as
# expected or not (and it doesn't work on Cygwin). But zsh does
# not use MAP_FIXED. If autoconf-2.71 or earlier is used on
# Cygwin, HAVE_MMAP is defined and the resulting zsh works fine.

It seems something is wrong in the code that is used when
USE_MMAP is not defined.

[2] The crash occurs in the test chunk
"Unsetting and recreation of tied normal parameters".
The chunk has 8 "print $STRING $string", and it crashes
at the 6th of them.

But If I run the code in the chunk alone in a zsh (built without
HAVE_MMAP) it does not crash. Maybe the crash is related with
some state of the heap?

[3] Back trace of the crashed zsh (obtained on Ubuntu-22.04):
#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:74
#1  0x000055e6f65cbffe in paramsubst (l=0x55e6f860b178, n=0x55e6f860b1c0, 
    str=0x7ffcebda63a0, qt=0, pf_flags=0, ret_flags=0x7ffcebda64b4)
    at subst.c:4322
#2  0x000055e6f65c2592 in stringsubst (list=0x55e6f860b178, 
    node=0x55e6f860b1c0, pf_flags=0, ret_flags=0x7ffcebda64b4, asssub=0)
    at subst.c:322
#3  0x000055e6f65c1878 in prefork (list=0x55e6f860b178, flags=0, 
    ret_flags=0x7ffcebda64b4) at subst.c:142
#4  0x000055e6f6549ec7 in execcmd_exec (state=0x7ffcebda6e00, 
    eparams=0x7ffcebda6a10, input=0, output=0, how=2, last1=2, 
    close_if_forked=-1) at exec.c:3282
#5  0x000055e6f6546636 in execpline2 (state=0x7ffcebda6e00, pcode=1219, how=2, 
    input=0, output=0, last1=0) at exec.c:2016
#6  0x000055e6f65451ca in execpline (state=0x7ffcebda6e00, slcode=5122, how=2, 
    last1=0) at exec.c:1741
#7  0x000055e6f654440c in execlist (state=0x7ffcebda6e00, dont_change_job=1, 
    exiting=0) at exec.c:1495
#8  0x000055e6f6543a2c in execode (p=0x55e6f8609ec0, dont_change_job=1, 
    exiting=0, context=0x55e6f65e10eb "eval") at exec.c:1276
#9  0x000055e6f6539bca in eval (argv=0x55e6f8608de0) at builtin.c:6203
#10 0x000055e6f653a8de in bin_eval (nam=0x55e6f8608ba8 "eval", 
    argv=0x55e6f8608de0, ops=0x7ffcebda6fa0, func=14) at builtin.c:6389

line 4322 in subst.c is:
            xlen = strlen(x);
It seems x (= aval[0]) points to an already freed memory, but
I currently have no time to investigate further.

Re: D04parameter.ztst crashes if USE_MMAP is not defined

[Bart Schaefer]

On Wed, Feb 28, 2024 at 8:23 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> [2] The crash occurs in the test chunk
> "Unsetting and recreation of tied normal parameters".
> The chunk has 8 "print $STRING $string", and it crashes
> at the 6th of them.

valgrind says, after the 5th "print":

==608496== Conditional jump or move depends on uninitialised value(s)
==608496==    at 0x1D144A: paramsubst (subst.c:3822)
==608496==    by 0x1C93E2: stringsubst (subst.c:322)
==608496==    by 0x1C86BE: prefork (subst.c:142)
==608496==    by 0x152CAB: execcmd_exec (exec.c:3282)
==608496==    by 0x14F4FA: execpline2 (exec.c:2016)
==608496==    by 0x14E081: execpline (exec.c:1741)
==608496==    by 0x14D2D7: execlist (exec.c:1495)
==608496==    by 0x14C905: execode (exec.c:1276)
==608496==    by 0x15B53B: runshfunc (exec.c:6164)
==608496==    by 0x15AA51: doshfunc (exec.c:6010)
==608496==    by 0x1596FE: execshfunc (exec.c:5548)
==608496==    by 0x1590D0: execfuncdef (exec.c:5408)

Line 3822 is:
   3822     if (isarr > 0 && !plan9 && (!aval || !aval[0])) {

That would be during one of the expansions ...

  STRING=a:b
  typeset -T STRING string
  print $STRING $string
  unset string
  STRING=x:y:z
  print $STRING $string  # ... here

Minimally reproducible from Src/zsh -f with

() {
  local STRING=a:b
  typeset -T STRING string
  unset string
  STRING=x:y:z
  print $STRING $string
}

BUT!  Only the first time after shell startup.  Repeated calls do not
produce repeated warnings from valgrind.

> Maybe the crash is related with
> some state of the heap?

That must be it, because the heap uses mapped memory when it can and I
don't see any other way HAVE_MMAP could be involved.

Re: D04parameter.ztst crashes if USE_MMAP is not defined

[Bart Schaefer]

On Wed, Feb 28, 2024 at 8:23 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> line 4322 in subst.c is:
>             xlen = strlen(x);
> It seems x (= aval[0]) points to an already freed memory

If I step through this with gdb, x should point to the result of
dupstring("") from

3823        val = dupstring("");

(gdb) p aval
$19 = (char **) 0x7ffff7fbc4a0
(gdb) p val
$21 = 0x7ffff7fbc4a8 ""
(gdb) p x
$22 = 0x7ffff7fbc4a8 ""

0x7ffff7fbc4a0 is returned from hcalloc() here:

3945        return pm->u.str ? pm->u.str : (char *) hcalloc(1);

However, getting there is a bit questionable:

getvaluearr (v=0x7fffffffd120) at params.c:686

689        else if (PM_TYPE(v->pm->node.flags) == PM_ARRAY)
690        return v->arr = v->pm->gsu.a->getfn(v->pm);
(gdb) s
strgetfn (pm=0x5555555ebd95 <fetchvalue+1539>) at params.c:3944

Why is gsu.a->getfn pointed at strgetfn ?  I could imagine that does
the wrong thing from time to time.

Re: D04parameter.ztst crashes if USE_MMAP is not defined

[Jun T]

> 2024/02/29 13:19、Bart Schaefer <schaefer@brasslantern.com>のメール:
> 
> However, getting there is a bit questionable:
> 
> getvaluearr (v=0x7fffffffd120) at params.c:686
> 
> 689        else if (PM_TYPE(v->pm->node.flags) == PM_ARRAY)
> 690        return v->arr = v->pm->gsu.a->getfn(v->pm);
> (gdb) s
> strgetfn (pm=0x5555555ebd95 <fetchvalue+1539>) at params.c:3944
> 
> Why is gsu.a->getfn pointed at strgetfn ?  I could imagine that does
> the wrong thing from time to time.

Why (PM_TYPE(v->pm->node.flags) == PM_ARRAY) is true here?
I believe the parameter here is 'string'. But didn't we unset it?

If I add 'typeset -p string' to D04:

--- a/Test/D04parameter.ztst
+++ b/Test/D04parameter.ztst
@@ -1222,6 +1222,7 @@
   typeset -T STRING string
   print $STRING $string
   unset string
+  typeset -p string
   STRING=x:y:z
   print $STRING $string
   STRING=a:b

then it crashes at this 'typeset -p string'. Back trace is:
(gdb) bt
#0  0x0000557c31da6511 in quotedzputs (
    s=0x65756e69746e6f63 <error: Cannot access memory at address 0x65756e69746e6f63>, stream=0x154592c1b780 <_IO_2_1_stdout_>) at utils.c:6439
#1  0x0000557c31d6aa57 in printparamvalue (p=0x557c323a3460, printflags=544)
    at params.c:5996
#2  0x0000557c31d6b070 in printparamnode (hn=0x557c323a3460, printflags=544)
    at params.c:6197
#3  0x0000557c31cf4c81 in bin_typeset (name=0x557c323bb490 "typeset", 
    argv=0x557c323bb4e8, assigns=0x0, ops=0x7ffd004cecf0, func=0)
    at builtin.c:3096

Line 5996 in params.c is in the block 'case PM_ARRAY:'.

(gdb) up 3
#3  0x000055e433bc1c81 in bin_typeset (name=0x55e4359839b8 "typeset", 
    argv=0x55e435983a10, assigns=0x0, ops=0x7fff8a133e70, func=0)
    at builtin.c:3096
3096			paramtab->printnode(hn, printflags);
(gdb) p *hn
$1 = {next = 0x55e43590e870, nam = 0x55e43596aa60 "string", flags = 1}

This means getnode2() at line 3092 (builin.c) can find an array parameter
 'string' in paramtab.


Moreover, if I run D04 ('typeset -p' added) with a normal zsh (i.e.
with mmap), then it fails as follows:

@@ -3,6 +3,7 @@
 a:b a b
 x y z
 a:b a b
+typeset -a string=(  )
 x:y:z
 a:b a b
 x:y:z
Test ./D04parameter.ztst failed: output differs from expected as shown above for:

So even with mmap, the parameter 'string' is not removed from paramtab...?

Re: D04parameter.ztst crashes if USE_MMAP is not defined

[Bart Schaefer]

On Fri, Mar 1, 2024 at 1:57 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> Why (PM_TYPE(v->pm->node.flags) == PM_ARRAY) is true here?
> I believe the parameter here is 'string'. But didn't we unset it?

Thanks, this was the needed clue.

In stdunsetfn:

3903        if ((pm->node.flags & (PM_SPECIAL|PM_TIED)) == PM_TIED) {
3904        if (pm->ename) {
3905            zsfree(pm->ename);
3906            pm->ename = NULL;
3907        }
3908        pm->node.flags &= ~PM_TIED;
3909        }
3910        pm->node.flags |= PM_UNSET;

> So even with mmap, the parameter 'string' is not removed from paramtab...?

No, it isn't.  It's just marked PM_UNSET.  Then back in unsetparam_pm:

3811            if (oldpm && !altpm->level) {
3812            oldpm->old = NULL;
3813            /* fudge things so removenode isn't called */
3814            altpm->level = 1;
3815            }
3816            unsetparam_pm(altpm, 1, exp);

Calling unsetparam_pm() on the scalar altpm ends up calling the setfn
of the array name again, which clears the PM_UNSET flag that was
applied at line 3910.

3819        zsfree(altremove);
3820        if (!(pm->node.flags & PM_SPECIAL))
3821            pm->gsu.s = &stdscalar_gsu;

We change the gsu pointer but not the parameter flags.  Then:

3825         * If this was a local variable, we need to keep the old
3826         * struct so that it is resurrected at the right level.
3827         * This is partly because when an array/scalar value is set
3828         * and the parameter used to be the other sort, unsetparam()
3829         * is called.  Beyond that, there is an ambiguity:  should
3830         * foo() { local bar; unset bar; } make the global bar
3831         * available or not?  The following makes the answer "no".

3836        if ((pm->level && locallevel >= pm->level) ||
3837        (pm->node.flags & (PM_SPECIAL|PM_REMOVABLE)) == PM_SPECIAL)
3838        return 0;

This should fix it:

diff --git a/Src/params.c b/Src/params.c
index 064dbd2bc..e83e4aa5e 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3813,12 +3813,15 @@ unsetparam_pm(Param pm, int altflag, int exp)
                /* fudge things so removenode isn't called */
                altpm->level = 1;
            }
-           unsetparam_pm(altpm, 1, exp);
+           unsetparam_pm(altpm, 1, exp); /* This resets pm to empty */
+           pm->node.flags |= PM_UNSET;   /* so we must repeat this */
        }

        zsfree(altremove);
-       if (!(pm->node.flags & PM_SPECIAL))
+       if (!(pm->node.flags & PM_SPECIAL)) {
            pm->gsu.s = &stdscalar_gsu;
+           pm->node.flags &= ~PM_ARRAY;
+       }
     }

     /*
diff --git a/Test/D04parameter.ztst b/Test/D04parameter.ztst
index 69a4fd3ec..0e2a04eb5 100644
--- a/Test/D04parameter.ztst
+++ b/Test/D04parameter.ztst
@@ -1222,6 +1222,7 @@
   typeset -T STRING string
   print $STRING $string
   unset string
+  typeset -p string
   STRING=x:y:z
   print $STRING $string
   STRING=a:b