Bug 1189294

Summary: Leak on unset of associative array
Product: Red Hat Enterprise Linux 6 Reporter: Paulo Andrade <pandrade>
Component: kshAssignee: Michal Hlavinka <mhlavink>
Status: CLOSED ERRATA QA Contact: Martin Kyral <mkyral>
Severity: high Docs Contact:
Priority: urgent    
Version: 6.7CC: btotty, fkrska, mhlavink, mkyral, ovasik
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: ksh-20120801-24.el6 Doc Type: Bug Fix
Doc Text:
Previously, after the user unset an associative array, the system did not free the newly-available memory. Consequently, ksh consumed more and more memory over time. The underlying source code has been modified to free the memory after the user unsets an associative array, thus fixing this problem.
 Story Points: ---
Clone Of: Environment:
Last Closed: 2015-07-22 06:56:34 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1189297    
Bug Blocks:    
Attachments:
Description Flags
test3.ksh
none
ksh-20120801-assoc-unset-leak.patch
none
ksh-20120801-assoc-unset-leak.patch
none
ksh-20120801-assoc-unset-leak.patch none

Description Paulo Andrade 2015-02-04 22:13:50 UTC 
Created attachment 988311 [details]
test3.ksh

The problem with a possible patch has been report upstream at:
http://lists.research.att.com/pipermail/ast-users/2015q1/004723.html

On the sample test case, every iteration of the loop
leaks some memory in src/cmd/ksh93/sh/array.c:

---8<---
/*
 *  This is the default implementation for associative arrays
 */
void *nv_associative(register Namval_t *np,const char *sp,int mode)
{
	register struct assoc_array *ap = (struct assoc_array*)nv_arrayptr(np);
	register int type;
	switch(mode)
	{
	    case NV_AINIT:
		if(ap = (struct assoc_array*)calloc(1,sizeof(struct assoc_array)))
---8<---

that calloc is leaking memory. After a few tests, I noticed
that if using "unset -f" in the test case, the leak would
no longer happen. But that is not supposed to be the way
to fix it, nor leaks are supposed to happen if not using it.

The proposed fix was done observing that in
src/cmd/ksh93/bltins/typeset.c, when using the -f option,
it would have this code:

...
		case 'f':
			troot = sh_subfuntree(1);
...
otherwise, it would have shp->var_tree passed as argument,
or set again to it if using the 'n' or 'v' unset options.

But if troot==shp->var_tree, it would continue in either
of these:

---8<---
#if SHOPT_FIXEDARRAY
				if((ap=nv_arrayptr(np)) && !ap->fixed  && name[strlen(name)-1]==']' && !nv_getsub(np))
#else
				if(nv_isarray(np) && name[strlen(name)-1]==']' && !nv_getsub(np))
#endif /* SHOPT_FIXEDARRAY */
---8<---

The email to upstream asks for the reason of the continue,
as it is causing a leak, so whatever is the reason, it is
buggy, and no test case exercises whatever it prevents, as
with or without the proposed patch, %check results are
basically the same.
Comment 1 Paulo Andrade 2015-02-04 22:15:07 UTC 
Created attachment 988312 [details]
ksh-20120801-assoc-unset-leak.patch

This patch corrects the problem.
Comment 3 Paulo Andrade 2015-02-05 00:29:29 UTC 
Created attachment 988343 [details]
test3.ksh

Previous reproducer was using "unset -f" (from a debug test),
what hides the problems.
Comment 4 Paulo Andrade 2015-02-09 11:35:27 UTC 
Created attachment 989652 [details]
ksh-20120801-assoc-unset-leak.patch

This is I believe the best approach to correct the
problem. And from the alternatives I tried, the one
with less test regressions. But it is a starting
point, due to these failures in %check, that I
believe are another bug:

---8<---
arith.sh[708]: typeset: cannot change associative array A to index array

    arrays.sh[84]: number of elements of y is not 6
    arrays.sh[87]: string length of unset element is not 0
    arrays.sh[106]: number of elements of left in variable foo is not 0
    arrays.sh[114]: number of elements of left in variable foo again is not 0
    arrays.sh[395]: ${!var[sub]} should be var[sub]
    arrays.sh[427]: subscript not evaluated for unset variable
arrays.sh[430]: typeset: cannot change associative array foo to index array

    bracket.sh[309]: ${foo[1]+x} should be empty
    bracket.sh[311]: bar[1] should not be set
    bracket.sh[315]: z[1] should not be set
    bracket.sh[320]: y[1] should not be set
---8<---

If I understand correctly, the above are mostly a side
effect of previously something like:

unset foo[bar]

straight releasing toplevel "foo"  (and leaking)
instead of releasing "foo[bar]".
Comment 5 Paulo Andrade 2015-02-10 20:51:50 UTC 
Created attachment 990258 [details]
ksh-20120801-assoc-unset-leak.patch

This patch has no regressions in the ksh test cases,
but may still be incomplete for special cases. It
releases memory just before it would become unreachable.

Posted upstream at
http://lists.research.att.com/pipermail/ast-users/2015q1/004729.html
Comment 6 Paulo Andrade 2015-02-12 13:42:48 UTC 
For the record, David Korn told me in a private email there
could be issues with indexed arrays. Basically, using
"typeset -a" and numeric indexes, instead of "typeset -A"
and symbolic indexes in the test case. I tested it and
indexed arrays do not leak, neither need the patch.
So, I do not know of other special cases that could
leak, and need patch customizations.
Comment 16 errata-xmlrpc 2015-07-22 06:56:34 UTC 
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-1450.html