fix a troff formatting error in the manpage. #5

jghub · 2020-03-09T12:27:04Z

fixes a trivial troff formatting error in the manpage which results in erroneous display of the default value of TIMEFORMAT with man ksh.

marcastel

Makes sense. Thanks :-)

jelmd · 2020-03-09T17:57:00Z

OK wrt.

ksh/src/cmd/ksh93/data/msg.c

Line 213 in e7f2542

const char e_timeformat[] = "\nreal\t%2lR\nuser\t%2lU\nsys\t%2lS";

jelmd

see discussion

dannyweldon · 2020-03-10T08:54:10Z

Should we be submitting these directly to the master branch though?

jelmd · 2020-03-10T22:36:52Z

Yes.

Somewhat notable changes in this commit: - The 'set +r' bugfix (re: 74b4162) is now documented in the changelog. - Missing options have been added to the synopsis section of the ksh man page. - The minor formatting fix from ksh-community/ksh#5 has been applied to the ksh man page. - A few fixes from att/ast@5e747cfb have been applied to the ksh man page. - The man page fixes from att#353 have been applied, being: - An addition to document the behavior of 'set -H'. - A fix for the cd section appending rksh93. - A fix for some options being indented too far. - Removal of a duplicate section documenting '-D'. - Reordering the options for 'set' in alphabetical order. - A minor fix for the documentation of 'ksh -i'. - One minor fix from Red Hat has been applied to the ksh man page: https://git.centos.org/rpms/ksh/blob/c8/f/SOURCES/ksh-20120801-manfix4.patch

Somewhat notable changes in this commit: - The 'set +r' bugfix (re: 74b4162) is now documented in the changelog. - Missing options have been added to the synopsis section of the ksh man page. - The minor formatting fix from ksh-community/ksh#5 has been applied to the ksh man page. - A few fixes from att/ast@5e747cfb have been applied to the ksh man page. - The man page fixes from att#353 have been applied, being: - An addition to document the behavior of 'set -H'. - A fix for the cd section appending rksh93. - A fix for some options being indented too far. - Removal of a duplicate section documenting '-D'. - Reordering the options for 'set' in alphabetical order. - A minor fix for the documentation of 'ksh -i'.

Somewhat notable changes in this commit: - The 'set +r' bugfix (re: 74b4162) is now documented in the changelog. - Missing options have been added to the synopsis section of the ksh man page. - The minor formatting fix from ksh-community/ksh#5 has been applied to the ksh man page. - A few fixes from att@5e747cfb have been applied to the ksh man page. - The man page fixes from att#353 have been applied, being: - An addition to document the behavior of 'set -H'. - A fix for the cd section appending rksh93. - A fix for some options being indented too far. - Removal of a duplicate section documenting '-D'. - Reordering the options for 'set' in alphabetical order. - A minor fix for the documentation of 'ksh -i'.

If the processing of a multibyte character was interrupted in UTF-8 locales, e.g. by reading just one byte of a two-byte character 'ü' (\303\274) with a command like: print -nr $'\303\274' | read -n1 g then the shellquoting algorithm was corrupted in such a way that the final quote in simple single-quoted string was missing. This bug may have had other, as yet undiscovered, effects as well. The problem was with corrupted multibyte character processing and not with the shell-quoting routine sh_fmtq() itself. Full trace and discussion at: ksh93/ksh#5 (which is also an attempt to begin to understand the esoteric workings of the libast mb* macros that process UTF-8 characters). src/lib/libast/comp/setlocale.c: utf8_mbtowc(): - If called from the mbinit() macro (i.e. if both pointer parameters are null), reset the global multibyte character synchronisation state variable. This fixes the problem with interrupted processing leaving an inconsistent state, provided that mbinit() is called before processing multibyte characters (which it is, in most (?) places that do this). Before this fix, calling mbinit() in UTF-8 locales was a no-op. src/cmd/ksh93/sh/string.c: sh_fmtq(): - Call mbinit() before potentially processing multibyte characters. Testing suggests that this could be superfluous, but at worst, it's harmless; better be sure. src/cmd/ksh93/tests/builtins.sh: - Add regression test for shellquoting with 'printf %q' after interrupting the processing of a multibyte characeter with 'read -n1'. This test only fails in a UTF-8 locale, e.g. when running: bin/shtests -u builtins SHELL=/buggy/ksh-2012-08-01 Fixes ksh-community#5.

Hopefully this doesn't introduce new bugs, but it does fix at least the following: 1. When whence -v/-a found an "undefined" (i.e. autoloadable) function in $FPATH, it actually loaded the function as a side effect of reporting on its existence (!). Now it only reports. 2. 'whence' will now canonicalise paths properly. Examples: $ whence ///usr/lib/../bin//./env /usr/bin/env $ (cd /; whence -v dev/../usr/bin//./env) dev/../usr/bin//./env is /usr/bin/env 3. 'whence' no longer prefixes a spurious double slash when doing something like 'cd / && whence bin/echo'. On Cygwin, an initial double slash denotes a network server, so this was not just a cosmetic problem. 4. 'whence -a' now reports a "tracked alias" (a.k.a. hash table entry, i.e. cached $PATH search) even if an actual alias by the same name exists. This needed fixing because in fact the hash table entry continues to be used when bypassing the alias. Aliases and "tracked aliases" are not remotely the same thing; confusing nomenclature is not a reason to report wrong results. 5. When using 'hash' or 'alias -t' on a command that is also a builtin to force caching a $PATH search for the external command, 'whence -a' double-reported the path: $ hash printf; whence -a printf printf is a shell builtin printf is /usr/bin/printf printf is a tracked alias for /usr/bin/printf This is now fixed so that the second output line is gone. Plus, if there were multiple versions of the command on $PATH, the tracked alias was reported at the end, which is the wrong order. This is also fixed. src/cmd/ksh93/bltins/whence.c: whence(): - Refactor the do...while loop that handles whence -v/-a for path searches in such a way that the code actually makes sense and stops looking like higher esotericism. Just doing this fixed ksh-community#2, ksh-community#4 and ksh-community#5 above (the latter two before I even noticed them). For instance, the path_fullname() call to canonicalise paths was already there; it was just never used. - Remove broken 'notrack' flaggery for deciding whether to report a hash table entry a.k.a. "tracked alias"; instead, check the hash table (shp->track_tree). src/cmd/ksh93/sh/path.c: - path_search(): Re ksh-community#3: When prefixing the PWD, first check if we're in '/' and if so, don't prefix it; otherwise, adding the next slash causes an initial double slash. (Since '/' is the only valid single-character absolute path, all we need to do is check if the second character pwd[1] is non-null.) - path_search(): Re ksh-community#1: Stop autoloading when called by 'whence': * The 'flag==2' check to avoid autoloading a function was broken. The flag value is 2 on the first whence() loop iteration, but 3 on subsequent ones. Change to 'flag >= 2'. * However, this only fixes it if the function file does not have the x permission bit, as executable files are handled by path_absolute() which unconditionally autoloads functions! So, pass on our flag parameter when callling path_absolute(). - path_absolute(): Re ksh-community#1: Add flag parameter. Do not autoload functions if flag >= 2. src/cmd/ksh93/include/path.h, src/cmd/ksh93/bltins/typeset.c, src/cmd/ksh93/sh/main.c, src/cmd/ksh93/sh/xec.c: - Re ksh-community#1: Update path_absolute() calls, adding a 0 flag parameter. src/cmd/ksh93/include/name.h: - Remove now-unused pathcomp member from union Value. It was introduced in 9906535 to allow examining the value of a tracked alias. This commit uses nv_getval() instead. src/cmd/ksh93/tests/builtins.sh, src/cmd/ksh93/tests/path.sh: - Add and tweak various related tests. Fixes: ksh93/ksh#84

