Re: Bug#245678: zsh: built-in rm -rf fills up the memory

[Peter Stephenson <pws@xxxxxxx>]

Bart Schaefer wrote:
> } [ h->used + (new - old) > HEAP_ARENA_SIZE ]
> } 63432 + (63504 - 63432) > 16360
> 
> Aha!  Note that h->used > HEAP_ARENA_SIZE all by itself!
> 
> Comparing to HEAP_ARENA_SIZE is likely wrong, it should compare to the 
> maximum of either HEAP_ARENA_SIZE or the previously-mmapped page size.

This is all ghastly even by zsh standards.

I thought what you thought, until... Look at zhalloc(), where either we
allocate n bytes normally, or we mmap an equivalent chunk of memory.
Neither of those explicitly use HEAP_ARENA_SIZE, which was used to get n
in the first place.

We are probably wasting space since the only thing we record for later
use is the original size asked for (`size') as the used size; we don't
record the number we've ended up with (`n'), goodness knows why.

For USE_MMAP, we do record n in h->size, but this is only used for
munmap, not for everything else --- it's obviously an afterthought.
(That's why it's zero when USE_MMAP isn't defined.)  The smart thing to
do would be always record it and use it for subsequent calculation
instead of all the jiggery pokery with rounding things to boundaries in
obscure ways.  This is basically your conclusion, however I think it's
just unfinished business rather than a programming error.

However, I don't think that's actually the source of the problem.
Closer to the point is this:

#ifndef USE_MMAP
	if (old > HEAP_ARENA_SIZE || new > HEAP_ARENA_SIZE) {
	    size_t n = HEAP_ARENA_SIZE > new ? HEAPSIZE : new + sizeof(*h);

	    if (ph)
		ph->next = h = (Heap) realloc(h, n);
	    else
		heaps = h = (Heap) realloc(h, n);
	}
	h->used = new;
	unqueue_signals();
	return arena(h);
#endif

Without USE_MMAP, this takes care of making sure the heap is the right
size when it's larger than normal.  Those tests would be triggering in
the case we're looking at.  As they're not, we're dropping through to
the zhalloc() below every time.

What we should do is the equivalent of realloc as an #else of the code
above --- mmap a larger chunk, unmap the old one, and return the new
one.

However, I don't think even *that* is the basic problem.  I think this:

    old = (old + H_ISIZE - 1) & ~(H_ISIZE - 1);
    new = (new + H_ISIZE - 1) & ~(H_ISIZE - 1);

is wrong --- it should be the heap size here, not H_ISIZE which is
simply sizeof(union mem_align), which is probably only 4 or 8 words.  We
only need to reallocate to heap size boundaries, i.e. 16384 possibly
minus something or other.  We've got away with it before because the
realloc() quoted above isn't too fussed, an hrealloc() isn't used that
often.  So actually Clint's original proposal is close to the mark.

I hope.

I might be tempted by proposed solutions involving the complete removal
of hrealloc.

Please don't use sentences containing `why' and ending with `?' or I'll
start to whimper.  Anyone fixing this could useful add comments.

-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************