zsh 5.2 build with --enable-stack-allocation crashes on large environments

zsh 5.2 build with --enable-stack-allocation crashes on large environments

[Charles Daffern]

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

Hi,


When building a copy of zsh 5.2 for personal use, I stumbled upon this
rather unusual bug.

You can reproduce it with something like this sequence of commands in
the zsh 5.2 source directory:


./configure --enable-stack-allocation

make

x=xxxxxxxx

for i ({1..6}) x=$x$x$x$x

export x

Src/zsh -f


The large environment variable causes zsh to crash on launch. Equally, a
large number of environment variables has the same effect.

Debugging shows this:


 1. Program received signal SIGSEGV, Segmentation fault.
 2. 0x000000000045cdbf in popheap () at mem.c:512
 3. 512 hl->next = NULL;
 4. (gdb) bt
 5. #0 0x000000000045cdbf in popheap () at mem.c:512
 6. #1 0x00000000004706f2 in createparamtable () at params.c:772
 7. #2 0x00000000004495ef in setupvals () at init.c:1045
 8. #3 0x0000000000449c9b in zsh_main (argc=<value optimized out>,
    argv=<value optimized out>) at init.c:1659
 9. #4 0x000003fff71d1d1d in __libc_start_main (main=0x40f3a0 <main>,
    argc=1, ubp_av=0x3ffffff8368, init=<value optimized out>,
    fini=<value optimized out>,
10. rtld_fini=<value optimized out>, stack_end=0x3ffffff8358) at
    libc-start.c:226
11. #5 0x000000000040f2d9 in _start ()


It's having trouble dereferencing "hl", and it turns out there's a
reason for that:


 1. Breakpoint 1, popheap () at mem.c:502
 2. (gdb) print h
 3. $1 = (struct heap *) 0x3fff7fec000
 4. (gdb) print heaps
 5. $2 = (Heap) 0x3fff7fe8000
 6. (gdb) c
 7. Continuing.
 8.
 9. Breakpoint 1, popheap () at mem.c:502
10. (gdb) print h
11. $3 = (struct heap *) 0x3fff7fe8000
12. (gdb) print heaps
13. $4 = (Heap) 0x3fff7fe8000


The above breakpoint is on the munmap in popheap. The entire "heaps"
variable is being freed at one point (because "h" is freed and they
became equal for some reason). I'm not familiar enough with the code to
figure out how this is happening though.


The bug is triggered by the --enable-stack-allocation option to
configure. Without that option, zsh works as expected.

Apologies in advance for the formatting, Thunderbird still thinks it's
in WYSIWYG mode and I'm not entirely sure how this email will end up
when it churns out the plain-text form.


Thanks,

Charles Daffern



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: zsh 5.2 build with --enable-stack-allocation crashes on large environments

[Bart Schaefer]

On Oct 18,  1:27pm, Charles Daffern wrote:
}
} The large environment variable causes zsh to crash on launch. Equally, a
} large number of environment variables has the same effect.

[...]

} The above breakpoint is on the munmap in popheap. The entire "heaps"
} variable is being freed at one point (because "h" is freed and they
} became equal for some reason). I'm not familiar enough with the code to
} figure out how this is happening though.

This would not be unusual in the event of a popheap() -- the whole point
is to be able to discard in one go at pop everything that was allocated
since the push.

With --enable-stack-allocation, the only uses of the heap up to the
point of your segfault are for the split_env_string() function, which
means that pushheap() in createparamtable() is a no-op because no heap
space has yet been allocated to need pushing.

This breaks an assumption in popheap() that there will always be at
least one ->next in the chain that points to memory belonging to a
pushheap() or zhalloc() from somewhere farther back in the call stack.
This is harmless if only a single "arena" has been used by the time
that popheap() is called, but with a large enough chunk needed by
split_env_string() there will be a second arena with no predecessor.

This is handled in freeheap() by leaving the munmap/free for popheap.
I'm not thrilled from the standpoint of all of our work to optimize
mem.c about having to special-case this in popheap() too, but the other
option is to special-case it in pushheap() which is worse, or to find
somewhere to allocate an unnecessary fragment of memory to be an arena
that's never released.

diff --git a/Src/mem.c b/Src/mem.c
index a1744c6..8c7eb80 100644
--- a/Src/mem.c
+++ b/Src/mem.c
@@ -497,7 +497,8 @@ popheap(void)
 		    continue;
 		}
 		h->next = NULL;
-	    }
+	    } else if (hl == h)	/* This is the last arena of all */
+		hl = NULL;
 #ifdef USE_MMAP
 	    munmap((void *) h, h->size);
 #else