Original patch: https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20140801-diskfull.patch Prior discussion: https://www.mail-archive.com/[email protected]/msg01037.html https://www.mail-archive.com/[email protected]/msg01038.html https://www.mail-archive.com/[email protected]/msg01042.html https://bugzilla.redhat.com/1212992 On Fri, 08 May 2015 14:37:45 -0700, Paulo Andrade wrote: > I have a user with a ksh crashing problem, and that has > some "Write error: No space left on device" messages > in /var/log/messages. > > After some debugging, and creating a chroot on a file > disk image, and a test user, and slowly filling the > "on file" filesystem, e.g. > > dd if=/dev/zero of=/mnt/tmp/zerosN bs=1M count=1024 > dd if=/dev/zero of=/mnt/tmp/zerosN bs=1K count=2 > > until leaving just around 12K, I managed to reproduce the > problem, and be able to debug it with valgrind and vgdb; > debugging on these conditions is tricky, as cannot tell > valgrind to spawn gdb, because then gdb itself would fail > to start. > > So, after following the code enough, I learned that at places > it handles SH_JMPEXIT, there was almost non existing > handling of SH_JMPERREXIT. > > ksh would evently cause a crash due to the struct > subshell allocated on stack, in sh/subshell.c:sh_subshell > kept set to the global subshell_data, after it siglongjmp > back the stack due to, not fully handling the out of disk > space errors. It would print a few messages, everytime > a pipe was created, e.g.: > > /etc/profile: line 28: write to 3 failed [No space left on device] > > until eventually crashing due to corrupted memory; e.g. the > references to stack data from sh_subsell in the global > subshell_data. One strange thing to me in coredump analysis > was that subshell_data prev field was pointing to itself when > it eventually crashed, what later was understood and expected... > > The attached patch handles SH_JMPERREXIT in the code > paths SH_JMPEXIT is handled, and the failed login, on > full disk, ends in a pause() call: > > ---terminal 1--- > $ valgrind -q --leak-check=full --free-fill=0x5a --vgdb=full > --vgdb-error=0 /bin/ksh -l > ==17730== (action at startup) vgdb me ... > ==17730== > ==17730== TO DEBUG THIS PROCESS USING GDB: start GDB like this > ==17730== /path/to/gdb /bin/ksh > ==17730== and then give GDB the following command > ==17730== target remote | /usr/lib64/valgrind/../../bin/vgdb --pid=17730 > ==17730== --pid is optional if only one valgrind process is running > ==17730== > ==17730== Syscall param mount(type) points to unaddressable byte(s) > ==17730== at 0x563377A: mount (in /usr/lib64/libc-2.17.so) > ==17730== by 0x493E58: fs3d_mount (fs3d.c:115) > ==17730== by 0x493C8B: fs3d (fs3d.c:57) > ==17730== by 0x423E41: sh_init (init.c:1302) > ==17730== by 0x405CD3: sh_main (main.c:141) > ==17730== by 0x405B84: main (pmain.c:45) > ==17730== Address 0x0 is not stack'd, malloc'd or (recently) free'd > ==17730== > ==17730== (action on error) vgdb me ... > ==17730== Continuing ... > /etc/profile: line 28: write to 3 failed [No space left on device] > ---8<--- > > ---terminal 2--- > (gdb) c > Continuing. > ^C > Program received signal SIGTRAP, Trace/breakpoint trap. > 0x00000000055fa470 in __pause_nocancel () from /lib64/libc.so.6 > (gdb) bt > #0 0x00000000055fa470 in __pause_nocancel () from /lib64/libc.so.6 > ksh-community#1 0x000000000041e73d in sh_done (ptr=0x793360 <sh>, sig=255) at > /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/fault.c:665 > ksh-community#2 0x0000000000407407 in exfile (shp=0x4542, iop=0xff, fno=0) at > /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/main.c:604 > ksh-community#3 0x0000000000405c43 in sh_source (shp=0x793360 <sh>, iop=0x0, > file=0x524804 <e_sysprofile> "/etc/profile") > at /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/main.c:109 > ksh-community#4 0x00000000004060e4 in sh_main (ac=2, av=0xfff000498, userinit=0x0) > at /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/main.c:202 > ksh-community#5 0x0000000000405b85 in main (argc=2, argv=0xfff000498) at > /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/pmain.c:45 > (gdb) > ---8<---

