Open McDutchie opened 4 years ago
If you unset the array within the subshell after setting it, then the memory leak does not disappear as expected, but gets about three times as large! Very odd.
for ((i=1; i<=10; i++)); do
for ((n=1; n<=100000; n++)); do
(typeset -A foo=([a]=1 [b]=2 [c]=3); unset foo)
done
ps -p "$$" -o rss=,vsz=
done
Output on macOS:
41004 4308796
80200 4348988
119384 4388188
158564 4427356
197748 4466556
236932 4505724
276128 4545948
315324 4586140
354508 4625340
393692 4664508
Just from the comments here I would suspect the leak stems from allocating tracking records for the index labels 'a', 'b', and 'c' in the parent context, not the subshell context, that are orphaned from association with "foo" on subshell exit, preventing cleanup "unset foo" might invoke when the array is declared directly, i.e. in { } pair or inline, not ( ) one.
edit: I figured out the cause of the size difference; it's that I'm no longer compiling ksh with vmalloc. So it leaks more with vmalloc than with the OS's malloc. This is not a surprise as vmalloc was already known to be wasteful. Unfortunately nothing changed about the actual leak.
This memory leak on macOS is currently about a third of the size it used to be:
7032 4289804
11788 4292876
16552 4299020
21308 4303116
26064 4307212
30840 4312336
35584 4316432
40352 4321552
45120 4326672
49876 4330768
It still grows a lot when adding an unset
:
26080 4323596
49888 4348176
73696 4370708
97504 4395284
121316 4417812
145128 4442392
168912 4463896
192724 4488472
216528 4512024
240336 4535576
If no real fix can be found, the usual forking workaround is effective. And this is not a performance-sensitive thing; only regression tests declare associative arrays thousands of times in a loop. So I might just end up committing this.
The forking needs to be done in sh_exec() at a stage before typeset
is actually executed, because ksh processes lists of assignments in declaration builtins before it ever invokes the actual builtin.
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1062,7 +1062,11 @@ int sh_exec(register const Shnode_t *t, int flags)
if(argn)
{
if(checkopt(com,'A'))
+ {
+ if(sh.subshell && !sh.subshare)
+ sh_subfork();
flgs |= NV_ARRAY;
+ }
else if(checkopt(com,'a'))
flgs |= NV_IARRAY;
}
For what it's worth this is what ASan outputs when running the reproducer with an added unset
:
$ arch/*/bin/ksh /tmp/foo
181848 21474914836
350784 21474914836
519700 21474914836
639256 21474914836
681560 21474914836
723432 21474914836
765280 21474914836
788308 21474914836
850388 21474914836
892244 21474914836
=================================================================
==495815==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 173999942 byte(s) in 2999999 object(s) allocated from:
#0 0x7f10c9851459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
#1 0x55c93816a0c7 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:248
#2 0x55c93812a656 in newnode /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:862
#3 0x55c93812c18b in nv_search /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:1099
#4 0x55c938150f56 in nv_associative /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:1770
#5 0x55c93814d41d in nv_putsub /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:1332
#6 0x55c93814f452 in nv_endsubscript /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:1552
#7 0x55c9381d0062 in nv_create /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1068
#8 0x55c9381d2c88 in nv_open /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1433
#9 0x55c9381cc2f3 in nv_setlist /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:610
#10 0x55c93822a20d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1143
#11 0x55c9381cbafb in nv_setlist /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:563
#12 0x55c93822a20d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1143
#13 0x55c9382340c0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2080
#14 0x55c93821a524 in sh_subshell /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/subshell.c:673
#15 0x55c938233007 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1961
#16 0x55c9382361b0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2273
#17 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#18 0x55c9382340c0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2080
#19 0x55c9382361b0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2273
#20 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#21 0x55c938122736 in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:602
#22 0x55c93811ff0c in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:363
#23 0x55c93811dffd in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:45
#24 0x7f10c9498b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
Direct leak of 6000000 byte(s) in 3000000 object(s) allocated from:
#0 0x7f10c9851279 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x55c93816a041 in sh_malloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:232
#2 0x55c9381d6dee in nv_putval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:2009
#3 0x55c93812495f in nv_putv /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:151
#4 0x55c938148342 in array_putval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/array.c:703
#5 0x55c938124902 in nv_putv /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/nvdisc.c:146
#6 0x55c9381d43ab in nv_putval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1634
#7 0x55c9381d3bc7 in nv_open /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:1552
#8 0x55c9381cc2f3 in nv_setlist /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:610
#9 0x55c93822a20d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1143
#10 0x55c9381cbafb in nv_setlist /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/name.c:563
#11 0x55c93822a20d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1143
#12 0x55c9382340c0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2080
#13 0x55c93821a524 in sh_subshell /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/subshell.c:673
#14 0x55c938233007 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1961
#15 0x55c9382361b0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2273
#16 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#17 0x55c9382340c0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2080
#18 0x55c9382361b0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2273
#19 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#20 0x55c938122736 in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:602
#21 0x55c93811ff0c in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:363
#22 0x55c93811dffd in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:45
#23 0x7f10c9498b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
Direct leak of 93 byte(s) in 1 object(s) allocated from:
#0 0x7f10c9851459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
#1 0x55c93816a0c7 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:248
#2 0x55c938203c45 in path_addcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1535
#3 0x55c938204fdd in path_addpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1674
#4 0x55c9381fc567 in defpath_init /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:439
#5 0x55c9381f9f0e in onstdpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:66
#6 0x55c9381fb8c5 in path_checkdup /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:362
#7 0x55c9381fbde1 in path_nextcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:400
#8 0x55c9381fec53 in path_absolute /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:783
#9 0x55c9381fe6dc in path_search /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:725
#10 0x55c93822b035 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1230
#11 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#12 0x55c9382361b0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2273
#13 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#14 0x55c938122736 in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:602
#15 0x55c93811ff0c in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:363
#16 0x55c93811dffd in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:45
#17 0x7f10c9498b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
Indirect leak of 97 byte(s) in 1 object(s) allocated from:
#0 0x7f10c9851459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
#1 0x55c93816a0c7 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:248
#2 0x55c938203c45 in path_addcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1535
#3 0x55c938204fdd in path_addpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1674
#4 0x55c9381fc567 in defpath_init /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:439
#5 0x55c9381f9f0e in onstdpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:66
#6 0x55c9381fb8c5 in path_checkdup /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:362
#7 0x55c9381fbde1 in path_nextcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:400
#8 0x55c9381fec53 in path_absolute /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:783
#9 0x55c9381fe6dc in path_search /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:725
#10 0x55c93822b035 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1230
#11 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#12 0x55c9382361b0 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2273
#13 0x55c93823413d in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:2084
#14 0x55c938122736 in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:602
#15 0x55c93811ff0c in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:363
#16 0x55c93811dffd in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:45
#17 0x7f10c9498b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
SUMMARY: AddressSanitizer: 180000132 byte(s) leaked in 6000001 allocation(s).
If no real fix can be found, the usual forking workaround is effective. And this is not a performance-sensitive thing; only regression tests declare associative arrays thousands of times in a loop. So I might just end up committing this.
@McDutchie Not sure if I understand completely the impact of your potential patch at https://github.com/ksh93/ksh/issues/94#issuecomment-1002397084 would have on ksh's behavior and performance. Would this patch introduce a difference in what kind of subshell [ forked/non-forked, {}/() , subshare/subfork ] is created when using an associative array?
Perhaps it is just removing some of ksh's optimization logic to not create a new fork as when the (ulimit -t unlimited; ...)
technique is used to force a forked subshell and not have ksh create a subshare instead.
{
unset foo
for ((i=1; i<=3; i++)); do
for ((n=1; n<=1000; n++)); do
#{ typeset -A foo=([a]=1 [b]=2 [c]=3); } # not forked
( typeset -A foo=([a]=1 [b]=2 [c]=3); ) # changing to forked
#( ulimit -t unlimited; typeset -A foo=([a]=1 [b]=2 [c]=3); ) # forked
done
ps -p "$$" -o rss=,vsz=
done
typeset -p foo
}
@McDutchie After reading your descriptive comments and your suggestion to add in an option to always fork subshells, in my opinion that works to allow ksh users to transition their scripts from using ( ... )
to { ...; }
or alter their scripts appropriately with a known behavior of ( ... )
in ksh by using the new subshell forking option.
Also based on your descriptive comments and insights, this subshell forking option will go away as command groupings (The New Kornshell, p173) of the subshell grouping type, aka ( ... )
, will always be forked into a new process and thus provide a more safe, secured, and enforced separation from the current environment. Brace groupings, aka { ...; }
/subshares, stay the same with their commands executed in the current environment.
I have been researching into why associative arrays have such memory leaks whereas indexed arrays and compound variables do not show such memory leaks in subshells.
{
unset arr
for ((i=1; i<=3; i++)); do
for ((n=1; n<=10000; n++)); do
#( ulimit -t unlimited; typeset -A arr=([a]=1 [b]=2 [c]=3); ) # forked, works
( typeset -A arr=([a]=1 [b]=2 [c]=3); ) # leak
#( typeset -a arr=(a b c); ) # works
#( typeset -C arr=(a=1;b=2;c=3); ) # works
done
ps -p "$$" -o rss=,vsz= # numbers should be stable
done
typeset -p arr
}
I found a 2015-02-10 email from David Korn entitled, "Re: [ast-users] [WORKING PATCH] Leak on unset of associative array" with a patch that has been applied to ksh93u+m. It is my thought that ksh93's associative array logic originally did not possess removal/cleanup logic based on a Paulo César Pereira de Andrade's submitted patch. Also, I believe Paulo's patch may not have accounted for subshells as that was not in his provided test case. Since index arrays and compound variables work, I was going to focus on why those work and and possibility adjust those code part(s) in support of associative arrays. Additionally, it may be that Paulo's patch needs adjustment to reference a subshell's namespace/dictionary. (https://github.com/ksh93/ksh/blob/4491bc6a991759b61c5f7d49dd0477a2f2bfb1a1/src/cmd/ksh93/sh/name.c#L1294-L1309
Thanks for reminding me of that, @hyenias.
I took a closer look and inserted some debug output calls:
--- a/src/cmd/ksh93/sh/name.c
+++ b/src/cmd/ksh93/sh/name.c
@@ -1293,11 +1293,14 @@ void nv_delete(Namval_t* np, Dt_t *root, int flags)
{
if(dtdelete(root,np))
{
+error(ERROR_warn(0),"[DEBUG] one: %s",np->nvname);
if(!(flags&NV_NOFREE) && ((flags&NV_FUNCTION) || !nv_subsaved(np,flags&NV_TABLE)))
{
Namarr_t *ap;
+error(ERROR_warn(0),"[DEBUG] two");
if(nv_isarray(np) && np->nvfun && (ap=nv_arrayptr(np)) && array_assoc(ap))
{
+error(ERROR_warn(0),"[DEBUG] three");
/* free associative array from memory */
while(nv_associative(np,0,NV_ANEXT))
nv_associative(np,0,NV_ADELETE);
With this reproducer:
for ((i=1; i<=10; i++)); do
for ((n=1; n<=100000; n++)); do
(typeset -A foo=([a]=1 [b]=2 [c]=3))
done
ps -p "$$" -o rss=,vsz=
done
…the first ten lines of the output are:
test00.sh: line 3: warning: [DEBUG] one: a
test00.sh: line 3: warning: [DEBUG] two
test00.sh: line 3: warning: [DEBUG] one: b
test00.sh: line 3: warning: [DEBUG] two
test00.sh: line 3: warning: [DEBUG] one: c
test00.sh: line 3: warning: [DEBUG] two
test00.sh: line 3: warning: [DEBUG] one: a
test00.sh: line 3: warning: [DEBUG] two
test00.sh: line 3: warning: [DEBUG] one: b
test00.sh: line 3: warning: [DEBUG] two
With this reproducer (unset
added):
for ((i=1; i<=10; i++)); do
for ((n=1; n<=100000; n++)); do
(typeset -A foo=([a]=1 [b]=2 [c]=3); unset foo)
done
ps -p "$$" -o rss=,vsz=
done
…the first ten lines of the output are:
test00.sh[3]: unset: warning: [DEBUG] one: a
test00.sh[3]: unset: warning: [DEBUG] one: b
test00.sh[3]: unset: warning: [DEBUG] one: c
test00.sh[3]: unset: warning: [DEBUG] one: a
test00.sh[3]: unset: warning: [DEBUG] one: b
test00.sh[3]: unset: warning: [DEBUG] one: c
test00.sh[3]: unset: warning: [DEBUG] one: a
test00.sh[3]: unset: warning: [DEBUG] one: b
test00.sh[3]: unset: warning: [DEBUG] one: c
test00.sh[3]: unset: warning: [DEBUG] one: a
With this reproducer (no subshell, does not leak):
for ((i=1; i<=10; i++)); do
for ((n=1; n<=100000; n++)); do
typeset -A foo=([a]=1 [b]=2 [c]=3); unset foo
done
ps -p "$$" -o rss=,vsz=
done
…the first ten lines of the output are:
test00.sh[3]: unset: warning: [DEBUG] one: a
test00.sh[3]: unset: warning: [DEBUG] two
test00.sh[3]: unset: warning: [DEBUG] one: b
test00.sh[3]: unset: warning: [DEBUG] two
test00.sh[3]: unset: warning: [DEBUG] one: c
test00.sh[3]: unset: warning: [DEBUG] two
test00.sh[3]: unset: warning: [DEBUG] one: a
test00.sh[3]: unset: warning: [DEBUG] two
test00.sh[3]: unset: warning: [DEBUG] one: b
test00.sh[3]: unset: warning: [DEBUG] two
nv_delete()
gets called for the array members, but never for the array itself.
three
is not output for any of the reproducers, so lines 1301-1305 are never executed at all. They're dead code, no matter what version of the reproducer you try: with or without unset
, with or without using a subshell at all.
So, something in commits 461a1aebc1eb8597f1a1ec6cd2b9e3d6d150d156 and/or e70925ce1033790b95e7c48af78a4134ac22dd2e is very broken.
Commit
three
is not output for any of the reproducers, so lines 1301-1305 are never executed at all. They're dead code, no matter what version of the reproducer you try: with or withoutunset
, with or without using a subshell at all.
They're not dead code, though. If we delete those lines, the regression test introduced in e70925ce1033790b95e7c48af78a4134ac22dd2e fails: https://github.com/ksh93/ksh/blob/4491bc6a991759b61c5f7d49dd0477a2f2bfb1a1/src/cmd/ksh93/tests/leaks.sh#L155-L167 That regression test unsets an associative array that is a member of another associative array. So that's what that code is for. The comment should be updated for clarity. (It may also be for unsetting an associative array that is a member of an indexed array; I haven't tested that yet.)
Unfortunately, a forking workaround for typeset -A
is insufficient because the memory leak also occurs if we remove the typeset -A
and implicitly declare an associative array using an assignment.
for ((i=1; i<=10; i++)); do
for ((n=1; n<=100000; n++)); do
(foo=([a]=1 [b]=2 [c]=3))
done
ps -p "$$" -o rss=,vsz=
done
6696 4287132
11460 4292252
16220 4297372
20980 4301468
25756 4307612
30516 4311712
35272 4315808
40028 4319904
44792 4326048
49556 4331168
for ((i=1; i<=10; i++)); do
for ((n=1; n<=100000; n++)); do
(foo=([a]=1 [b]=2 [c]=3); unset foo)
done
ps -p "$$" -o rss=,vsz=
done
25768 4315804
49572 4338336
73384 4361892
97180 4385444
120980 4408996
144800 4433576
168608 4456104
192412 4480680
216220 4504232
240040 4527784
FWIW the first version of ksh93 to exhibit this memory leak is ksh93s- 2006-09-12, which is the version that adds typeset -a
for indexed arrays (note that unsetting an associative array in this version makes no difference for the memory leak).
I've found that this change in ksh93s- is what introduced the memory leak for associative arrays in subshells:
--- b/src/cmd/ksh93/sh/name.c
+++ a/src/cmd/ksh93/sh/name.c
@@ -291,8 +291,7 @@ void nv_setlist(register struct argnod *arg,register int flags)
sh.prefix = prefix;
if(nv_isarray(np) && (mp=nv_opensub(np)))
np = mp;
- if(nv_isnull(np))
- nv_setvtree(np);
+ nv_setvtree(np);
continue;
}
cp = arg->argval;
Reverting that change fixes the leak in 93s-, but the code appears to have changed substantially since then (the diff doesn't apply cleanly to 93u+m). Regardless, it may help provide clues for fixing the memory leak.
It looks like @atheik found an effective fix. Many thanks!
diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index b5965170..308c2b66 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -353,6 +353,8 @@ static void nv_restore(struct subshell *sp)
if(nv_isarray(mp))
nv_putsub(mp,NIL(char*),ARRAY_SCAN);
nofree = mp->nvfun?mp->nvfun->nofree:0;
+ if(mp->nvalue.cp!=Empty && np->nvalue.cp==Empty)
+ nv_offattr(mp,NV_NOFREE);
_nv_unset(mp,NV_RDONLY|NV_CLONE);
if(nv_isarray(np))
{
Well, it's effective for the original reproducer, but there is still a leak when the unset
is added.
It leaks in different ways when unset
is added or when passing -i
to typeset
.
Here's one way to fix these leaks: Edit: This is broken.
diff --git a/src/cmd/ksh93/bltins/typeset.c b/src/cmd/ksh93/bltins/typeset.c
index e1131990..92f7eb77 100644
--- a/src/cmd/ksh93/bltins/typeset.c
+++ b/src/cmd/ksh93/bltins/typeset.c
@@ -1373,7 +1373,21 @@ static int unall(int argc, char **argv, register Dt_t *troot)
nofree_attr = nv_isattr(np,NV_NOFREE); /* note: returns bitmask, not boolean */
if(!nv_isnull(np) || nv_size(np) || nv_isattr(np,~(NV_MINIMAL|NV_NOFREE)))
+ {
+ Namarr_t *ap;
+ if(sh.subshell && nv_isattr(np,NV_ARRAY|NV_NOFREE)==NV_ARRAY && (ap=nv_arrayptr(np)) && array_assoc(ap))
+ {
+ Namval_t *onp = nv_associative(np,0,NV_ACURRENT);
+ while(nv_associative(np,0,NV_ANEXT))
+ {
+ Namval_t *mp = nv_associative(np,0,NV_ACURRENT);
+ if(mp && mp->nvalue.cp && mp->nvalue.cp!=Empty)
+ nv_offattr(mp,NV_NOFREE);
+ }
+ nv_associative(np,(char*)onp,NV_ASETSUB);
+ }
_nv_unset(np,0);
+ }
if(troot==sh.var_tree && sh.st.real_fun && (dp=sh.var_tree->walk) && dp==sh.st.real_fun->sdict)
nv_delete(np,dp,NV_NOFREE);
else if(isfun)
diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c
index b0c085d5..0224bb84 100644
--- a/src/cmd/ksh93/sh/name.c
+++ b/src/cmd/ksh93/sh/name.c
@@ -3056,6 +3056,8 @@ void nv_newattr (register Namval_t *np, unsigned newatts, int size)
free(cp);
cp = 0;
}
+ if(sh.subshell && nv_isattr(np,NV_ARRAY|NV_RDONLY)==NV_ARRAY && sp && sp!=Empty && sp!=Null)
+ free((void*)sp);
}
while(ap && nv_nextsub(np));
#if SHOPT_FIXEDARRAY
diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index e0bab084..e7cd0c41 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -352,6 +352,16 @@ static void nv_restore(struct subshell *sp)
if(nv_isarray(mp))
nv_putsub(mp,NIL(char*),ARRAY_SCAN);
nofree = mp->nvfun?mp->nvfun->nofree:0;
+ if(np->nvalue.cp==Empty)
+ {
+ if(nv_isnull(mp) && !nv_isvtree(np))
+ {
+ free((void*)mp);
+ goto skip;
+ }
+ if(mp->nvalue.cp && mp->nvalue.cp!=Empty)
+ nv_offattr(mp,NV_NOFREE);
+ }
_nv_unset(mp,NV_RDONLY|NV_CLONE);
if(nv_isarray(np))
{
diff --git a/src/cmd/ksh93/tests/leaks.sh b/src/cmd/ksh93/tests/leaks.sh
index 05d8040b..fcb0d5cd 100755
--- a/src/cmd/ksh93/tests/leaks.sh
+++ b/src/cmd/ksh93/tests/leaks.sh
@@ -167,9 +167,15 @@ DO
DONE
# https://github.com/ksh93/ksh/issues/94
-TEST title='defining associative array in subshell' known=y url=https://github.com/ksh93/ksh/issues/94
+TEST title='defining associative array in subshell'
DO
(typeset -A foo=([a]=1 [b]=2 [c]=3))
+ (typeset -Ai foo=([a]=1 [b]=2 [c]=3))
+DONE
+TEST title='defining and unsetting associative array in subshell'
+DO
+ (typeset -A foo=([a]=1 [b]=2 [c]=3); unset foo)
+ (typeset -Ai foo=([a]=1 [b]=2 [c]=3); unset foo)
DONE
# ======
The change to nv_newattr()
deals with the leak that happens when passing -i
to typeset
.
The change to unall()
is needed since part of the unset
leak happens before nv_restore()
.
Maybe this can help people find the proper way to fix these leaks.
I relied on bin/shtests
having good coverage.
Unfortunately, on my system that patch makes two tests (in functions.sh and namespace.sh) crash with attempts to free unallocated pointers. The stack trace is the same in both cases:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_kernel.dylib 0x00007fff70d392c2 __pthread_kill + 10
1 libsystem_pthread.dylib 0x00007fff70df4bf1 pthread_kill + 284
2 libsystem_c.dylib 0x00007fff70ca3745 __abort + 144
3 libsystem_c.dylib 0x00007fff70ca36b5 abort + 142
4 libsystem_malloc.dylib 0x00007fff70db2077 malloc_vreport + 545
5 libsystem_malloc.dylib 0x00007fff70db1e38 malloc_report + 151
6 ksh 0x000000010dffe178 _nv_unset + 1496 (name.c:2553)
7 ksh 0x000000010dfaff1a nv_putv + 586
8 ksh 0x000000010dfc4b1b array_putval + 1483 (array.c:693)
9 ksh 0x000000010dfafed8 nv_putv + 520 (nvdisc.c:145)
10 ksh 0x000000010dffdf6e _nv_unset + 974 (name.c:2520)
11 ksh 0x000000010e021ca0 nv_restore + 464 (subshell.c:366)
12 ksh 0x000000010e020903 sh_subshell + 3203 (subshell.c:694)
13 ksh 0x000000010dff1ef6 comsubst + 2678 (macro.c:2182)
14 ksh 0x000000010dff2a91 varsub + 1425 (macro.c:1183)
15 ksh 0x000000010dfef3cc copyto + 3308 (macro.c:621)
16 ksh 0x000000010dfee620 sh_mactrim + 416 (macro.c:171)
17 ksh 0x000000010dffa95e nv_setlist + 638 (name.c:297)
18 ksh 0x000000010e0256ab sh_exec + 3819 (xec.c:1106)
19 ksh 0x000000010e02922a sh_exec + 19050 (xec.c:2072)
20 ksh 0x000000010e0299ae sh_exec + 20974 (xec.c:2192)
21 ksh 0x000000010e02922a sh_exec + 19050 (xec.c:2072)
22 ksh 0x000000010e02a365 sh_exec + 23461 (xec.c:2338)
23 ksh 0x000000010dfaf29f exfile + 3247 (main.c:605)
24 ksh 0x000000010dfae29f sh_main + 3551 (main.c:367)
25 ksh 0x000000010df936e6 main + 38 (pmain.c:45)
26 libdyld.dylib 0x00007fff70bfe3d5 start + 1
Meanwhile I think I've found a forking workaround that works reliably – it must be done in nv_associative() for the NV_AINIT action which initialises an associative array.
diff --git a/src/cmd/ksh93/sh/array.c b/src/cmd/ksh93/sh/array.c
index b103ce88b..7c7153d62 100644
--- a/src/cmd/ksh93/sh/array.c
+++ b/src/cmd/ksh93/sh/array.c
@@ -1656,6 +1656,8 @@ void *nv_associative(register Namval_t *np,const char *sp,int mode)
switch(mode)
{
case NV_AINIT:
+ if(sh.subshell && !sh.subshare)
+ sh_subfork(); /* work around https://github.com/ksh93/ksh/issues/94 */
ap = (struct assoc_array*)sh_calloc(1,sizeof(struct assoc_array));
ap->header.table = dtopen(&_Nvdisc,Dtoset);
ap->cur = 0;
diff --git a/src/cmd/ksh93/tests/leaks.sh b/src/cmd/ksh93/tests/leaks.sh
index 05d8040b1..eaa3d3f14 100755
--- a/src/cmd/ksh93/tests/leaks.sh
+++ b/src/cmd/ksh93/tests/leaks.sh
@@ -167,9 +167,14 @@ DO
DONE
# https://github.com/ksh93/ksh/issues/94
-TEST title='defining associative array in subshell' known=y url=https://github.com/ksh93/ksh/issues/94
+TEST title='defining associative array in subshell'
DO
(typeset -A foo=([a]=1 [b]=2 [c]=3))
+ (typeset -Ai foo=([a]=1 [b]=2 [c]=3))
+ (typeset -A foo=([a]=1 [b]=2 [c]=3); unset foo)
+ (typeset -Ai foo=([a]=1 [b]=2 [c]=3); unset foo)
+ (foo=([a]=1 [b]=2 [c]=3))
+ (foo=([a]=1 [b]=2 [c]=3); unset foo)
DONE
# ======
Edit: I spoke too soon. That patch causes regressions:
test statics begins at 2022-06-04+08:43:05
statics.sh[635]: FAIL: associative_compound_array_editelement: Expected stdout == a=$'4, b=5\n;a=4, b=5\n;', got $';a=4, b=5\n;a=3, b=4\n;a=2, b=3\n;;a=4, b=5\n;a=3, b=4\n;a=2, b=3\n;'
statics.sh[636]: FAIL: associative_compound_array_editelement: Expected empty stderr, got $'/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[15]: ar: line 11: s[5].a++, s[5].b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[15]: ar: line 11: s[5].a++, s[5].b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[15]: ar: line 11: s[5].a++, s[5].b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[16]: ar: line 11: s[5].a++, s[5].b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[16]: ar: line 11: s[5].a++, s[5].b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[16]: ar: line 11: s[5].a++, s[5].b++ : parameter not set'
statics.sh[635]: FAIL: associative_compound_array_nameref_editelement: Expected stdout == a=$'4, b=5\n;a=4, b=5\n;', got a=$'4, b=5\n;;a=4, b=5\n;;'
statics.sh[636]: FAIL: associative_compound_array_nameref_editelement: Expected empty stderr, got $'/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[21]: ar[19]: ar_n: line 6: sn.a++, sn.b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[21]: ar[19]: ar_n: line 6: sn.a++, sn.b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[21]: ar[19]: ar_n: line 6: sn.a++, sn.b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[22]: ar[19]: ar_n: line 6: sn.a++, sn.b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[22]: ar[19]: ar_n: line 6: sn.a++, sn.b++ : parameter not set\n/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh[22]: ar[19]: ar_n: line 6: sn.a++, sn.b++ : parameter not set'
test statics failed at 2022-06-04+08:43:05 with exit code 4 [ 7 tests 4 errors ]
When doing this in a virtual/non-forked subshell:
a memory leak occurs. The memory occupied by the array is not freed when exiting the subshell.
A reproducer with
ps
can confirm this bug exists on 93u+ 2012-08-01:Output on macOS:
Reproducer with
vmstate
,which is compiled into 93u+m by default(edit: as of f9364b17098ca05dc6ab886d8906ee55bfb2126c, you have to pass-D_AST_vmalloc
to enable it):Output (note increasing
busy
and decreasingfree
):