Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20140801-diskfull.patch
Prior discussion:
https://www.mail-archive.com/ast-users@lists.research.att.com/msg01037.htmlhttps://www.mail-archive.com/ast-users@lists.research.att.com/msg01038.htmlhttps://www.mail-archive.com/ast-users@lists.research.att.com/msg01042.htmlhttps://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
> #1 0x000000000041e73d in sh_done (ptr=0x793360 <sh>, sig=255) at
> /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/fault.c:665
> #2 0x0000000000407407 in exfile (shp=0x4542, iop=0xff, fno=0) at
> /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/main.c:604
> #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
> #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
> #5 0x0000000000405b85 in main (argc=2, argv=0xfff000498) at
> /home/pcpa/rhel/ksh/ksh-20120801/src/cmd/ksh93/sh/pmain.c:45
> (gdb)
> ---8<---
Prior discussion:
https://bugzilla.redhat.com/1217236
Summary: doing
( some_simple_command & )
i.e., launching a background job from within a subshell, on a ksh
interactive login shell, caused misbehaviour (command not run).
For me on 93u+m, the misbehaviour was different -- an outright
crash in the job handling code following SIGCHLD, backtracing to:
0 ksh job_unpost + 49 (jobs.c:1655)
1 ksh job_reap + 1632 (jobs.c:468)
2 libsystem_platform.dylib _sigtramp + 29
3 ??? 0 + 4355528544
4 ksh ed_getchar + 102 (edit.c:1048)
5 ksh ed_emacsread + 659 (emacs.c:280)
[...]
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-nohupfork.patch
Lines 1874-1886 in sh_exec() optimise the highly specific case of
'( simple_command & )' by avoiding a sh_subshell() call that sets
up a virtual subshell before forking:
https://github.com/ksh93/ksh/blob/bd283959/src/cmd/ksh93/sh/xec.c#L1874-L1886
1874 else if(((type=t->par.partre->tre.tretyp)&FAMP) && ((type&COMMSK)==TFORK))
1875 {
1876 pid_t pid;
1877 sfsync(NIL(Sfio_t*));
1878 while((pid=fork())< 0)
1879 _sh_fork(shp,pid,0,0);
1880 if(pid==0)
1881 {
1882 sh_exec(t->par.partre,flags);
1883 shp->st.trapcom[0]=0;
1884 sh_done(shp,0);
1885 }
1886 }
1887 else
1888 sh_subshell(shp,t->par.partre,flags,0);
The original patch inserts the following before the sh_done call on
line 1884:
sh_offoption(SH_INTERACTIVE);
sh_done() checks for SH_INTERACTIVE and only runs job handling code
if that option is on.
Also, I had missed the need for an update of shgd->current_pid
here. Since 843b546c replaced lots of getpid() calls by reading
that variable, this could cause ksh to SIGCHLD the wrong process.
But even after adding the shgd->current_pid update to the RH patch,
I was still able to make it crash.
src/cmd/ksh93/sh/xec.c: sh_exec(): case TPAR:
- Disable this optimisation outright for interactive or job
control-enabled shells. I really don't trust it at all, but there
have been no problem reports for scripts without job control, so
keep it until such reports surface.
- Update shgd->current_pid so the child doesn't end up signalling
the wrong process (re: 843b546c).
___________________________________________________________________
P.S.:
It was noted in the Red Hat discussion that ( ... & ) does a double
fork, i.e., a virtual/non-forked subshell always forks before
forking the background job. This extra fork is done by the
following two lines in under 'case TFORK:' in sh_exec() in xec.c:
https://github.com/ksh93/ksh/blob/bd283959/src/cmd/ksh93/sh/xec.c#L1534-L1535
1534 if((type&(FAMP|TFORK))==(FAMP|TFORK))
1535 sh_subfork();
This is executed if we're in a virtual/non-forked subshell, i.e.
shp->subshell is true (note it is false for forked subshells). So
it forks the virtual subshell (the first fork) before running the
background job (the second fork). A background job is recognised by
'type' having both the FAMP (AMP == ampersand == &) and TFORK bits
set and no others.
So the obvious way to remove the double fork is to comment out
these two lines. Indeed, testing that confirms it gone and ksh
appears to work fine at first glance. Does it really? Nearly!
There are just four regression failures in a 'bin/shtests -p' run:
options.sh[394]: & job delayed by --pipefail, expected '1212 or 1221', got '1122'
signal.sh[280]: subshell ignoring signal does not send signal to parent (expected 'SIGUSR1', got 'done')
signal.sh[289]: subshell catching signal does not send signal to parent (expected 'SIGUSR1', got 'done')
subshell.sh[467]: sleep& in subshell hanging
So, those two lines cannot be removed now and we have to keep the
double fork. But removing them doesn't appear to break very much,
so it may be possible to add some tests so we only do an extra fork
when really necessary. That is a project for another day.
For one Red Hat customer, the following reproducer consistently
crashed, tough I was not able to reproduce it and neither was RH.
However, the crash analysis is sound (see below).
function dlog
{
fc -ln -0
}
trap dlog DEBUG
>/tmp/blah
Original patch: https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20140801-arraylen.patch
The Red Hat bug thread is closed to the public as it also contains
some correspondence with their customer. But it has an excellent
crash analysis from Thomas Gardner which I'm including here for the
record (the line numbers are for their ksh at the time, not 93u+m).
===begin analysis===
> The creation of an empty file instead of a command that executes
> anything causes the coredump.
[...]
> Here is my analysis on the core that was provided by the customer:
>
> (gdb) bt
> #0 sh_fmtq (string=0x1 <Address 0x1 out of bounds>)
> at /usr/src/debug/ksh-20120801/src/cmd/ksh93/sh/string.c:340
> #1 0x0000000000457e96 in out_string (cp=<value optimized out>, c=32,
> quoted=<value optimized out>, iop=<value optimized out>)
> at /usr/src/debug/ksh-20120801/src/cmd/ksh93/sh/xec.c:444
> #2 0x000000000045804c in sh_debug (shp=0x76d180, trap=0x7f2f13a821e0 "dlog",
> name=<value optimized out>, subscript=<value optimized out>,
> argv=0x76e070, flags=<value optimized out>)
> at /usr/src/debug/ksh-20120801/src/cmd/ksh93/sh/xec.c:548
> #3 0x000000000045a867 in sh_exec (t=0x7f2f13aafad0, flags=4)
> at /usr/src/debug/ksh-20120801/src/cmd/ksh93/sh/xec.c:1265
> [...need go no further...]
>
> In frame 2, we can see it cycling through your classic
> (char **)argv array like:
>
> 543 while(cp = *argv++)
> 544 {
> 545 if((flags&ARG_EXP) && argv[1]==0)
> 546 out_pattern(iop, cp,' ');
> 547 else
> 548 out_string(iop, cp,' ',n?0: (flags&(ARG_RAW|ARG_NOGLOB))||*argv);
> 549 }
> 550 if(flags&ARG_ASSIGN)
> 551 sfputc(iop,')');
> 552 else if(iop==stkstd)
>
> (we seg-fault after going down the out_string function in line
> 548 up there). The string pointer that points to = 0x1 up in
> frame #0 (sh_fmtq) traces back to the "cp" variable in line 548
> up there. The "argv" variable being referenced up there just gets
> passed in as the fifth argument to this function.
>
> In frame #3 (sh_exec, line 1265), the line that makes the call
> that takes us to frame 2 is:
>
> 1265 int n = sh_debug(shp,trap,(char*)0,(char*)0, com, ARG_R AW);
>
> so "com" (the fifth argument) is what's going wrong as it
> descends down through these calls. Looking at where it comes
> from, well, it's assigned here:
>
> 1241 if(argn==0)
> 1242 {
> 1243 /* fake 'true' built-in */
> 1244 np = SYSTRUE;
> 1245 *argv = nv_name(np);
> 1246 com = argv;
> 1247 }
>
> because as we can see:
>
> (gdb) f 3
> #3 0x000000000045a867 in sh_exec (t=0x7f2f13aafad0, flags=4)
> at /usr/src/debug/ksh-20120801/src/cmd/ksh93/sh/xec.c:1265
> 1265 int n = sh_debug(shp,trap,(char*)0,(char*)0, com, ARG_RAW);
> (gdb) p argn
> $2 = 0
> (gdb)
>
> argn is == 0 here. The tip-off here is that nv_name clearly
> returns a simple pointer to an array of characters, not an array
> of pointers to arrays of characters as is evidenced by the fact
> that the assignment is "*argv = nv_name(np);" not "argv =
> nv_name(np);". Looking at the function nv_name proves that it
> does indeed return a single pointer to an array of characters,
> not a pointer to an array of pointers to arrays of characters.
> Now, com is defined as a 'char **':
>
> 1002 char *cp=0, **com=0, *comn;
>
> (as it is expected to be in the calls that follow) also, that
> argv is also defined as the effective equivalent a 'char **':
>
> 1237 static char *argv[1];
>
> Yup, argv is actually an array of pointers (char ** equivalent),
> but that array is restricted to having exactly one element.
> Recalling the assignment in the previously quoted line:
>
> 1245 *argv = nv_name(np);
>
> we see that the one and only element in that argv array is
> getting assigned a pointer to an array of characters here.
> Nothing necessarily wrong with that, but remember the loop we
> looked at earlier in frame #2 (sh_debug). It went like:
>
> 543 while(cp = *argv++)
> 544 {
> 545 if((flags&ARG_EXP) && argv[1]==0)
> 546 out_pattern(iop, cp,' ');
> 547 else
> 548 out_string(iop, cp,' ',n?0: (flags&(ARG_RAW|ARG_NOGLOB))||*argv);
> 549 }
>
> which is clearly expecting argv in this context (com in frame 3,
> which really points to that static local single element array
> that is also pointed to by argv in frame 2) to be an array of
> pointers of indefinite size, each element being a pointer, but
> whose last element will be a null pointer. Well, in frame 3 it is
> clearly an array with only a single element, and that one element
> is NOT pointing to null. Watch this:
>
> (gdb) f 3
> #3 0x000000000045a867 in sh_exec (t=0x7f2f13aafad0, flags=4)
> at /usr/src/debug/ksh-20120801/src/cmd/ksh93/sh/xec.c:1265
> 1265 int n = sh_debug(shp,trap,(char*)0,(char*)0, com, ARG_RAW);
> (gdb) p com
> $8 = (char **) 0x76e060
> (gdb) p &argv
> $9 = (char *(*)[1]) 0x76e060
> (gdb) p com[0]
> $11 = 0x5009c6 "true"
> (gdb) p com[1]
> $10 = 0x1 <Address 0x1 out of bounds>
> (gdb) p argv[0]
> $12 = 0x5009c6 "true"
> (gdb) p argv[1]
> $13 = 0x1 <Address 0x1 out of bounds>
> (gdb)
>
> So, as expected, com and &argv point to the same place, the first
> element points to the constant string "true", but since the array
> is defined as having only one element, when you refer to a second
> element in that array, you get well, whatever random crap happens
> to be in that memory location. When we try to reproduce this
> problem, apparently we're getting 0 there (or we're not quite
> following this same code path, which is also possible), but the
> customer happens to have a "1" in that memory location.
===end analysis===
src/cmd/ksh93/sh/xec.c: sh_exec():
- When processing TCOM (simple command) with an empty/null command,
increase the size of the static dummy argv[1] array to argv[2],
ensuring a terminating NULL element so that 'while(cp = *argv++)'
loops don't crash. (Note that static objects are automatically
initialised to zero in C.)
src/cmd/ksh93/tests/io.sh:
- Adapt the reproducer, testing a null-command redirection 1000x.
Of course I was wrong to say the bug had nothing to do with
functions; traps in ksh functions are local, are handled the same
way as traps that are local to virtual subshells, and had the same
crashing bug. So this adds a test for that as well.
Contrary to the RH bug report, this is yet another bug with
virtual/non-forked subshells and has nothing to do with functions.
If a signal is ignored (empty trap) in the main shell while any
trap (empty or not) is set on the same signal in a subshell, a
crash eventually occurred upon restoring state when leaving the
subshell.
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-trapcom.patch
Prior discussion:
https://bugzilla.redhat.com/1117404
Paulo Andrade wrote there:
> The problem is that the sh_subshell function was saving pointers
> that could change, and when restoring, bad things would happen.
[...]
> The only comment I added:
> /* contents of shp->st.trapcom may change */
> may be a bit misleading, the "bad" save/restore already knows it,
> probably I should have added a better description telling that the
> data is, usually, modified in code like:
>
> tmp = buf[i]; buf[i] = strdup(tmp); free(tmp);
>
> so the shp->st.trapcom needs a "deep copy", as done in the
> patch, to properly save/restore pointers.
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- sh_subshell(), sh_funscope(): Make *savsig/*savstak into a
**savsig array. Use strdup(3) to save the data and get known
pointers that will not change. Free these upon restore.
- Change the comment from the patch as Paulo wished he had done.
src/cmd/ksh93/tests/subshell.sh:
- Test 2500 times. This should trigger the crash most of the time.
Another Red Hat patch. "Prior to this update, the result of a
command substitution was lost if a file descriptor used for the
substitution was previously explicitly closed. With this update,
ksh no longer reuses file descriptors that were closed during the
execution of a command substitution. Now, command substitutions
work as expected in the described situation."
Prior discussion:
https://bugzilla.redhat.com/1116072
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20140929-safefd.patch
src/cmd/ksh93/include/io.h,
src/cmd/ksh93/sh/io.c:
- Add sh_iosafefd() function to get a file descriptor that is not
in use or otherwise occupied (including marked as closed).
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Use that function to obtain a safe FD upon restoring state when
exiting a command substitution. I don't really know the how and
why -- all that I/O magic is still beyond me and the code is
uncommented as usual.
src/cmd/ksh93/tests/subshell.sh:
- Add regression test from the reproducer in the bug, reduced to
the minimum necessary.
The undocumented alarm builtin executes actions unsafely so that
'read' with an IFS assignment crashed when an alarm was triggered.
This applies an edited version of a Red Hat patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-alarmifs.patch
Prior discussion:
https://bugzilla.redhat.com/1176670
src/cmd/ksh93/bltins/alarm.c:
- Add a TODO note based on dgk's 2014 email cited in the RH bug.
- When executing the trap function, save and restore the IFS table.
src/cmd/ksh93/sh/init.c: get_ifs():
- Remove now-unnecessary SHOPT_MULTIBYTE preprocessor directives as
8477d2ce lets the compiler optimise out multibyte code if needed.
- Initialise the 0 position of the IFS table to S_EOF. This
corresponds with the static state tables in data/lexstates.c.
src/cmd/ksh93/tests/builtins.sh:
- Crash test.
This applies the following Red Hat patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-cdfork.patch
The associated bug report is public, but nearly all info (such as
a reproducer) has been wiped: https://bugzilla.redhat.com/1168611
However, the errata blurb is mildly informative:
"Previously, ksh sometimes incorrectly initialized a variable
holding the path of the working directory. If a program changed the
working directory between forking and ksh execution, then ksh could
contain an incorrect value in the working directory variable. With
this update, initialization of the working directory variable has
been corrected, and ksh now contains the correct value in the
aforementioned situation."
Also, the patch makes a lot of sense on the face of it. It removes
an optimisation in path_pwd() that checks for the directory defined
by e_crondir[] in data/msg.c, which is:
const char e_crondir[] = "/usr/spool/cron/atjobs";
Of /usr/spool not existed on any system for decades as it is common
to mount usr as read-only, so all the writable stuff was moved to
/var. So that would never check out. And if 'flag' is nonzero, the
optimizing 'count++' is executed regardless of whether that
directory exists, ensuring that it never gets the real PWD and
defaults to returning ".".
src/cmd/ksh93/sh/path.c:
- Apply patch as described.
- Mark 'flag' variable as NOT_USED to suppress compiler warning.
Keep it for backwards compat, as some programs that link with
libshell might use this function (though it's undocumented).
src/cmd/ksh93/include/path.h,
src/cmd/ksh93/data/msg.c:
- Remove now-unused e_crondir[].
This imports a new version of the code to import environment
variable values that was sent to Red Hat from upstream in 2014.
It avoids importing environment variables whose names are not valid
in the shell language, as it would be impossible to change or unset
them. However, they stay in the environment to be passed to child
processes.
Prior discussion: https://bugzilla.redhat.com/1147645
Original patch: https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-oldenvinit.patch
src/cmd/ksh93/sh/init.c:
- env_init(): Import new, simplified code to import environment
variable name/value pairs. Instead of doing the heavy lifting
itself, this version uses nv_open(), passing the NV_IDENT flag to
reject and skip invalid names.
- Get rid of gotos and a static var by splitting off the code to
import attributes into a new env_import_attributes() function.
This is a better way to avoid importing attributes when
initialising the shell in POSIX mode (re: 00d43960
- Remove an nv_mapchar() call that was based on some unclear
flaggery which was also removed by upstream as sent to Red Hat.
I don't know what that did, if anything; looks like it might have
had something to do with typeset -u/-l, but those particular
attributes have never been successfully inherited through the
environment.
(Maybe that's another bug, or maybe I just don't care as
inheriting attributes is a misfeature anyway; we have to put up
with it because legacy scripts might use it. Maybe someone can
prove it's an unacceptable security risk to import attributes
like readonly from an environment variable that is inherently
vulnerable to manipulation. That would be nice, as a CVE ID
would give us a solid reason to get rid of this nonsense.)
- Remove an 'else cp += 2;' that was very clearly a no-op; 'cp' is
immediately overwritten on the next loop iteration and not used
past the loop.
src/cmd/ksh93/tests/variables.sh:
- Test.
According to 'whence --man', 'whence -f' should ignore functions:
-f Do not check for functions.
Right now this is only accomplished partially. As of commit
a329c22d 'whence -f' avoids any output when encountering a
function (in ksh93u+ 'whence -f' has incorrect output). The
return value is still wrong though:
$ foo() { true; }
$ whence -f foo; echo $?
0
This commit fixes the return value and makes 'type -f' error out
when given a function (like in Bash).
src/cmd/ksh93/bltins/whence.c:
- If -f was passed, set 'cp' to NULL since functions should be
ignored (as documented).
- Simplify return value by avoiding bitwise logic.
src/cmd/ksh93/tests/builtins.sh:
- Add regression tests for 'whence -f' and 'type -f'.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
Another Red Hat patch of a patch. With the new comsub mechanism,
functions could sometimes return the wrong exit status when invoked
from a command substitution.
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-fununset.patch
I have determined that the extra setexit() in the Red Hat patch,
which copies the current exit status to $?, is not needed, as the
code for running functions already sets $? on termination. I've
added extra regression tests to prove this.
By the way, the setexit() macro is defined like this in defs.h:
#define exitset() (sh.savexit=sh.exitval)
That's more evidence (see also 3654ee73) that it does not
matter whether you address the shell's status struct via a
pointer. That macro is used in places that use shp pointers.
But, that aside...
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- When waiting within a command substitution for a forked process
to end, save & restore sh.exitval (the exit status of the command
currently being run) so that job_wait() cannot override it.
src/cmd/ksh93/tests/functions.sh:
- Add tests based in part on the reproducer from rhbz#1116508.
src/cmd/ksh93/sh/xec.c: sh_funct():
- The np->nvalue.rp pointer was dereferenced before the check that
it is non-null. Do this check before dereferencing it.
Since at least 1999, whence -v on pdksh (and its successor mksh)
reports the path where an autoloadable function may be found:
$ mkdir ~/fun; FPATH=~/fun
$ echo 'myfn() { echo hi; }' >~/fun/myfn
$ whence -v myfn
myfn is a undefined (autoload from /home/user/fun/myfn) function
Whereas ksh93 only reports, rather uselessly:
myfn is an undefined function
As of this commit, whence -v/-a on ksh 93u+m does the same as
pdksh, but with correct grammar:
myfn is an undefined function (autoload from /home/user/fun/myfn)
This may be a small violation of my own "no new features" policy
for 93u+m, but I couldn't resist. This omission has been annoying
me, and it's just embarrassing to lack a pdksh feature :)
src/cmd/ksh93/include/path.h,
src/cmd/ksh93/data/msg.c:
- Add e_autoloadfrom[] = " (autoload from %s)" message.
src/cmd/ksh93/bltins/whence.c: whence():
- Report the path (if any) when reporting an undefined function.
This needs to be done in two places:
1. When a function has been explicitly marked undefined with
'autoload', we need to do a quick path_search() loop to find
the path. (These undefined functions take precedence over
regular commands, so are reported first.)
2. When a function is not explicitly autoloaded but merely
available in $FPATH, that path search was already done, so all
we need to do is report it. (These are reported last.)
Note that the output remains as on 93u+ if no function definition
file is found on $FPATH. This is also like pdksh/mksh.
src/cmd/ksh93/data/builtins.c:
- Bump 'whence' version date. The inline docs never detailed very
exactly what 'whence -v' reports, so no need for further edits.
src/cmd/ksh93/tests/path.sh:
- Regress-test the new whence behaviour plus actual autoloading,
including the command override behaviour of autoloaded functions.
The fixargs() function is invoked when ksh needs to run a script
without a #!/hashbang/path. Instead of letting the kernel invoke a
shell, ksh exfile()s the script itself from sh_main(). In the
forked child, it calls fixargs() to set the argument list in the
environment to the args of the new script, so that 'ps' and
/proc/PID/cmdline show the expected output.
But fixargs() is broken because, on systems other than HP-UX (on
which ksh uses pstat(2)), ksh simply inserts a terminating zero.
The arguments list is not a zero-terminated C string. Unix systems
expect the entire arguments buffer to be zeroed out, otherwise 'ps'
and /proc/*/cmdline will have fragments of previous command lines
in the output.
The Red Hat patch for this bug is:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-argvfix.patch
However, that fix is incomplete because 'command_len' was also
hardcoded to be limited to 64 characters (!), which still gave
invalid 'ps' output if the erased command line was longer.
src/cmd/ksh93/sh/main.c: fixargs():
- Remove CMD_LENGTH macro which was defined as 64.
- Remove code that limited the erasure of the arguments buffer to
CMD_LENGTH characters. That code also had quite a dodgy strdup()
call -- it copies arguments to the heap, but they are never freed
(or even used), so it's a memory leak. Also, none of this is
ever done if the length is calculated using pstat(2) on HP-UX,
which is a clear indication that it's unnecessary.
(I think this code block must have been some experiment they
forgot to remove. One reason why I think so is that a 64 byte
arguments limit never made sense, even in the 1980s when they
wrote ksh on 80-column CRT displays. Another indication of this
is that fixing it didn't require adding anything; the code to do
the right thing was already there, it was just being overridden.)
- Zero out the full arguments length as in the Red Hat patch.
src/cmd/ksh93/tests/basic.sh:
- Add test. It's sort of involved because 'ps' is one of the least
portable commands in practice, in spite of standardisation.
This fixes a regression introduced in commit f9c127e3.
When the legacy code for older versions of libast was
removed, the fmtident wrapper wasn't removed. As a result,
the version string output by Ctrl+Alt+V is garbled because
the fmtident wrapper doesn't do any formatting:
$ <Ctrl+Alt+V>
^J@(#)$Id: Version AJM 93u+m 2020-09-14
src/cmd/ksh93/sh/string.c:
- Remove the old version of fmtident that was overriding
the current version of fmtident provided by libast
(in src/lib/libast/string/fmtident.c).
There was no check for the -B/braceexpand option before calling
path_expand() to process brace expansion, making it impossible to
turn off brace expansion within command substitutions. Normally the
lexer flags brace expansion so that this code is not reached, but
shell code within command substitutions is handled differently.
Red Hat patches this by adding this check to the function itself:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20140301-fikspand.patch
But I think it's more logical to patch it at the point of decision.
src/cmd/ksh93/sh/macro.c: endfield():
- Decide to call either path_generate() or path_expand() based on
the state of the SH_BRACEEXPAND shell option.
- Fix '#if SHOPT_BRACEPAT' preprocessor check that previously
hardcoded this decision at compile time.
src/cmd/ksh93/tests/options.sh:
- Add tests.
The new command substitution mechanism imported in 970069a6 from
Red Hat patches introduced this bug: backtick-style command
substitutions hang when processing about 117KiB of data or more.
It is fixed by another Red Hat patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20140415-hokaido.patch
It saves the value of the shp->comsub flag so that it is set to 2
(usually meaning new-style $(comsubs)) in two specific cases even
when processing backtick comsubs. This stops the sh_subtmpfile()
function in subshell.c from creating a /tmp file. However, I think
that approach is quite ugly, so I'm taking a slightly different one
that has the same effect.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/subshell.c:
- Redefine sh_subtmpfile() to pass the comsub flag as an argument.
(Remove the shp pointer argument, which is redundant; a pointer
to the shell state can easily be obtained in the function.)
src/cmd/ksh93/sh/xec.c: sh_exec():
- Apply the Red Hat fix by passing flag 2 to sh_subtmpfile().
src/cmd/ksh93/tests/subshell.sh:
- Move regress test from ce68e1be from basic.sh to here; this is
the place for command substitution tests as they are subshells.
- Add regress test for this bug.
All other changed files:
- Update sh_subtmpfile() calls to pass on the shp->comsub flag.
When using typeset -l or -u on a variable that cannot be changed
when the shell is in restricted mode, ksh crashed.
This fixed is inspired by this Red Hat fix, which is incomplete:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-tpstl.patch
The crash was caused by the nv_shell() function. It walks though a
discipline function tree to get the pointer to the interpreter
associated with it. Evidently, the problem is that some pointer in
that walk is not set correctly for all special variables.
Thing is, ksh only has one shell language interpreter, and only one
global data structure (called 'sh') to keep its main state[*]. Yet,
the code is full of 'shp' pointers to that structure. Most (not
all) functions pass that pointer around to each other, accessing
that struct indirectly, ostensibly to account for the non-existent
possibility that there might be more than one interpreter state.
The "why" of that is an interesting cause for speculation that I
may get to sometime. For now, it is enough to know that, in the
code as it is, it matters not one iota what pointer to the shell
interpreter state is used; they all point to the same thing (unless
it's broken, as in this bug).
So, rather than fixing nv_shell() and/or associated pointer
assignments, this commit simply removes it, and replaces it with
calls to sh_getinterp(), which always returns a pointer to sh (see
init.c, where that function is defined as literally 'return &sh').
[*] Defined in shell.h, with the _SH_PRIVATE part in defs.h
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/name.c:
- Remove nv_shell().
src/cmd/ksh93/sh/init.c:
- In all the discipline functions for special variables, initialise
shp using sh_getinterp() instead of nv_shell().
src/cmd/ksh93/tests/variables.sh:
- Add regression test for typeset -l/-u on all special variables.
Now that we have ${.sh.pid} a.k.a. shgd->current_pid, which is
updated using getpid() whenever forking a new process, there is no
need for anything else to ever call getpid(); we can use the stored
value instead. There were a lot of these syscalls kicking around,
some of them in performance-sensitive places.
The following lists only changes *other* than changing getpid() to
shgd->currentpid.
src/cmd/ksh93/include/defs.h:
- Comments: clarify what shgd->{pid,ppid,current_pid} are for.
src/cmd/ksh93/sh/main.c,
src/cmd/ksh93/sh/init.c:
- On reinit for a new script, update shgd->{pid,ppid,current_pid}
in the sh_reinit() function itself instead of calling sh_reinit()
from sh_main() and then updating those immediately after that
call. It just makes more sense this way. Nothing else ever calls
sh_reinit() so there are no side effects.
src/cmd/ksh93/sh/xec.c: _sh_fork():
- Update shgd->current_pid in the child early, so that the rest of
the function can use it instead of calling getpid() again.
- Remove reassignment of SH_PIDNOD->nvalue.lp value pointer to
shgd->current_pid (which makes ${.sh.pid} work in the shell).
It's constant and was already set on init.
This imports another fix from Red Hat/Fedora. Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-crash.patch
src/cmd/ksh93/include/jobs.h,
src/cmd/ksh93/sh/jobs.c,
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Import the Red Hat fix with these differences:
- Rename the 'hack1_waitall' variable to 'bktick_waitall' and add
a comment describing what it's for.
- Remove unused 'pipefail' variable.
src/cmd/ksh93/tests/basic.sh:
- Regression test from reproducer given in the Red Hat bug report.
- Add special handling to SIGKILL it, as it might freeze hard.
var=$(< file) now reads the file even if the standard inout,
standard output and/or standard error file descriptors are closed.
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-filecomsubst.patch
src/cmd/ksh93/sh/io.c: sh_redirect():
- When processing the '<' redirector as part of $(< ...), i.e. if
flag==3, make sure the FD of the file to read is > 2 by calling
sh_iomovefd(). Unlike the RedHat patch, this checks for flag==3
to avoid unnecessary sh_iomovefd() calls for normal redirections,
as there was no bug with those.
src/cmd/ksh93/tests/io.sh:
- Add test.
When ksh was compiled with SHOPT_SPAWN (the default), any command
substitution embedded in a here-document returned an empty string.
The bug was also present in 93u+ 2012-08-01 (although not in every
case as some systems compile it without SHOPT_SPAWN).
This fixes it by applying a slightly edited combination of two Red
Hat patches (the second containing a fix for the first), which
backport a new command substitution mechanism from the abandoned
ksh 93v- beta version. The originals are:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-macro.patchhttps://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-fd2lost.patch
src/cmd/ksh93/include/io.h:
- The iopipe() function from xec.c is now needed in sh_subshell()
(subshell.c), so rename it to sh_iounpipe() and declare it as an
extern here. The 93v- beta did it as well. (The Red Hat patch did
this without renaming it.)
src/cmd/ksh93/sh/xec.c:
- Backport new versions of iousepipe() and sh_iounpipe() from ksh
93v-. New 'type' flaggery is introduced to distinguish between
different command substitution conditions. What all that means
remains to be determined.
- sh_exec(): I made one change to the Red Hat patch myself: if in a
subshell and the type flags FAMP (for "ampersand" as in '&' as in
background job) and TFORK are set, continue to call sh_subfork()
to fork the subshell unconditionally, instead of only if we're in
a command substitution connected to an unseekable file. Maybe the
latter works for the 93v- code, but on 93u+(m) it causes a couple
of regressions, which are fixed by my change:
signal.sh[273]: subshell ignoring signal does not send signal to parent
signal.sh[276]: subshell catching signal does not send signal to parent
Details: https://github.com/ksh93/ksh/issues/104#issuecomment-696341902
src/cmd/ksh93/sh/macro.c,
src/cmd/ksh93/sh/subshell.c:
- Updates that go with those new functions.
Fixes: https://github.com/ksh93/ksh/issues/104
Affects: https://github.com/ksh93/ksh/issues/124
This fixes two memory leaks in old-style command substitutions
(one when invoking an alias, one when invoking an autoloaded
function), as well as a possible third leak with an unknown
reproducer, by applying this Red Hat patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-mlikfiks.patch
src/cmd/ksh93/sh/macro.c: comsubst():
- For as-yet unknown reasons, the alias leak did not occur when
adding a space at the end of the command substitution, as in
a=`some_alias `. This fix is a workaround that simply writes
an extra space to the stack. TODO: a real fix.
src/cmd/ksh93/sh/path.c: funload():
- Add missing free() before return. This fixes the leak with
autoloaded functions.
src/cmd/ksh93/sh/lex.c: alias_exceptf():
- This function is called "whenever an end of string is found with
alias". This adds a check for an SF_FINAL stream status flag when
deciding whether to call free(). In sfio.h this is commented as:
#define SF_FINAL 11 /* closing is done except stream free */
When I revert this change, none of the regression tests fail, so
I don't know how to trigger this supposed leak. But it makes some
sense given the sfio.h comment, so I'll keep it.
src/cmd/ksh93/tests/leaks.sh:
- Add the reproducers from rhbz#982142 as regression tests
(including an extra one for nested command substitutions that was
already fixed as of 93u+, but testing is good).
I replaced the external 'expr' and 'ls' commands by uses of
the 'true' builtin, otherwise the tests take far too long to run
with 16384 iterations. At least the alias leak was still behaving
identically after replacing 'ls' by 'true'.
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#2,
#4 and #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 #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 #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 #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 #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 99065353 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: https://github.com/ksh93/ksh/issues/84
{Brace,expansion} is potentially incompatible with POSIX scripts,
because in POSIX those are simple literal strings with no special
meaning. So the POSIX option should really turn that off.
As of b301d417, the 'posix' option was also forcing 'letoctal'
behaviour on, without actually setting that option. I've since
found that to be a botch; 'let' may recognise octals without that
option being set, and that looks like a bug.
So as of this commit, the '-o posix' option actually toggles both
of these options off/on and on/of, respectively. 'set +o posix'
toggles them inversely. However, it is now possible to control both
options (and their associated behaviour) independently in between
'set -o posix' and 'set +o posix'. Much better.
src/cmd/ksh93/sh/main.c: sh_main():
- If SH_POSIX was set on init, turn on SH_LETOCTAL by default
instead of SH_BRACEEXPAND.
src/cmd/ksh93/sh/args.c: sh_applyopts():
- Turn off SH_BRACEEXPAND and turn on SH_LETOCTAL when SH_POSIX is
turned on (but not if it was already on).
- Turn on SH_BRACEEXPAND and turn off SH_LETOCTAL when SH_POSIX is
turned off (but not if it was already off).
src/cmd/ksh93/sh/arith.c: arith():
- Revert to pre-b301d417 and only check SH_LETOCTAL option when
deciding whether 'let' should skip initial zeros.
src/cmd/ksh93/tests/options.sh:
- Update $- test to allow '-o posix' to switch B = braceexpand.
src/cmd/ksh93/sh.1:
- Update.
- Edit for clarity.
This allows running 'bin/shtests leaks' on a ksh without the
vmstate builtin and/or that is not compiled with AST vmalloc.
It falls back to 'ps -o rss= -p $$' to get the memory state.
This is in preparation for the beta and release versions, which
will not use vmalloc due to its defects[*]. Unfortunately,
abandoning vmalloc means abandoning the vmstate builtin which makes
it extremely efficient to test for memory leaks.
Because 'ps' only has a KiB granularity and also shows larger
artefacts/variations than vmalloc on most systems, we need many
more iterations (16384) and also tolerate a higher number of bytes
per iterations (8). So the run takes much longer. To tolerate only
2 bytes per iteration, we would need four times as many iterations,
which would make it take too long to run. Since a single word (e.g.
one pointer) on a 64-bit system is 8 bytes, it still seems very
unlikely for a real memory leak to be that small.
This is coded to make it easy to detect and add iteration and
tolerance parameters for a new method to get the memory state,
if some efficient or precise system-specific way is discovered.
I've also managed to trigger a false leak with shcomp in a UTF-8
locale on CentOS on a ksh with vmalloc/vmstate. So this increases
the tolerance for vmalloc from 2 to 4 bytes/iteration.
[*] Discussion: https://github.com/ksh93/ksh/issues/95
When compiling ksh with '-O0 -g -D_std_malloc' on my Mac, the
paths.sh regress test set crashed. This is the test that crashed:
print 'FPATH=../fun' > bin/.paths
cat <<- \EOF > fun/myfun
function myfun
{
print myfun
}
EOF
x=$(FPATH= PATH=$PWD/bin $SHELL -c ': $(whence less);myfun') 2> /dev/null
[[ $x == myfun ]] || err_exit 'function myfun not found'
The crash occurred on the second-to-last line. The backtrace
suggests an invalid use of strcpy() with overlapping memory:
0 libsystem_kernel.dylib __pthread_kill + 10
1 libsystem_pthread.dylib pthread_kill + 284
2 libsystem_c.dylib abort + 127
3 libsystem_c.dylib abort_report_np + 177
4 libsystem_c.dylib __chk_fail + 48
5 libsystem_c.dylib __chk_fail_overlap + 16
6 libsystem_c.dylib __chk_overlap + 34
7 libsystem_c.dylib __strcpy_chk + 64
8 ksh path_chkpaths + 1038 (path.c:1534)
9 ksh path_addcomp + 1032 (path.c:1481)
10 ksh path_addpath + 395 (path.c:1598)
11 ksh put_restricted + 626 (init.c:329)
[...]
src/cmd/ksh93/sh/path.c: path_chkpaths():
- When reading the '.paths' file, use memmove(3) instead of
strcpy(3) as the former does a non-destructive copy with
tolerance for overlap.
The following set of commands caused ksh to crash:
$ unalias history; unalias r
Memory fault
When ksh is compiled with -D_std_malloc, the crash always
occurs when the 'r' alias is removed with 'unalias r',
although with vmalloc 'unalias history' must be run first
for the crash to occur. With the native malloc, the crash
message is also different:
$ unalias history; unalias r
free(): invalid pointer
Abort
This crash happens because when an alias is unset, _nv_unset
removes the NV_NOFREE flag which results in an invalid use
of free(3) as nv_isattr no longer detects NV_NOFREE afterward.
The history and r aliases shouldn't be freed from memory by
nv_delete because those aliases are given the NV_NOFREE attribute.
src/cmd/ksh93/bltins/typeset.c:
- Save the state of NV_NOFREE for aliases to fix the crash
caused by 'unalias r'.
src/cmd/ksh93/tests/alias.sh:
- Use unalias on both history and r to check for the crash.
'unalias -a' can't be used to replicate the crash.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This patch from Red Hat fixes the following:
1. ksh was ignoring the -m (-o monitor) option when specified on
the invocation command line.
2. Scripts did not properly terminate their background processes
on Ctrl+C if the -m option was turned off. Reproducer:
xterm &
read junk
When run as a script without turning on -m, pressing Ctrl+C
should terminate the xterm, and now does.
3. Scripts no longer attempt to set the terminal foreground process
group ID, as only interactive shells should be doing that.
This makes some progress on https://github.com/ksh93/ksh/issues/119
but we're a long way from fixing all of that.
src/cmd/ksh93/sh/main.c: exfile():
- On non-interactive shells, do not turn off the monitor option.
Instead, if it was turned on, turn on the SH_MONITOR state flag.
src/cmd/ksh93/edit/edit.c: ed_getchar():
- On Ctrl+C, issue SIGINT to the current process group using
killpg(2) instead of going via sh_fault(), which handles a
signal only for the current shell process.
src/cmd/ksh93/sh/jobs.c: job_reap(), job_reset(),
src/cmd/ksh93/sh/xec.c: sh_exec():
- Only attempt to set the terminal foreground process group ID
using tcsetpgrp(3) if the shell is interactive.
Original patch: https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-kshmfix.patch
This was applied to Red Hat's ksh 93u+ on 8 July 2013.
src/cmd/ksh93/data/signals.c includes two checks for the JOBS
identifier; if it is not defined then the interactive shell's
background job signal messages for SIGINT and SIGPIPE are empty.
The cause was that the "jobs.h" header, which defines that ID, was
not #included in signals.c. This commit adds that #include.
(ksh 93u+, ksh 93v- and ksh2020 all have this bug as well.)
Before:
$ sleep 30 &
[1] 86430
$ kill -s INT "$!"
[1] + sleep 30 &
$
After:
$ sleep 30 &
[1] 86445
$ kill -s INT "$!"
[1] + Interrupt sleep 30 &
$