Currently, running ksh under ASan without the ASAN_OPTIONS variable set to 'detect_leaks=0' usually ends with ASan complaining about a memory leak in defpathinit() (this leak doesn't grow in a loop, so no regression test was added to leaks.sh). Reproducer: $ ENV=/dev/null arch/*/bin/ksh $ cp -? cp: invalid option -- '?' Try 'cp --help' for more information. $ exit ================================================================= ==225132==ERROR: LeakSanitizer: detected memory leaks Direct leak of 85 byte(s) in 1 object(s) allocated from: #0 0x7f6dab42d459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 ksh-community#1 0x5647b77fe144 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:265 ksh-community#2 0x5647b788fea9 in path_addcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1567 ksh-community#3 0x5647b78911ed in path_addpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1705 ksh-community#4 0x5647b7888e82 in defpathinit /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:442 ksh-community#5 0x5647b78869f3 in ondefpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:67 --- cut --- SUMMARY: AddressSanitizer: 174 byte(s) leaked in 2 allocation(s). Analysis: The previous code was leaking memory because defpathinit() returns a pointer from path_addpath(), which has memory allocated to it in path_addcomp(). This is the code ASan traced as having allocated memory: 442: return(path_addpath((Pathcomp_t*)0,(defpath),PATH_PATH)); In path_addpath(): 1705: first = path_addcomp(first,old,cp,type); [...] 1729: return(first); In path_addcomp(): 1567: pp = sh_newof((Pathcomp_t*)0,Pathcomp_t,1,len+1); The ondefpath() function doesn't save a reference to the pointer returned by defpathinit(), which causes the memory leak: 66: if(!defpath) 67: defpathinit(); The changes in this commit avoid this problem by setting the defpath variable without also calling path_addpath(). src/cmd/ksh93/sh/path.c: - Move the code for allocating defpath from defpathinit() into its own dedicated function called std_path(). This function is called by defpathinit() and ondefpath() to obtain the current string stored in the defpath variable. This bugfix is adapted from a fork of ksh2020: l0stman/ksh@db5c83a6

I didn't trust this back in e3d7bf1 (which disabled it for interactive shells) and I trust it less now. In af6a32d/6b380572, this was also disabled for virtual subshells as it caused program flow corruption there. Now, on macOS 10.14.6, a crash occurs when repeatedly running a command with this optimisation: $ ksh -c 'for((i=0;i<100;i++));do print -n "$i ";(sleep 1&);done' 0 1 2 3 4 5 6 7 Illegal instruction Oddly enough it seems that I can only reproduce this crash on macOS -- not on Linux, OpenBSD, or Solaris. It could be a macOS bug, particularly given the odd message in the stack trace below. I've had enough, though. Out it comes. Things now work fine, the reproducer is fixed on macOS, and it didn't optimise much anyway. The double-fork issue discussed in e3d7bf1 remains. ________ For future reference, here's an lldb debugger session with a stack trace. It crashes on calling calloc() (via sh_calloc(), via sh_newof()) in jobsave_create(). This is not an invalid pointer problem as we're allocating new memory, so it does look like an OS bug. The "BUG IN CLIENT OF LIBPLATFORM" message is interesting. $ lldb -- arch/*/bin/ksh -c 'for((i=0;i<100;i++));do print -n "$i ";(sleep 1&);done' (lldb) target create "arch/darwin.i386-64/bin/ksh" Current executable set to 'arch/darwin.i386-64/bin/ksh' (x86_64). (lldb) settings set -- target.run-args "-c" "for((i=0;i<100;i++));do print -n \"$i \";(sleep 1&);done" (lldb) run error: shell expansion failed (reason: lldb-argdumper exited with error 2). consider launching with 'process launch'. (lldb) process launch Process 35038 launched: '/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh' (x86_64) 0 1 2 3 4 5 6 7 8 9 Process 35038 stopped * thread ksh-community#1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) frame #0: 0x00007fff70deb1c2 libsystem_platform.dylib`_os_unfair_lock_recursive_abort + 23 libsystem_platform.dylib`_os_unfair_lock_recursive_abort: -> 0x7fff70deb1c2 <+23>: ud2 libsystem_platform.dylib`_os_unfair_lock_unowned_abort: 0x7fff70deb1c4 <+0>: movl %edi, %eax 0x7fff70deb1c6 <+2>: leaq 0x1a8a(%rip), %rcx ; "BUG IN CLIENT OF LIBPLATFORM: Unlock of an os_unfair_lock not owned by current thread" 0x7fff70deb1cd <+9>: movq %rcx, 0x361cb16c(%rip) ; gCRAnnotations + 8 Target 0: (ksh) stopped. (lldb) bt * thread ksh-community#1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) * frame #0: 0x00007fff70deb1c2 libsystem_platform.dylib`_os_unfair_lock_recursive_abort + 23 frame ksh-community#1: 0x00007fff70de7c9a libsystem_platform.dylib`_os_unfair_lock_lock_slow + 239 frame ksh-community#2: 0x00007fff70daa3bd libsystem_malloc.dylib`tiny_malloc_should_clear + 188 frame ksh-community#3: 0x00007fff70daa20f libsystem_malloc.dylib`szone_malloc_should_clear + 66 frame ksh-community#4: 0x00007fff70dab444 libsystem_malloc.dylib`malloc_zone_calloc + 99 frame ksh-community#5: 0x00007fff70dab3c4 libsystem_malloc.dylib`calloc + 30 frame ksh-community#6: 0x000000010003fa5d ksh`sh_calloc(nmemb=1, size=16) at init.c:264:13 frame ksh-community#7: 0x000000010004f8a6 ksh`jobsave_create(pid=35055) at jobs.c:272:8 frame ksh-community#8: 0x000000010004ed42 ksh`job_reap(sig=20) at jobs.c:363:9 frame ksh-community#9: 0x000000010004ff6f ksh`job_waitsafe(sig=20) at jobs.c:511:3 frame ksh-community#10: 0x00007fff70de9b5d libsystem_platform.dylib`_sigtramp + 29 frame ksh-community#11: 0x00007fff70d39ac4 libsystem_kernel.dylib`__fork + 12 frame ksh-community#12: 0x00007fff70c57d80 libsystem_c.dylib`fork + 17 frame ksh-community#13: 0x000000010009590d ksh`sh_exec(t=0x0000000101005d30, flags=4) at xec.c:1883:16 frame ksh-community#14: 0x0000000100096013 ksh`sh_exec(t=0x0000000101005d30, flags=4) at xec.c:2019:4 frame ksh-community#15: 0x0000000100096c4f ksh`sh_exec(t=0x0000000101005a40, flags=5) at xec.c:2213:9 frame ksh-community#16: 0x0000000100096013 ksh`sh_exec(t=0x0000000101005a40, flags=5) at xec.c:2019:4 frame ksh-community#17: 0x000000010001c23f ksh`exfile(iop=0x0000000100405750, fno=-1) at main.c:603:4 frame ksh-community#18: 0x000000010001b23c ksh`sh_main(ac=3, av=0x00007ffeefbff4f0, userinit=0x0000000000000000) at main.c:365:2 frame ksh-community#19: 0x0000000100000776 ksh`main(argc=3, argv=0x00007ffeefbff4f0) at pmain.c:45:9 frame ksh-community#20: 0x00007fff70bfe3d5 libdyld.dylib`start + 1

The ASan crash in basic.sh when sourcing multiple files is caused by a bug that is similar to the crash fixed in 59a5672. This is the trace for the regression test crash (note that in order to see the trace, the 2>/dev/null redirect must be disabled): ==1899388==ERROR: AddressSanitizer: heap-use-after-free on address 0x6150000005b0 at pc 0x55a5e3f9432a bp 0x7ffeb91ea110 sp 0x7ffeb91ea100 WRITE of size 8 at 0x6150000005b0 thread T0 #0 0x55a5e3f94329 in funct /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:967 ksh-community#1 0x55a5e3f96f77 in item /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:1349 ksh-community#2 0x55a5e3f90c9f in term /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:642 ksh-community#3 0x55a5e3f90ac1 in list /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:613 ksh-community#4 0x55a5e3f90845 in sh_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:561 ksh-community#5 0x55a5e3f909e0 in sh_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:586 ksh-community#6 0x55a5e3f8fd5e in sh_parse /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:438 ksh-community#7 0x55a5e3fc43c1 in sh_eval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:635 ksh-community#8 0x55a5e4012172 in b_dot_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/misc.c:318 ksh-community#9 0x55a5e3fca3cb in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1254 ksh-community#10 0x55a5e3fd01d4 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1932 ksh-community#11 0x55a5e3fc4544 in sh_eval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:651 ksh-community#12 0x55a5e4012172 in b_dot_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/misc.c:318 ksh-community#13 0x55a5e3fca3cb in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1254 ksh-community#14 0x55a5e3ecc1cd in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:604 ksh-community#15 0x55a5e3ec9e7f in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:369 ksh-community#16 0x55a5e3ec801d in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:41 ksh-community#17 0x7f637b4db2cf (/usr/lib/libc.so.6+0x232cf) ksh-community#18 0x7f637b4db389 in __libc_start_main (/usr/lib/libc.so.6+0x23389) ksh-community#19 0x55a5e3ec7f24 in _start ../sysdeps/x86_64/start.S:115 Code in question: https://github.com/ksh93/ksh/blob/8d57369b0cb39074437dd82924b604155e30e1e0/src/cmd/ksh93/sh/parse.c#L963-L968 To avoid any more similar crashes, all of the fixes introduced in 7e317c5 that set slp->slptr to null have been improved with the fix in 59a5672.

When ksh executes a script without a #! path (note that the AT&T team had a real disliking for #! paths), ksh forks and goes through a quick reinitialisation procedure. This is much faster than invoking a fully new shell but should have the same effect if it all works well. Unfortunately it's not worked all that well so far. Even after recent improvements (see referenced commits) I've been finding corner case problems. FYI, running a script without #! basically goes like this: * in path_spawn(), execve() fails with ENOEXEC because the file is not a binary executable and does not start with #! * this triggers 'case ENOEXEC:' which: * forks ksh * calls exscript() * exscript() cleans up & calls siglongjmp(*sh.jmplist,SH_JMPSCRIPT) * SH_JMPSCRIPT is the highest longjmp value, so *all* the previous sigsetjmp/sh_pushcontext calls are unwinded in reverse order, triggering all sorts of cleanup, state restoration, removal of local scopes, etc. * eventually, this lands us at the top sigsetjmp in sh_main() * sh_main() calls sh_reinit(), then resumes as if the shell had just been started This commit makes the following interrelated changes for the correct functioning of this procedure: 1. exscript() now exports the environment into a dedicated Stk_t buffer and sets environ[] to that. 2. Instead of re-using existing variables, sh_reinit() deletes everything and reinits all name-value trees from scratch, then re-imports the environment from environ[]. 3. Variable values that were imported from the environment are no longer treated specially with an NV_IMPORT attribute and the np->nvenv pointer to their value in environ[], fixing at least one crash.[*1] Details of the changes follow: src/cmd/ksh93/sh/path.c: - exscript(): Generate a new environ[] by activating a dedicated AST stack that will not be overwritten before calling sh_envgen(). This will allow sh_reinit() to delete all variables and then reimport the environment. The exporting must be done here, before siglongjmp, otherwise locally scoped exported variables won't be included (siglongjmp with SH_JMPSCRIPT triggers cleanup of all scopes). src/cmd/ksh93/sh/init.c: - sh_reinit(): Largely rewritten as follows. - Reset shell options first. This has the beneficial side effect of unsetting SH_RESTRICTED which interferes with unsetting certain variables, like PATH. - Remove workarounds for FPATH, SHLVL and tilde expansion disciplines; these will not be needed now. - Properly unset and delete all functions and built-ins. Since we now unset a function before deleting it, this should now free up their memory. (See nvdisc.c below for a change allowing removal of special built-ins.) - Properly unset all variables (which includes any associated discipline functions). Incorporate here the needed logic from sh_envnolocal() in name.c; most of it is unneeded (that function was previously used to cleanup local variables but has not been used for that for decades). So sh_envnolocal() is now unused. - Delete variables in a separate pass after unsetting variables and unsetting and deleting functions; this avoids use-after- free problems as well as possible "no parent" problems with namespace variables (e.g., .rc.status in our new kshrc.sh). - After all that, close and free up all function, alias, tracked alias, type and variable trees. - Free the contiguous built-in node space and the Init_t init context (with all the special variable discipline pointers). - Call nv_init (previously only called from sh_init) to reinitialise all of the above name-value stuff from scratch. It's the only way to be sure. - Re-import the environment as stored by exscript() above. - env_init(): - Per item 3 above and footnote 1 below, no longer set NV_IMPORT attribute and no longer point np->nvenv to the item in environ. - POSIX says, for 'environ': "Any application that directly modifies the pointers to which the environ variable points has undefined behavior."[*2] Yet, env_init() is indeed juggling the environ[] pointers to deal with variables that cannot be imported because their names are invalid (because they still need to be saved to be passed on to child processes). Replace the current approach with one where those env vars get allocated on the heap, pointed to by sh.save_env and counted by sh.save_env_n (renamed from sh.nenv). This only needs to be done once as ksh cannot use or change these variables. src/cmd/ksh93/sh/name.c: - sh_envgen(): Update to match env_init() change above. - pushnam() (called by sh_envgen()): Remove NV_IMPORT attribute check as per above and never get the value from the nvenv pointer -- simply always use nv_getval(). As of this change, the NV_IMPORT attribute is unused. The next commit will remove it and do related cleanups. - staknam(): is only called if value!=NULL, so remove that 'if'. - sh_envnolocal(): Removed. src/cmd/ksh93/sh/nvdisc.c: - assign(): Remove a check for the SH_INIT state bit that avoids freeing functions during sh_reinit(). This works fine now. - sh_addbuiltin(): Allow sh_reinit() to delete special builtins by checking for the SH_INIT state bit before throwing an error. src/cmd/ksh93/sh/nvtree.c: - outval(): Add a workaround for a use-after-free, introduced by the changes above, that occurred in the types.sh tests for #!-less scripts (types.sh:675-722). The use-after-free occurred here (abridged ASan trace follows; line numbers are current as of this commit): ==30849==ERROR: AddressSanitizer: heap-use-after-free [...] #0 in dttree dttree.c:393 ksh-community#1 in sh_reinit init.c:1637 ksh-community#2 in sh_main main.c:136 [...] The pointer was freed in the same loop via nv_delete() in outval: #0 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:[...]) ksh-community#1 in nv_delete name.c:1318 ksh-community#2 in outval nvtree.c:731 ksh-community#3 in genvalue nvtree.c:905 ksh-community#4 in walk_tree nvtree.c:1042 ksh-community#5 in put_tree nvtree.c:1108 ksh-community#6 in nv_putv nvdisc.c:144 ksh-community#7 in _nv_unset name.c:2437 ksh-community#8 in sh_reinit init.c:1645 ksh-community#9 in sh_main main.c:136 [...] So, what happened was that the nv_delete() call on name.c:1318 (eventually resulting from the _nv_unset call on init.c:1645) freed the node pointed to by np, so that the next loop iteration crashed on line 1637 as the dtnext() macro now gets a freed np. Now, why on earth should _nv_unset() *ever* indirectly call nv_delete()? That's a question for another day; I suspect it may be a bug, or it may be needed for compound variables for some reason. For now, I'm adding a workaround: simply avoid calling nv_delete() if the SH_INIT state bit is on, indicating sh_reinit() is in the call stack. This allows the variables unset loop in sh_reinit() to continue without crashing. sh_reinit() handles deletion later anyway. src/cmd/ksh93/sh/main.c: - sh_main(): remove zeroing of sh.fun_depth and sh.dot_depth; these are known to be 0, coming from either sh_init() or sh_reinit(). ________ [*1] This NV_IMPORT/nvenv usage is a redundant holdout from ancient ksh code; the imported value is easily available as a normal shell variable value via nv_getval(). Plus, the nvenv pointer is overloaded with too many other purposes: so far I've discovered it's used for pointers to subarrays of arrays (multidimentional arrays), compound variables, builtins, and other things. This mess caused at least one crash in set_instance() (xec.c) due to incorrectly using that nvenv pointer. The current kshrc script triggers this. Reproducer: $ export PS1 $ bin/package use «0»26:…/src/ksh93/ksh[dev] $ typeset +x PS1 ...and crash. That is now fixed. [*2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/environ.html

The referenced commit left one test unexecuted because it crashes. Minimal reproducer: typeset -a arr=((a b c) 1) got=$( typeset -a arr=( ( ((a b c)1))) ) The crash occurs when the array is redefined in a subshell. Here are abridged ASan stack traces for the crash, for the use after free, and for when it was freed: ================================================================= ==73147==ERROR: AddressSanitizer: heap-use-after-free [snippage] READ of size 8 at 0x000107403eb0 thread T0 #0 0x104fded40 in nv_search nvdisc.c:1007 ksh-community#1 0x104fbeb1c in nv_create name.c:860 ksh-community#2 0x104fb8b9c in nv_open name.c:1440 ksh-community#3 0x104fb1edc in nv_setlist name.c:309 ksh-community#4 0x104fb4a30 in nv_setlist name.c:475 ksh-community#5 0x105055b58 in sh_exec xec.c:1079 ksh-community#6 0x105045cd4 in sh_subshell subshell.c:654 ksh-community#7 0x104f92c1c in comsubst macro.c:2266 [snippage] 0x000107403eb0 is located 0 bytes inside of 80-byte region [snippage] freed by thread T0 here: #0 0x105c5ade4 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3ede4) ksh-community#1 0x105261da0 in dtclose dtclose.c:52 ksh-community#2 0x104f178cc in array_putval array.c:671 ksh-community#3 0x104fd7f4c in nv_putv nvdisc.c:144 ksh-community#4 0x104fbc5f0 in _nv_unset name.c:2435 ksh-community#5 0x104fb3250 in nv_setlist name.c:364 ksh-community#6 0x105055b58 in sh_exec xec.c:1079 ksh-community#7 0x105045cd4 in sh_subshell subshell.c:654 ksh-community#8 0x104f92c1c in comsubst macro.c:2266 [snippage] So the crash is caused because array_putval (array.c:671) calls dtclose, freeing ap->table, which is then reused after a recursive nv_setlist call via nv_open() -> nv_create() -> nv_search(). This only happens whwn we're in a virtual subshell. src/cmd/ksh93/sh/array.c: - array_putval(): When redefining an array in a virtual subshell, do not free the old ap->table; it will be needed by the parent shell environment.

Currently, running ksh under ASan without the ASAN_OPTIONS variable set to 'detect_leaks=0' usually ends with ASan complaining about a memory leak in defpathinit() (this leak doesn't grow in a loop, so no regression test was added to leaks.sh). Reproducer: $ ENV=/dev/null arch/*/bin/ksh $ cp -? cp: invalid option -- '?' Try 'cp --help' for more information. $ exit ================================================================= ==225132==ERROR: LeakSanitizer: detected memory leaks Direct leak of 85 byte(s) in 1 object(s) allocated from: #0 0x7f6dab42d459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154 ksh-community#1 0x5647b77fe144 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:265 ksh-community#2 0x5647b788fea9 in path_addcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1567 ksh-community#3 0x5647b78911ed in path_addpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1705 ksh-community#4 0x5647b7888e82 in defpathinit /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:442 ksh-community#5 0x5647b78869f3 in ondefpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:67 --- cut --- SUMMARY: AddressSanitizer: 174 byte(s) leaked in 2 allocation(s). Analysis: The previous code was leaking memory because defpathinit() returns a pointer from path_addpath(), which has memory allocated to it in path_addcomp(). This is the code ASan traced as having allocated memory: 442: return(path_addpath((Pathcomp_t*)0,(defpath),PATH_PATH)); In path_addpath(): 1705: first = path_addcomp(first,old,cp,type); [...] 1729: return(first); In path_addcomp(): 1567: pp = sh_newof((Pathcomp_t*)0,Pathcomp_t,1,len+1); The ondefpath() function doesn't save a reference to the pointer returned by defpathinit(), which causes the memory leak: 66: if(!defpath) 67: defpathinit(); The changes in this commit avoid this problem by setting the defpath variable without also calling path_addpath(). src/cmd/ksh93/sh/path.c: - Move the code for allocating defpath from defpathinit() into its own dedicated function called std_path(). This function is called by defpathinit() and ondefpath() to obtain the current string stored in the defpath variable. This bugfix is adapted from a fork of ksh2020: l0stman/ksh@db5c83a6

I didn't trust this back in e3d7bf1 (which disabled it for interactive shells) and I trust it less now. In af6a32d/6b380572, this was also disabled for virtual subshells as it caused program flow corruption there. Now, on macOS 10.14.6, a crash occurs when repeatedly running a command with this optimisation: $ ksh -c 'for((i=0;i<100;i++));do print -n "$i ";(sleep 1&);done' 0 1 2 3 4 5 6 7 Illegal instruction Oddly enough it seems that I can only reproduce this crash on macOS -- not on Linux, OpenBSD, or Solaris. It could be a macOS bug, particularly given the odd message in the stack trace below. I've had enough, though. Out it comes. Things now work fine, the reproducer is fixed on macOS, and it didn't optimise much anyway. The double-fork issue discussed in e3d7bf1 remains. ________ For future reference, here's an lldb debugger session with a stack trace. It crashes on calling calloc() (via sh_calloc(), via sh_newof()) in jobsave_create(). This is not an invalid pointer problem as we're allocating new memory, so it does look like an OS bug. The "BUG IN CLIENT OF LIBPLATFORM" message is interesting. $ lldb -- arch/*/bin/ksh -c 'for((i=0;i<100;i++));do print -n "$i ";(sleep 1&);done' (lldb) target create "arch/darwin.i386-64/bin/ksh" Current executable set to 'arch/darwin.i386-64/bin/ksh' (x86_64). (lldb) settings set -- target.run-args "-c" "for((i=0;i<100;i++));do print -n \"$i \";(sleep 1&);done" (lldb) run error: shell expansion failed (reason: lldb-argdumper exited with error 2). consider launching with 'process launch'. (lldb) process launch Process 35038 launched: '/usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh' (x86_64) 0 1 2 3 4 5 6 7 8 9 Process 35038 stopped * thread ksh-community#1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) frame #0: 0x00007fff70deb1c2 libsystem_platform.dylib`_os_unfair_lock_recursive_abort + 23 libsystem_platform.dylib`_os_unfair_lock_recursive_abort: -> 0x7fff70deb1c2 <+23>: ud2 libsystem_platform.dylib`_os_unfair_lock_unowned_abort: 0x7fff70deb1c4 <+0>: movl %edi, %eax 0x7fff70deb1c6 <+2>: leaq 0x1a8a(%rip), %rcx ; "BUG IN CLIENT OF LIBPLATFORM: Unlock of an os_unfair_lock not owned by current thread" 0x7fff70deb1cd <+9>: movq %rcx, 0x361cb16c(%rip) ; gCRAnnotations + 8 Target 0: (ksh) stopped. (lldb) bt * thread ksh-community#1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) * frame #0: 0x00007fff70deb1c2 libsystem_platform.dylib`_os_unfair_lock_recursive_abort + 23 frame ksh-community#1: 0x00007fff70de7c9a libsystem_platform.dylib`_os_unfair_lock_lock_slow + 239 frame ksh-community#2: 0x00007fff70daa3bd libsystem_malloc.dylib`tiny_malloc_should_clear + 188 frame ksh-community#3: 0x00007fff70daa20f libsystem_malloc.dylib`szone_malloc_should_clear + 66 frame ksh-community#4: 0x00007fff70dab444 libsystem_malloc.dylib`malloc_zone_calloc + 99 frame ksh-community#5: 0x00007fff70dab3c4 libsystem_malloc.dylib`calloc + 30 frame ksh-community#6: 0x000000010003fa5d ksh`sh_calloc(nmemb=1, size=16) at init.c:264:13 frame ksh-community#7: 0x000000010004f8a6 ksh`jobsave_create(pid=35055) at jobs.c:272:8 frame ksh-community#8: 0x000000010004ed42 ksh`job_reap(sig=20) at jobs.c:363:9 frame ksh-community#9: 0x000000010004ff6f ksh`job_waitsafe(sig=20) at jobs.c:511:3 frame ksh-community#10: 0x00007fff70de9b5d libsystem_platform.dylib`_sigtramp + 29 frame ksh-community#11: 0x00007fff70d39ac4 libsystem_kernel.dylib`__fork + 12 frame ksh-community#12: 0x00007fff70c57d80 libsystem_c.dylib`fork + 17 frame ksh-community#13: 0x000000010009590d ksh`sh_exec(t=0x0000000101005d30, flags=4) at xec.c:1883:16 frame ksh-community#14: 0x0000000100096013 ksh`sh_exec(t=0x0000000101005d30, flags=4) at xec.c:2019:4 frame ksh-community#15: 0x0000000100096c4f ksh`sh_exec(t=0x0000000101005a40, flags=5) at xec.c:2213:9 frame ksh-community#16: 0x0000000100096013 ksh`sh_exec(t=0x0000000101005a40, flags=5) at xec.c:2019:4 frame ksh-community#17: 0x000000010001c23f ksh`exfile(iop=0x0000000100405750, fno=-1) at main.c:603:4 frame ksh-community#18: 0x000000010001b23c ksh`sh_main(ac=3, av=0x00007ffeefbff4f0, userinit=0x0000000000000000) at main.c:365:2 frame ksh-community#19: 0x0000000100000776 ksh`main(argc=3, argv=0x00007ffeefbff4f0) at pmain.c:45:9 frame ksh-community#20: 0x00007fff70bfe3d5 libdyld.dylib`start + 1

The ASan crash in basic.sh when sourcing multiple files is caused by a bug that is similar to the crash fixed in f24040e. This is the trace for the regression test crash (note that in order to see the trace, the 2>/dev/null redirect must be disabled): ==1899388==ERROR: AddressSanitizer: heap-use-after-free on address 0x6150000005b0 at pc 0x55a5e3f9432a bp 0x7ffeb91ea110 sp 0x7ffeb91ea100 WRITE of size 8 at 0x6150000005b0 thread T0 #0 0x55a5e3f94329 in funct /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:967 ksh-community#1 0x55a5e3f96f77 in item /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:1349 ksh-community#2 0x55a5e3f90c9f in term /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:642 ksh-community#3 0x55a5e3f90ac1 in list /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:613 ksh-community#4 0x55a5e3f90845 in sh_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:561 ksh-community#5 0x55a5e3f909e0 in sh_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:586 ksh-community#6 0x55a5e3f8fd5e in sh_parse /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/parse.c:438 ksh-community#7 0x55a5e3fc43c1 in sh_eval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:635 ksh-community#8 0x55a5e4012172 in b_dot_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/misc.c:318 ksh-community#9 0x55a5e3fca3cb in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1254 ksh-community#10 0x55a5e3fd01d4 in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1932 ksh-community#11 0x55a5e3fc4544 in sh_eval /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:651 ksh-community#12 0x55a5e4012172 in b_dot_cmd /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/bltins/misc.c:318 ksh-community#13 0x55a5e3fca3cb in sh_exec /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/xec.c:1254 ksh-community#14 0x55a5e3ecc1cd in exfile /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:604 ksh-community#15 0x55a5e3ec9e7f in sh_main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:369 ksh-community#16 0x55a5e3ec801d in main /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:41 ksh-community#17 0x7f637b4db2cf (/usr/lib/libc.so.6+0x232cf) ksh-community#18 0x7f637b4db389 in __libc_start_main (/usr/lib/libc.so.6+0x23389) ksh-community#19 0x55a5e3ec7f24 in _start ../sysdeps/x86_64/start.S:115 Code in question: https://github.com/ksh93/ksh/blob/8d57369b0cb39074437dd82924b604155e30e1e0/src/cmd/ksh93/sh/parse.c#L963-L968 To avoid any more similar crashes, all of the fixes introduced in 69d37d5 that set slp->slptr to null have been improved with the fix in f24040e.

When ksh executes a script without a #! path (note that the AT&T team had a real disliking for #! paths), ksh forks and goes through a quick reinitialisation procedure. This is much faster than invoking a fully new shell but should have the same effect if it all works well. Unfortunately it's not worked all that well so far. Even after recent improvements (see referenced commits) I've been finding corner case problems. FYI, running a script without #! basically goes like this: * in path_spawn(), execve() fails with ENOEXEC because the file is not a binary executable and does not start with #! * this triggers 'case ENOEXEC:' which: * forks ksh * calls exscript() * exscript() cleans up & calls siglongjmp(*sh.jmplist,SH_JMPSCRIPT) * SH_JMPSCRIPT is the highest longjmp value, so *all* the previous sigsetjmp/sh_pushcontext calls are unwinded in reverse order, triggering all sorts of cleanup, state restoration, removal of local scopes, etc. * eventually, this lands us at the top sigsetjmp in sh_main() * sh_main() calls sh_reinit(), then resumes as if the shell had just been started This commit makes the following interrelated changes for the correct functioning of this procedure: 1. exscript() now exports the environment into a dedicated Stk_t buffer and sets environ[] to that. 2. Instead of re-using existing variables, sh_reinit() deletes everything and reinits all name-value trees from scratch, then re-imports the environment from environ[]. 3. Variable values that were imported from the environment are no longer treated specially with an NV_IMPORT attribute and the np->nvenv pointer to their value in environ[], fixing at least one crash.[*1] Details of the changes follow: src/cmd/ksh93/sh/path.c: - exscript(): Generate a new environ[] by activating a dedicated AST stack that will not be overwritten before calling sh_envgen(). This will allow sh_reinit() to delete all variables and then reimport the environment. The exporting must be done here, before siglongjmp, otherwise locally scoped exported variables won't be included (siglongjmp with SH_JMPSCRIPT triggers cleanup of all scopes). src/cmd/ksh93/sh/init.c: - sh_reinit(): Largely rewritten as follows. - Reset shell options first. This has the beneficial side effect of unsetting SH_RESTRICTED which interferes with unsetting certain variables, like PATH. - Remove workarounds for FPATH, SHLVL and tilde expansion disciplines; these will not be needed now. - Properly unset and delete all functions and built-ins. Since we now unset a function before deleting it, this should now free up their memory. (See nvdisc.c below for a change allowing removal of special built-ins.) - Properly unset all variables (which includes any associated discipline functions). Incorporate here the needed logic from sh_envnolocal() in name.c; most of it is unneeded (that function was previously used to cleanup local variables but has not been used for that for decades). So sh_envnolocal() is now unused. - Delete variables in a separate pass after unsetting variables and unsetting and deleting functions; this avoids use-after- free problems as well as possible "no parent" problems with namespace variables (e.g., .rc.status in our new kshrc.sh). - After all that, close and free up all function, alias, tracked alias, type and variable trees. - Free the contiguous built-in node space and the Init_t init context (with all the special variable discipline pointers). - Call nv_init (previously only called from sh_init) to reinitialise all of the above name-value stuff from scratch. It's the only way to be sure. - Re-import the environment as stored by exscript() above. - env_init(): - Per item 3 above and footnote 1 below, no longer set NV_IMPORT attribute and no longer point np->nvenv to the item in environ. - POSIX says, for 'environ': "Any application that directly modifies the pointers to which the environ variable points has undefined behavior."[*2] Yet, env_init() is indeed juggling the environ[] pointers to deal with variables that cannot be imported because their names are invalid (because they still need to be saved to be passed on to child processes). Replace the current approach with one where those env vars get allocated on the heap, pointed to by sh.save_env and counted by sh.save_env_n (renamed from sh.nenv). This only needs to be done once as ksh cannot use or change these variables. src/cmd/ksh93/sh/name.c: - sh_envgen(): Update to match env_init() change above. - pushnam() (called by sh_envgen()): Remove NV_IMPORT attribute check as per above and never get the value from the nvenv pointer -- simply always use nv_getval(). As of this change, the NV_IMPORT attribute is unused. The next commit will remove it and do related cleanups. - staknam(): is only called if value!=NULL, so remove that 'if'. - sh_envnolocal(): Removed. src/cmd/ksh93/sh/nvdisc.c: - assign(): Remove a check for the SH_INIT state bit that avoids freeing functions during sh_reinit(). This works fine now. - sh_addbuiltin(): Allow sh_reinit() to delete special builtins by checking for the SH_INIT state bit before throwing an error. src/cmd/ksh93/sh/nvtree.c: - outval(): Add a workaround for a use-after-free, introduced by the changes above, that occurred in the types.sh tests for #!-less scripts (types.sh:675-722). The use-after-free occurred here (abridged ASan trace follows; line numbers are current as of this commit): ==30849==ERROR: AddressSanitizer: heap-use-after-free [...] #0 in dttree dttree.c:393 ksh-community#1 in sh_reinit init.c:1637 ksh-community#2 in sh_main main.c:136 [...] The pointer was freed in the same loop via nv_delete() in outval: #0 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:[...]) ksh-community#1 in nv_delete name.c:1318 ksh-community#2 in outval nvtree.c:731 ksh-community#3 in genvalue nvtree.c:905 ksh-community#4 in walk_tree nvtree.c:1042 ksh-community#5 in put_tree nvtree.c:1108 ksh-community#6 in nv_putv nvdisc.c:144 ksh-community#7 in _nv_unset name.c:2437 ksh-community#8 in sh_reinit init.c:1645 ksh-community#9 in sh_main main.c:136 [...] So, what happened was that the nv_delete() call on name.c:1318 (eventually resulting from the _nv_unset call on init.c:1645) freed the node pointed to by np, so that the next loop iteration crashed on line 1637 as the dtnext() macro now gets a freed np. Now, why on earth should _nv_unset() *ever* indirectly call nv_delete()? That's a question for another day; I suspect it may be a bug, or it may be needed for compound variables for some reason. For now, I'm adding a workaround: simply avoid calling nv_delete() if the SH_INIT state bit is on, indicating sh_reinit() is in the call stack. This allows the variables unset loop in sh_reinit() to continue without crashing. sh_reinit() handles deletion later anyway. src/cmd/ksh93/sh/main.c: - sh_main(): remove zeroing of sh.fun_depth and sh.dot_depth; these are known to be 0, coming from either sh_init() or sh_reinit(). ________ [*1] This NV_IMPORT/nvenv usage is a redundant holdout from ancient ksh code; the imported value is easily available as a normal shell variable value via nv_getval(). Plus, the nvenv pointer is overloaded with too many other purposes: so far I've discovered it's used for pointers to subarrays of arrays (multidimentional arrays), compound variables, builtins, and other things. This mess caused at least one crash in set_instance() (xec.c) due to incorrectly using that nvenv pointer. The current kshrc script triggers this. Reproducer: $ export PS1 $ bin/package use «0»26:…/src/ksh93/ksh[dev] $ typeset +x PS1 ...and crash. That is now fixed. [*2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/environ.html

The referenced commit left one test unexecuted because it crashes. Minimal reproducer: typeset -a arr=((a b c) 1) got=$( typeset -a arr=( ( ((a b c)1))) ) The crash occurs when the array is redefined in a subshell. Here are abridged ASan stack traces for the crash, for the use after free, and for when it was freed: ================================================================= ==73147==ERROR: AddressSanitizer: heap-use-after-free [snippage] READ of size 8 at 0x000107403eb0 thread T0 #0 0x104fded40 in nv_search nvdisc.c:1007 ksh-community#1 0x104fbeb1c in nv_create name.c:860 ksh-community#2 0x104fb8b9c in nv_open name.c:1440 ksh-community#3 0x104fb1edc in nv_setlist name.c:309 ksh-community#4 0x104fb4a30 in nv_setlist name.c:475 ksh-community#5 0x105055b58 in sh_exec xec.c:1079 ksh-community#6 0x105045cd4 in sh_subshell subshell.c:654 ksh-community#7 0x104f92c1c in comsubst macro.c:2266 [snippage] 0x000107403eb0 is located 0 bytes inside of 80-byte region [snippage] freed by thread T0 here: #0 0x105c5ade4 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3ede4) ksh-community#1 0x105261da0 in dtclose dtclose.c:52 ksh-community#2 0x104f178cc in array_putval array.c:671 ksh-community#3 0x104fd7f4c in nv_putv nvdisc.c:144 ksh-community#4 0x104fbc5f0 in _nv_unset name.c:2435 ksh-community#5 0x104fb3250 in nv_setlist name.c:364 ksh-community#6 0x105055b58 in sh_exec xec.c:1079 ksh-community#7 0x105045cd4 in sh_subshell subshell.c:654 ksh-community#8 0x104f92c1c in comsubst macro.c:2266 [snippage] So the crash is caused because array_putval (array.c:671) calls dtclose, freeing ap->table, which is then reused after a recursive nv_setlist call via nv_open() -> nv_create() -> nv_search(). This only happens whwn we're in a virtual subshell. src/cmd/ksh93/sh/array.c: - array_putval(): When redefining an array in a virtual subshell, do not free the old ap->table; it will be needed by the parent shell environment.

fix a troff formatting error in the manpage.

9f600f5

jghub added the documentation Improvements or additions to documentation label Mar 9, 2020

jghub requested review from jelmd and marcastel March 9, 2020 12:27

marcastel approved these changes Mar 9, 2020

View reviewed changes

jelmd approved these changes Mar 9, 2020

View reviewed changes

JohnoKing mentioned this pull request Jun 25, 2020

Fix some errors in the documentation and other minor issues ksh93/ksh#42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a troff formatting error in the manpage. #5

fix a troff formatting error in the manpage. #5

jghub commented Mar 9, 2020

marcastel left a comment

jelmd commented Mar 9, 2020

jelmd left a comment

dannyweldon commented Mar 10, 2020

jelmd commented Mar 10, 2020

fix a troff formatting error in the manpage. #5

Are you sure you want to change the base?

fix a troff formatting error in the manpage. #5

Conversation

jghub commented Mar 9, 2020

marcastel left a comment

Choose a reason for hiding this comment

jelmd commented Mar 9, 2020

jelmd left a comment

Choose a reason for hiding this comment

dannyweldon commented Mar 10, 2020

jelmd commented Mar 10, 2020