The referenced commit introduced the following bug:
> The closing quote does not appear to be registering during the
> parse of the following:
>
> echo ${var:+'{}'}
>
> Within a script, this will result in:
>
> syntax error at line 1: `'' unmatched
src/cmd/ksh93/data/lexstates.c,
src/cmd/ksh93/include/lexstates.h:
- Add new ST_MOD1 state table that is a copy of ST_QUOTE, but adds
a special meaning (ST_LIT) for the single quote (position 39).
src/cmd/ksh93/sh/lex.c: sh_lex():
- For parameter expansion operators with old-style quoting
(S_MOD1), use the new ST_MOD1 state table instead of ST_QUOTE.
This causes single quotes within them to be processed properly.
src/cmd/ksh93/tests/quoting2.sh:
- Add tests.
Thanks to @gkamat for the bug report.
Resolves: https://github.com/ksh93/ksh/issues/290
Previously, command substitutions executed as virtual subshells
were always forked if any command was run within them that
redireceted standard output, even if the redirection was local to
that command.
Commit 500757d7 removed the check for a shared-state command
substitution (subshare), so introduced a bug where even that would
fork, causing it to stop sharing its state.
We can further improve on that fix by only forking if the
redirection is permanent as with `exec` or `redirect`. There should
be no need to do that if the redirection is local to a command run
within the command substitution, as the file descriptor is restored
when that command finishes, which is still within the command
substitution.
src/cmd/ksh93/sh/io.c: sh_redirect():
- Only fork upon redirecting stdout if the virtual subshell is a
command substitution, and if the redirection is permanent
(flag==1 or flag==2).
The commands within a process substitution used as an argument to a
redirection (e.g. < <(...) or > >(...)) are simply not included in
parse trees dumped by shcomp. This can be verified with a command
like hexdump -C. As a result, these process substitutions do not
work when running a bytecode-compiled shell script.
The fix is surprisingly simple. A process substitution is encoded
as a complete parse tree. When used with a redirection, that parse
tree is used as the file name for the redirection. All we need to
do is treat the "file name" as a parse tree instead of a string if
flags indicate a process substitution.
A process substitution is detected by the struct ionod field
'iofile'. Checking the IOPROCSUB bit flag is not enough. We also
need to exclude the IOLSEEK flag as that form of redirection may
use the IOARITH flag which has the same bit value as IOPROCSUB (see
include/shnodes.h).
src/cmd/ksh93/sh/tdump.c: p_redirect():
- Call p_tree() instead of p_string() for a process substitution.
src/cmd/ksh93/sh/trestore.c: r_redirect():
- Call r_tree() instead of r_string() for a process substitution.
src/cmd/ksh93/include/version.h:
- Bump the shcomp binary header version as this change is not
backwards compatible; previous trestore.c versions don't know how
to read the newly compiled process substitutions and would crash.
src/cmd/ksh93/tests/io.sh:
- Add test.
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/options.sh:
- Revert shcomp workarounds. (re: 6701bb30)
Resolves: https://github.com/ksh93/ksh/issues/165
Johnothan King writes:
> There are two regressions related to how ksh handles syntax
> errors in the .kshrc file. If ~/.kshrc or the file pointed to by
> $ENV have a syntax error, ksh exits during startup. Additionally,
> the error message printed is incorrect:
>
> $ cat /tmp/synerror
> ((
> echo foo
>
> # ksh93u+m
> $ ENV=/tmp/synerror arch/*/bin/ksh -ic 'echo ${.sh.version}'
> /tmp/synerror: syntax error: `/t/tmp/synerror' unmatched
>
> # ksh93u+
> $ ENV=/tmp/synerror ksh93u -ic 'echo ${.sh.version}'
> /tmp/synerror: syntax error: `(' unmatched
> Version AJM 93u+ 2012-08-01
>
> The regression that causes the incorrect error message was
> introduced by commit cb67a01. The other bug that causes ksh to
> exit on startup was introduced by commit ceb77b1.
src/cmd/ksh93/sh/lex.c: fmttoken():
- Call stakfreeze(0) to terminate a possible unterminated previous
stack item before writing the token string onto the stack. This
fixes the bug with garbage in a syntax error message.
src/cmd/ksh93/sh/main.c: exfile():
- Revert Red Hat's ksh-20140801-diskfull.patch applied in ceb77b13.
This fixes the bug with interactive ksh exiting on syntax error
in a profile script. Testing by @JohnoKing showed the patch is no
longer necessary to fix a login crash on disk full, as commit
970069a6 (which applied Red Hat patches ksh-20120801-macro.patch
and ksh-20120801-fd2lost.patch) also fixes that crash.
src/cmd/ksh93/README:
- Fix typos. (re: fdc08b23)
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
Resolves: https://github.com/ksh93/ksh/issues/281
This commit fixes three problems with getconf pathbound builtin:
1. The -l/--lowercase option did not change all variable names to
lower case.
2. The -q/--quote option now quotes all string values. Previously,
it only quoted string values that had a space or other
non-shellsafe character.
3. The -c/--call, -n/--name and -s/--standard options matched all
variable names provided by 'getconf -a', even if none were
actual matches.
Additionally, references to the confstr and sysconf functions have
been updated to reference section 3 of the man pages instead of
section 2.
src/lib/libast/port/astconf.c:
- Previously, only values that had spaces in them were quoted. Change
that behavior to quote all string values by using the FMT_ALWAYS
flag. Bug report: https://github.com/att/ast/issues/1173
- Not all variable names were printed in lowercase by 'getconf -l'.
Fix it by adding a few missing instances of fmtlower.
Bug report: https://github.com/att/ast/issues/1171
- Add the missing code to the '#if _pth_getconf_a' block to handle
-c/-n/-s while parsing the OS's native 'getconf -a' output. This
approach reuses code for name matching from other parts of
astconflist(). Resolves: https://github.com/ksh93/ksh/issues/279
src/lib/libcmd/getconf.c:
- Update the documentation to note the -q flag only quotes strings.
src/cmd/ksh93/tests/bulitins.sh:
- Add regression tests for the getconf bugs fixed in this commit.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
Immediately after tab-completing the name of a directory, it is
not possible to type digits after the slash; ksh eats them as it
parses them as a menu selection for a nonexistent menu.
Reproducer:
$ mkdir -p emacstest/123abc
$ cd emacste[tab]123abc
Actual results:
$ cd emacstest/abc
Expected results:
$ cd emacstest/123abc
Workarounds are to press a non-numeric key followed by backspace,
or hit [tab] again to get a list of options.
Originally reported by Arnon Weinberg, 2012-12-23 07:15:19 UTC, at:
https://bugzilla.redhat.com/889745
The fix had been partially backported from ksh 93v- by AT&T
(16e4824c), which made things worse, so it was reverted (e8b3274a).
This commit backports a slightly edited version of the complete
fix. Thanks to @JohnoKing for finding the correct code. Discussion:
https://github.com/ksh93/ksh/issues/198#issuecomment-820178514
src/cmd/ksh93/edit/emacs.c: escape():
- Backport the fix for this bug that was implemented in ksh 93v-
alpha 2013-10-10. Immediately after a slash, do not stay in "\"
mode (file name completion) and reset the tab count.
src/cmd/ksh93/tests/pty.sh:
- Test the fix.
Resolves: https://github.com/ksh93/ksh/issues/198
The <>; operator doesn't work correctly if it's used as the last
command of a -c script. Reproducer:
$ echo test > a; ksh -c 'echo x 1<>; a'; cat a
x
st
This bug is caused by ksh running the last command of -c scripts
with execve(2) instead of posix_spawn(3) or fork(2). The <>;
operator is noted by the man page as being incompatible with the
exec builtin (see also the ksh93u+ man page), so it's not
surprising this bug occurs when ksh runs a command using execve:
> <>;word cannot be used with the exec and redirect built-ins.
The ksh2020 fix simply removed the code required for ksh to use
this optimization at all. It's not a performance friendly fix and
only papers over the bug, so this commit provides a better fix.
This bug was first reported at:
https://github.com/att/ast/issues/9
In addition, this commit re-enables the execve(2) optimization for
the last command for scripts loaded from a file. It was enabled in
in older ksh versions, and was only disabled in interactive shells:
https://github.com/ksh93/ast-open-history/blob/2011-06-30/src/cmd/ksh93/sh/main.c#L593-L599
It was changed on 2011-12-24 to only be used for -c scripts:
https://github.com/ksh93/ast-open-history/blob/2011-12-24/src/cmd/ksh93/sh/main.c#L593-L599
We think there is no good reason why scripts loaded from a file
should be optimised less than scripts loaded from a -c argument.
They're both scripts; there's no essential difference between them.
So this commit reverts that change. If there is a bug left in the
optimization after this fix, this revert increases the chance of
exposing it so that it can be fixed.
src/cmd/ksh93/sh/xec.c:
- The IOREWRITE flag is set when handling the <>; operator, so to
fix this bug, avoid exec'ing the last command if it uses <>;. See
also commit 17ebfbf6, which fixed another issue related to the
execve optimization.
src/cmd/ksh93/tests/io.sh:
- Enable a regression test that was failing because of this bug.
- Add the reproducer from https://github.com/att/ast/issues/9 as a
regression test.
src/cmd/ksh93/sh/main.c:
- Only avoid the non-forking optimization in interactive shells.
src/cmd/ksh93/tests/signal.sh:
- Add an extra comment to avoid the non-forking optimization in the
regression test for rhbz#1469624.
- If the regression test for rhbz#1469624 fails, show the incorrect
exit status in the error message.
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/options.sh:
- This bugfix was causing the options regression test to segfault
when run under shcomp. The cause is the same as
<https://github.com/ksh93/ksh/issues/165>, so as a workaround,
avoid parsing process substitutions with shcomp until that is
fixed. This workaround should also avoid the other problem
detailed in <https://github.com/ksh93/ksh/issues/274>.
Resolves: https://github.com/ksh93/ksh/issues/274
Path-bound builtins on ksh (such as /opt/ast/bin/cat) break some
basic assumptions about paths in the shell that should hold true,
e.g., that a path output by whence -p or command -v should actually
point to an executable command. This commit should fix the
following:
1. Path-bound built-ins (such as /opt/ast/bin/cat) can now be
executed by invoking the canonical path (independently of the
value of $PATH), so the following will now work as expected:
$ /opt/ast/bin/cat --version
version cat (AT&T Research) 2012-05-31
$ (PATH=/opt/ast/bin:$PATH; "$(whence -p cat)" --version)
version cat (AT&T Research) 2012-05-31
In the event an external command by that path exists, the
path-bound builtin will now override it when invoked using the
canonical path. To invoke a possible external command at that
path, you can still use a non-canonical path, e.g.:
/opt//ast/bin/cat or /opt/ast/./bin/cat
2. Path-bound built-ins will now also be found on a PATH set
locally using an assignment preceding the command, so something
like the following will now work as expected:
$ PATH=/opt/ast/bin cat --version
version cat (AT&T Research) 2012-05-31
The builtin is not found by sh_exec() because the search for
builtins happens long before invocation-local preceding
assignments are processsed. This only happens in sh_ntfork(),
before forking, or in sh_fork(), after forking. Both sh_ntfork()
and sh_fork() call path_spawn() to do the actual path search, so
a check there will cover both cases.
This does mean the builtin will be run in the forked child if
sh_fork() is used (which is the case on interactive shells with
job.jobcontrol set, or always after compiling with SHOPT_SPAWN
disabled). Searching for it before forking would mean
fundamentally redesigning that function to be basically like
sh_ntfork(), so this is hard to avoid.
src/cmd/ksh93/sh/path.c: path_spawn():
- Before doing anything else, check if the passed path appears in
the builtins tree as a pathbound builtin. If so, run it. Since a
builtin will only be found if a preceding PATH assignment
temporarily changed the PATH, and that assignment is currently in
effect, we can just sh_run() the builtin so a nested sh_exec()
invocation will find and run it.
- If 'spawn' is not set (i.e. we must return), set errno to 0 and
return -2. See the change to sh_ntfork() below.
src/cmd/ksh93/sh/xec.c:
- sh_exec(): When searching for built-ins and the restricted option
isn't active, also search bltin_tree for names beginning with a
slash.
- sh_ntfork(): Only throw an error if the PID value returned is
exactly -1. This allows path_spawn() to return -2 after running a
built-in to tell sh_ntfork() to do the right things to restore
state.
src/cmd/ksh93/sh/parse.c: simple():
- When searching for built-ins at parse time, only exclude names
containing a slash if the restricted option is active. This
allows finding pointers to built-ins invoked by literal path like
/opt/ast/bin/cat, as long as that does not result from an
expansion. This is not actually necessary as sh_exec() will also
cover this case, but it is an optimisation.
src/lib/libcmd/getconf.c:
- Replace convoluted deferral to external command by a simple
invocation of the path to the native getconf command determined
at compile time (by src/lib/libast/comp/conf.sh). Based on:
https://github.com/ksh93/ksh/issues/138#issuecomment-816384871
If there is ever a system that has /opt/ast/bin/getconf as its
default native external 'getconf', then there would still be an
infinite recursion crash, but this seems extremely unlikely.
Resolves: https://github.com/ksh93/ksh/issues/138
Previous discussion: https://github.com/att/ast/issues/485
If ksh attempts to execute a non-executable command found in the
PATH, in some instances the error message and return status are
incorrect. In the example below, ksh returns with exit status 126
when using the -c execve(2) optimization or when using fork(2) in
an interactive shell. However, using posix_spawn(3) causes the exit
status to change:
$ echo 'print cannot execute' > /tmp/x
# Runs command with spawnveg (i.e., posix_spawn or vfork)
$ ksh -c 'PATH=/tmp; x; echo $?'
ksh: x: not found
127
# Runs command with execve
$ ksh -c 'PATH=/tmp; x'; echo $?
ksh: x: cannot execute [Permission denied]
126
# Runs command with fork
$ ksh -ic 'PATH=/tmp; x; echo $?'
ksh: x: cannot execute [Permission denied]
126
Since 'x' is in the PATH but can't be executed, the correct exit
status is 126, not 127. It's worth noting this bug doesn't cause
the regression tests to fail with ksh93u+m, but it does cause one
test to fail when run under dtksh:
path.sh[706]: Long nonexistent command name: got status 126, ''
This commit backports various fixes for this bug from ksh2020, with
additional fixes applied (since there were still some additional
issues the ksh2020 patch didn't fix). The lacking regression test
for exit status 126 in path.sh has been rewritten to test for more
scenarios where ksh failed to return the correct error message
and/or exit status. I can also confirm with this patch applied the
path.sh regression tests now pass when run under dtksh.
src/cmd/ksh93/sh/path.c:
- Add a comment to path_absolute() describing 'oldpp' is the
current pointer in the while loop and 'pp' is the next pointer.
Backported from:
a6cad450
- The patch from ksh2020 didn't fix this bug in the SHOPT_SPAWN
code (because ksh2020 prefers fork(2)), so issues with the exit
status could still occur when using spawnveg. To fix this, always
set 'noexec' to the value of errno if can_execute fails. Before
this fix, errno was discarded if 'pp' was a null pointer and
can_execute failed.
- If a command couldn't be executed and the error wasn't ENOENT,
save errno in a 'not_executable' variable. If an executable
command couldn't be found in the PATH, exit with status 126 and
set errno to the saved value. This was based on a ksh2020 bugfix,
but it has been reworked a little bit to fix a bug that caused a
mismatch between the error message shown and errno. Example with
a non-executable file in PATH:
$ nonexec
ksh2020: nonexec: cannot execute [No such file or directory]
The ksh2020 patch: <https://github.com/att/ast/pull/493>
- Backport a ksh2020 bugfix for directories in the PATH when
running one of the added regression tests on OpenBSD:
https://github.com/att/ast/pull/767
src/cmd/ksh93/data/msg.c,
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/{path,xec}.c:
- If a command name is too long (ENAMETOOLONG), then it wasn't
found in the PATH. For that case return exit status 127, like
for ENOENT.
src/cmd/ksh93/tests/path.sh:
- Replace the old test with a new set of more extensive tests.
These tests check the error message and exit status when ksh
attempts to run a command using any of the following:
- execve(2), used with the last command run with -c (*A tests).
- posix_spawn(3)/vfork(2), used in noninteractive scripts (*B tests).
- fork(2), used in interactive shells with job control (*C tests).
- command -x (*D tests).
- exec(1) (*E tests).
- Add a regression test from ksh2020 for attempting to execute a
directory:
https://github.com/att/ast/pull/758
src/lib/libast/include/ast.h,
src/lib/libast/include/wait.h:
- Avoid bitshifts in macros for static error codes. The return
values of command not found and exec related errors are static
values and should not require any macro magic for calculation.
Backported from: c073b102
- Simplify EXIT_* and W* macros to use 8 bits.
This commit fixes a segmentation fault when an attempt was made to
unset the default KSH_VERSION variable prior any other nameref
activity such as creating another nameref or even reassigning the
nameref KSH_VERSION to something else.
(new shell without prior nameref activity)
$ nameref
KSH_VERSION=.sh.version
$ unset -n KSH_VERSION
Memory fault
src/cmd/ksh93/sh/name.c: _nv_unset():
- Add a 'Refdict' check before attempting to remove a value from it
as apparently one does not exist until some sort of nameref
activity occurs after shell startup as the default nameref of
'KSH_VERSION=.sh.version' does not create one.
The bugfix for BUG_CMDSPASGN backported in commit fae8862c caused
two regressions with the += operator:
1. The += operator did not append to variables. Reproducer:
$ integer foo=3
$ foo+=2 command eval 'echo $foo'
2
2. The += operator ignored the readonly attribute, modifying readonly
variables in the same manner as above. Reproducer
$ readonly bar=str
$ bar+=ing command eval 'echo $bar'
ing
Both of the regressions above were caused by nv_putval() failing to
clone the variable from the previous scope into the invocation-local
scope. As a result, 'foo+=2' was effectively 0 + 2 (since ksh didn't
clone 3). The first regression was noticed during the development of
ksh93v-, so to fix both bugs I've backported the bugfix for the
regression from the ksh93v- 2013-10-10 alpha version:
https://www.mail-archive.com/ast-users@lists.research.att.com/msg00369.html
src/cmd/ksh93/sh/name.c:
- To fix both of the bugs above, find the variable to modify with
nv_search(), then clone it into the invocation local scope. To
fix the readonly bug as well, this is done before the NV_RDONLY
check (otherwise np will be missing that attribute and be
incorrectly modified in the invocation-local scope).
- Update a nearby comment describing what sh_assignok() does (per this
comment: https://github.com/ksh93/ksh/pull/249#issuecomment-811381759)
src/cmd/ksh93/tests/builtins.sh:
- Add regression tests for both of the now fixed regressions,
loosely based on the regression tests in ksh93v-.
The recursion level for arithmetic expressions is kept track of in
a static 'level' variable in streval.c. It is reset when arithmetic
expressions throw an error.
But an error for an arithmetic expression may also occur elsewhere
-- at least in one case: when an arithmetic expression attempts to
change a read-only variable. In that case, the recursion level is
never reset because that code does not have access to the static
'level' variable.
If many such conditions occur (as in the new readonly.sh regression
tests), an arithmetic command like 'i++' may eventually fail with a
'recursion too deep' error.
To mitigate the problem, MAXLEVEL in streval.c was changed from 9
to 1024 in 264ba48b (as in the ksh 93v- beta). This commit leaves
that increase, but adds a proper fix.
src/cmd/ksh93/include/defs.h:
- Add global sh.arithrecursion (a.k.a. shp->arithrecursion)
variable to keep track of the arithmetic recursion level,
replacing the static 'level' variable in streval.c.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Reset sh.arithrecursion before starting a new simple command
(TCOM), a new subshell with parentheses (TPAR), a new pipe
(TFIL), or a new [[ ... ]] command (TTST). These are the same
places where 'echeck' is set to 1 for --errexit and ERR trap
checks, so it should cover everything.
src/cmd/ksh93/sh/streval.c:
- Change all uses of 'level' to sh.arithrecursion.
- _seterror, aritherror(): No longer bother to reset the level
to zero here; xec.c should have this covered for all cases now.
src/cmd/ksh93/tests/arith.sh:
- Add tests for main shell and subshell.
src/cmd/ksh93/sh/args.c: sh_argopts():
- Remove special-casing for --posix (see also data/builtins.c) and
move the case -5: to the case ':' instead, so this option is
handled like all other long options. This change fixes two bugs:
1. 'set --posix' had no effect on the letoctal or braceexpand
options. Reproducer:
$ set --posix
$ [[ -o braceexpand ]]; echo $?
0
$ [[ -o letoctal ]]; echo $?
1
2. 'ksh --posix' could not run scripts correctly because it
wrongly enabled '-c'. Reproducer:
$ ksh --posix < <(echo 'exit 0')
ksh: -c requires argument
Usage: ksh [--posix] [arg ...]
Help: ksh [ --help | --man ] 2>&1
- Don't allow 'set --default' to unset the restricted option.
src/cmd/ksh93/tests/options.sh:
- Add regression tests for the bugs described above, using -o posix
and --posix.
src/cmd/ksh93/tests/restricted.sh:
- Add a regression test for 'set --default' in rksh.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
If a system administrator prefixes /opt/ast/bin to the path and
then invokes the shell in restricted mode, they clearly intend for
the user to run those AST utilities.
Similarly, if a system administrator sets a PATH for a restricted
shell that includes libraries listed in the .paths file, they must
have intended for the user to use those loadable built-ins, as they
will be associated with the pathnames of their respective
libraries. Since the user cannot change PATH or use the builtin
command, they still cannot load just any built-in they choose.
src/cmd/ksh93/sh/path.c:
- Remove SH_RESTRICTED check when handling path-bound builtins
or dynamic libaries containining builtins in $PATH.
src/cmd/ksh93/tests/builtins.sh:
- Add test verifying a restricted user can use /opt/ast/bin/cat
via a PATH search.
Progresses: https://github.com/ksh93/ksh/issues/138
The current version of 93u+m does not have proper support for the
LC_TIME variable. Setting LC_TIME has no effect on printf %T, and
if the locale is invalid no error message is shown:
$ LC_TIME=ja_JP.UTF-8
$ printf '%T\n' now
Wed Apr 7 15:18:13 PDT 2021
$ LC_TIME=invalid.locale
$ # No error message
src/cmd/ksh93/data/variables.c,
src/cmd/ksh93/include/variables.h,
src/cmd/ksh93/sh/init.c:
- Add support for the $LC_TIME variable. ksh93v- attempted to add
support for LC_TIME, but the patch from that version was extended
because the variable still didn't function correctly.
src/cmd/ksh93/tests/variables.sh:
- Add LC_TIME to the regression tests for LC_* variables.
Simple reproducer:
set -A arr a b c d; : ${arr[1..2]}; unset arr[1]; echo ${arr[@]}
Output:
a
Expected output:
a c d
The ${arr[1..2]} expansion broke the subsequent 'unset' command
so that it unsets element 1 and on, instead of only 1.
This regression was introduced in nv_endsubscript() on 2009-07-31:
c47896b4/src/cmd/ksh93/sh/array.c
That change checks for the ARRAY_SCAN attribute which enables
processing ranges of array elements instead of single array
elements, and restores it after. That restore is evidently not
correct as it causes the subsequent unset command to malfunction.
If we revert that change, the bug disappears and the regression
tests show no failures. However, I don't know what this was meant
to accomplish and what other bug we might introduce by reverting
this. However, no corresponding regression test was added along
with the 2009-07-31 change, nor is there any corresponding message
in the changelog. So this looks to be one of those mystery changes
that we'll never know the reason for.
Since we currently have proof that this change causes breakage and
no evidence that it fixes anything, I'll go ahead and revert it
(and add a regression test, of course). If that causes another
regression, hopefully someone will find it at some point.
src/cmd/ksh93/sh/array.c: nv_endsubscript():
- Revert the 2009-07-31 change that saves/restores the ARRAY_SCAN
attribute.
- Keep the 'ap' pointer as it is now used by newer code. Move the
declaration up to the beginning of the block, as is customary.
src/cmd/ksh93/sh/init.c:
- Cosmetic change: remove an unused array_scan() macro that I found
when grepping the code for ARRAY_SCAN. The macro was introduced
in version 2001-06-01 but the code that used it was replaced in
version 2001-07-04, without removing the macro itself.
Resolves: https://github.com/ksh93/ksh/issues/254
Ksh currently restricts readonly scalar variables from having their
values directly changed via a value assignment. However, since ksh
allows variable attributes to be altered, the variable's value can
be indirectly altered. For instance, if TMOUT=900 (for a 15 minute
idle timeout) was set to readonly, all that is needed to alter the
value of TMOUT from 900 to 0 is to issue 'typeset -R1 TMOUT',
perhaps followed by a 'typeset -i TMOUT' to turn off the shell's
timeout value.
In addition, there are problems with arrays. The following is
incorrectly allowed:
typeset -a arr=((a b c) 1)
readonly arr
arr[0][1]=d
arr=(alphas=(a b c);name=x)
readonly arr.alphas
arr.alphas[1]=([b]=5)
arr=(alphas=(a b c);name=x)
readonly arr.alphas
arr.alphas[1]=(b)
typeset -C arr=(typeset -r -a alphas=(a b c);name=x)
arr.alphas[1]=()
src/cmd/ksh93/bltins/typeset.c: setall():
- Relocate readonly attribute check higher up the code and widen
its application to issue an error message if the pre-existing
name-pair has the readonly bit flag set.
- To avoid compatibility problems, don't check for readonly if
NV_RDONLY is the only attribute set (ignoring NV_NOFREE). This
allows 'readonly foo; readonly foo' to keep working.
src/cmd/ksh93/sh/array.c: nv_endsubscript():
- Apply a readonly flag check when an array subscript or append
assignment occurs, but allow type variables (typeset -T) as they
utilize '-r' for 'required' sub-variables.
src/cmd/ksh93/tests/readonly.sh:
- New file. Create readonly tests that validate the warning message
and validate that the readonly variable did not change.
src/cmd/ksh93/sh/streval.c:
- Bump MAXLEVEL from 9 to 1024 as a workaround for arithmetic
expansion, avoiding a spurious error about too much recursion
when the readonly.sh tests are run. This change is backported
from ksh 93v-.
TODO: debug a spurious increase in arithmetic recursion level
variable when readonly.sh tests with 'typeset -i' are run.
That is a different bug for a different commit.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This commit fixes a bug in the ksh uname builtin's -d option that could
change the output of -o (I was only able to reproduce this on Linux):
$ builtin uname
$ uname -o
GNU/Linux
$ uname -d
(none)
$ uname -o
(none)
I identified this patch from ksh2020 as a fix for this bug:
<https://github.com/att/ast/pull/1187>
The linked patch was meant to fix a crash in 'uname -d', although I've
had no luck reproducing it: <https://github.com/att/ast/issues/1184>
src/lib/libcmd/uname.c:
- Pass correct buffer to getdomainname() while executing uname -d.
src/cmd/ksh93/tests/builtins.sh:
- Add a regression test for the reported 'uname -d' crash.
- Add a regression test for the output of 'uname -o' after 'uname -d'.
- To handle potential crashes when running the regression tests in older
versions of ksh, fork the command substitutions that run 'uname -d'.
This bug was first reported at <https://github.com/att/ast/issues/8>.
The 'cd' command currently takes the value of $OLDPWD from the
wrong scope. In the following example 'cd -' will change the
directory to /bin instead of /tmp:
$ OLDPWD=/bin ksh93 -c 'OLDPWD=/tmp cd -'
/bin
src/cmd/ksh93/bltins/cd_pwd.c:
- Use sh_scoped() to obtain the correct value of $OLDPWD.
- Fix a use-after-free bug. Make the 'oldpwd' variable a static
char that points to freeable memory. Each time cd is used, this
variable is freed if it points to a freeable memory address and
isn't also a pointer to shp->pwd.
src/cmd/ksh93/sh/path.c: path_pwd():
- Simplify and add comments.
- Scope $PWD properly.
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/leaks.sh:
- Backport the ksh2020 regression tests for 'cd -' when $OLDPWD is
set.
- Add test for $OLDPWD and $PWD after subshare.
- Add test for $PWD after 'cd'.
- Add test for possible memory leak.
- Add testing for 'unset' on OLDPWD and PWD.
src/cmd/ksh93/COMPATIBILITY:
- Add compatibility note about changes to $PWD and $OLDPWD.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This bug was originally reported at <https://github.com/att/ast/issues/1467>.
A crash can occur when using the 'b' or 'B' vi mode commands to go back
one word. I was able to reproduce these crashes with 100% consistency on
an OpenBSD virtual machine when ksh is compiled with -D_std_malloc.
Reproducer:
$ set -o vi
$ asdf <ESC> <b or B>
The fix is based on Matthew DeVore's analysis:
> I suspect this is caused by this line:
>> while (vi_isalph(tcur_virt) && tcur_virt >= first_virt) --tcur_virt;
> which is in the b codepath. It checks vi_isalph(tcur_virt) before checking
> if tcur_virt is in range. These two clauses should be reversed. Note that
> line 316 is a similar check for pressing B, and there the tcur_virt value
> is checked first.
src/cmd/ksh93/edit/vi.c:
- Check tcur_virt before using isalph() or isblank() to fix both crashes.
At the start of the backword() while loop this check was performed
twice, so the redundant check has been removed.
src/cmd/ksh93/tests/pty.sh:
- Add a regression test for the b, B, w and W editor commands.
When test is passed the '=~' operator, it will silently fail with
exit status 1:
$ test foo =~ foo; echo $?
1
This bug is caused by test_binop reaching the 'NOTREACHED' area of
code. The bugfix was adapted from ksh2020:
https://github.com/att/ast/issues/1152
src/cmd/ksh93/bltins/test.c: test_binop():
- Error out with a message suggesting usage of '[[ ... ]]' if '=~'
is passed to the test builtin.
- Special-case TEST_END (']]') as that is not really an operator.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
One of the best-kept secrets of libast/ksh93 is that the code
includes support for case-insensitive file name generation (a.k.a.
pathname expansion, a.k.a. globbing) as well as case-insensitive
file name completion on interactive shells, depending on whether
the file system is case-insensitive or not. This is transparently
determined for each directory, so a path pattern that spans
multiple file systems can be part case-sensitive and part case-
insensitive. In more precise terms, each slash-separated path name
component pattern P is treated as ~(i:P) if its parent directory
exists on a case-insensitive file system. I recently discovered
this while dealing with <https://github.com/ksh93/ksh/issues/223>.
However, that support is dead code on almost all current systems.
It depends on pathconf(2) having a _PC_PATH_ATTRIBUTES selector.
The 'c' attribute is supposedly returned if the given directory is
on a case insensitive file system. There are other attributes as
well (at least 'l', see src/lib/libcmd/rm.c). However, I have been
unable to find any system, current or otherwise, that has
_PC_PATH_ATTRIBUTES. Google and mailing list searches yield no
relevant results at all. If anyone knows of such a system, please
add a comment to this commit on GitHub, or email me.
An exception is Cygwin/Windows, on which the "c" attribute was
simply hardcoded, so globbing/completion is always case-
insensitive. As of Windows 10, that is wrong, as it added the
possibility to mount case-sensitive file systems.
On the other hand, this was never activated on the Mac, even
though macOS has always used a case-insensitive file like Windows.
But, being UNIX, it can also mount case-sensitive file systems.
Finally, Linux added the possibility to create individual case-
insensitive ext4 directories fairly recently, in version 5.2.
https://www.collabora.com/news-and-blog/blog/2020/08/27/using-the-linux-kernel-case-insensitive-feature-in-ext4/
So, since this functionality latently exists in the code base, and
three popular OSs now have relevant file system support, we might
as well make it usable on those systems. It's a nice idea, as it
intuitively makes sense for globbing and completion behaviour to
auto-adapt to file system case insensitivity on a per-directory
basis. No other shell does this, so it's a nice selling point, too.
However, the way it is coded, this is activated unconditionally on
supported systems. That is not a good idea. It will surprise users.
Since globbing is used with commands like 'rm', we do not want
surprises. So this commit makes it conditional upon a new shell
option called 'globcasedetect'. This option is only compiled into
ksh on systems where we can actually detect FS case insensitivity.
To implement this, libast needs some public API additions first.
*** libast changes ***
src/lib/libast/features/lib:
- Add probes for the linux/fs.h and sys/ioctl.h headers.
Linux needs these to use ioctl(2) in pathicase(3) (see below).
src/lib/libast/path/pathicase.c,
src/lib/libast/include/ast.h,
src/lib/libast/man/path.3,
src/lib/libast/Mamfile:
- Add new pathicase(3) public API function. This uses whatever
OS-specific method it can detect at compile time to determine if
a particular path is on a case-insensitive file system. If no
method is available, it only sets errno to ENOSYS and returns -1.
Currently known to work on: macOS, Cygwin, Linux 5.2+, QNX 7.0+.
- On systems (if any) that have the mysterious _PC_PATH_ATTRIBUTES
selector for pathconf(2), call astconf(3) and check for the 'c'
attribute to determine case insensitivity. This should preserve
compatibility with any such system.
src/lib/libast/port/astconf.c:
- dynamic[]: As case-insensitive globbing is now optional on all
systems, do not set the 'c' attribute by default on _WINIX
(Cygwin/Windows) systems.
- format(): On systems that do not have _PC_PATH_ATTRIBUTES, call
pathicase(3) to determine the value for the "c" (case
insensitive) attribute only. This is for compatibility as it is
more efficient to call pathicase(3) directly.
src/lib/libast/misc/glob.c,
src/lib/libast/include/glob.h:
- Add new GLOB_DCASE public API flag to glob(3). This is like
GLOB_ICASE (case-insensitive matching) except it only makes the
match case-insensitive if the file system for the current
pathname component is determined to be case-insensitive.
- gl_attr(): For efficiency, call pathicase(3) directly instead of
via astconf(3).
- glob_dir(): Only call gl_attr() to determine file system case
insensitivity if the GLOB_DCASE flag was passed. This makes case
insensitive globbing optional on all systems.
- glob(): The options bitmask needs to be widened to fit the new
GLOB_DCASE option. Define this centrally in a new GLOB_FLAGMASK
macro so it is easy to change it along with GLOB_MAGIC (which
uses the remaining bits for a sanity check bit pattern).
src/lib/libast/path/pathexists.c:
- For efficiency, call pathicase(3) directly instead of via
astconf(3).
*** ksh changes ***
src/cmd/ksh93/features/options,
src/cmd/ksh93/SHOPT.sh:
- Add new SHOPT_GLOBCASEDET compile-time option. Set it to probe
(empty) by default so that the shell option is compiled in on
supported systems only, which is determined by new iffe feature
test that checks if pathicase(3) returns an ENOSYS error.
src/cmd/ksh93/data/options.c,
src/cmd/ksh93/include/shell.h:
- Add -o globcasedetect shell option if compiling with
SHOPT_GLOBCASEDET.
src/cmd/ksh93/sh/expand.c: path_expand():
- Pass the new GLOB_DCASE flag to glob(3) if the
globcasedetect/SH_GLOBCASEDET shell option is set.
src/cmd/ksh93/edit/completion.c:
- While file listing/completion is based on globbing and
automatically becomes case-insensitive when globbing does, it
needs some additional handling to make a string comparison
case-insensitive in corresponding cases. Otherwise, partial
completions may be deleted from the command line upon pressing
tab. This code was already in ksh 93u+ and just needs to be
made conditional upon SHOPT_GLOBCASEDET and globcasedetect.
- For efficiency, call pathicase(3) directly instead of via
astconf(3).
src/cmd/ksh93/sh.1:
- Document the new globcasedetect shell option.
Upon encountering two filenames with multibyte characters starting
with the same byte, a partial multibyte character was completed.
Reproducer (to run in UTF-8 locale):
$ touch XXXá XXXë
$ : XX <== pres tab
$ : XXX^? <== partial multibyte character appears
Note: á is $'\xc3\xa1' and ë is $'\xc3\xab' (same initial byte).
src/cmd/ksh93/edit/completion.c:
- Add multibyte support to the charcmp() and overlaid() functions.
Thanks to Harald van Dijk for useful code and suggestions.
- Add a few missing mbinit() calls. The state of multibyte
processing must be reset before starting a new loop in case a
previous processing run was interrupted mid-character.
src/cmd/ksh93/tests/pty.sh:
- Add test based on Harald's reproducer.
Resolves: https://github.com/ksh93/ksh/issues/223
A discipline function could incorrectly influence the value of $?
(exit status of last command) outside its context if it was
triggered without another command being run, e.g. when a prompt
variable is read, or COLUMNS or LINES is set.
Reproducers include:
PS1 prompt:
$ PS1.get() { true; }
$ false
$ echo $?
0
PS2 prompt:
$ PS2.get() { return 13; }
$ \
>
$ echo $?
13
The set discipline is affected too, e.g. COLUMNS and LINES:
$ COLUMNS.set() { return 13; }
$ true
$ (press return)
$ echo $?
13
There are probably other contexts where the shell reads or changes
variables without running commands, allowing their get or set
disciplines to influence $?. So this commit makes ksh save $? for
all .get, .set, .append, and .unset discipline calls.
src/cmd/ksh93/sh/nvdisc.c:
- assign(): Save/restore $? when running a .set/.append/.unset
discipline function.
- lookup(): Save/restore $? when running a .get discipline.
src/cmd/ksh93/tests/pty.sh:
- Add a regression test for $? after displaying a prompt
and when setting a LINES.set discipline function.
src/cmd/ksh93/tests/return.sh:
- The above test fails in script form on ksh93u+ and ksh2020, as
it exposes another form of #117 that occurs after running a
subshell. Add the above regression test here as well
(re: 092b90da).
Co-authored-by: Martijn Dekker <martijn@inlv.org>
There was an issue with tilde expansion if the HOME var is unset.
$ unset HOME
$ echo ~
martijn
Only the username is returned. Users are more likely to expect the
current user's home directory as configured in the OS.
POSIXly, the expansion of ~ is based on the value of HOME. If HOME
is unset, the results are unspecified. After unsetting HOME, in
bash, ~ returns the user's home directory as specified by the OS,
whereas in all other shells, ~ expands to the empty string. Only
ksh93 returns the username. The behaviour of bash is more useful.
Discussion:
https://github.com/ksh93/ksh/pull/225#issuecomment-799074107
src/cmd/ksh93/sh/macro.c,
src/cmd/ksh93/tests/tilde.sh:
- sh_tilde(): Backport fix by Mike Gilbert from ksh2020.
See: https://github.com/att/ast/issues/1391https://github.com/att/ast/pull/1396070d365d
- Add test.
src/cmd/ksh93/COMPATIBILITY:
- Note this change.
This commit fixes a long-standing bug (present since at least
ksh93r) that caused a file descriptor leak when passing a process
substitution to a function, or (if compiled with SHOPT_SPAWN) to a
nonexistent command.
The leaks only occurred when ksh was compiled with SHOPT_DEVFD; the
FIFO method was unaffected.
src/cmd/ksh93/sh/xec.c: sh_exec():
- When a process substitution is passed to a built-in, the
remaining file descriptor is closed with sh_iorestore. Do the
same thing when passing a process substitution to a function.
This is done by delaying the sh_iorestore() call to 'setexit:'
where both built-ins and functions terminate and set the exit
status ($?).
This means that call now will not be executed if a longjmp is
done, e.g. due to an error in a special built-in. However, there
is already another sh_iorestore() call in main.c, exfile(), line
418, that handles that scenario.
- sh_ntfork() can fail, so rather than assume it will succeed,
handle a failure by closing extra file descriptors with
sh_iorestore(). This fixes the leak on command not found with
SHOPT_SPAWN.
src/cmd/ksh93/include/defs.h:
- Since the file descriptor leaks are now fixed, remove the
workaround that forced ksh to use the FIFO method.
src/cmd/ksh93/SHOPT.sh:
- Add SHOPT_DEVFD as a configurable option (default: probe).
src/cmd/ksh93/tests/io.sh:
- Add a regression test for the 'not found' file descriptor leak.
- Add a test to ensure it keeps working with 'command'.
Fixes: https://github.com/ksh93/ksh/issues/67
On systems where ksh needs to use the older and less secure FIFO
method for process substitutions (which is currently all of them as
the more modern and solid /dev/fd method is still broken, see #67),
process substitutions could leave background processes hanging in
these two scenarios:
1. If the parent process exits without opening a pipe to the child
process forked by the process substitution. The fifo_check()
function in xec.c, which is periodically called to check if the
parent process still exists while waiting for it to open the
FIFO, verified the parent process's existence by checking if the
PPID had reverted to 1, the traditional PID of init. However,
POSIX specifies that the PPID can revert to any implementation-
defined system process in that case. So this breaks on certain
systems, causing unused process substitutions to hang around
forever as they never detect that the parent disappeared.
The fix is to save the current PID before forking and having the
child check if the PPID has changed from that saved PID.
2. If command invoked from the main shell is passed a process
substitution, but terminates without opening the pipe to the
process substitution. In that case, the parent process never
disappears in the first place, because the parent process is the
main shell. So the same infinite wait occurs in unused process
substitutions, even after correcting problem 1.
The fix is to remember all FIFOs created for any number of
process substitutions passed to a single command, and unlink any
remaining FIFOs as they represent unused command substitutions.
Unlinking them FIFOs causes sh_open() in the child to fail with
ENOENT on the next periodic check, which can easily be handled.
Fixing these problems causes the FIFO method to act identically to
the /dev/fd method, which is good for compatibility. Even when #67
is fixed this will still be important, as ksh also runs on systems
that do not have /dev/fd (such as AIX, HP-UX, and QNX), so will
fall back to using FIFOs.
--- Fix problem 1 ---
src/cmd/ksh93/sh/xec.c:
- Add new static fifo_save_ppid variable.
- sh_exec(): If a FIFO is defined, save the current PID in
fifo_save_ppid for the forked child to use.
- fifo_check(): Compare PPID against the saved value instead of 1.
--- Fix problem 2 ---
To keep things simple I'm abusing the name-value pair routines used
for variables for this purpose. The overhead is negligible. A more
elegant solution is possible but would involve adding more code.
src/cmd/ksh93/include/defs.h: _SH_PRIVATE:
- Define new sh.fifo_tree pointer to a new FIFO cleanup tree.
src/cmd/ksh93/sh/args.c: sh_argprocsubs():
- After launching a process substitution in the background,
add the FIFO to the cleanup list before freeing it.
src/cmd/ksh93/sh/xec.c:
- Add fifo_cleanup() that unlinks all FIFOs in the cleanup list and
clears/closes the list. They should only still exist if the
command never used them, however, just run 'unlink' and don't
check for existence first as that would only add overhead.
- sh_exec():
* Call fifo_cleanup() on finishing all simple commands (when
setting $?) or when a special builtin fails.
* When forking, clear/close the cleanup list; we do not want
children doing duplicate cleanup, particularly as this can
interfere when using multiple process substitutions in one
command.
* Process substitution handling:
> Change FIFO check frequency from 500ms to 50ms.
Note that each check sends a signal that interrupts open(2),
causing sh_open() to reinvoke it. This causes sh_open() to
fail with ENOENT on the next check when the FIFO no longer
exists, so we do not need to add an additional check for
existence to fifo_check(). Unused process substitutions now
linger for a maximum of 50ms.
> Do not issue an error message if errno == ENOENT.
- sh_funct(): Process substitutions can be passed to functions as
well, and we do not want commands within the function to clean up
the FIFOs for the process substitutions passed to it from the
outside. The problem is solved by simply saving fifo_tree in a
local variable, setting it to null before running the function,
and cleaning it up before restoring the parent one at the end.
Since sh_funct() is called recursively for multiple-level
function calls, this correctly gives each function a locally
scoped fifo_tree.
--- Tests ---
src/cmd/ksh93/tests/io.sh:
- Add tests covering the failing scenarios.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
These expansions are supposed to yield all variable names beginning
with the indicated prefix. This should include the variable name
that is identical to the prefix (as 'prefix' begins with 'prefix').
This bugfix is backported from the abandoned ksh 93v- beta, so AT&T
intended this change. It also makes ksh work like bash in this.
src/cmd/ksh93/sh/macro.c: varsub(): M_NAMESCAN:
- Check if the prefix itself exists. If so, start with that.
src/cmd/ksh93/tests/variables.sh:
- Add tests for these expansions.
src/cmd/ksh93/sh.1:
- Fix the incomplete documentation of these expansions.
src/cmd/ksh93/COMPATIBILITY:
- Note the change as it's potentially incompatible in corner cases.
Resolves: https://github.com/ksh93/ksh/issues/183
The fix for '.' and '..' in regular globbing broke '.' and '..' in
globstar. No globstar pattern that contains '.' or '..' as any
pathname component still matched. This commit fixes that.
This commit also makes symlink/** mostly work, which it never has
done in any ksh93 version. It is correct and expected that symlinks
found by patterns are not resolved, but symlinks were not resolved
even when specified as explicit non-pattern pathname components.
For example, /tmp/** breaks if /tmp is a symlink (e.g. on macOS),
which looks like a bug.
src/lib/libast/include/glob.h,
src/lib/libast/misc/glob.c: glob_dir():
- Make symlink/** work. we can check if the string pointed to by
pat is exactly equal to *. If so, we are doing regular globbing
for that particular pathname element, and it's okay to resolve
symlinks. If not (if it's **), we're doing globstar and we should
not be matching symlinks.
- Let's also introduce proper identification of symlinks (GLOB_SYM)
and not lump them in with other special files (GLOB_DEV).
- Fix the bug with literal '.' and '..' components in globstar
patterns. In preceding code, the matchdir pointer gets set to the
complete glob pattern if we're doing globstar for the current
pathname element, null if not. The pat pointer gets set to the
elements of the pattern that are still left to be processed;
already-done elements are trimmed from it by increasing the
pointer. So, to do the right thing, we need to make sure that '.'
or '..' is skipped if, and only if, it is the final element in
the pattern (i.e., if pat does not contain a slash) and is not
specified literally as '.' or '..', i.e., only if '.' or '..' was
actually resolved from a glob pattern. After this change,
'**/.*', '**/../.*', etc. do the right thing, showing all your
hidden files and directories without undesirable '.' and '..'
results; '.' and '..' are skipped as final elements, unless you
literally specify '**/.', '**/..', '**/foo/bar/..', etc.
src/cmd/ksh93/COMPATIBILITY:
- Note the symlink/** globstar change.
src/cmd/ksh93/sh.1:
- Try to document the current globstar behaviour more exhausively.
src/cmd/ksh93/tests/glob.sh:
- Add tests. Try to cover all the corner cases.
src/cmd/ksh93/tests/shtests:
- Since tests in glob.sh do not use err_exit, they were not
counted. Special-case glob.sh for counting the tests: count the
lines starting with a test_* function call.
Resolves: https://github.com/ksh93/ksh/issues/146
These POSIX expansions first assign y to x if x is unset or empty,
respectively, and then they yield the value of x. This was not
working on any ksh93 version if x was typeset as numeric (integer
or float) but still unset, as in not assigned a value.
$ unset a; typeset -i a; printf '%q\n' "${a:=42}" "$a"
0
''
Expected output:
42
42
src/cmd/ksh93/sh/macro.c:
- Fix the test for set/unset variable. It was broken because it
only checked for the existence of the node, which exists after
'typeset', but did not check if a value had been assigned. This
additional check needs to be done with the nv_isnull() macro, but
only for expansions of the regular M_BRACE type. Special
expansions cannot have an unset state.
- As of commit 95294419, we know that an nv_optimize() call may be
needed before using nv_isnull() if the shell is compiled with
SHOPT_OPTIMIZE. Move the nv_optimize() call from that commit
forward to before the new check that calls nv_isnull(), and only
bother with it if the type is M_BRACE.
src/cmd/ksh93/tests/variables.sh:
- Add tests for this bug. Test float and integer, and also check
that ${a=b} and ${a:=b} correctly treat the value of 'b' as an
arithmetic expression of which the result is assigned to 'a' if
'a' was typeset as numeric.
src/cmd/ksh93/tests/attributes.sh,
src/cmd/ksh93/tests/comvar.sh,
src/cmd/ksh93/tests/nameref.sh,
src/cmd/ksh93/tests/types.sh:
- Fix a number of tests to report failures correctly.
Resolves: https://github.com/ksh93/ksh/issues/157
The old Bourne shell failed to check for closing quotes and command
substitution backticks when encountering end-of-file in a parser
context (such as a script). ksh93 implemented a hack for partial
compatibility with this bug, tolerating unbalanced quotes and
backticks in backtick command subsitutions, 'eval', and command
line invocation '-c' scripts only.
This hack became broken for backtick command substitutions in
fe20311f/350b52ea as a memory leak was fixed by adding a newline to
the stack at the end of the command substitution. That extra
newline becomes part of any string whose quotes are not properly
terminated, causing problems such as the one detailed here:
https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01889.html
$ touch abc
$ echo `ls "abc`
ls: abc
: not found
No other fix for the memory leak is known that doesn't cause other
problems. (The alternative fix detailed in the referenced mailing
list post causes a different corner-case regression.)
Besides, the hack has always caused other corner case bugs as well:
$ ksh -c '((i++'
Actual: ksh: i++(: not found
(If an external command 'i++(' existed, it would be run)
Expect: ksh: syntax error at line 1: `(' unmatched
$ ksh -c 'i=0; echo $((++i'
Actual: (empty line; the arithmetic expansion is ignored)
Expect: ksh: syntax error at line 1: `(' unmatched
$ ksh -c 'echo $(echo "hi)'
Actual: ksh: syntax error at line 1: `(' unmatched
Expect: ksh: syntax error at line 1: `"' unmatched
So, it's time to get rid of this hack. The old Bourne shell is
dead and buried. No other shell tries to support this breakage.
Tolerating syntax errors is just asking for strange side effects,
inconsistent states, and corner case bugs. We should not want to do
that. Old scripts that rely on this will just need to be fixed.
src/cmd/ksh93/sh/lex.c:
- struct lexdata: Remove 'char balance' member for remembering an
unbalanced quote or backtick.
- sh_lex(): Remove the back to remember and compensate for
unbalanced quotes/backticks that was executed only if we were
executing a script from a string, as opposed to a file.
src/cmd/ksh93/COMPATIBILITY:
- Note the change.
Resolves: https://github.com/ksh93/ksh/issues/199
This commit fixes an arbitrary command execution vulnerability in
array subscripts used within the arithmetic subsystem.
One of the possible reproducers is:
var='1$(echo INJECTION >&2)' ksh -c \
'typeset -A a; ((a[$var]++)); typeset -p a'
Output before this commit:
INJECTION
typeset -A a=([1]=1)
The 'echo' command has been surreptitiously executed from an
external environment variable.
Output after this commit:
typeset -A a=(['1$(echo INJECTION >&2)']=1)
The value is correctly used as an array subscript and nothing in it
is parsed or executed. This is as it should be, as ksh93 supports
arbitrary subscripts for associative arrays.
If we think about it logically, the C-style arithmetic subsystem
simply has no business messing around with shell expansions or
quoting at all, because those don't belong to it. Shell expansions
and quotes are properly resolved by the main shell language before
the arithmetic subsystem is even invoked. It is particularly
important to maintain that separation because the shell expansion
mechanism also executes command substitutions.
Yet, the arithmetic subsystem subjected array subscripts that
contain `$` (and only array subscripts -- how oddly specific) to
an additional level of expansion and quote resolution. For some
unfathomable reason, there are two lines of code doing specifically
this. The vulnerability is fixed by simply removing those.
Incredibly, variants of this vulnerability are shared by bash, mksh
and zsh. Instead of fixing it, it got listed in Bash Pitfalls!
http://mywiki.wooledge.org/BashPitfalls#y.3D.24.28.28_array.5B.24x.5D_.29.29
src/cmd/ksh93/sh/arith.c:
- scope(): Remove these two lines that implement the vulnerability.
if(strchr(sub,'$'))
sub = sh_mactrim(shp,sub,0);
- scope(), arith(): Remove the NV_SUBQUOTE flag from two
nv_endsubscript() calls. That flag causes the array subscript to
retain the current level of shell quoting. The shell quotes
everything as in "double quotes" before invoking the arithmetic
subsystem, and the bad sh_mactrim() call removed one level of
quoting. Since we're no longer doing that, this flag should no
longer be passed, or subscripts may get extra backslash escapes.
src/cmd/ksh93/include/name.h,
src/cmd/ksh93/sh/array.c:
- nv_endsubscript(): The NV_SUBQUOTE flag was only passed from
arith.c. Since it is now unused, remove it.
src/cmd/ksh93/tests/arith.sh:
- Tweak some tests: fix typos, report wrong values.
- Add 21 tests. Most are based on reproducers contributed by
@stephane-chazelas and @hyenias. They verify that this
vulnerability is gone and that no quoting bugs were introduced.
Resolves: https://github.com/ksh93/ksh/issues/152
Corrected the size of attribute(s) being overwritten with 0 when
'readonly' or 'typeset -r' was applied to an existing variable. Since
one cannot set any attributes with the 'readonly' command, its function
call to setall() needs to be adjusted to acquire the current size from
the old size or existing size of the variable. A plain 'typeset -r' is
the same as 'readonly' in that it needs to load the old size as its
current size for use in the subsequent to call to nv_newattr().
src/cmd/ksh93/bltins/typeset.c: setall():
- Both 'readonly' and 'typeset -r' end up calling setall(). setall()
has full visibility into all user supplied values and existing
values that are needed to differentiate whereas name.c newattr()
acquires combined state flags.
- Added a conditional check if the readonly flag was requested by
user then meets the criteria of having present size of 0, cannot
be a numeric nor binary string, and is void of presence of any of
the justified string attributes.
- -L/R/Z justified string attributes if not given a value default
to a size of 0 which means to autosize. A binary string can have
a fixed field size, e.g. -bZ. The present of any of the -L/R/Z
attribules means that current size is valid and should be used
even if it is zero.
src/cmd/ksh93/tests/attributes.sh:
- Added various tests to capture and reiterate that 'readonly' should
be equivalent to 'typeset -r' and applying them should not alter the
previous existing size unless additional attributes are set along
with typeset command.
1. The editor accepted literal tabs without escaping in certain
cases, causing buggy and inconsistent completion behaviour.
https://github.com/ksh93/ksh/issues/71#issuecomment-656970959https://github.com/ksh93/ksh/issues/71#issuecomment-657216472
2. After completing a filename by choosing from a file completion
menu, the terminal cursor was placed one position too far to the
right, corrupting command line display. This happened with
multiline active.
https://github.com/ksh93/ksh/issues/71#issue-655093805
3. A completion menu was displayed if the file name to be completed
was at the point where the rest of it started with a number,
even if that part uniquely identified it so the menu had 1 item.
https://www.mail-archive.com/ast-users@lists.research.att.com/msg00436.html
src/cmd/ksh93/edit/emacs.c:
- Cosmetic consistency: change two instances of cntl('[') to ESC.
- ed_emacsread(): Fix number 1 by refusing to continue into default
processing if a tab character was not used for tab completion.
Instead, beep and continue to the next read loop iteration. This
behaviour is consistent with most other shells, so I doubt there
will be objections. To enter a literal tab it's simple enough to
escape it with ^V (the 'stty lnext' character) or \.
- draw(): Fix number 2 by correcting an off-by-one error in the
ed_setcursor() call that updates the terminal's cursor display
in multiline mode. The 'old' and 'new' parameters need to have
identical values in this particular call to avoid the cursor
position being off by one to the right. This change makes it
match the corresponding ed_setcursor() call in vi.c. See below*
for details. Thanks to Lev Kujawski for the help in analysing.
src/cmd/ksh93/edit/completion.c: ed_expand():
- Fix number 3 by changing from '=' mode (menu-based completion) to
'\' mode (ordinary filename completion) if the menu would only
show one option, which was pointless and annoying. This never
happened in vi mode, so possibly the ed_expand() call in emacs.c
could have been improved instead. But I'm comfortable with fixing
it here and not in emacs.c, because this fixes it at a more
fundamental level, plus it's straightforward and obvious here.
Resolves: https://github.com/ksh93/ksh/issues/71
____
* Further details on bug number 2:
At https://github.com/ksh93/ksh/issues/71#issuecomment-786391565
Martijn Dekker wrote:
> I'm back to my original hypothesis that there is somehow an
> off-by-one error related to the ed_setcursor() call that gets
> executed when in multiline mode. I cannot confirm whether that
> off-by-one error is actually in the call itself, or occurs
> sometime earlier on one of the many possible occasions where
> ep->cursor is changed. But everything else appears to work
> correctly, so it's not unlikely that the problem is in the call
> itself.
>
> For reference, this is the original version of that call in
> emacs.c:
>
> ksh/src/cmd/ksh93/edit/emacs.c
> Lines 1556 to 1557 in df2b9bf
> if(ep->ed->e_multiline && option == REFRESH)
> ed_setcursor(ep->ed, ep->screen, ep->cursor-ep->screen, ep->ed->e_peol, -1);
>
> There is a corresponding call in the vi.c refresh() function
> (which does the same thing as draw() in emacs.c), where the third
> (old) and fourth (new) arguments are actually identical:
>
> ksh/src/cmd/ksh93/edit/vi.c
>
> Lines 2086 to 2087 in df2b9bf
> if(vp->ed->e_multiline && vp->ofirst_wind==INVALID)
> ed_setcursor(vp->ed, physical, last_phys+1, last_phys+1, -1);
>
> The expectation for this particular call is in fact that they
> should be identical, so that a delta of zero is calculated in
> that function. Delta not being zero is what causes the cursor to
> be positioned wrong.
>
> In vi.c, last_phys is a macro that is defined as editb.e_peol,
> and editb is a macro that is defined as (*vp->ed). Which means
> last_phys means vp->ed->e_peol, which is the same as
> ep->ed->e_peol in emacs.c. (These editors were originally
> separate programs by different authors, and I suppose this is how
> it shows. Korn didn't want to change all the variable names to
> integrate them, so made macros instead.)
>
> That leaves the question of why vi.c adds 1 to both last_phys
> a.k.a. e_peol arguments, and emacs.c uses e_peol for new without
> adding anything. Analysing the ed_setcursor() code could answer
> that question.
>
> So, this patch makes emacs.c do it the same way vi.c does. Let's
> make the third argument identical to the fourth. My brief testing
> shows the bug is fixed, and the regression tests yield no
> failures. This fix is also the most specific change possible, so
> there are few opportunities for side effects (I hope).
At https://github.com/ksh93/ksh/issues/71#issuecomment-786466652
Lev Kujawski wrote:
> I did a bit of research on this, and I think the fix to have the
> Emacs editing mode do the same as Vi is correct.
>
> From RELEASE:
> 08-05-01 In multiline edit mode, the refresh operation will now clear
> the remaining portion of the last line.
>
> Here's a fragment from the completion.c of the venerable but
> dated CDE DtKsh:
>
> else
> while (*com)
> {
> *out++ = ' ';
> out = strcopy(out,*com++);
> }
> *cur = (out-outbuff);
> /* restore rest of buffer */
> out = strcopy(out,stakptr(0));
> *eol = (out-outbuff);
>
> Noticeably missing is the code to add a space after file name
> completions. So, it seems plausible that if multiline editing
> mode was added beforehand,the ep->ed->p_eol !=
> ep->cursor-ep->screen case might never have occurred during
> testing.
>
> Setting the 'first' parameter to -1 seems to be a pretty explicit
> indicator that the author(s) intended the line clearing code to
> run, hence the entry in RELASE.
>
> The real issue is that if we update the cursor by calling
> ed_setcursor on line 1554 with old != new, the later call to
> setcursor on line 1583, here:
>
> I = (ncursor-nscreen) - ep->offset;
> setcursor(ep,i,0);
>
> will use outdated screen information to call setcursor, which,
> coincidentally, calls ed_setcursor.
In the emacs editor:
1. press ESC
2. change the size of your terminal window
and your screen is mysteriously cleared. (Until recent fixes, the
shell probably also crashed somewhere in the job control code.)
The cause is the way SIGWINCH is handled by ed_read() in edit.c.
For the emacs editor, it sends a Ctrl+L character to the input
buffer. The Ctrl+L command refreshes the command line. And it so
happens that ESC plus Ctrl+L is a command to clear the screen in
the emacs editor.
With the exeption of vi insert/command mode for which it uses a
shared flag, edit.c does not know the state of the editor, because
their data are internal to emacs.c and vi.c. So it doesn't know
whether you're in some mode that treats keyboard input specially.
Which means this way of dealing with SIGWINCH is fundamentally
misdesigned and is not worth fixing.
It gets sillier: in addition to sending keyboard commands, edit.c
was also communicating directly with emacs.c and vi.c via a flag,
e_nocrnl, which means 'please don't make Ctrl+L emit a linefeed'
(it normally refreshes on a new line but that is undesirable for
SIGWINCH). So there is already a hack that breaks the barrier
between edit.c and emacs.c/vi.c. Let's do that properly instead.
As of this commit, ed_read() does not send any fake keystrokes.
Instead, two extern functions, emacs_redraw() and vi_redraw(), are
defined for redrawing the command line. These are put in emacs.c
and vi.c so they have access to relevant static data and functions.
Then, instead of sending keyboard commands to the editor and
returning, ed_read() simply calls the redraw function for the
active editor, then continues and waits for input. Much cleaner.
src/cmd/ksh93/include/edit.h:
- Remove e_nocrnl flag from Edit_t struct.
- Define externs emacs_redraw() and vi_redraw(). Since Emacs_t and
Vi_t types are not known here, we have to declare void* pointers
and the functions will have to use typecasts.
src/cmd/ksh93/edit/edit.c:
- ed_read(): Call emacs_redraw() or vi_redraw() as per above.
- ed_getchar(): Remove comment about a nonexistent while loop.
src/cmd/ksh93/edit/emacs.c:
- Updates corresponding to removal of e_nocrnl flag.
- Add emacs_redraw(). This one is pretty simple. Refresh the
command line, then ed_flush() to update the cursor display.
src/cmd/ksh93/edit/vi.c:
- Updates corresponding to removal of e_nocrnl flag. Also remove a
similar internal 'nonewline' flag which is now equally redundant.
- Move the Ctrl+L handling code (minus writing the newline) into
the vi_redraw() function.
- Change two cases where vi set nonewline and sent Ctrl+L to itself
into simple vi_redraw() calls.
- Add vi_redraw(). This is more complicated as it incorporates the
previous Ctrl+L code. It needs an added refresh() call with a
check whether we're currently in command or insert mode, as those
use different refresh methods. Luckily edit.c already maintains
an *e_vi_insert flag in ed_getchar() that we can use. Since vi's
refresh() already calls ed_flush(), we don't need to add that.
This commit fixes a bug in the 'read' built-in: it did not properly
skip over multibyte characters. The bug never affects UTF-8 locales
because all UTF-8 bytes have the high-order bit set. But Shift-JIS
characters may include a byte corresponding to the ASCII backslash
character, which cauased buggy behaviour when using 'read' without
the '-r' option that disables backslash escape processing.
It also makes the regression tests compatible with Shift-JIS
locales. They failed with syntax errors.
src/cmd/ksh93/bltins/read.c:
- Use the multibyte macros when skipping over word characters.
Based on a patch from the old ast-developers mailing list:
https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01848.html
src/cmd/ksh93/include/defs.h:
- Be a bit smarter about causing the compiler to optimise out
multibyte code when SHOPT_MULTIBYTE is disabled. See the updated
comment for details.
src/cmd/ksh93/tests/locale.sh:
- Put all the supported locales in an array for future tests.
- Add test for the 'read' bug. Include it in a loop that tests
64 SHIFT-JIS character combinations. Only one fails on old ksh:
the one where the final byte corresponds to the ASCII backslash.
It doesn't hurt to test all the others anyway.
src/cmd/ksh93/tests/basic.sh,
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/quoting2.sh:
- Fix syntax errors that occurred in SHIFT-JIS locales as the
parser was processing literal UTF-8 characters. Not executing
that code is not enough; we need to make sure it never gets
parsed as well. This is done by wrapping the commands containing
literal UTF-8 strings in an 'eval' command as a single-quoted
operand.
.github/workflows/ci.yml:
- Run the tests in the ja_JP.SJIS locale instead of ja_JP.UTF-8.
UTF-8 is already covered by the nl_NL.UTF-8 test run; that should
be good enough.
This commit fixes the following:
1. Emacs mode ignores --nobackslashctrl (re: 24598fed) when in
reverse search.
2. When entering more than one backslash, emacs reverse search mode
deletes multiple backslashes after pressing backspace once.
Reproducer:
$ set --emacs --nobackslashctrl
$ <Ctrl+R> \\\\<Backspace>
3. Except when in reverse search, the backslash fails to escape a
subsequent interrupt character (^C). Reproducer:
$ set --emacs --backslashctrl
$ teststring \<Ctrl+C>
src/cmd/ksh93/edit/emacs.c:
- Disable escaping backslashes in emacs reverse search if
'nobackslashctrl' is enabled.
- Fix the buggy behavior of backslashes in emacs reverse
search by processing backslashes in a loop.
src/cmd/ksh93/tests/pty.sh:
- Add regression tests.
src/cmd/ksh93/sh.1:
- Fix a minor documentation error (^C is the usual interrupt
character, not ^?).
Co-authored-by: Martijn Dekker <martijn@inlv.org>
The following emacs editor 'feature' kept making me want to go back
to bash. I forget a backslash escape in a command somewhere. So I
go back to insert it. I type the \, then want to go forward. My
right arrow key, instead of moving the cursor, then replaces my
backslash with garbage. Why? The backslash escapes the following
control character at the editor level and inserts it literally.
The vi editor has a variant of this which is much less harmful. It
only works in insert mode and the backslash only escapes the next
kill or erase character.
In both editors, this feature is completely redundant with the
'stty lnext' character which is ^V by default -- and works better
as well because it also escapes ^C, ^J (linefeed) and ^M (Return).
[In fact, you could even issue 'stty lnext \\' and get a much more
consistent version of this feature on any shell. You have to type
two backslashes to enter one, but it won't kill your cursor keys.]
If it were up to me alone, I'd simply remove this misfeature from
both editors. However, it is long-standing documented behaviour.
It's in the 1995 book. Plus, POSIX specifies the vi variant of it.
So, this adds a shell option instead. It was quite trivial to do.
Now I can 'set --nobackslashctrl' in my ~/.kshrc. What a relief!
Note: To keep .kshrc compatibile with older ksh versions, use:
command set --nobackslashctrl 2>/dev/null
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/data/options.c:
- Add new SH_NOBACKSLCTRL/"nobackslashctrl" long-form option. The
"no" prefix shows it to the user as "backslashctrl" which is on
by default. This avoids unexpectedly changing historic behaviour.
src/cmd/ksh93/edit/emacs.c: ed_emacsread(),
src/cmd/ksh93/edit/vi.c: getline():
- Only set the flag for special backslash handling if
SH_NOBACKSLCTRL is off.
src/cmd/ksh93/sh.1,
src/cmd/ksh93/data/builtins.c:
- Document the new option (as "backslashctrl", on by default).
Other minor tweaks:
src/cmd/ksh93/edit/edit.c:
- ed_setup(): Add fallback #error if no tput method is set. This
should never be triggered; it's to catch future editing mistakes.
- escape(): cntl('\t') is nonsense as '\t' is already a control
character, so change this to just '\t'.
- xcommands(): Let's enable the ^X^D command for debugging
information on non-release builds.
src/cmd/ksh93/features/cmds:
- The tput feature tests assumed a functioning terminal in $TERM.
However, for all we know we might be compiling with no tty and
TERM=dumb. The tput commands won't work then. So set TERM=ansi
to use a standard default.
This fixes the following:
1. 'set --posix' now works as an equivalent of 'set -o posix'.
2. The posix option turns off braceexpand and turns on letoctal.
Any attempt to override that in a single command such as 'set -o
posix +o letoctal' was quietly ignored. This now works as long
as the overriding option follows the posix option in the command.
3. The --default option to 'set' now stops the 'posix' option, if
set or unset in the same 'set' command, from changing other
options. This allows the command output by 'set +o' to correctly
restore the current options.
src/cmd/ksh93/data/builtins.c:
- To make 'set --posix' work, we must explicitly list it in
sh_set[] as a supported option so that AST optget(3) recognises
it and won't override it with its own default --posix option,
which converts the optget(3) string to at POSIX getopt(3) string.
This means it will appear as a separate entry in --man output,
whether we want it to or not. So we might as well use it as an
example to document how --optionname == -o optionname, replacing
the original documentation that was part of the '-o' description.
src/cmd/ksh93/sh/args.c: sh_argopts():
- Add handling for explitit --posix option in data/builtins.c.
- Move SH_POSIX syncing SH_BRACEEXPAND and SH_LETOCTAL from
sh_applyopts() into the option parsing loop here. This fixes
the bug that letoctal was ignored in 'set -o posix +o letoctal'.
- Remember if --default was used in a flag, and do not sync options
with SH_POSIX if the flag is set. This makes 'set +o' work.
src/cmd/ksh93/include/argnod.h,
src/cmd/ksh93/data/msg.c,
src/cmd/ksh93/sh/args.c: sh_printopts():
- Do not potentially translate the 'on' and 'off' labels in 'set
-o' output. No other shell does, and some scripts parse these.
src/cmd/ksh93/sh/init.c: sh_init():
- Turn on SH_LETOCTAL early along with SH_POSIX if the shell was
invoked as sh; this makes 'sh -o' and 'sh +o' show expected
options (not that anyone does this, but correctness is good).
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/include/shell.h:
- The state flags were in defs.h and most (but not all) of the
shell options were in shell.h. Gather all the shell state and
option flag definitions into one place in shell.h for clarity.
- Remove unused SH_NOPROFILE and SH_XARGS option flags.
src/cmd/ksh93/tests/options.sh:
- Add tests for these bugs.
src/lib/libast/misc/optget.c: styles[]:
- Edit default optget(3) option self-documentation for clarity.
Several changed files:
- Some SHOPT_PFSH fixes to avoid compiling dead code.
Another longstanding whopper of a bug in basic ksh93 functionality:
run a ${ shared-state; } command substitution twice and job control
promptly loses track of all your running jobs. New jobs are tracked
again until you run another two shared-state command substitutions.
This is in at least 93t+, 93u-, 93u+, 93v- and ksh2020.
$ sleep 300 &
[1] 56883
$ jobs # OK
[1] + Running sleep 300 &
$ v=${ echo hi1; }
$ jobs # OK
[1] + Running sleep 300 &
$ v=${ echo hi2; }
$ jobs # Nothing!
$ fg
ksh: fg: no such job
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- The current environment number shp->curenv (a.k.a. sh.curenv) was
not being restored if the virtual subshell we're leaving is of
the shared-state command substitution variety as it was wrongly
considered to be part of the environment that didn't need
restoring. This caused it to be out of sync with shp->jobenv
(a.k.a. sh.jobenv) which did get restored from savedcurenv.
Restore both from savedcurenv at the same time for any subshell.
(How these numbers are used exactly remains to be discovered.)
src/cmd/ksh93/tests/jobs.sh:
- Added, with a test for this bug to start it off. There is no
other test script where job control fits, and a lot more related
fixes are anticipated: https://github.com/ksh93/ksh/issues/119
This is some nonsense: redirections that store a file descriptor
greater than 9 in a variable, like {var}<&2 and the like, stopped
working if brace expansion was turned off. '{var}' is not a brace
expansion as it doesn't contain ',' or '..'; something like 'echo
{var}' is always output unexpanded. And redirections and brace
expansion are two completely unrelated things. It wasn't documented
that these redirections require the -B/braceexpand option, either.
src/cmd/ksh93/sh/lex.c: sh_lex():
- Remove incorrect check for braceexpand option before processing
redirections of this form.
src/cmd/ksh93/COMPATIBILITY:
- Insert a brief item mentioning this.
src/cmd/ksh93/sh.1:
- Correction: these redirections do not yield a file descriptor >
10, but > 9, a.k.a. >= 10.
- Add a brief example showing how these redirections can be used.
src/cmd/ksh93/tests/io.sh:
- Add a quick regression test.
This fixes part of https://github.com/ksh93/ksh/issues/87:
Scalar arrays (-a) and associative arrays (-A) of a type created by
'enum' did not consistently block values not specified by the enum
type, yielding corrupted results.
An expansion of type "${array[@]}" yielded random numbers instead
of values for associative arrays of a type created by 'enum'.
This does not yet fix another problem: ${array[@]} does not yield
all values for associative enum arrays.
src/cmd/ksh93/bltins/enum.c: put_enum():
- Always throw an error if the value is not in the list of possible
values for an enum type. Remove incorrect check for the NV_NOFREE
flag. Whatever that was meant to accomplish, I've no idea.
src/cmd/ksh93/sh/array.c: nv_arraysettype():
- Instead of sh_eval()ing a shell assignment, use nv_putval()
directly. Also use the stack (see src/lib/libast/man/stk.3)
instead of malloc to save the value; it's faster and will be
auto-freed at some point. This shortens the function and makes it
faster by not entering into a whole new shell context -- which
also fixes another problem: the error message from put_enum()
didn't cause the shell to exit for indexed enum arrays.
src/cmd/ksh93/sh/name.c: nv_setlist():
- Apply a patch from David Korn that correctly sets the data type
for associative arrays, fixing the ${array[@]} expansion yielding
random numbers. Thanks to @JohnoKing for the pointer.
https://github.com/ksh93/ksh/issues/87#issuecomment-662613887https://www.mail-archive.com/ast-developers@lists.research.att.com/msg00697.html
src/cmd/ksh93/tests/enum.sh:
- Add tests checking that invalid values are correctly blocked for
indexed and associative arrays of an enum type.
Makes progress on: https://github.com/ksh93/ksh/issues/87
Turns out the assumption I was operating on, that Linux and macOS
align arguments on 32 or 64 bit boundaries, is incorrect -- they
just need some extra bytes per argument. So we can use a bit more
of the arguments buffer on these systems than I thought.
src/cmd/ksh93/features/externs:
- Change the feature test to simply detect the # of extra bytes per
argument needed. On *BSD and commercial Unices, ARG_EXTRA_BYTES
shows as zero; on Linux and macOS (64-bit), this yields 8. On
Linux (32-bit), this yields 4.
src/cmd/ksh93/sh/path.c: path_xargs():
- Do not try to calculate alignment, just add ARG_EXTRA_BYTES to
each argument.
- Also add this when substracting the length of environment
variables and leading and trailing static command arguments.
src/cmd/ksh93/tests/path.sh:
- Test command -v/-V with -x.
- Add a robust regression test for command -x.
src/cmd/ksh93/data/builtins.c, src/cmd/ksh93/sh.1:
- Tweak docs. Glob patterns also expand to multiple words.
This commit fixes 'command -x' to adapt to OS limitations with
regards to data alignment in the arguments list. A feature test is
added that detects if the OS aligns the argument on 32-bit or
64-bit boundaries or not at all, allowing 'command -x' to avoid
E2BIG errors while maximising efficiency.
Also, as of now, 'command -x' is a way to bypass built-ins and
run/query an external command. Built-ins do not limit the length of
their argument list, so '-x' never made sense to use for them. And
because '-x' hangs on Linux and macOS on every ksh93 release
version to date (see acf84e96), few use it, so there is little
reason not to make this change.
Finally, this fixes a longstanding bug that caused the minimum exit
status of 'command -x' to be 1 if a command with many arguments was
divided into several command invocations. This is done by replacing
broken flaggery with a new SH_XARG state flag bit.
src/cmd/ksh93/features/externs:
- Add new C feature test detecting byte alignment in args list.
The test writes a #define ARG_ALIGN_BYTES with the amount of
bytes the OS aligns arguments to, or zero for no alignment.
src/cmd/ksh93/include/defs.h:
- Add new SH_XARG state bit indicating 'command -x' is active.
src/cmd/ksh93/sh/path.c: path_xargs():
- Leave extra 2k in the args buffer instead of 1k, just to be sure;
some commands add large environment variables these days.
- Fix a bug in subtracting the length of existing arguments and
environment variables. 'size -= strlen(cp)-1;' subtracts one less
than the size of cp, which makes no sense; what is necessary is
to substract the length plus one to account for the terminating
zero byte, i.e.: 'size -= strlen(cp)+1'.
- Use the ARG_ALIGN_BYTES feature test result to match the OS's
data alignment requirements.
- path_spawn(): E2BIG: Change to checking SH_XARG state bit.
src/cmd/ksh93/bltins/whence.c: b_command():
- Allow combining -x with -p, -v and -V with the expected results
by setting P_FLAG to act like 'whence -p'. E.g., as of now,
command -xv printf
is equivalent to
whence -p printf
but note that 'whence' has no equivalent of 'command -pvx printf'
which searches $(getconf PATH) for a command.
- When -x will run a command, now set the new SH_XARG state flag.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Change to using the new SH_XARG state bit.
- Skip the check for built-ins if SH_XARG is active, so that
'command -x' now always runs an external command.
src/lib/libcmd/date.c, src/lib/libcmd/uname.c:
- These path-bound builtins sometimes need to run the external
system command by the same name, but they did that by hardcoding
an unportable direct path. Now that 'command -x' runs an external
command, change this to using 'command -px' to guarantee using
the known-good external system utility in the default PATH.
- In date.c, fix the format string passed to 'command -px date'
when setting the date; it was only compatible with BSD systems.
Use the POSIX variant on non-BSD systems.
An unquoted variable expansion evaluated in a DEBUG trap action
caused IFS field splitting to be deactivated in code executed after
the trap action. Thanks to Koichi Nakashima for the reproducer:
| v=''
| trap ': $v' DEBUG
| A="a b c"
| set -- $A
| printf '%s\n' "$@"
|
| Expected
|
| a
| b
| c
|
| Actual
|
| a b c
src/cmd/ksh93/sh/fault.c: sh_trap():
- Remove incorrect save/restore of sh.ifstable, the internal state
table for field splitting. This reverts three lines added in ksh
93t+ 2009-11-30. Analysis: As an expansion is split into fields
(macro.c, lines 2367-2471), sh.ifstable is modified. If that
happens within a DEBUG trap, any modifications in ifstable are
undone by the restoring memccpy, leaving an inconsistent state.
src/cmd/ksh93/COMPATIBILITY:
- Document the DEBUG trap fixes, particularly the incorrect
inheritance by subshells and functions that some scripts may now
rely on because this bug is so longstanding. (re: 2a835a2d)
src/cmd/ksh93/tests/basic.sh:
- Add relevant tests.
Resolves: https://github.com/ksh93/ksh/issues/155
TODO: add a -T (-o functrace) option as in bash, which should allow
subshells and ksh-style functions to inherit DEBUG traps.
P.S.: The very handy multishell repo allows us to use 'git blame'
to trace the origin of the recently fixed DEBUG trap bugs.
The off-by-one error causing various bugs, reverted in 2a835a2d,
was introduced in ksh 93t 2008-07-25:
8e947ccf
(fault.c, line 321)
The incorrect check causing the exit status bug, reverted in
d00b4b39, was introduced in ksh 93t 2008-11-04:
b1ade268
(fault.c, line 459)
The ifstable save/restore causing the field splitting bug, reverted
in this commit, was introduced in ksh 93t+ 2009-11-30:
53d9f009
(fault.c, lines 440, 444, 482)
So all the bugs reported in #155 were fixed by simply reverting
these specific changes. I think that they are some experiments that
the developers simply forgot to remove. I've suspected such a thing
multiple times before. ksh93 was developed by researchers who were
genius innovators, but incredibly sloppy maintainers.
This trap failed to be restored correctly when being trapped in
a subshell, causing corruption or a crash when restoring the
parent shell environment's trap upon leaving the subshell.
Thanks to Koichi Nakashima for the report and reproducer.
src/cmd/ksh93/sh/fault.c: sh_sigreset():
- Fix an off-by-one error in the loop that restores the
pseudosignal traps.
src/cmd/ksh93/tests/basic.sh:
- Test overwriting the main shell trap in a subshell for all
pseudosignals.
Makes progress on: https://github.com/ksh93/ksh/issues/155
src/cmd/ksh93/include/version.h:
- Centrally define the 93u+m copyright (SH_RELEASE_CPYR) for adding
to the original AT&T copyright in 'ksh --man' and 'shcomp --man'.
- Centrally define the binary header version number for bytecode
generated by shcomp: SHCOMP_HDR_VERSION.
- Bump SHCOMP_HDR_VERSION from 3 to 4. Converting all the preset
aliases to builtin commands has caused new bytecode to be
incompatible with old ksh. (However, old bytecode runs fine on
93u+m, because shcomp pre-expands the preset aliases.)
src/cmd/ksh93/sh/shcomp.c:
- Instead of keeping its own version date (not changed since 2003),
use the same version string as ksh itself (SH_RELEASE).
- Use SH_RELEASE_CPYR for the extra 93u+m copyright string.
- Use SHCOMP_HDR_VERSION for the bytecode header.
src/cmd/ksh93/sh/parse.c: sh_parse():
- Use SHCOMP_HDR_VERSION for the bytecode version check.
src/cmd/ksh93/data/builtins.c: opt_ksh[]:
- Use SH_RELEASE_CPYR for the extra 93u+m copyright string.
src/cmd/ksh93/COMPATIBILITY:
- Mention that 93u+m shcomp bytecode won't run on older ksh.
- Document changes in printf %T (re: 9526b3fa).
src/cmd/ksh93/README:
- Mention that we run on UnixWare (with major regressions).
https://github.com/ksh93/ksh/pull/159#issuecomment-764667929
This fixes the following:
trap ':' DEBUG
r=$(exit 123)
echo $? # Expected 123, but actually 0.
Thanks to Koichi Nakashima for the report and reproducer.
src/cmd/ksh93/sh/fault.c: sh_trap():
- Restore the saved current exit status (exitval) for all traps.
Do not except the DEBUG trap from doing that. I've no idea why
this exception was made, but it's not correct.
src/cmd/ksh93/tests/basic.sh:
- Add tests.
Makes progress on: https://github.com/ksh93/ksh/issues/155
