ksh 93u+ has a subshell leak bug where a variable exported in
function in a subshell is also visible in a different subshell.
This is long fixed in 93u+m, but there wasn't a regression test for
this particular bug yet, so this commit adds one.
This fixes the following bug filed with Solaris: "22964338 ksh93
appears to send SIGHUP to unrelated processes on occasion". It is
fixed by applying this patch by Lijo George from the Solaris repo:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/260-22964338.patch
The ksh2020 upstream rejected this, but if it's in production use
in Solaris, Solaris, it's probably good enough for 93u+m. If any
breakage is left, it can be fixed later.
https://github.com/att/ast/pull/1
src/cmd/ksh93/include/jobs.h,
src/cmd/ksh93/sh/fault.c,
src/cmd/ksh93/sh/jobs.c:
- Use a new job_hup() function instead of job_kill() to send SIGHUP
to job processes on termination. The new function checks if a job
is in fact still live before issuing SIGHUP to it.
This pulls a new version of sh_iosafefd() from:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/285-30771135.patch
It was written by Kurtis Rader for ksh2020:
https://github.com/att/ast/issues/198https://github.com/att/ast/pull/199
It is presumably better than the Red Hat version and also comes
with more regression test cases (although it still doesn't fix
modernish BUG_CSUBSTDO, which remains in the TODO file).
This commit does not go along with other peripheral changes from
that patch, i.e. a different name and location of this function.
src/cmd/ksh93/sh/io.c:
- Replace sh_iosafefd() as above.
src/cmd/ksh93/tests/subshell.sh:
- Add and tweak tests from the patch.
This change is pulled from here:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/280-23332860.patch
Info and reproducers:
https://github.com/att/ast/issues/36
In a -c script (like ksh -c 'commands'), the last command
misredirects standard output if an EXIT or ERR trap is set.
This appears to be a side effect of the optimisation that
runs the last command without forking.
This applies a patch by George Lijo that flags these specific
cases and disables the optimisation.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/bltins/trap.c,
src/cmd/ksh93/sh/init.c,
src/cmd/ksh93/sh/main.c,
src/cmd/ksh93/sh/xec.c:
- Apply patch as above.
src/cmd/ksh93/tests/io.sh:
- Add the reproducers from the bug report as regression tests.
This change is pulled from here:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/275-20855453.patchhttps://github.com/att/ast/issues/30
George Lijo wrote on 17 Feb 2017:
> Here's a reproducible testcase on a Solaris11 host running
> ksh93u+(2012-08-01).
> $ cat a.sh
> #!/bin/sh
>
> AAA="aaa"
> echo 'insert character'
> BBB=`echo ${AAA} | sed "s/aaa/bbb/g"`
> logger "variable BBB = ${BBB}"
>
> $ cat t.sh
> #!/bin/ksh
>
> sleep 10
> /bin/ksh ./a.sh
> exit 0
>
> $
>
> $ ./t.sh
>
> The expected result is:
>
> Apr 9 12:43:34 lab user: [ID 702911 user.notice] variable BBB = bbb
>
> because variable "BBB" is supposed to be set to 'bbb' in a.sh.
>
> But if the parent shell is terminated, the variable is wrongly set.
>
> user@xxxxx$ telnet lab
> ...
> $ ./t.sh & <--- Run t.sh in background.
> [1] 2067
> $ logout <--- CTRL + D to exit while t.sh is running.
> Connection to lab closed by foreign host.
>
> Again, access the system and check the output:
>
> user@xxxxx$ telnet lab
> ...
> $ tail -f /var/adm/messages
> :
> Apr 9 12:47:47 lab user: [ID 702911 user.notice] variable BBB =
> insert character <--- !!!
> Apr 9 12:47:47 lab bbb
> <--- !!!
>
> Thus the variable is wrongly set. (The previous echo string was
> not cleared.)
>
> The issue happens because the EIO error during the logout is not
> handled properly.
src/cmd/ksh93/sh/io.c,
src/lib/libast/include/error.h:
- Amend the ERROR_PIPE() macro to check for EIO as well as EPIPE
and ECONNRESET.
This change is pulled from here:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/240-22461939.patch
Information:
https://github.com/att/ast/issues/6
George Lijo wrote on 14 Mar 2016:
> I observed this issue in a Solaris 11 system on ksh2012-08-01
> [...]. The issue can be reproduced if we add Asian locales to
> ibus (such as Korean). In the ksh93 shell prompt, input some
> Asian character. ksh promptly dumps core [...].
>
> The coredump happens at the following line no 320 in
> src/cmd/ksh93/edit/emacs.c
> if(c!='\t' && c!=ESC && !isdigit(c)).
>
> I referred the vi.c code and added the digit(c) macro, i.e.
> ((c&~STRIP)==0 && isdigit(c)) and replaced the isdigit(c) usage
> with the "digit(c)" macro.
This change is pulled from here:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/211-21547336.patch
src/cmd/ksh93/bltins/cd_pwd.c: b_cd():
- The functions path_pwd() and path_relative() in sh/path.c may
return a pointer to e_dot[] (".") as a fallback if they fail to
determine a path. This is a string in read-only memory
(data/msg.c), so must not be freed. A pointer to that string may
end up in sh.pwd (== shp->pwd), so b_cd() needs a check for that.
This change is pulled from here:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/110-CR7061011.patch
Unfortunately there is no publicly available documentation on why
this change was needed. We just have to assume the Solaris people
knew what they were doing. ksh2020 upstreamed this too (as well as
all the other Solaris patches applied here).
src/cmd/ksh93/sh/nvdisc.c: setdisc():
- If no <event> is known for <np>, return a null pointer instead
of a pointer to the empty string.
The forking fix implemented in 102868f8 and 9d428f8f, which stops
the main shell's hash table from being cleared if PATH is changed
in a subshell, can cause a significant performance penalty for
certain scripts that do something like
( PATH=... command foo )
in a subshell, especially if done repeatedly. This is because the
hash table is cleared (and hence a subshell forks) even for
temporary PATH assignments preceding commands.
It also just plain doesn't work. For instance:
$ hash -r; (ls) >/dev/null; hash
ls=/bin/ls
Simply running an external command in a subshell caches the path in
the hash table that is shared with a main shell. To remedy this, we
would have to fork the subshell before forking any external
command. And that would be an unacceptable performance regression.
Virtual subshells do not need to fork when changing PATH if they
get their own hash tables. This commit adds these. The code for
alias subshell trees (which was removed in ec888867 because they
were broken and unneeded) provided the beginning of a template for
their implementation.
src/cmd/ksh93/sh/subshell.c:
- struct subshell: Add strack pointer to subshell hash table.
- Add sh_subtracktree(): return pointer to subshell hash table.
- sh_subfuntree(): Refactor a bit for legibility.
- sh_subshell(): Add code for cleaning up subshell hash table.
src/cmd/ksh93/sh/name.c:
- nv_putval(): Remove code to fork a subshell upon resetting PATH.
- nv_rehash(): When in a subshell, invalidate a hash table entry
for a subshell by creating the subshell scope if needed, then
giving that entry the NV_NOALIAS attribute to invalidate it.
src/cmd/ksh93/sh/path.c: path_search():
- To set a tracked alias/hash table entry, use sh_subtracktree()
and pass the HASH_NOSCOPE flag to nv_search() so that any new
entries are added to the current subshell table (if any) and do
not influence any parent scopes.
src/cmd/ksh93/bltins/typeset.c: b_alias():
- b_alias(): For hash table entries, use sh_subtracktree() instead
of forking a subshell. Keep forking for normal aliases.
- setall(): To set a tracked alias/hash table entry, pass the
HASH_NOSCOPE flag to nv_search() so that any new entries are
added to the current subshell table (if any) and do not influence
any parent scopes.
src/cmd/ksh93/sh/init.c: put_restricted():
- Update code for clearing the hash table (when changing $PATH) to
use sh_subtracktree().
src/cmd/ksh93/bltins/cd_pwd.c:
- When invalidating path name bindings to relative paths, use the
subshell hash tree if applicable by calling sh_subtracktree().
- rehash(): Call nv_rehash() instead of _nv_unset()ting the hash
table entry; this is needed to work correctly in subshells.
src/cmd/ksh93/tests/leaks.sh:
- Add leak tests for various PATH-related operations in the main
shell and in a virtual subshell.
- Several pre-existing memory leaks are exposed by the new tests
(I've confirmed these in 93u+). The tests are disabled and marked
TODO for now, as these bugs have not yet been fixed.
src/cmd/ksh93/tests/subshell.sh:
- Update.
Resolves: https://github.com/ksh93/ksh/issues/66
The SHOPT_2DMATCH code block in sh_setmatch() modifies the 'ap'
pointer, which is initialised as nv_arrayptr(SH_MATCHNOD). This
caused a (rarely occurring) segfault in the following line near the
end of the function:
ap->nelem -= x;
as this line assumed that 'ap' still had the initial value.
src/cmd/ksh93/sh/init.c: sh_setmatch():
- On init, save ap in ap_save and use ap_save instead of ap where
it should be pointing to SH_MATCHNOD. This also allows removing
two redundant nv_arrayptr(SH_MATCHNOD) calls, slightly increasing
the efficiency of this function.
To ignore binaries generated by 'bin/package flat make', the
entries all need an initial '/' or they will be ignored on deeper
hierarchy levels as well.
Scripts that use floating point shell arithmetic confusingly fail
with 'arithmetic syntax error' when running in a locale that uses
',' as the radix point, because '.' is generally assumed by
scripts. The error message is confounding as the problem is not a
syntax error but a locale that is incompatible with the script.
src/cmd/ksh93/sh/arith.c:
- If the locale's radix point is not '.' but a '.' is found in its
place, issue an informative error message that instructs setting
LC_NUMERIC=C.
Resolves: https://github.com/ksh93/ksh/issues/145
@stephane-chazelas writes:
> Per POSIX[*], cd should skip the $CDPATH processing if the first
> component of the directory given to cd is . or ...
>
> Yet, with ksh93u+m 2021-01-03 at least, while that's OK with ..,
> it's not with . with or without the posix option:
>
> $ CDPATH=/ ./ksh -o posix -c 'cd -P ./etc && pwd'
> /etc
> /etc
>
> It seems to be a regression introduced with ksh93u+ as I can't
> reproduce it with ksh93u or any version prior to that. I can also
> reproduce in u+, v- and the ksh2020 from the Ubuntu 20.04
> package.
src/cmd/ksh93/bltins/cd_pwd.c: b_cd():
- Skip $CDPATH processing not only if the path is absolute, but
also if the initial path component is '.' or '..' (in the latter
case the $CDPATH processing was done but appeared to be a no-op).
src/cmd/ksh93/tests/builtins.sh:
- Add regression test.
[*] https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/cd.html
Fixes: https://github.com/ksh93/ksh/issues/151
As of this commit, ksh 93u+m has a standard semantic version number
<https://semver.org/>, beginning with 1.0.0-alpha. This is added to
the version string in a way that should be compatible with scripts
parsing ${.sh.version} or $(ksh --version). This addition does not
replace the release date and does not affect $((.sh.version)).
For non-release builds, the version string will be:
FORK/VERSION+HASH YYYY-MM-DD
e.g.: 93u+m/1.0.0-alpha+41ef7f76 2021-01-03
For release builds, it will be:
FORK/VERSION YYYY-MM-DD
e.g.: 93u+m/1.0.0 2021-01-03
It is now automatically decided by bin/package whether to build a
release or development build. When building from a directory that
is not a git repository, or if the current git branch name starts
with a number (e.g. '1.0'), the release build is enabled; otherwise
a development build is the default. This is arranged by adding -D
flags to $CCFLAGS as described below. These flags are prepended to
$CCFLAGS, so they can be overridden by adding your own -D or -U
flags via the environment.
In addition, AST vmalloc is disabled for release builds as of this
commit, forcing the use of the OS's standard malloc(3). In 2021,
this is generally more reliable, faster, and more economical with
memory than AST vmalloc. Several memory leaks and crashing bugs are
avoided, e.g.: <https://github.com/ksh93/ksh/issues/95>.
For development builds, vmalloc stays enabled (along with its known
bugs) because this allows the use of the vmstat builtin, making it
much more efficient to test for memory leaks. For more info, see
the regression test script: src/cmd/ksh93/tests/leaks.sh
bin/package, src/cmd/INIT/package.sh:
- Add flags for build type. In $CCFLAGS, define _AST_ksh_release if
we're not on any git branch or on a git branch whose name starts
with a number. Otherwise, define _AST_git_commit as the first 8
characters of the current git commit hash.
src/lib/libast/features/vmalloc:
- If _AST_ksh_release is defined, disable vmalloc and force use of
the operating system's malloc. Discussion:
https://github.com/ksh93/ksh/issues/95
src/cmd/ksh93/include/version.h:
- Define new format for version string, adding a semantic version
number as well as (for non-release builds) the git commit hash.
src/cmd/ksh93/sh/init.c: e_version[]:
- Add a 'v' to the ${.sh.version} feature string if ksh was
compiled with vmalloc enabled. This allows scripts, such as
regression tests, to detect this.
src/cmd/ksh93/data/builtins.c: sh_optksh[]:
- Add a copyright line crediting the contributors to ksh 93u+m.
Resolves: https://github.com/ksh93/ksh/issues/95
When starting ksh +s, it gets stuck in an infinite loop continually
trying to parse its own binary as a shell script and rejecting it:
$ arch/linux.i386-64/bin/ksh +s
arch/linux.i386-64/bin/ksh: arch/linux.i386-64/bin/ksh: cannot execute [Exec format error]
arch/linux.i386-64/bin/ksh: arch/linux.i386-64/bin/ksh: cannot execute [Exec format error]
arch/linux.i386-64/bin/ksh: arch/linux.i386-64/bin/ksh: cannot execute [Exec format error]
arch/linux.i386-64/bin/ksh: arch/linux.i386-64/bin/ksh: cannot execute [Exec format error]
arch/linux.i386-64/bin/ksh: arch/linux.i386-64/bin/ksh: cannot execute [Exec format error]
[...]
$ echo 'echo "this is stdin"' | arch/linux.i386-64/bin/ksh +s
arch/linux.i386-64/bin/ksh: arch/linux.i386-64/bin/ksh: cannot execute [Exec format error]
(no loop, but still ksh trying to parse itself)
src/cmd/ksh93/sh/init.c: sh_init():
- When forcing on the '-s' option upon finding no command
arguments, also update sh.offoptions, a.k.a. shp->offoptions.
This avoids the inconsistent state causing this problem.
In main.c, there is:
if(sh_isoption(SH_SFLAG))
fdin = 0;
else
(code to open $0 as a file)
This was entering the else block because sh_isoption(SH_SFLAG)
was returning 0, and $0 is set to the ksh binary as it is
supposed to when no other script is provided. When I looked for
why sh_isoption was returning 0, I found main.c's
for(i=0; i<elementsof(shp->offoptions.v); i++)
shp->options.v[i] &= ~shp->offoptions.v[i];
Before this loop, shp->offoptions tracks which options were
explicitly disabled by the user on the command line. The effect
of this loop is to make "explicitly disabled" take precedence
over "implicitly enabled". My patch removes the registration of
the +s option.
Fixes: https://github.com/ksh93/ksh/issues/150
Co-authored-by: Martijn Dekker <martijn@inlv.org>
A recent change in Github's Ubuntu test runners exposed a problem
in the way the all_paths test function replicates 'whence -a'
functionality, causing spurious regression test failures.
The problem occurs when e.g. /bin is a symlink to /usr/bin, but
both are in $PATH. The all_paths function treats them as separate
but 'whence -a' detects a duplicate and will not output /usr/bin/*.
I have not succeeded in making all_paths match 'whence -a' exactly.
src/cmd/ksh93/tests/path.sh: function all_paths:
- Using the -ef test expression operator, remove entries from $PATH
that point to the same directory as another entry in $PATH (e.g.
when /bin is a symlink to /usr/bin but both are in $PATH).
Avoiding such dupes works around the problem.
src/cmd/ksh93/sh/name.c:
- The Empty macro (a constant pointer to the empty string) is already
defined in include/defs.h, so does not need to be repeated here.
GMT and UTC have identical time but are used in different contexts.
When the system time zone is set to GMT (e.g. in the UK at winter
time), the 'printf %T' test could fail as it correctly uses GMT
whereas the test expects UTC.
src/cmd/ksh93/tests/builtins.sh:
- Fix possible false negative in 'printf %T\\n now' test by
replacing GMT with UTC in both 'date' output and 'printf %T'
output, instead of only the former.
Issuing typeset floating point numerics having a precision of 0
failed as the precision/size was being overwritten with the string
length of the value, e.g. 'typeset -F0 x=5.67' would result in
'typeset -F 4 x=5.6700' as len('5.67') is 4.
src/cmd/ksh93/include/nval.h:
- Created a symbolic name of NV_FLTSIZEZERO to respresent a float
having a precision/size of 0. NV_FLTSIZEZERO needs to be a
negative value.
src/cmd/ksh93/bltins/typeset.c:
- In b_typeset(), added code to set tdata.argnum to NV_FLTSIZEZERO
for E, F, X options.
- In setall(), adjusted code to allow for tp->argnum to be negative.
src/cmd/ksh93/sh/name.c: nv_newattr():
- Adjusted option value only change code to handle NV_FLTSIZEZERO as
well as changed to directly setting np->nvsize instead of using
nv_setsize(np,size) as nv_setsize might contain conflicting and/or
redundant code.
- Added missing conditional check of '!(newatts&NV_INTEGER)' to
constrain the size==0 code block to justified strings as
NV_LJUST, NV_RJUST, or NV_ZFILL are only valid for strings if
NV_INTEGER is not set. This code block was mistakenly setting
the precision/size value to the length of the value of an
assignment for floats whereas it should only be performing
auto assignment length for justified strings.
'typeset -xu' and 'typeset -xl' would export the variable but fail
to change case in the value as the check between old and new
attributes did not provide the necesssary insight for lower or
upper case transcoding due to the lower or upper case attribute
being set within typeset.c prior to calling name.c nv_newattr
function.
Previous rhbz#1188377 patch added a conditional check for size==-1
which in effect caused the nv_newattr export code block return
optimization to never be executed as one cannot set any attributes
using the readonly builtin. By altering the size==-1 check to !trans
the export only optimization can run.
Also, the rhbz#1188377 patch altered new_attr function by setting
the new size to oldsize if run by the readonly builtin. The result
of setting size==oldsize allowed the succeeding if statement to
run more frequently and if size was a non-zero value resulted in
nv_setsize resetting the value to what it already was. Investigation
yielded that size was always 0 coming from the readonly builtin.
src/cmd/ksh93/bltins/typeset.c:
- Remove the setting of tdata.argnum to -1 as it is not needed due to
existing name.c nv_newattr() logic.
src/cmd/ksh93/sh/name.c: nv_newattr():
- Corrected the export only check optimization by using !trans instead
of using size==-1.
- Removed previous condition check to set size=oldsize if coming from
the readonly builtin. nv_newattr already had existing logic to
prevent changing the size via nv_setsize as size is always 0 when
coming from readonly builtin.
src/cmd/ksh93/tests/builtins.sh:
- Remove redundant extra bincat=$(whence -p cat).
- Move whence -v/-a tests to path.sh.
- Fix 'whence -q' test so errors are counted outside of a subshell.
src/cmd/ksh93/tests/path.sh:
- Add all_paths function that is basically a reimplementation of
'whence -a -p' in shell. Useful for testing 'whence'.
- Move whence -v/-a tests to here, changing them to use all_paths
where needed. Also fix the 'whence -a' function autoloading
regression test to do the same. This fixes the tests for systems
(such as Slackware) where commands such as 'ls' or 'chmod' have
more than one path in even the default $PATH.
The '-o nolog' option (which prevented function definitions from being
recorded in the history file) was removed a long time ago, leaving
only a stub for backwards compatibility to stop 'set' from erroring
out if the option is set. But some other vestiges remained.
src/cmd/ksh93/sh/path.c,
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Remove a few pointless 'sh_onstate(SH_NOLOG)' statements. As of
93u+ or earlier, this is never checked for anywhere.
src/cmd/ksh93/sh.1:
- They forgot to remove the 'nolog' option documentation here.
Specify that it's obsolete and has no effect.
src/cmd/ksh93/data/builtins.c: sh_set[]:
- Be more concise.
Virtual/non-forking subshells that change the present working
directory (PWD) with 'cd' suffer from a serious race condition. The
PWD is changed within the same process. This means it may not be
possible to change back to the original PWD when exiting the
subshell, as some other process may destroy the PWD or modify its
permissions in the meantime. ksh did not handle this error
condition at all, so, after exiting a subshell that invoked 'cd',
it could silently end up running the script's following command(s)
in the wrong directory. Which might be 'rm -rf *'. So, ouch.
The proper and obvious fix is never to allow a virtual subshell to
change the PWD, as it can never be guaranteed you can return to a
previous directory. If the PWD is changed in a child process, there
is no need to restore it in the parent process, and this whole
problem is avoided. So subshells really should always fork on
encountering a 'cd' command.
But forking is slow. It is not uncommon for scripts to 'cd' in a
subshell that is run repeatedly in a loop.
There is also the issue of custom builtins that can be added to ksh
via shared libraries. In the standard shell language, 'cd' is the
only command that changes the PWD, so we could just make that
command fork the subshell it is run from. But there's no telling
what a custom builtin might do.
So this commit implements a compromise that will not affect
performance unless there is the pathological condition of a PWD
that has been rendered inaccessible in some way:
1. When entering a virtual subshell, if the parent shell's PWD
proves inaccessible upon saving it, the subshell will now fork into
a separate process, avoiding the unrestorable PWD problem.
2. If some attack renders the parent shell's PWD unrestorable
*after* ksh enters a virtual subshell, ksh will now error out when
exiting it. There is nothing else left to do then. Continuing would
mean running arbitrary commands in the wrong PWD.
src/cmd/ksh93/sh/subshell.c:
- Put all the code/variables only needed for fchdir() behind '#if
_lib_fchdir'. This makes it clearer what's what.
(I don't know if there is still any system out there without
fchdir(3); I haven't found any. The chdir(3) fallback version may
be removed later as there is no way to make it remotely secure.)
- Fix the attempt to use the O_PATH mode for open(2) as a fallback
for nonexistent O_SEARCH on Linux. Define _GNU_SOURCE on Linux,
or <fcntl.h> (which is included indirectly) won't define O_PATH.
- Fix use of O_SEARCH. The code was simply wrong, repeating an
open(".",O_RDONLY) instead. Since a nonexistent O_SEARCH is now
redefined as either O_PATH or O_RDONLY, we can simply
open(".",O_SEARCH) and be done with it.
- Fix fatal error handling. Introduce fatal error condition for
failure to fchdir(3) back to the parent's PWD; rename 'duped' to
'fatalerror' and use it for error numbers; save and restore errno
on fatal error so the message will report the cause. (We must
call errormsg() near the end of sh_subshell() to avoid crashes.)
- If open(".",O_SEARCH) was not able get a file descriptor to our
PWD on entry, then call sh_subfork() immediately before running
the subshell commands. (Forking earlier causes a crash.)
- When restoring the PWD, if fchdir(3) fails, do *not* fall back to
chdir(3). We already know the PWD is inaccessible, so if chdir(3)
"succeeds" then, it's very likely to be a substitute injected by
an attacker.
src/cmd/ksh93/bltins/cd_pwd.c:
- If we don't have fchdir(3), then sh_subshell() must fall back to
chdir(2) to restore the PWD. That is highly vulnerable, as a
well-timed rename would allow an attacker to usurp the PWD. We
can't do anything about that if some custom builtin changes the
PWD, but we can at least make 'cd' always fork a subshell, which
slows down ksh but removes the need for the parent shell ever to
restore the PWD. (There is certainly no popular system where this
is relevant and there might not be any such current system.)
This commit adds no regression test because a portable regression
test is not really doable. Different kernels, external /bin/pwd
utilities, etc. all have quite different behaviour under the
pathological condition of an inaccessible PWD, so both the
before-fix and the after-fix behaviour differs. See link below.
Resolves: https://github.com/ksh93/ksh/issues/141
Thanks to Stéphane Chazelas for the bug report.
src/cmd/ksh93/features/options:
- Fix unportable SHELLMAGIC test:
1. /bin/echo does not work on all systems, but /usr/bin/env is a
de-facto standard path (even NixOS gave in and has it).
2. Do not try to execute a temp file in /tmp as it might be
mounted noexec. Use our own $EXECROOT instead.
- rm unnecessary 'exec 9<&-' (it was never opened globally).
- Remove broken SHOPT_UCB test. It had a syntax error, but the
result is not used anywhere.
src/cmd/ksh93/bltins/typeset.c: b_typeset():
- For integer bases change argnum check to default values that
are < 2 or > 64 to 10 instead of allowing invalid base values
that ksh cannot process.
src/cmd/ksh93/bltins/typeset.c: setall():
- Remove argnum check for integer base of 1 as base cannot be 1.
- Relocate strlen(name) to inside of conditional check for
np->nvfun as this code does not need to run all.
- Remove no-op oldname code
src/cmd/ksh93/tests/attributes.sh:
- Add typeset -i base bounds checks to default base 10
src/cmd/ksh93/tests/builtins.sh:
- Fix a test so it doesn't fail if 'whence -a' finds multiple paths
for 'ls'.
src/cmd/ksh93/tests/coprocess.sh
- Update known failure comment with current information.
src/cmd/ksh93/sh/args.c: sh_argprocsub():
- Save and restore state more efficiently by just saving and
restoring all the state bits in one go using the
sh_{get,set}state() macros, which are defined in defs.h as:
#define sh_getstate() (sh.st.states)
#define sh_setstate(x) (sh.st.states = (x))
(and there is yet more evidence that it doesn't matter whether
we use a 'shp->' pointer or 'sh.' direct access).
src/cmd/ksh93/sh/main.c: exfile():
- Remove a no-op 'sh_offstate(SH_INTERACTIVE);'. It was in the
'else' clause of 'if(sh_isstate(SH_INTERACTIVE))' so if we get
there, it is known that this flag is already off.
- To properly disable job control, we also have to save and restore
the job.jobcontrol variable.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Remove some no-op flaggery from this highly performance-sensitive
point in the code. Given the immediately preceding:
volatile int was_errexit = sh_isstate(SH_ERREXIT);
volatile int was_monitor = sh_isstate(SH_MONITOR);
the following:
sh_offstate(SH_ERREXIT);
if(was_errexit&flags)
sh_onstate(SH_ERREXIT);
can be reformulated as:
if(!(flags & sh_state(SH_ERREXIT)))
sh_offstate(SH_ERREXIT);
(IOW, if it was already on, don't turn it off and then on again)
...and the following:
if(was_monitor&flags)
sh_onstate(SH_MONITOR);
can be removed; it's a no-op because it wasn't preceded by an
sh_offstate() and if 'was_monitor' is true, this option is known
to be on. (I considered they may have forgotten an 'sh_offstate'
there like in the SH_ERREXIT case, but adding that causes several
regressions in a shtests run.)
src/cmd/ksh93/include/defs.h:
- Remove comment that is evidently long outdated; there is not (or
no longer) a Shscoped_t type defined anywhere, nor are these
struct fields replicated in any other type definition.
- Add comment to clarify what the 'states' int in 'struct
sh_scoped' is for.
By definition, subshells are never interactive, so they should
disable behaviour associated with interactive shells even if the
main shell is interactive.
Most visibly, running a background job from a subshell like
( some_command & )
now no longer prints a job ID that you cannot use in the main shell.
This behaviour change matches pdksh/mksh, bash, zsh, dash, et al.
Prior discussion:
https://www.mail-archive.com/austin-group-l@opengroup.org/msg06456.html
(plus the preceding thread)
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Before running the command(s) in the subshell using sh_exec(),
turn off the SH_INTERACTIVE shell state flag. (No need to add
code to restore it as this function already saves and restores
the entire shell state.)
src/cmd/ksh93/bltins/misc.c: b_bg():
- If there is no job control when using 'bg', 'fg' or 'disown',
always print the "no job control" error message and not only if
the shell is in the interactive state. This is also what
pdksh/mksh, bash and zsh do.
Mildly interesting: apparently there was once an idea to implement
shared-state command substitutions as a shell option like 'set -o
subshare'. They were implemented using a new ${ syntax; } instead,
but there is a vestigial SH_SUBSHARE option ID in shell.h plus a
check for it in subshell.c that would cause backtick-style command
substitutions (comsub==1) to share their state. That option isn't
defined in data/options.c so it's impossible for a user to set it.
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/subshell.c:
- Remove SH_SUBSHELL option vestiges.
src/cmd/ksh93/include/defs.h:
- Correct my comment on 'comsub' flag; I was wrong about what the
values meant. 2 is for a shared-state comsub. (re: 4ce486a7)
Autoloading a function caused the calling script's $LINENO to be
off by the number of lines in the function definition file. In
addition, while running autoloaded functions, errors/warnings were
reported with wrong line numbers.
src/cmd/ksh93/sh/path.c:
- Save $LINENO (shp->inlineno) before autoloading a function, reset
it to 1 so that the correct line number offset is remembered for
the function definition, and restore it after.
src/cmd/ksh93/tests/variables.sh:
- Add regression test for $LINENO, directly and in error messages,
within and outside a non-autoloaded and an autoloaded function.
Fixes: https://github.com/ksh93/ksh/issues/116
ksh 93u+ introduced a regression in the combination of the
'set -o pipefail' and 'set -e'/'set -o errexit' options:
$ ksh93 -o errexit -o pipefail -c \
'(exit 3) | true; echo "still here despite $? status"'
still here despite 3 status
The bug is in how the the huge sh_exec() function in xec.c handles
the 'echeck' flag. Near the end of sh_exec(), this flag triggers a
sh_chktrap() call to check whether to trigger any traps, including
the ERR trap -- and that same function also handles the errexit
option, which is basically the same as 'trap "exit" ERR'.
We can learn more easily how sh_exec() works by inserting debug
warnings in all its 'switch(type&COMMSK)' cases, like:
case TCOM:
errormsg(SH_DICT,ERROR_warn(0),"[DEBUG] TCOM");
... and same for all the others. With that done, the output
of a very simple dummy pipeline looks as follows:
$ arch/*/bin/ksh -c 'true | true | true'
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TFIL
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TFORK
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TFORK
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TSETIO
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TCOM
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TCOM
arch/darwin.i386-64/bin/ksh: warning: [DEBUG] TCOM
So, it looks like sh_exec() handles this pipeline as follows:
TFIL
|_____TFORK
| |_____TCOM
|_____TFORK
| |_____TCOM
|_____TSETIO
|_____TCOM
Each time a pipeline like command1 | command2 | ... is executed,
sh_exec() is invoked with type TFIL; this then recursively invokes
sh_exec() to handle the individual elements. The last element of
the pipe triggers a sh_exec() run with type TSETIO; since it is run
in the current shell environment, it is effectively treated as a
command with an input redirection. All the previous elements are of
type TFORK instead, because they are executed asynchronously in
separate, forked subshell processes. Finally, the TFORK or TSETIO
code then recursively calls sh_exec() again with type TCOM to
actually execute the commands.
When reading the code, we find that the 'echeck' flag is set as
part of the TSETIO code. This makes sense of why only an error in
the last element of the pipe triggers the errexit/ERR trap action.
So that's the bug: the flag is set in the wrong place.
This can be fixed by setting that flag in the TFIL handling code
instead, as this is what calls everything else and collects all the
exit statuses. So the sh_chktrap() call is now executed after
handling the entire pipeline, at the TFIL recursion level.
This also allows getting rid of the special-casing in the buggy
TSETIO version. The SH_ERREXIT state is restored at the end of each
sh_exec() call, so since we're now doing this at a lower recursion
level, it will already have been restored.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Fix the bug as per the above.
src/cmd/ksh93/tests/options.sh:
- Add tests for errexit and ERR trap combined with pipefail.
src/cmd/ksh93/tests/basic.sh:
- Tweak a couple of tests that reported a trap wasn't triggered
even if it was actually triggered more than once.
Fixes: https://github.com/ksh93/ksh/issues/121
Thanks to Stéphane Chazelas for the bug report.
'typeset -xu' and 'typeset -xl' would export the variable but fail
to change case in the value under certain conditions.
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-xufix.patch
This applies the patch essentially without change and adds a
regression test based on the reproducer provided in the RH bug.
Unfortunately there is no description of how the patch works and
it's a little obscure to me. As far as I can figure out, the cause
of the problem was that nv_newattr() erroneously processed a
nonexistent size option-argument such as what can be given to
options like typeset -F, e.g. typeset -F3 for 3 digits after the
dot. A nonexistent size argument is represented by the value of -1.
Red Hat erratum: https://bugzilla.redhat.com/1221766
"Previously, the gcc utility optimized out a non-NULL test in the
ksh implementation of the strdup() function. This caused an
unexpected termination when ksh was executed in a clean chroot
environment. With this update, ksh compilation parameters have been
updated to prevent optimizing out a non-NULL test, and ksh no
longer crashes in clean chroot environments."
The optimizer bug occurs in that function's single-line body:
return (s && (t = oldof(0, char, n = strlen(s) + 1, 0))) ? (char*)memcpy(t, s, n) : (char*)0;
So it must be the test for non-NULL 's' that fails. And 's' is
declared in the function definition, as follows:
extern char*
strdup(register const char* s)
So that makes me wonder if we can work around the bug by simply
removing the 'const' (and the 'register' while we're at it).
However, I have no easy way to verify that at the moment. The Red
Hat patch instead tells gcc to disable optimization for this
function using a #pragma directive.
I have no idea if that gcc optimiser bug has been fixed in the
meantime, but experience from c258a04f has shown that we cannot
trust that it has been fixed (that other optimizer bug is at least
a decade old and still not fixed). So, in it goes, until someone
shows evidence that we no longer need it.
Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-badgcc.patch
src/lib/libast/string/strdup.c:
- Tell GCC to disable all optimisations for strdup().
Discussion/analysis: https://bugzilla.redhat.com/1506344
iousepipe() might write out of bounds, causing a crash, if
subpipe[2] is set to a value >= sh.gd.lim.open_max.
src/cmd/ksh93/sh/xec.c: iousepipe():
- Validate the FD using sh_iovalidfd() before the write.
Prior discussion:
https://bugzilla.redhat.com/1454804
On 2017-05-23 13:33:25 UTC, Paulo Andrade wrote:
> In previous ksh versions, when exiting the scope of a ksh
> (not posix) function, it would restore the trap table of
> the "calling context" and if the reason the function exited
> was a signal, it would call sh_fault() passing as argument
> the signal value.
> Newer ksh checks it, but calls kill(getpid(), signal_number)
> after restoring the trap table, but only calls for SIGINT and
> SIGQUIT.
[...]
> The old way appears to have been more appropriate, but there
> must be a reason to only pass SIGINT and SIGQUIT as it is an
> explicit patch.
The last paragraph is where I differ. This would not be the first
example of outright breakage that appeared to be added deliberately
and that 93u+m has fixed or removed, see e.g. 8477d2ce ('printf %H'
had code that deleted all multibyte characters), cefe087d, or
781f0a39. Sometimes it seems the developers added a little
experiment and then forgot all about it, so it became a misfeature.
In this instance, the correct pre-2012 ksh behaviour is still
explicitly documented in (k)sh.1: "A trap condition that is not
caught or ignored by the function causes the function to terminate
and the condition to be passed on to the caller". Meaning, if there
is no function-local trap, the signal defaults to the parent scope.
There is no language that limits this to SIGINT and SIGQUIT only.
It also makes no sense at all to do so -- signals such as SIGPIPE,
SIGTERM, or SIGSEGV need to be caught by default and to do
otherwise results in misbehaviour by default.
src/cmd/ksh93/sh/xec.c: sh_funscope():
- When resending a signal after restoring the global traps state,
remove the spurious check that limits this to SIGINT and SIGQUIT.
- Replace it with a check for nsig!=0, as that means there were
parent trap states to restore. Otherwise 'kill' may be called
with an invalid signal argument, causing a crash on macOS.
src/cmd/ksh93/tests/signal.sh:
- Update a test to check that a function-local SIGTERM trap is
triggered correctly when signalled from another process.
- Complete the tests for 3aee10d7; this bug needed fixing before
we could test that previous fix in a ksh function scope.
- Add a test for triggering global traps from ksh functions,
testing multiple POSIX-standard signals.
