Fix a crash in the shbench fibonacci benchmark

[JohnoKing]

If the fibslow function in the shbench fibonacci benchmark is run with an argument greater than 20, ksh will crash with a memory fault. Below is a diff that can be applied to shbench for this crash to occur:

--- a/bench/fibonacci.ksh
+++ b/bench/fibonacci.ksh
@@ -55,6 +55,6 @@ typeset -i para=19
 ((penalty > 0)) || penalty=1
 [[ $refshell == zsh ]] && ((penalty *= 3))
 typeset -i num=$((para - penalty/3))
-r1=$(fibslow $num)
+r1=$(fibslow 21)
 r2=$(fibfast $num)
 [[ $r1 == "$r2" ]] || exit 1

$ ksh ./bench/fibonacci.ksh
Memory fault

This was caused by subshell numbers only being 16-bits long, which isn't big enough when running a large number of subshells. Extending the subshell number length to 64-bit fixes this issue (${.sh.subshell} doesn't affect the crash, so it's unchanged), but with vmalloc a similar crash still occurs. The fibonacci benchmark no longer crashes with the above diff, but the regression test this pull request adds only works with standard malloc. With ksh93u+ vmalloc the following script still crashes:

$ cat ./vmalloc-subshell-crash.ksh
#!/bin/ksh
(
    for ((n=0; n != 50000; n++)) do
        (
            function foo {
                (true)
            }
            foo
         )
    done
)
$ ksh ./vmalloc-subshell-crash.ksh
Memory fault

[McDutchie]

I've a strong feeling that this fix is a kludge that doesn't address the root cause.

Sure enough, upon reading the sh_subshell() function, we find a global static int subenv, a virtual subshell ID that is increased upon entering a virtual subshell, then never decreased upon leaving it. So that is eventually going to overflow, as the following debug command can show:

diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index 5b12f92..aefb681 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -481,6 +481,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
 	sh_pushcontext(shp,&buff,SH_JMPSUB);
 	shp->subshell++;		/* increase level of virtual subshells */
 	SH_SUBSHELLNOD->nvalue.s++;	/* increase ${.sh.subshell} */
+errormsg(SH_DICT,ERROR_warn(0),"[DEBUG] subenv == %d; shp->subshell == %d", subenv, shp->subshell);
 	sp->prev = subshell_data;
 	sp->shp = shp;
 	sp->sig = 0;

An int is 32 bits long on modern systems (31 bits for positive values), so that overflow would take a fairly long time. But the curenv and jobenv subshell structure members, which are both based on this int value, are defined as (signed) shorts, 16 bits (15 for positive values). So that's wrong; they should be the same type. An overflow happens after entering and leaving 2^15=32767 virtual subshells.

So the OpenSUSE fix simply delays the overflow, albeit by a very very long time.

The overflow doesn't cause a crash when executing >32767 subshells sequentially, but when you nest subshells (as in your reproducer), a crash can occur. Which makes sense, as a child virtual subshell would use an overflowed negative value inconsistent with that of the parent.

However, ksh is not multithreaded, and never will be, so virtual subshells are never executed simultaneously. So a fix that addresses the root cause is to simply decrease the global subenv variable again when leaving a virtual subshell. The following change should be the minimum necessary to fix the Fibonacci test crash:

diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
index 5b12f92..aefb681 100644
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@@ -756,6 +757,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
 	{
 		shp->subshell--;		/* decrease level of virtual subshells */
 		SH_SUBSHELLNOD->nvalue.s--;	/* decrease ${.sh.subshell} */
+		subenv--;
 	}
 	subshell_data = sp->prev;
 	if(!argsav  ||  argsav->dolrefcnt==argcnt)

This causes no regression test failures.

When applying both this diff and the debug diff, it becomes clear that subenv and shp->subshell now always have the same value. So it would be worth trying to eliminate subenv altogether and just use shp->subshell, which is the same as the global sh.subshell.

Then we still need to fix the variable types, but long is excessive. After this fix I think it should be more than enough to make them unsigned 32-bit integers, uint32_t, allowing for 2^32=4294967296 levels of nested virtual subshells. This would avoid a performance degradation on 32-bit systems.

[JohnoKing]

I'm getting inconsistent CI test failures after making the variables unsigned values:
https://github.com/JohnoKing/ksh/runs/973100438
https://github.com/JohnoKing/ksh/runs/973063175

I'll be reverting that change since fixing that issue will likely require refactoring code, which I'd rather avoid.

edit: The test failures may be caused by something else, see the comment below.

[JohnoKing]

... And the CI test are failing again, with the same test failure: https://github.com/JohnoKing/ksh/runs/973205340

test subshell begins at 2020-08-11+21:32:50
	subshell.sh[532]: exit status from subshells not being preserved
test subshell failed at 2020-08-11+21:32:58 with exit code 1 [ 87 tests 1 error ]
test subshell(C.UTF-8) begins at 2020-08-11+21:32:58
test subshell(C.UTF-8) passed at 2020-08-11+21:33:05 [ 87 tests 0 errors ]
test subshell(shcomp) begins at 2020-08-11+21:33:05
	shcomp-subshell.ksh[532]: exit status from subshells not being preserved
test subshell(shcomp) failed at 2020-08-11+21:33:13 with exit code 1 [ 87 tests 1 error ]

I'll have to debug this more, although it will take a while. I'll see if I can reproduce the test failure when curenv, jobenv and p_env are long, since as of now it has only occurred with unsigned int and int.

[McDutchie]

I cannot reproduce that failure at all, on macOS, FreeBSD, Linux (Debian), or HP-UX. I'm using the diff with the unsigned int types. Everything appears to work perfectly.

However, I did find a potential problem: sh.subshell (a.k.a. shp->subshell), etc. is still a short. That should be an unsigned int as well, plus all the variables in various places where that value is saved and restored.

[JohnoKing]

I cannot reproduce that failure at all, on macOS, FreeBSD, Linux (Debian), or HP-UX. I'm using the diff with the unsigned int types. Everything appears to work perfectly.

As far as I'm aware the test failure only happens in the CI build. I have done more testing with the CI though and it seems the subshell test failure in CI only occurs after applying the subenv-- change. When I revert that the CI test failure goes away completely regardless of the data type used, yet with subenv-- on an otherwise clean branch the CI build failed on my first try (ref https://github.com/JohnoKing/ksh/runs/973509718):

test subshell begins at 2020-08-11+23:18:28
test subshell passed at 2020-08-11+23:18:36 [ 86 tests 0 errors ]
test subshell(C.UTF-8) begins at 2020-08-11+23:18:36
	subshell.sh[532]: exit status from subshells not being preserved
test subshell(C.UTF-8) failed at 2020-08-11+23:18:44 with exit code 1 [ 86 tests 1 error ]
test subshell(shcomp) begins at 2020-08-11+23:18:44
test subshell(shcomp) passed at 2020-08-11+23:18:52 [ 86 tests 0 errors ]

It could just be a timing issue, but I suspect ksh depends on subenv not decrementing after a subshell finishes in the following block of code (where shp->curenv is set to subenv):

ksh/src/cmd/ksh93/sh/subshell.c

Lines 474 to 479 in f485fe0

	if(shp->curenv==0)
	{
	subshell_data=0;
	subenv = 0;
	}
	shp->curenv = ++subenv;

On an unrelated note, I got another test failure in the CI build (ref https://github.com/JohnoKing/ksh/runs/973581717). This has occurred in some of my older pull requests as well, so it's not caused by the subshell changes:

test sigchld begins at 2020-08-11+23:46:57
	sigchld.sh[89]: SIGCHLD trap queueing failed -- expected 'running=0 maxrunning=4', got 'running=1 maxrunning=4'
test sigchld failed at 2020-08-11+23:47:01 with exit code 1 [ 14 tests 1 error ]
test sigchld(C.UTF-8) begins at 2020-08-11+23:47:01
test sigchld(C.UTF-8) passed at 2020-08-11+23:47:06 [ 14 tests 0 errors ]
test sigchld(shcomp) begins at 2020-08-11+23:47:06
test sigchld(shcomp) passed at 2020-08-11+23:47:11 [ 14 tests 0 errors ]

However, I did find a potential problem: sh.subshell (a.k.a. shp->subshell), etc. is still a short. That should be an unsigned int as well, plus all the variables in various places where that value is saved and restored.

I applied that change in 4b9dbe1, but right now it has been reverted (edit: I've reapplied it to the pull request). The length of shp->subshell from my testing doesn't affect the CI test failure.

[McDutchie]

Hmm. What fails in the CI run with the subenv-- change is the wait builtin, not the (forked, asynchronous) subshell itself. wait is unable to obtain the exit status from the background job.

I don't yet understand the code responsible for doing that, but it must be ultimately dependent on subenv being a unique ID over time. Which does make some sort of sense.

What I don't understand at all is why it doesn't fail on any other system I can get my hands on and only fails on the CI runners.

And without that decrement it is eventually going to overflow. Maybe to avoid a crash it is sufficient to make it an unsigned value so at least it doesn't wrap to negative? I don't know, but it's possible.

[JohnoKing]

From what I can tell the test failure is because of a race condition (although IDK why only the CI runners are affected). Adding sleep .1 allows the test to work with subenv-- in the CI build:

--- a/src/cmd/ksh93/tests/subshell.sh
+++ b/src/cmd/ksh93/tests/subshell.sh
@@ -528,6 +528,7 @@ date=$(whence -p date)
 err() { return $1; }
 ( err 12 ) & pid=$!
 : $( $date)
+sleep .1
 wait $pid
 exit_status=$?
 [[ $exit_status == 12 ]] || err_exit "exit status from subshells not being preserved (expected 12, got $exit_status)"

[McDutchie]

When the current longenv branch is compiled with either of:

bin/package make CCFLAGS='-Os -D_std_malloc -D_AST_std_malloc'
bin/package make CCFLAGS='-Os -D_std_malloc' 

The regression test still intermittently crashes on macOS 10.14.16, in various ways:

$ cat >test.sh
(
	for ((n=0; n != 50000; n++)) do
		(
			function foo {
				(true)
			}
			foo
		)
	done
)
$ while :; do echo ===; arch/*/bin/ksh test.sh; done
===
===
Memory fault
===
Memory fault
===
===
Memory fault
===
ksh(15286,0x1046905c0) malloc: Incorrect checksum for freed object 0x7ff126405718: probably modified after being freed.
Corrupt value: 0x1000002000000000
ksh(15286,0x1046905c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Bus error
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
===
Memory fault
===
Bus error
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
ksh(15310,0x1147bc5c0) malloc: Incorrect checksum for freed object 0x7fc84f4057b8: probably modified after being freed.
Corrupt value: 0x307fc84f404e0
ksh(15310,0x1147bc5c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
ksh(15322,0x10994f5c0) malloc: Incorrect checksum for freed object 0x7ff16f4057b8: probably modified after being freed.
Corrupt value: 0x307ff16f404e0
ksh(15322,0x10994f5c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort
===
Memory fault
===
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
ksh(15329,0x10d7af5c0) malloc: Incorrect checksum for freed object 0x7f866dd01ec8: probably modified after being freed.
Corrupt value: 0xe000002000000000
ksh(15329,0x10d7af5c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
Memory fault
===
===
Memory fault
===
Memory fault
===
^C

[McDutchie]

Backtraces of the above crashes after recompiling with -O0 -g -D_std_malloc look like one of these:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ksh                                 0x000000010e393f6f _sfpmove + 111 (sfpool.c:236)
1   ksh                                 0x000000010e387d45 sfclose + 837 (sfclose.c:96)
2   ksh                                 0x000000010e34f359 stkclose + 121 (stk.c:317)
3   ksh                                 0x000000010e2fbb24 sh_exec + 10628 (xec.c:1516)
4   ksh                                 0x000000010e2fd9af sh_exec + 18447 (xec.c:2012)
5   ksh                                 0x000000010e2f578d sh_subshell + 2605 (subshell.c:605)
6   ksh                                 0x000000010e2fd32f sh_exec + 16783 (xec.c:1886)
7   ksh                                 0x000000010e2fe632 sh_exec + 21650 (xec.c:2201)
8   ksh                                 0x000000010e2fd9af sh_exec + 18447 (xec.c:2012)
9   ksh                                 0x000000010e2f578d sh_subshell + 2605 (subshell.c:605)
10  ksh                                 0x000000010e2fd32f sh_exec + 16783 (xec.c:1886)
11  ksh                                 0x000000010e280ffb exfile + 3243 (main.c:582)
12  ksh                                 0x000000010e281f27 sh_main + 3367 (main.c:353)
13  ksh                                 0x000000010e2679d6 main + 38 (pmain.c:45)
14  libdyld.dylib                       0x00007fff670df3d5 start + 1

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ksh                                 0x000000010ae0ae1e _sfpclose + 62 (sfmode.c:267)
1   ksh                                 0x000000010ae03012 sfclose + 1554 (sfclose.c:148)
2   ksh                                 0x000000010adca359 stkclose + 121 (stk.c:317)
3   ksh                                 0x000000010ad76b24 sh_exec + 10628 (xec.c:1516)
4   ksh                                 0x000000010ad789af sh_exec + 18447 (xec.c:2012)
5   ksh                                 0x000000010ad7078d sh_subshell + 2605 (subshell.c:605)
6   ksh                                 0x000000010ad7832f sh_exec + 16783 (xec.c:1886)
7   ksh                                 0x000000010ad79632 sh_exec + 21650 (xec.c:2201)
8   ksh                                 0x000000010ad789af sh_exec + 18447 (xec.c:2012)
9   ksh                                 0x000000010ad7078d sh_subshell + 2605 (subshell.c:605)
10  ksh                                 0x000000010ad7832f sh_exec + 16783 (xec.c:1886)
11  ksh                                 0x000000010acfbffb exfile + 3243 (main.c:582)
12  ksh                                 0x000000010acfcf27 sh_main + 3367 (main.c:353)
13  ksh                                 0x000000010ace29d6 main + 38 (pmain.c:45)
14  libdyld.dylib                       0x00007fff670df3d5 start + 1

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ksh                           	0x0000000109ac0c44 sfstack + 948 (sfstack.c:85)
1   ksh                           	0x0000000109aadb11 sfclose + 273 (sfclose.c:54)
2   ksh                           	0x0000000109a75359 stkclose + 121 (stk.c:317)
3   ksh                           	0x0000000109a21b24 sh_exec + 10628 (xec.c:1516)
4   ksh                           	0x0000000109a239af sh_exec + 18447 (xec.c:2012)
5   ksh                           	0x0000000109a1b78d sh_subshell + 2605 (subshell.c:605)
6   ksh                           	0x0000000109a2332f sh_exec + 16783 (xec.c:1886)
7   ksh                           	0x0000000109a24632 sh_exec + 21650 (xec.c:2201)
8   ksh                           	0x0000000109a239af sh_exec + 18447 (xec.c:2012)
9   ksh                           	0x0000000109a1b78d sh_subshell + 2605 (subshell.c:605)
10  ksh                           	0x0000000109a2332f sh_exec + 16783 (xec.c:1886)
11  ksh                           	0x00000001099a6ffb exfile + 3243 (main.c:582)
12  ksh                           	0x00000001099a7f27 sh_main + 3367 (main.c:353)
13  ksh                           	0x000000010998d9d6 main + 38 (pmain.c:45)
14  libdyld.dylib                 	0x00007fff670df3d5 start + 1

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ksh                                 0x0000000102591a33 sfstack + 419 (sfstack.c:68)
1   ksh                                 0x000000010257eb11 sfclose + 273 (sfclose.c:54)
2   ksh                                 0x0000000102546359 stkclose + 121 (stk.c:317)
3   ksh                                 0x00000001024f2b24 sh_exec + 10628 (xec.c:1516)
4   ksh                                 0x00000001024f49af sh_exec + 18447 (xec.c:2012)
5   ksh                                 0x00000001024ec78d sh_subshell + 2605 (subshell.c:605)
6   ksh                                 0x00000001024f432f sh_exec + 16783 (xec.c:1886)
7   ksh                                 0x00000001024f5632 sh_exec + 21650 (xec.c:2201)
8   ksh                                 0x00000001024f49af sh_exec + 18447 (xec.c:2012)
9   ksh                                 0x00000001024ec78d sh_subshell + 2605 (subshell.c:605)
10  ksh                                 0x00000001024f432f sh_exec + 16783 (xec.c:1886)
11  ksh                                 0x0000000102477ffb exfile + 3243 (main.c:582)
12  ksh                                 0x0000000102478f27 sh_main + 3367 (main.c:353)
13  ksh                                 0x000000010245e9d6 main + 38 (pmain.c:45)
14  libdyld.dylib                       0x00007fff670df3d5 start + 1

[McDutchie]

Even when reverting the longenv branch to the original OpenSUSE fix (with long types), at 34fbf89, the same crashes still occur with that regression test, with the same backtraces. (This is all still after compiling with -O0 -g -D_std_malloc.)

When reverting back to HEAD, at 34fbf89, it still crashes, but with different backtraces. Some of them are the same (various crashes after calling sfclose()), but others are in sh_assignok() or in nv_restore() as below. So the fix at least had an influence. However, that regression test should not be part of the current fix as the regression test appears to expose a different bug.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ksh                                 0x0000000107b655d1 sh_assignok + 33 (subshell.c:250)
1   ksh                                 0x0000000107b45fa6 nv_putval + 358 (name.c:1636)
2   ksh                                 0x0000000107b725e3 sh_funct + 979 (xec.c:3242)
3   ksh                                 0x0000000107b6ca18 sh_exec + 10376 (xec.c:1503)
4   ksh                                 0x0000000107b6e9ac sh_exec + 18460 (xec.c:2012)
5   ksh                                 0x0000000107b6675e sh_subshell + 2606 (subshell.c:605)
6   ksh                                 0x0000000107b6e32b sh_exec + 16795 (xec.c:1886)
7   ksh                                 0x0000000107b6f62f sh_exec + 21663 (xec.c:2201)
8   ksh                                 0x0000000107b6e9ac sh_exec + 18460 (xec.c:2012)
9   ksh                                 0x0000000107b6675e sh_subshell + 2606 (subshell.c:605)
10  ksh                                 0x0000000107b6e32b sh_exec + 16795 (xec.c:1886)
11  ksh                                 0x0000000107af1f8b exfile + 3243 (main.c:582)
12  ksh                                 0x0000000107af2eb7 sh_main + 3367 (main.c:353)
13  ksh                                 0x0000000107ad8916 main + 38 (pmain.c:45)
14  libdyld.dylib                       0x00007fff670df3d5 start + 1

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                                 000000000000000000 0 + 0
1   ksh                                 0x000000010c99284d nv_restore + 909 (subshell.c:374)
2   ksh                                 0x000000010c9918cd sh_subshell + 2973 (subshell.c:629)
3   ksh                                 0x000000010c99932b sh_exec + 16795 (xec.c:1886)
4   ksh                                 0x000000010c99a62f sh_exec + 21663 (xec.c:2201)
5   ksh                                 0x000000010c9999ac sh_exec + 18460 (xec.c:2012)
6   ksh                                 0x000000010c99175e sh_subshell + 2606 (subshell.c:605)
7   ksh                                 0x000000010c99932b sh_exec + 16795 (xec.c:1886)
8   ksh                                 0x000000010c91cf8b exfile + 3243 (main.c:582)
9   ksh                                 0x000000010c91deb7 sh_main + 3367 (main.c:353)
10  ksh                                 0x000000010c903916 main + 38 (pmain.c:45)
11  libdyld.dylib                       0x00007fff670df3d5 start + 1

[McDutchie]

When we remove the function definition from the regression test, like this:

(
	for ((n=0; n != 50000; n++)) do
		(
			(true)
		)
	done
)

the crash disappears. So this is a bug with defining a function in a virtual subshell, which is a different bug from this one.