This bug was first reported in <https://www.illumos.org/issues/7694>.
The time keyword currently overrides the errexit shell option,
allowing failing scripts to continue after an error:
$ cat 1.sh
#!/bin/sh
time false # This should cause the script to exit
echo FAILURE
true
$ ksh -o errexit 1.sh
real 0m0.00s
user 0m0.00s
sys 0m0.00s
FAILURE
src/cmd/ksh93/sh/xec.c:
- When the time keyword runs a command, pass the errexit state flag
to the sh_exec call. This state flag is required for ksh to exit
when a command fails while the errexit option is on.
src/cmd/ksh93/tests/basic.sh:
- Add a regression test based on the reproducer.
This reverts commit 2b9cbbbc8e.
This is not ready for prime time. Crashses when running a $PS2
discipline function. This needs fixing and more testing in
development before making it into the 1.0 branch. In the meantime,
that terrible problem with types is back, sorry about that.
This commit mitigates the effects of the hack explained in the
referenced commit so that dummy built-in command nodes added by the
parser for declaration/assignment purposes do not leak out into the
execution level, except in a relatively harmless corner case.
Something like
if false; then
typeset -T Foo_t=(integer -i bar)
fi
will no longer leave a broken dummy Foo_t declaration command. The
same applies to declaration commands created with enum.
The corner case remaining is:
$ ksh -c 'false && enum E_t=(a b c); E_t -a x=(b b a c)'
ksh: E_t: not found
Since the 'enum' command is not executed, this should have thrown
a syntax error on the 'E_t -a' declaration:
ksh: syntax error at line 1: `(' unexpected
This is because the -c script is parsed entirely before being
executed, so E_t is recognised as a declaration built-in at parse
time. However, the 'not found' error shows that it was successfully
eliminated at execution time, so the inconsistent state will no
longer persist.
This fix now allows another fix to be effective as well: since
built-ins do not know about virtual subshells, fork a virtual
subshell into a real subshell before adding any built-ins.
src/cmd/ksh93/sh/parse.c:
- Add a pair of functions, dcl_hactivate() and dcl_dehacktivate(),
that (de)activate an internal declaration built-ins tree into
which check_typedef() can pre-add dummy type declaration command
nodes. A viewpath from the main built-ins tree to this internal
tree is added, unifying the two for search purposes and causing
new nodes to be added to the internal tree. When parsing is done,
we close that viewpath. This hides those pre-added nodes at
execution time. Since the parser is sometimes called recursively
(e.g. for command substitutions), keep track of this and only
activate and deactivate at the first level.
- We also need to catch errors. This is done by setting libast's
error_info.exit variable to a dcl_exit() function that tidies up
and then passes control to the original (usually sh_exit()).
- sh_cmd(): This is the most central function in the parser. You'd
think it was sh_parse(), but $(modern)-form command substitutions
use sh_dolparen() instead. Both call sh_cmd(). So let's simply
add a dcl_hacktivate() call at the beginning and a
dcl_deactivate() call at the end.
- assign(): This function calls path_search(), which among many
other things executes an FATH search, which may execute arbitrary
code at parse time (!!!). So, regardless of recursion level,
forcibly dehacktivate() to avoid those ugly parser side effects
returning in that context.
src/cmd/ksh93/bltins/enum.c: b_enum():
- Fork a virtual subshell before adding a built-in.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Fork a virtual subshell when detecting typeset's -T option.
Improves fix to https://github.com/ksh93/ksh/issues/256
Symptom:
$ ksh -c 'command enum -i P_t=(a b); P_t -A v=([f]=b); typeset -p v'
ksh: syntax error at line 1: `(' unexpected
Expected: no syntax error, and output of 'P_t -A v=([f]=b)'.
src/cmd/ksh93/sh/parse.c: check_typedef():
- For enum, skip over any possible 'command' prefixes before
pre-parsing options with optget (or, technically, skip anything
else that might come before 'enum', though I don't think anything
else is possible).
- The sh_addbuiltin() call at the end to pre-add the builtin
obtained the node pointer to the built-in and the node flags from
the parser tree. This did not work if a 'command' prefix was
present. However, we don't actually need this. For parsing
purposes, the BLT_DCL flag for a declaration built-in is
sufficient; this is what gets the parser to accept
assignment-arguments including parentheses. So just apply that.
In addition, let's point it to an actual dummy built-in, 'true'
(SYSTRUE), so that if a user does run something like 'if false;
then enum Foo_t=(...); fi', the leaked Foo_t dummy at least won't
do anything (not even crash).
When giving an invalid or incompatible option to a typeset option
equivalent command (former default alias) such as 'compound' or
'integer', the resulting usage messages are incorrect. Example:
$ ksh -c 'compound -T foo=(typeset -a bar[1]=23)'
ksh: compound: -T cannot be used with other options
Usage: compound [-bflmnprstuxACHS] [-a[[type]]] [-i[base]] [-E[n]]
[-F[n]] [-L[n]] [-M[mapping]] [-R[n]] [-X[n]]
[-h string] [-T[tname]] [-Z[n]] [name[=value]...]
Or: compound -f [name...]
Or: compound -m [name=name...]
Or: compound -n [name=name...]
Or: compound -T [tname[=(type definition)]...]
Help: compound [ --help | --man ] 2>&1
The error message is wrong (there were no other options) and some
of the listed usages are invalid, like 'compound -f'.
Typeset option equivalent commands should just use 'typeset' in all
their error messages to avoid confusion. This is done by setting
error_info.id to the name of the typeset builtin.
There is quite a bit of no-op code in the job_hup() function due
to conditions that always test false. This commit removes that code
and clarifies the rest, making the purpose of this function clear.
job_hup() (before 62cf88d0: job_terminate()) is called via
job_walk() by sh_done() in fault.c to issue SIGHUP, the "hang up"
signal, to every background job's process group when the current
session is ungracefully disconnected. (One way to trigger such a
disconnection is to forcibly terminate a ssh session by typing '~.'
on a new prompt.)
The bug that Solaris patch 260-22964338 fixed is that ksh then
killed all non-disowned jobs' process groups without considering
that ksh still remembers a job even when all its processes are
finished (have the P_DONE flag). In that condition, the process
group ID may well be reused by another process by now, so it is
dangerous to killpg() it; we risk killing unrelated processes!
This is *not* a hypothetical problem; the Solaris patch exists
because this happened to a Solaris customer. However, the bug
exists on all operating systems. It's rarely triggered but serious,
and it's more likely to occur on heavy workloads that re-use
process/group IDs a lot. And it's on every currently released
non-Solaris version of ksh93. Eesh.
src/cmd/ksh93/sh/jobs.c:
src/cmd/ksh93/include/jobs.h:
- Remove job_terminate() which was unused as of 62cf88d0.
It could have been fixed instead of replaced. Oh well.
- Refactor job_hup():
- Remove code that will never be executed because, at
those points, it is known that pw->p_pgrp != 0.
- Simplify the loop that checks that there is at least
one non-P_DONE process so it doesn't need a flag.
For documentation purposes, below is a reproducer for the bug
before the Solaris patch. It is rather involved.
1. Compile the C program below (cpid).
2. In one terminal, 'ssh localhost'.
3. Within the ssh session:
- 'exec -a-ksh /path/to/buggy/ksh' to get a ksh login shell.
- 'sleep 1 &' and let it finish. Note down the reported PID.
That is the one we will reuse. Let's say 26650.
4. In another terminal, run: ./cpid 26650 (the PID from the
previous step). Now wait until it says "PID 26650 is ready"; it
has now succeeded at re-using that PID, and will just sit there.
This process will never voluntarily terminate. If we have the
bug, the termination of this process will be the symptom.
5. In the first terminal, forcibly terminate the ssh session by
typing, on a new prompt: ~. (tilde, dot). This triggers the
buggy routine to issue SIGHUP to all of ksh's background jobs.
6. In the second terminal, the bug is reproduced if cpid has been
terminated, reporting 'waitpid return 26650, status 0x0001', so
ksh just killed this process that it had nothing to do with.
(Note that status 0x0001 refers to being killed by signal 1
which is SIGHUP.)
cpid.c follows (written by George Lijo, tweaked by me):
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <sys/wait.h>
int main(int argc, char *argv[])
{
pid_t pid, rpid, opid;
int i, status, npid;
if (argc != 2)
{
fprintf(stderr, "Usage: cpid <PID to re-use>\n");
exit(1);
}
rpid = atoi(argv[1]);
opid = getpid();
for (;;)
{
if ((pid = fork()) == 0)
{
setpgrp();
pause();
_exit(0);
}
if (pid == rpid)
break;
kill(pid, SIGKILL);
waitpid(pid, NULL, 0);
if (opid < rpid && pid > rpid)
printf("Cannot create PID %d\n", rpid);
opid = pid;
}
printf("PID %d is ready\n", pid);
i = waitpid(pid, &status, 0);
printf("waitpid return %d, status 0x%4.4x\n", i, status);
return status;
}
Since the arithmetic recursion level only becomes incorrect when
an error interrupts the arithmetic subsystem, and all such error
messages call sh_exit(), it should be good enough to reset it
there, so we don't need to do that for nearly every sh_exec() run.
The -d flag implemented in the rm builtin is completely broken. No
matter what you do it refuses to remove directories, even if -r is
also passed. Reproducer:
$ mkdir /tmp/empty
$ PATH=/opt/ast/bin rm -d /tmp/empty
rm: /tmp/empty: directory
$ PATH=/opt/ast/bin rm -dr /tmp/empty
rm: /tmp/empty: directory not removed [Is a directory]
Additionally, the description of 'rm -d' in the man page contradicts
how it's specified in <https://www.austingroupbugs.net/view.php?id=802>.
The ksh93v- rm builtin fixed nearly all of these issues, so I've
backported it to 93u+m and applied one additional fix for 'rm -rd'.
src/lib/libcmd/rm.c:
- Backported the fixes from the ksh93v- rm builtin's -d flag when
used on empty directories.
- Backported the man page update for rm(1) from ksh93v-.
- The ksh93v- rm builtin had one additional bug that caused the -r
option to fail when combined with -d. This was fixed by
overriding -d if -r is also passed.
src/cmd/ksh93/tests/builtins.sh:
- Add regression tests for the rm builtin's -d option.
This function adds the NV_ADD flag to its 'flags' variable for
nv_serach() calls subject to some checks. However, every call that
uses that variable explicitly turns off the NV_ADD bit again.
A search in the ast-open-history repo reveals that this check
briefly made a difference between versions 2010-06-25 and
2010-08-11, but it's been a complete no-op ever since.
src/cmd/ksh93/sh/arith.c: scope():
- Remove no-op code.
- Resolve the constant expressions involving the 'flags' variable,
get rid of the variable, and just indicate the flag bitmasks
directly in the nv_search() calls.
- Detangle and split up the excessively long 'if' construct.
No change in behaviour.
Previously noticed by Kurtis Rader for ksh2020:
https://github.com/att/ast/commit/d5ce3b05
POSIXly, '.' loads only files, not functions.
This only applies to '.', not 'source' (which is not in POSIX).
src/cmd/ksh93/bltins/misc.c: b_source():
- For ksh function lookup, add an additional check that we're not
in POSIX mode and running the '.' (SYSDOT) builtin.
Defining a .sh.tilde.get or .sh.tilde.set discipline function to
extend tilde expansion works well as long as the discipline
function doesn't get interrupted (e.g. with Crtl+C) or produce an
error message. Either of those will cause the shell to become
unstable and crash.
This feature is now removed from the 1.0 branch as it is not ready
for prime time. It can return to a release branch if/when we manage
to fix it on the master branch.
Related: https://github.com/ksh93/ksh/issues/346
In 93v-/ksh2020, namespace defs in any function are a syntax error.
This commit blocks namespace defs for ksh functions only, at the
execution level. This follows some of AT&T original intention while
working around some of the known bugs with namespaces.
Related: https://github.com/ksh93/ksh/issues/325
As of the previous commit, I finally know how to properly check for
a variable of a type created by 'enum'. We need to check for both
the NV_UINT16 attribute and the ENUM_disc discipline.
Also:
- regression test tweaks
- add missing tests for previous commit (f600a5ea)
Within arithmetic expressions, enumeration values of variables of a
type created with the 'enum' command translate to index numbers
from 0 to the number of elements minus 1. However, there was no
range checking on this in the arithmetic subsystem, allowing the
assignment of out-of-range values that did not correspond to any
enumeration value.
Variables of an enum type are internally unsigned short integers
(NV_UINT16), like those created with 'integer -su', except with an
additional discipline function (ENUM_disc).
src/cmd/ksh93/bltins/enum.c,
src/cmd/ksh93/include/builtins.h:
- To implement range checking, the arithmetic system needs access
to the 'nelem' (number of elements) member of 'struct Enum'. This
is only defined locally in enum.c. We could move that to name.h
so arith.c can access it, but enum.c has code that supports
compiling as standalone. So, instead, define a quick extern
function, b_enum_elem(), that does the necessary type conversion
and returns a type's number of elements.
- Add --man documentation for the arithmetic subsystem behaviour
for enum types. Tell the enuminfo() function, which dynamically
inserts values into the documentation, how to process new \f tags
'lastv' (the last-defined value) and 'lastn' (the number of the
last element).
src/cmd/ksh93/sh/arith.c: arith():
- For NV_UINT16 variables with an ENUM_disc discipline, check the
range using b_enum_elem() and error out if necessary.
Resolves: https://github.com/ksh93/ksh/issues/335
This commit adds support for 'stty size' to the stty builtin, as
defined in <https://austingroupbugs.net/view.php?id=1053>. The size
mode is used to display the terminal's number of rows and columns.
Note that stty isn't included in the default list of builtin
commands; testing this addition requires adding CMDLIST(stty) to
the table of builtins in src/cmd/ksh93/data/builtins.c.
src/lib/libcmd/stty.c:
- Add support for the size mode to the stty builtin. This mode is
only used to display the terminal's number of rows and columns,
so error out if any arguments are given that attempt to set the
terminal size.
For some reason, Void Linux (with musl libc) sets SIGCONT to
ignored on the Linux console, causing the 'sleep -s' test in
builtins.sh to fail spuriously as it relies on SIGCONT to work.
src/cmd/ksh93/tests/shtests:
- Reset SIGCONT using the unadvertised 'trap + SIGCONT' feature.
Resolves: https://github.com/ksh93/ksh/issues/301
As I got to know the code better, it now seems painfully obvious
that getting test/[ to issue an exit status >= 2 on error only
requires a simple check in sh_exit() in fault.c, which is called
whenever the shell issues an error message.
Parser limitations prevent shcomp or source from handling enum
types correctly:
$ cat /tmp/colors.sh
enum Color_t=(red green blue orange yellow)
Color_t -A Colors=([foo]=red)
$ shcomp /tmp/colors.sh > /dev/null
/tmp/colors.sh: syntax error at line 2: `(' unexpected
$ source /tmp/colors.sh
/bin/ksh: source: syntax error: `(' unexpected
Yet, for types created using 'typeset -T', this works. This is done
via a check_typedef() function that preliminarily adds the special
declaration builtin at parse time, with details to be filled in
later at execution time.
This hack will produce ugly undefined behaviour if the definition
command creating that type built-in is then not actually run at
execution time before the type built-in is accessed.
But the hack is necessary because we're dealing with a fundamental
design flaw in the ksh language. Dynamically addable built-ins that
change the syntactic parsing of the shell language on the fly are
an absurdity that violates the separation between parsing and
execution, which muddies the waters and creates the need for some
kind of ugly hack to keep things like shcomp more or less working.
This commit extends that hack to support enum.
src/cmd/ksh93/sh/parse.c:
- check_typedef():
- Add 'intypeset' parameter that should be set to 1 for typeset
and friends, 2 for enum.
- When processing enum arguments, use AST getopt(3) to skip over
enum's options to find the name of the type to be defined.
(getopt failed if we were running a -c script; deal with this
by zeroing opt_info.index first.)
- item(): Update check_typedef() call, passing lexp->intypeset.
- simple(): Set lexp->intypeset to 2 when processing enum.
The rest of the changes are all to support the above and should be
fairly obvious, except:
src/cmd/ksh93/bltins/enum.c:
- enuminfo(): Return on null pointer, avoiding a crash upon
executing 'Type_t --man' if Type_t has not been fully defined due
to the definition being pre-added at parse time but not executed.
It's all still wrong, but a crash is worse.
Resolves: https://github.com/ksh93/ksh/issues/256
Listing types with 'typeset -T' will list not only types created with
typeset, but also types created with enum. However, the types created
by enum are not displayed correctly in the resulting output:
$ enum Foo_t=(foo bar)
$ typeset -T
typeset -T Foo_t
typeset -T Foo_t=fo)
The fix for this bug was backported from ksh93v- 2013-10-08.
src/cmd/ksh93/sh/nvtype.c:
- sh_outtype(): Skip over enums when listing types with 'typeset -T'.
The referenced commit did not fix the symptoms on the 1.0 branch
(no vmalloc) on the GitHub CI runners.
The failures are intermittent and are not reproduced with vmalloc
or on other operating systems.
Though the failures occur on a different test each time, the total
amount of "leaked" bytes is always 36864, e.g.:
leaks.sh[388]: run command with preceding PATH assignment in
main shell (leaked approx 36864 bytes after 4096 iterations)
36864/4096 equals exactly 9. An odd number, literally and
figuratively, but I suppose that's the tolerance Linux needs.
src/cmd/ksh93/tests/leaks.sh
- Increase tolerance of bytes per iteration from 8 to 9.
This commit fixes an issue I found in the subshell $RANDOM
reseeding code.
The main issue is a performance regression in the shbench fibonacci
benchmark, introduced in commit af6a32d1. Performance dropped in
this benchmark because $RANDOM is always reseeded and restored,
even when it's never used in a subshell. Performance results from
before and after this performance fix (results are on Linux with
CC=gcc and CCFLAGS='-O2 -D_std_malloc'):
$ ./shbench -b bench/fibonacci.ksh -l 100 ./ksh-0f06a2e ./ksh-af6a32d ./ksh-f31e368 ./ksh-randfix
benchmarking ./ksh-0f06a2e, ./ksh-af6a32d, ./ksh-f31e368, ./ksh-randfix ...
*** fibonacci.ksh ***
# ./ksh-0f06a2e # Recent version of ksh93u+m
# ./ksh-af6a32d # Commit that introduced the regression
# ./ksh-f31e368 # Commit without the regression
# ./ksh-randfix # Ksh93u+m with this patch applied
-------------------------------------------------------------------------------------------------
name ./ksh-0f06a2e ./ksh-af6a32d ./ksh-f31e368 ./ksh-randfix
-------------------------------------------------------------------------------------------------
fibonacci.ksh 0.481 [0.459-0.515] 0.472 [0.455-0.504] 0.396 [0.380-0.442] 0.407 [0.385-0.439]
-------------------------------------------------------------------------------------------------
src/cmd/ksh93/include/variables.h,
src/cmd/ksh93/sh/{init,subshell}.c:
- Rather than reseed $RANDOM every time a subshell is created, add
a sh_save_rand_seed() function that does this only when the
$RANDOM variable is used in a subshell. This function is called
by the $RANDOM discipline functions nget_rand() and put_rand().
As a minor optimization, sh_save_rand_seed doesn't reseed if it's
called from put_rand().
- Because $RANDOM may have a seed of zero (i.e., RANDOM=0),
sp->rand_seed isn't enough to tell if $RANDOM has been reseeded.
Add sp->rand_state for this purpose.
- sh_subshell(): Only restore the former $RANDOM seed and state if
it is necessary to prevent a subshell leak.
src/cmd/ksh93/tests/variables.sh:
- Add two regression tests for bugs I ran into while making this
patch.
The ksh manual page is one of the few places that calls globbing
"file name generation". The mksh and zsh manuals use the same term.
But every other shell's manual calls it "pathname expansion": bash,
dash, yash, FreeBSD sh. So does ksh's built-in documentation (alias
--man, export --man, readonly --man, set --man, typeset --man).
What's more, the authoritative ksh reference, Bolsky & Korn's 1995
"The New Kornshell" book, also calls it "pathname expansion", and
so does the POSIX standard.
Similarly, "arithmetic substitution" should be called "arithmetic
expansion" per Bolsky & Korn as well as POSIX.
This commit has several other miscellaneous documentation tweaks as
well.
'printf' on bash and zsh has a popular -v option that allows
assigning formatted output directly to variables without using a
command substitution. This is much faster and avoids snags with
stripping final linefeeds. AT&T had replicated this feature in the
abandoned 93v- beta version. This backports it with a few tweaks
and one user-visible improvement.
The 93v- version prohibited specifying a variable name with an
array subscript, such as printf -v var\[3\] foo. This works fine on
bash and zsh, so I see no reason why this should not work on ksh,
as nv_putval() deals with array subscripts just fine.
src/cmd/ksh93/bltins/print.c: b_print():
- While processing the -v option when called as printf, get a
pointer to the variable, creating it if necessary. Pass only the
NV_VARNAME flag to enforce a valid variable name, and not (as
93v- does) the NV_NOARRAY flag to prohibit array subscripts.
- If a variable was given, set the output file to an internal
string buffer and jump straight to processing the format.
- After processing the format, assign the contents to the string
buffer to the variable.
src/cmd/ksh93/data/builtins.c:
- Document the new option, adding a warning that unquoted square
brackets may trigger pathname expansion.
%(pattern)q is equivalent to %P. It's also equivalent to %#P, but
since the alternative format specifier '#' does nothing for %P,
%P and %#P are the same and documenting #%P is just confusing.
Thanks to @stephane-chazelas for the report.
src/cmd/ksh93/bltins/print.c:
- In the printmap struct, document %P as equivalent of %(pattern)q.
- Sort it alphabetically.
- Do not pointlessly repeat the string "Equivalent to". Instead,
let the discipline function infof() insert it for each entry.
(This is the function used to dynamically insert the equivalents
documentation into the --man output at the \fextra\f tag in
sh_optprintf[] in data/builtins.c.)
Resolves: https://github.com/ksh93/ksh/issues/338
An obsolete struct was left that passed some variables on between
b_exec() and the deleted B_login(). We can simply make those local
variables now. Let's get rid of the redundant sh pointer, too.
As the (original AT&T) comment at the top says, "the trickiest part
of the tests is avoiding typeahead in the pty dialogue".
Two tests failed to [p]eek at the prompt before they started
'typing'. This causes unpredictable results. On Debian Bullseye
this triggers typeahead, which produces unwanted echo to the
terminal, killing the tests.
src/cmd/ksh93/tests/pty.sh:
- Add missing 'p' commands for the first prompt to the tests
'nobackslashctrl in emacs' and 'emacs backslash escaping'.
Resolves: https://github.com/ksh93/ksh/issues/332
Tried to compile on Solaris 10.1 for the first time in a while.
Turns out the obsolete Bourne /bin/sh does not support 'test -e'.
bin/package, src/cmd/INIT/package.sh:
- Use 'test -f' instead.
In C/POSIX arithmetic, a leading 0 denotes an octal number, e.g.
010 == 8. But this is not a desirable feature as it can cause
problems with processing things like dates with a leading zero.
In ksh, you should use 8#10 instead ("10" with base 8).
It would be tolerable if ksh at least implemented it consistently.
But AT&T made an incredible mess of it. For anyone who is not
intimately familiar with ksh internals, it is inscrutable where
arithmetic evaluation special-cases a leading 0 and where it
doesn't. Here are just some of the surprises/inconsistencies:
1. The AT&T maintainers tried to honour a leading 0 inside of
((...)) and $((...)) and not for arithmetic contexts outside it,
but even that inconsistency was never quite consistent.
2. Since 2010-12-12, $((x)) and $(($x)) are different:
$ /bin/ksh -c 'x=010; echo $((x)) $(($x))'
10 8
That's a clear violation of both POSIX and the principle of
least astonishment. $((x)) and $(($x)) should be the same in
all cases.
3. 'let' with '-o letoctal' acts in this bizarre way:
$ set -o letoctal; x=010; let "y1=$x" "y2=010"; echo $y1 $y2
10 8
That's right, 'let y=$x' is different from 'let y=010' even
when $x contains the same string value '010'! This violates
established shell grammar on the most basic level.
This commit introduces consistency. By default, ksh now acts like
mksh and zsh: the octal leading zero is disabled in all arithmetic
contexts equally. In POSIX mode, it is enabled equally.
The one exception is the 'let' built-in, where this can still be
controlled independently with the letoctal option as before (but,
because letoctal is synched with posix when switching that on/off,
it's consistent by default).
We're also removing the hackery that causes variable expansions for
the 'let' builtin to be quietly altered, so that 'x=010; let y=$x'
now does the same as 'let y=010' even with letoctal on.
Various files:
- Get rid of now-redundant sh.inarith (shp->inarith) flag, as we're
no longer distinguishing between being inside or outside ((...)).
src/cmd/ksh93/sh/arith.c:
- arith(): Let disabling POSIX octal constants by skipping leading
zeros depend on either the letoctal option being off (if we're
running the "let" built-in") or the posix option being off.
- sh_strnum(): Preset a base of 10 for strtonll(3) depending on the
posix or letoctal option being off, not on the sh.inarith flag.
src/cmd/ksh93/include/argnod.h,
src/cmd/ksh93/sh/args.c,
src/cmd/ksh93/sh/macro.c:
- Remove astonishing hackery that violated shell grammar for 'let'.
src/cmd/ksh93/sh/name.c (nv_getnum()),
src/cmd/ksh93/sh/nvdisc.c (nv_getn()):
- Remove loops for skipping leading zeroes that included a broken
check for justify/zerofill attributes, thereby fixing this bug:
$ typeset -Z x=0x15; echo $((x))
-ksh: x15: parameter not set
Even if this code wasn't redundant before, it is now: sh_arith()
is called immediately after the removed code and it ignores
leading zeroes via sh_strnum() and strtonll(3).
Resolves: https://github.com/ksh93/ksh/issues/334
When starting a new interactive ksh with the -v or -x option, an
annoying symptom occurs: the 'tput' command that ed_setup() issues
to get the escape sequence for cursor-up is xtraced or echoed,
corrupting prompt display, for example ('▂' is the cursor):
$ ksh -x
$ + /usr/bin/tput cuu1
+ 2> /dev/null
+ .sh.subscript=$'\E[A'
▂
or
$ ksh -v
$ .sh.subscript=$(/usr/bin/tput cuu1 2>/dev/null)▂
src/cmd/ksh93/edit/edit.c: ed_setup():
- Turn off xtrace and verbose while sh_trap()ing tput.
So, shcomp has messed up my terminal once too often by writing
compiled binary data to it. While fixing that I've done some other
tweaks as well.
src/cmd/ksh93/sh/shcomp.c: main():
- Fix error/warning message id (the "name:" prefix before messages)
so it makes sense to the user. Save shcomp's argv[0] id for error
messages that are directly from shcomp's main(), and use the
argv[1] script id (set by sh_init()) for warnings produced by the
compilation process. If there is no script id because we're
reading from stdin, set it to "(stdin)".
- If no arguments are given, refuse to read from standard input if
it's on a tty. Instead, write a brief usage message (with pointer
to --help and --man, see e21a053e) and exit. This is far more
helpful; people will rarely want to compile a script by manually
typing it in. If you really want to do that, use /dev/stdin as
the input filename. :)
- Error out if we're about to write binary data to a tty (even if
/dev/stdout was given as the output filename).
- Turn off SH_MULTILINE to avoid some pointless editor init in case
we're reading from stdin on a terminal.
- Do not attempt to copy remaining data if we're already at EOF.
This fixes a bug that required the user to press Ctrl+D twice
when manually entering a script on the terminal. Pressing Ctrl+D
once and then entering more data would corrupt the bytecode.
This commit fixes two file descriptor leaks in the hist built-in.
The bugfix for the first file descriptor leak was backported from
ksh2020. See:
https://github.com/att/ast/issues/872https://github.com/att/ast/commit/73bd61b5
Reproducer:
$ echo no
$ hist -s no=yes
The second file descriptor leak occurs after a substitution error
in the hist built-in (this leak wasn't fixed in ksh2020).
Reproducer:
$ echo no
$ ls /proc/$$/fd
$ hist -s no=yes
$ hist -s no=yes
$ ls /proc/$$/fd
src/cmd/ksh93/bltins/hist.c:
- Close leftover file descriptors when an error occurs and after
'hist -s' runs a command.
src/cmd/ksh93/tests/builtins.sh:
- Add two regression tests for both of the file descriptor leaks.
When testing whether subshell $RANDOM reseeding worked, checking
for non-identical numbers is not sufficient. There is no check for
randomly occurring duplicate numbers, nor can there be, because
subshells cannot (or, in the case of virtual subshells, should not)
influence each other or the parent shell.
src/cmd/ksh93/tests/variables.sh:
- Try up to three times, tolerating identical numbers twice.
The --posix compliance option now disables the case-insensitive
special floating point constants Inf and NaN so that all case
variants of $((inf)) and $((nan)) refer to the variables by those
names as the standard requires. (BUG_ARITHNAN)
src/cmd/ksh93/sh/arith.c: arith():
- Only do case-insensitive checks for "Inf" and "NaN" if the POSIX
option is off.
e_badnum from streval.h and e_number from shell.h are both defined
as "%s: bad number". We only need one. Remove the one that is used
only once: e_badnum.
Turns out there is a way to check what built-in we're running at
any time. It is done for 'let' in arith.c:
sh.bltindata.bnode==SYSLET
For test/[, that would be (see include/builtins.h):
sh.bltindata.bnode==SYSTEST || sh.bltindata.bnode==SYSBRACKET
ksh crashed after unsetting .sh.match and then matching a pattern:
$ unset .sh.match
$ [[ bar == ba* ]]
Memory fault
src/cmd/ksh93/sh/init.c: sh_setmatch():
- Do nothing if we cannot get an array pointer to SH_MATCHNOD.
There are one or two leaks that show up intermittently on the
Github runners for the 1.0 branch (which is compiled as a release,
i.e. no vmalloc). If they're intermittent, they must be false
positives due to malloc artefacts. Let's double the number of
iterations for the /proc/$$/stat method and see what happens.
Symptoms:
$ test \( string1 -a string2 \)
/usr/local/bin/ksh: test: argument expected
$ test \( string1 -o string2 \)
/usr/local/bin/ksh: test: argument expected
The parentheses should be irrelevant and this should be a test for
the non-emptiness of string1 and/or string2.
src/cmd/ksh93/bltins/test.c:
- b_test(): There is a block where the case of 'test' with five or
less arguments, the first and last one being parentheses, is
special-cased. The parentheses are removed as a workaround: argv
is increased to skip the opening parenthesis and argc is
decreased by 2. However, there is no corresponding increase of
tdata.av which is a copy of this function's argv. This renders
the workaround ineffective. The fix is to add that increase.
- e3(): Do not handle '!' as a negator if not followed by an
argument. This allows a right-hand expression that is equal to
'!' (i.e. a test for the non-emptiness of the string '!').
In ksh88, the test/[ built-in supported both the '<' and '>'
lexical sorting comparison operators, same as in [[. However, in
every version of ksh93, '<' does not work though '>' still does!
Still, the code for both is present in test_binop():
src/cmd/ksh93/bltins/test.c
548: case TEST_SGT:
549: return(strcoll(left, right)>0);
550: case TEST_SLT:
551: return(strcoll(left, right)<0);
Analysis: The binary operators are looked up in shtab_testops[] in
data/testops.c using a macro called sh_lookup, which expands to a
sh_locate() call. If we examine that function in sh/string.c, it's
easy to see that on systems using ASCII (i.e. all except IBM
mainframes), it assumes the table is sorted in ASCII order.
src/cmd/ksh93/sh/string.c
64: while((c= *tp->sh_name) && (CC_NATIVE!=CC_ASCII || c <= first))
The problem was that the '<' operator was not correctly sorted in
shtab_testops[]; it was sorted immediately before '>', but after
'='. The ASCII order is: < (60), = (61), > (62). This caused '<' to
never be found in the table.
The test_binop() function is also used by [[, yet '<' always worked
in that. This is because the parser has code that directly checks
for '<' and '>' within [[ (in sh/parse.c, lines 1949-1952).
This commit also adds '=~' to 'test', which took three lines of
code and allowed eliminating error handling in test_binop() as
test/[ and [[ now support the same binary ops. (re: fc2d5a60)
src/cmd/ksh93/*/*.[ch]:
- Rename a couple of very misleadingly named macros in test.h:
. For == and !=, the TEST_PATTERN bit is off for pattern compares
and on for literal string compares! Rename to TEST_STRCMP.
. The TEST_BINOP bit does not denote all binary operators, but
only the logical -a/-o ops in test/[. Rename to TEST_ANDOR.
src/cmd/ksh93/bltins/test.c: test_binop():
- Add support for =~. This is only used by test/[. The method is
implemented in two lines that convert the ERE to a shell pattern
by prefixing it with ~(E), then call test_strmatch with that
temporary string to match the ERE and update ${.sh.match}.
- Since all binary ops from shtab_testops[] are now accounted for,
remove unknown op error handling from this function.
src/cmd/ksh93/data/testops.c:
- shtab_testops[]:
. Correctly sort the '<' (TEST_SLT) entry.
. Remove ']]' (TEST_END). It's not an op and doesn't belong here.
- Update sh_opttest[] documentation with =~, \<, \>.
- Remove now-unused e_unsupported_op[] error message.
src/cmd/ksh93/sh/lex.c: sh_lex():
- Check for ']]' directly instead of relying on the removed
TEST_END entry from shtab_testops[].
src/cmd/ksh93/tests/bracket.sh:
- Add relevant tests.
src/cmd/ksh93/tests/builtins.sh:
- Fix an old test that globally deleted the 'test' builtin. Delete
it within the command substitution subshell only.
- Remove the test for non-support of =~ in test/[.
- Update the test for invalid test/[ op to use test directly.
POSIX requires
test "$a" -a "$b"
to return true if both $a and $b are non-empty, and
test "$a" -o "$b"
to return true if either $a or $b is non-empty.
In ksh, this fails if "$a" is '!' or '(' as this causes ksh to
interpret the -a and -o as unary operators (-a being a file
existence test like -e, and -o being a shell option test).
$ test ! -a ""; echo "$?"
0 (expected: 1/false)
$ set -o trackall; test ! -o trackall; echo "$?"
1 (expected: 0/true)
$ test \( -a \); echo "$?"
ksh: test: argument expected
2 (expected: 0/true)
$ test \( -o \)
ksh: test: argument expected
2 (expected: 0/true)
Unfortunately this problem cannot be fixed without risking breakage
in legacy scripts. For instance, a script may well use
test ! -a filename
to check that a filename is nonexistent. POSIX specifies that this
always return true as it is a test for the non-emptiness of both
strings '!' and 'filename'.
So this commit fixes it for POSIX mode only.
src/cmd/ksh93/bltins/test.c: e3():
- If the posix option is active, specially handle the case of
having at least three arguments with the second being -a or -o,
overriding their handling as unary operators.
src/cmd/ksh93/data/testops.c:
- Update 'test --man --' date and say that unary -a is deprecated.
src/cmd/ksh93/sh.1:
- Document the fix under the -o posix option.
- For test/[, explain that binary -a/-o are deprecated.
src/cmd/ksh93/tests/bracket.sh:
- Add tests based on reproducers in bug report.
Resolves: https://github.com/ksh93/ksh/issues/330
On Linux, the 'su' program sets $0 to '-su' when doing 'su -' or
'su - username'. When ksh is the target account's default shell,
this caused ksh to consider itself to be launched as a standard
POSIX sh, which (among other things) disables the default aliases
on interactive shells. This caused confusion for at least one user
as they lost their 'history' alias after 'su -':
https://www.linuxquestions.org/questions/slackware-14/in-current-with-downgrade-to-ksh93-lost-the-alias-history-4175703408/
bash does not consider itself to be sh when invoked as su, so ksh
probably shouldn't, either. The behaviour was also undocumented,
making it even more surprising.
src/cmd/ksh93/sh/init.c: sh_type():
- Only set the SH_TYPE_POSIX bit if we're invoked as 'sh' (or, on
windows, as 'sh.exe').
A change in FreeBSD 13 now causes extremely long command names to
exit with errno set to E2BIG if the name can't fit in the list of
arguments. This was causing the regression tests for ENAMETOOLONG
to fail on FreeBSD 13 because the exit status for these errors
differ (ENAMETOOLONG uses status 127 while E2BIG uses status 126).
src/cmd/ksh93/tests/path.sh:
- To fix the failing regression tests, the command name has been
shortened to twice the length of NAME_MAX. This length is still
long enough to trigger an ENAMETOOLONG error without causing an
E2BIG failure on FreeBSD 13.
Fixes https://github.com/ksh93/ksh/issues/331
The sh_trace() function, which prints an xtrace line to standard
error, clears the SF_SHARE and SF_PUBLIC flags from the sfstderr
stream during the xtrace in order to guarantee an atomic trace
write. But it only restored those flags if the passed argv pointer
is non-NULL. Redirections are traced with a NULL argv parameter, so
the stderr state was not restored for them.
This somehow caused unpredictable behaviour, including (on some
systems) a crash in sfwrite(3) when running the heredoc.sh tests
with xtrace on.
src/cmd/ksh93/sh/xec.c: sh_xtrace():
- Move the sfset() invocation that restores the SF_SHARE|SF_PUBLIC
flags to sfstderr out of the if(argv) block.
- Since we're here, don't bother wasting cycles initialising local
variable values if xtrace is not on. Move that inside the
if(sh_isoption(SH_XTRACE)) block.
Resolves: https://github.com/ksh93/ksh/issues/306
This was:
/*
* -lcmd specific workaround to handle
* fts_namelen
* fts_pathlen
* fts_level
* changing from [unsigned] short bit to [s]size_t
*
* ksh (or any other main application) that pulls in -lcmd
* at runtime may result in old -last running with new -lcmd
* which is not a good situation (tm)
*
* probably safe to drop after 20150101
*/
According to the version check in fts_fix.c, this change occurred
in the libast API version 2010-01-02, which is also the API version
of the bundled libast (see src/lib/libast/misc/state.c).
src/lib/libcmd/fts_fix.{c,h}:
- Removed.
src/lib/libcmd/{chgrp,chmod,cksum,cp,rm}.c:
- Change uses of fts_fix.h to fts.h from libast.
src/lib/libcmd/Mamfile:
- Update accordingly.
Stéphane Chazelas reported:
> As noted in this austin-group-l discussion[*] (relevant to this
> issue):
>
> $ ksh93u+m -c 'pwd; echo "$?" >&2; echo test; echo "$?" >&2' >&-
> 0
> 1
> /home/chazelas
>
> when stdout is closed, pwd does claim it succeeds (by returning a
> 0 exit status), while echo doesn't (not really relevant to the
> problem here, only to show it doesn't affect all builtins), and
> the output that pwd failed to write earlier ends up being written
> on stderr here instead of stdout upon exit (presumably) because
> of that >&2 redirection.
>
> strace shows ksh93 attempting write(1, "/home/chazelas\n", 15) 6
> times (1, the last one, successful).
>
> It gets even weirder when redirecting to a file:
>
> $ ksh93u+m -c 'pwd; echo "$?" >&2; echo test; echo "$?" > file' >&-
> 0
> $ cat file
> 1
> 1
> ome/chazelas
In my testing, the problem does not occur when closing stdout at
the start of the -c script itself (using redirect >&- or exec >&-);
it only occurs if stdout was closed before initialising the shell.
That made me suspect that the problem had to do with an
inconsistent file descriptor state in the shell. ksh uses internal
sh_open() and sh_close() functions, among others, to maintain that
state.
src/cmd/ksh93/sh/main.c: sh_main():
- If the shell is initialised with stdin, stdout or stderr closed,
then make the shell's file descriptor state tables reflect that
fact by calling sh_close() for the closed file descriptors.
This commit also improves the BUG_PUTIOERR fix from 93e15a30. Error
checking after sfsync() is not sufficient. For instance, on
FreeBSD, the following did not produce a non-zero exit status:
ksh -c 'echo hi' >/dev/full
even though this did:
ksh -c 'echo hi >/dev/full'
Reliable error checking requires not only checking the result of
every SFIO command that writes output, but also synching the buffer
at the end of the operation and checking the result of that.
src/cmd/ksh93/bltins/print.c:
- Make exitval variable global to allow functions called by
b_print() to set a nonzero exit status.
- Check the result of all SFIO output commands that write output.
- b_print(): Always sfsync() at the end, except if the s (history)
flag was given. This allows getting rid of the sfsync() call that
required the workaround introduced in 846ad932.
[*] https://www.mail-archive.com/austin-group-l@opengroup.org/msg08056.html
Resolves: https://github.com/ksh93/ksh/issues/314
The documentation for the supported unit suffixes for options
accepting numeric arguments was woefully outdated in 'head --man'
and 'tail --man'.
A quick look at the very short head(1) code shows that it does not
know or care about unit suffixes at all – it leaves that to libast
optget(3) which in turn calls strtoll() which is implemented in
strtoi.h where the multiplier suffixes are handled.
Note that on GNU head/tail, single-letter suffixes use power-of-2
units, e.g. k == KiB, etc. Libast used to do the same but this is
not standards compliant and AT&T changed/fixed this in 2011. From
libast/RELEASE:
10-04-11 string/strtoi.h: k (1000) and ki (1024) now differentiated
(They didn't mention the same change applies to all handled units.)
Note that the tail(1) builtin is currently not compiled in by
default. This can be changed in src/cmd/ksh93/data/builtins.c.
src/lib/libcmd/head.c, src/lib/libcmd/tail.c:
- Update the internal head/tail man pages to reflect what is
handled in strtoi.h.
Resolves: https://github.com/ksh93/ksh/issues/319
The functions of the three flags controlling job control are
crucial to understand in order to maintain the code, so they should
be documented in the comments and not just in the git log.
This commit does not change any code.
Bug 1: POSIX requires numbers used as arguments for all the %d,
%u... in printf to be interpreted as in the C language, so
printf '%d\n' 010
should output 8 when the posix option is on. However, it outputs 10.
This bug was introduced as a side effect of a change introduced in
the 2012-02-07 version of ksh 93u+m, which caused the recognition
of leading-zero numbers as octal in arithmetic expressions to be
disabled outside ((...)) and $((...)). However, POSIX requires
leading-zero octal numbers to be recognised for printf, too.
The change in question introduced a sh.arith flag that is set while
we're processing a POSIX arithmetic expression, i.e., one that
recognises leading-zero octal numbers.
Bug 2: Said flag is not reset in a command substitution used within
an arithmetic expression. A command substitution should be a
completely new context, so the following should both output 10:
$ ksh -c 'integer x; x=010; echo $x'
10 # ok; it's outside ((…)) so octals are not recognised
$ ksh -c 'echo $(( $(integer x; x=010; echo $x) ))'
8 # bad; $(comsub) should create new non-((…)) context
src/cmd/ksh93/bltins/print.c: extend():
- For the u, d, i, o, x, and X conversion modifiers, set the POSIX
arithmetic context flag before calling sh_strnum() to convert the
argument. This fixes bug 1.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- When invoking a command substitution, save and unset the POSIX
arithmetic context flag. Restore it at the end. This fixes bug 2.
Reported-by: @stephane-chazelas
Resolves: https://github.com/ksh93/ksh/issues/326
When invoking a script without an interpreter (#!hashbang) path,
ksh forks, but there is no exec syscall in the child. The existing
command line is overwritten in fixargs() with the name of the new
script and associated arguments. In the generic/fallback version of
fixargs() which is used on Linux and macOS, if the new command line
is longer than the existing one, it is truncated. This works well
when calling a script with a shorter name.
However, it generates a misleading name in the common scenario
where a script is invoked from an interactive shell, which
typically has a short command line. For instance, if "/tmp/script"
is invoked, "ksh" gets replaced with "/tm" in "ps" output.
A solution is found in the fact that, on these systems, the
environment is stored immediately after the command line arguments.
This space can be made available for use by a longer command line
by moving the environment strings out of the way.
src/cmd/ksh93/sh/main.c: fixargs():
- Refactor BSD setproctitle(3) version to be more self-contained.
- In the generic (Linux/macOS) version, on init (i.e. mode==0), if
the command line is smaller than 128 bytes and the environment
strings have not yet been moved (i.e. if they still immediately
follow the command line arguments in memory), then strdup the
environment strings, pointing the *environment[] members to the
new strings and adding the length of the strings to the maximum
command line buffer size.
Reported-by: @gkamat
Resolves: https://github.com/ksh93/ksh/pull/300
ksh93 currently has three command substitution mechanisms:
- type 1: old-style backtick comsubs that use a pipe;
- type 3: $(modern) comsubs that use a temp file, currently with
fallback to a pipe if a temp file cannot be created;
- type 2: ${ shared-state; } comsubs; same as type 3, but shares
state with parent environment.
Type 1 is buggy. There are at least two reproducers that make it
hang. The Red Hat patch applied in 4ce486a7 fixed a hang in
backtick comsubs but reintroduced another hang that was fixed in
ksh 93v-. So far, no one has succeeded in making pipe-based comsubs
work properly.
But, modern (type 3) comsubs use temp files. How does it make any
sense to have two different command substitution mechanisms at the
execution level? The specified functionality between backtick and
modern command substitutions is exactly the same; the difference
*should* be purely syntactic.
So this commit removes the type 1 comsub code at the execution
level, treating them all like type 3 (or 2). As a result, the
related bugs vanish while the regression tests all pass.
The only side effect that I can find is that the behaviour of bug
https://github.com/ksh93/ksh/issues/124 changes for backtick
comsubs. But it's broken either way, so that's neutral.
So this commit can now be added to my growing list of ksh93 issues
fixed by simply removing code.
src/cmd/ksh93/sh/xec.c:
- Remove special code for type 1 comsubs from iousepipe(),
sh_iounpipe(), sh_exec() and _sh_fork().
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/subshell.c:
- Remove pipe support from sh_subtmpfile(). This also removes the
use of a pipe as a fallback for $(modern) comsubs. Instead, panic
and error out if temp file creation fails. If the shell cannot
create a temporary file, there are fatal system problems anyway
and a script should not continue.
- No longer pass comsub type to sh_subtmpfile().
All other changes:
- Update sh_subtmpfile() calls.
src/cmd/ksh93/tests/subshell.sh:
- Add two regression tests based on reproducers from bug reports.
Resolves: https://github.com/ksh93/ksh/issues/305
Resolves: https://github.com/ksh93/ksh/issues/316
This upstreams the patch 'src__cmd__ksh93__sh__array.c.diff' from
Apple's ksh 93u+ distribution in ksh-28.tar.gz:
https://opensource.apple.com/tarballs/ksh/
src/cmd/ksh93/sh/array.c: array_putval(), nv_associative():
- Zero two table pointers after closing/freeing the tables with
libast's dtclose(). No information is available from Apple as to
what specific problems this fixes, but at worst this is harmless.
I don't expect anyone else to actually use ksh93 on a museum-grade
Power Mac G5 running Mac OS X 10.3.7, but ancient platforms are
great bug and compatibility testing tools. These tweaks restore the
ability to build on that platform.
Also, to avoid a strange path search bug on that platform and
possibly other ancient ones, set SHOPT_DYNAMIC to 0 in SHOPT.sh.
Bug: [[ ! ! 1 -eq 1 ]] returns false, but should return true.
This bug was reported for bash, but ksh has it too:
https://lists.gnu.org/archive/html/bug-bash/2021-06/msg00006.html
Op 24-05-21 om 17:47 schreef Chet Ramey:
> On 5/22/21 2:45 PM, Vincent Menegaux wrote:
>> Previously, these commands:
>>
>> [[ ! 1 -eq 1 ]]; echo $?
>> [[ ! ! 1 -eq 1 ]]; echo $?
>>
>> would both result in `1', since parsing `!' set CMD_INVERT_RETURN
>> instead of toggling it.
>
> Interestingly, ksh93 produces the same result as bash. I agree
> that it's more intuitive to toggle it.
Also interesting is that '!' as an argument to the simple
'test'/'[' command does work as expected (on both bash and ksh93):
'test ! ! 1 -eq 1' and '[ ! ! 1 -eq 1 ]' return 0/true.
Even the man page for [[ is identical for bash and ksh93:
| ! expression
| True if expression is false.
This suggests it's supposed to be a logical negation operator, i.e.
'!' is implicitly documented to negate another '!'. Bolsky & Korn's
1995 ksh book, p. 167, is slightly more explicit about it:
"! test-expression. Logical negation of test-expression."
I also note that multiple '!' negators in '[[' work as expected on
mksh, yash and zsh.
src/cmd/ksh93/sh/parse.c: test_primary():
- Fix bitwise logic for '!': xor the TNEGATE bit into tretyp
instead of or'ing it, which has the effect of toggling it.
The memory leak regression tests added in commit 05683ec7 only leak memory
in the C.UTF-8 locale if ksh is compiled with vmalloc. I've ran these
regression tests against ksh93v- and neither fail in that version of
ksh, which indicates the bug causing these tests to fail may be similar to
the one that causes <https://github.com/ksh93/ksh/issues/95>.
Since the memory leak tests work with -D_std_malloc, only set $LANG to
'C' if ksh is compiled with vmalloc enabled.
This regression also exists on ksh 93v- and ksh2020, from which it
was backported.
Reproducer:
$ (fn() { true; }; fn >/dev/null/ne; true) 2>/dev/null; echo $?
1
Expected output: 0 (as on ksh 93u+).
FreeBSD sh and NetBSD sh are the only other known shells that share
this behaviour. POSIX currently allows both behaviours, but may
require the ksh 93u+ behaviour in future. In any case, this causes
an incompatibility with established ksh behaviour that could easily
break existing ksh scripts.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Commit 23f2e23 introduced a check for jmpval > SH_JMPIO (5).
When a function call pushes context for a redirection, this is
done with the jmpval exit value of SH_JMPCMD (6). Change that to
SH_JMPIO to avoid triggering that check.
src/cmd/ksh93/tests/exit.sh:
- Add regression tests for exit behaviour on various kinds of
shell errors as listed in the POSIX standard, including an error
in a redirection of a function call.
Fixes: https://github.com/ksh93/ksh/issues/310
Problem:
$ exec ksh
$ echo $SHLVL
2
$ exec ksh
$ echo $SHLVL
3
$ exec ksh
$ echo $SHLVL
4
...etc. SHLVL is supposed to acount the number of shell processes
that you need to exit before you get logged out. Since ksh was
replacing itself with a new shell in the same process using 'exec',
SHLVL should not increase.
src/cmd/ksh93/bltins/misc.c: b_exec():
- When about to replace the shell and we're not in a subshell,
decrease SHLVL to cancel out a subsequent increase by the
replacing shell. Bash and zsh also do this.
On NetBSD, for some reason, the wctrans(3) and towctrans(3) C
library functions exist, but have no effect; the "toupper" and
"tolower" maps don't even translate case for ASCII, never mind wide
characters. This kills 'typeset -u' and 'typeset -l' on ksh, which
was the cause of most of the regression test failures on NetBSD.
Fallback versions for these functions are provided in init.c, but
were not being used on NetBSD because the feature test detected the
presence of these functions in the C library.
src/cmd/ksh93/features/locale:
- Replace the simple test for the presence of wctrans(3),
towctrans(3), and the wctrans_t type by an actual feature test
that checks that these functions not only compile, but are also
capable of changing an ASCII 'q' to upper case and back again.
src/cmd/ksh93/sh/init.c: towctrans():
- Add wide character support to the fallback function, for whatever
good that may do; on NetBSD, the wide-character towupper(3) and
towlower(3) functions only change case for ASCII.
After the last increase from 4 to 6 bytes, there are still
intermittent false leaks.sh failures (different ones on each run)
on the GitHub CI runner on the 1.0 branch, which is compiled with
the OS's malloc (as opposed to ast vmalloc). Increase the byte
tolerance for the leaks test from 6 to 8 bytes on Linux when
compiling with standard malloc.
The last commit still failed to build on macOS M1. That va_listval
macro keeps causing trouble. It's an AST thing that is defined in
src/lib/libast/features/common. That looks like some incredibly
opaque attempt to make it compatible with everything, and clearly
it no longer works properly on all systems. I don't dare touch it,
though. That code looks like any minimal change will probably break
the build on some system or other.
src/lib/libast/features/hack:
- Add feature test to check if that macro needs (0) no workaround,
or (1) the workaround from the 93v- beta, or (2) the FreeBSD one.
Whichever version compiles first, it will use. If none does, this
test will not output a value, which will be treated as 0.
src/lib/libast/hash/hashalloc.c,
src/lib/libast/string/tokscan.c:
- Update to use the result of the hack feature test.
src/lib/libast/Mamfile:
- Update for new #include dependencies.
src/cmd/ksh93/tests/{shtests,_common}:
- When xtrace is active, set SECONDS to the float type so that
the $SECONDS expansion in $PS4 shows fractional seconds.
src/cmd/ksh93/tests/*.sh:
- Various fixes to avoid command substitutions incorporating xtrace
output into their results. Sometimes this is done by avoiding a
preceding assignment on a command that redirects 2>&1 (as that
will also redirect the preceding assignment and its xtrace,
causing the command substitution to capture the xtrace); other
times it was easiest to just turn off xtrace outright within the
command substitution.
src/cmd/ksh93/tests/math.sh:
- Remove an obsolete 'fixme' note.
There are intermittent false failures on the GitHub CI runners on
the 1.0 branch, which is compiled with the OS's malloc (as opposed
to ast vmalloc). Increase the byte tolerance for the leaks test
from 4 to 6 bytes on Linux when compiling with standard malloc.
hyenias writes, re the referenced commit:
> This has caused my Ubuntu 18.04 ARMv7 to fail to compile.
>
> /dev/shm/ksh/src/lib/libast/hash/hashalloc.c: In function 'hashalloc':
> /dev/shm/ksh/src/lib/libast/hash/hashalloc.c:156:11: error:
> incompatible types when assigning to type 'va_list * {aka
> __va_list *}' from type 'va_list {aka __va_list}'
> tmpval = va_listval(va_arg(ap, va_listarg));
> ^
> In file included from ./ast_common.h:192:0,
> from /dev/shm/ksh/src/lib/libast/include/ast_std.h:37,
> from /dev/shm/ksh/src/lib/libast/include/ast.h:36,
> from /dev/shm/ksh/src/lib/libast/hash/hashlib.h:34,
> from /dev/shm/ksh/src/lib/libast/hash/hashalloc.c:33:
> /dev/shm/ksh/src/lib/libast/hash/hashalloc.c:157:16: error:
> incompatible type for argument 2 of '__builtin_va_copy'
> va_copy(ap, tmpval);
> ^
> /dev/shm/ksh/src/lib/libast/hash/hashalloc.c:157:16: note: expected
> '__va_list' but argument is of type 'va_list * {aka __va_list *}'
> mamake [lib/libast]: *** exit code 1 making hashalloc.o
> mamake: *** exit code 1 making lib/libast
> mamake: *** exit code 1 making all
> package: make done at Fri May 14 06:10:16 EDT 2021 in
> /dev/shm/ksh/arch/linux.arm
src/lib/libast/hash/hashalloc.c,
src/lib/libast/string/tokscan.c:
- Revert the FreeBSD fix.
- Backport a conditional workaround for clang from ksh 93v- beta.
On some systems, the following won't compile because of the way the
macros are defined in the system headers:
va_copy(ap, va_listval(va_arg(ap, va_listarg)));
The error from clang is something like:
.../hashalloc.c:155:16: error: non-const lvalue reference to type
'__builtin_va_list' cannot bind to a temporary of type 'va_list'
(aka 'char *')
va_copy(ap, va_listval(va_arg(ap, va_listarg)));
~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./ast_common.h:200:23: note: expanded from macro 'va_listval'
#define va_listval(p) (p)
^
.../include/stdarg.h:27:53: note: expanded from macro 'va_copy'
#define va_copy(dest, src) __builtin_va_copy(dest, src)
^~~
1 error generated.
mamake [lib/libast]: *** exit code 1 making hashalloc.o
This commit backports a FreeBSD build fix from:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=255308
Thanks to Chase <nicetrynsa@protonmail.ch> for the bug report.
src/lib/libast/hash/hashalloc.c,
src/lib/libast/string/tokscan.c:
- Store va_listval() result in variable and pass that to va_copy().
Since a command substitution no longer forks on non-permanently
redirecting standard output within it for a specific command,
test -t 1, [ -t 1 ], and [[ -t 1 ]] broke as follows:
v=$(test -t 1 >/dev/tty && echo ok) did not assign 'ok' to v.
This is because the assumption in tty_check() that standard output
is never on a terminal in a non-forked command substitution, added
in 55f0f8ce, was made invalid by 090b65e7.
src/cmd/ksh93/edit/edit.c: tty_check():
- Implement a new method. Return false if the file descriptor
stream is of type SF_STRING, which is the case for non-forked
command substitutions -- it means the sfio stream writes directly
into a memory area. This can be checked with the sfset(3)
function (see src/lib/libast/man/sfio.3). To avoid a segfault
when accessing sh.sftable, we need to validate the FD first.
src/cmd/ksh93/tests/pty.sh:
- Add the above reproducer.
The bitmask of attributes to export was repeatedly defined in three
different places, and that fix changed only one of them.
src/cmd/ksh93/sh/name.c:
- Single point of truth: define ATTR_TO_EXPORT macro with the
bitmask of all the attributes to export (excluding NV_RDONLY).
- attstore(), pushnam(), sh_envgen(): Use the ATTR_TO_EXPORT macro,
removing superflous NV_RDONLY handling from the former two.
Commit 92f7ca54 broke compilation with tcc on Linux. The following
error would occur while compiling ksh with tcc:
In file included from /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/data/strdata.c:105:
./FEATURE/math:91: error: too many basic types
mamake [cmd/ksh93]: *** exit code 1 making strdata.o
The build failure is fixed by backporting the relevant bugfix from
the 93v- version of iffe.
src/cmd/INIT/iffe.sh:
- Backport the 2013 iffe bugfix for the intrinsic function test to
rule out type names (dated 2013-08-11 in the 93v- changelog).
The build started failing on Solaris Studio cc when 'noreturn' was
introduced, because the wrappers pass the -xc99 flag which sets the
compiler to C99 mode. 'noreturn' is a C11 feature. The
stdnoreturn.h header was correctly included but the compiler still
threw a syntax error (long path abbreviated below):
".../stk.c", line 124: warning: _Noreturn is a keyword in ISO C11
".../stk.c", line 124: warning: old-style declaration or incorrect
type for: _Noreturn
".../stk.c", line 124: syntax error before or at: static
src/cmd/INIT/cc.sol11.*:
- Pass -std=c11 to cc instead of -xc99. At least on i386-64, this
is sufficient to fix the build.
README.md, src/cmd/ksh93/README.md:
- Remove -xc99 from the Solaris build flags example as that is
incompatible with -std=c11 (and was already redundant with the
-xc99 in the wrappers).
src/cmd/ksh93/tests/basic.sh:
- Don't run a newly backported 93v- regression test on Solaris
because it uses the 'join' command with process subsitutions;
Solaris 11.4's join(1) hangs when trying to read from /dev/fd.
This is not ksh's fault. (re: 59bacfd4)
On Fedora, this regression test failure occurs:
locale.sh[84]: 'read' doesn't skip multibyte input
correctly (ja_JP.ujis, \x95\x5c)
This is a problem with the test; this Shift-JIS specific test
should not be run in a non-Shift-JIS locale. So this commit skips
it unless the locale string ends in '.SJIS' (case insensitive).
It also adds cleanup for the 'chr' variable's special attributes
in case that name is ever going to be used in another test.
nmake was removed long ago (2940b3f5) and so were the outdated
Makefiles (6cc2f6a0). However, the build system still looked for an
AT&T nmake in $PATH. If a user had it installed, the build would
fail as the system tried to use it.
https://groups.google.com/g/korn-shell/c/2VK8kS0_VhA/m/-Rlnv7PRAgAJ
bin/package, src/cmd/INIT/package.sh:
- Remove all the code supporting nmake.
- Make 'bin/package test' work by simply exec'ing bin/shtests.
src/cmd/INIT/Mamfile:
- Do not install *.mk nmake support files.
lib/package/*.mk, src/cmd/INIT/*.mk:
- nmake support files removed.
src/cmd/ksh93/sh.1:
- The POSIX option description still said that attributes "such as
integer and readonly" aren't imported from the environment. But
as of 7954855f, the readonly attribute is never imported or
exported. So change that to another example (left/right justify).
- Tweak idiosyncratic use of hyphens.
- be inputted => be input.
In May 2020, when every KornShell (ksh93) development project was
abandoned, development was rebooted in a new fork based on the last
stable AT&T version: ksh 93u+. Now, one year and hundreds of bug
fixes later, the first beta version is ready, and KornShell lives
again. This new fork is called ksh 93u+m as a permanent nod to its
origin; a standard semantic version number is added starting at
1.0.0-beta.1. Please test the beta and report any bugs you find,
or help us fix known bugs.
src/cmd/ksh93/data/math.tab:
- Added exp10().
- Remove int() as being an alias to floor().
- Created entries for local float() and local int() which are
defined in features/math.sh.
src/cmd/ksh93/features/math.sh:
- Backport floor() and int() related code from ksh93v-.
src/cmd/ksh93/sh.1:
- Sync man page to math.tab's potential functions.
src/cmd/ksh93/sh/xec.c: sh_exec(): TCOM:
- In the referenced commit I'd accidentally deleted this line:
shgd->current_pid = getpid();
from the routine to optimise the ( simple_command & ) case.
This resulted in the following regression test failure on
ARM boxes:
variables.sh[71]: Test 4: $RANDOM seed in ( simple_command & )
The cause was that the current PID shgd->current_pid, which is
factored into the seed, was not updated before reseeding.
Apparently the system clock on ARM systems is not fine-grained
enough to compensate.
This adds a #pragma to disable -Wdeprecated-register* on newer
versions of clang. We could remove all use of the register keyword
instead, as modern compilers ignore it. But it's not harmful, and
for the time being I prefer not to do doing any reformatting or
changing the historic character of this code base.
The #pragmas are removed from src/lib/libast/include/ast.h, because
they're better placed in src/lib/libast/features/common which
generates ast_common.h which is included by everything.
* https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-register
src/cmd/ksh93/tests/builtins.sh:
- An original AT&T test for 'read -s' was disabled and marked
FIXME. Fix the invalid invocation and check that 'read -s'
actually writes to the history file.
- Remove a temporary 'command -p ls' debug test that I accidentally
committed (re: a197b042).
I did not realize that lvalue->nosub and lvalue->sub variables are
not reset when another assignment occurs later down the line.
Example: (( arr[0][1]+=1, arr[2]=7 ))
src/cmd/ksh93/sh/arith.c: arith():
- For assignment operations, reset lvalue's nosub and sub variables
so the target for the next assignment is not redirected.
src/cmd/ksh93/tests/arrays2.sh:
- Add in a few regression tests that utilize compound arithmetic
expressions having at least an assignment operation (+=) followed
by a normal assignment (=).
BUG 1: Though 'command' is specified/documented as a regular
builtin, preceding assignments survive the invocation (as with
special or declaration builtins) if 'command' has no command
arguments in these cases:
$ foo=wrong1 command; echo $foo
wrong1
$ foo=wrong2 command -p; echo $foo
wrong2
$ foo=wrong3 command -x; echo $foo
wrong3
Analysis: sh_exec(), case TCOM (simple command), contains the
following loop that skips over 'command' prefixes, preparsing any
options and remembering the offset in the 'command' variable:
src/cmd/ksh93/sh/xec.c
1059 while(np==SYSCOMMAND || !np && com0
&& nv_search(com0,shp->fun_tree,0)==SYSCOMMAND)
1060 {
1061 register int n = b_command(0,com,&shp->bltindata);
1062 if(n==0)
1063 break;
1064 command += n;
1065 np = 0;
1066 if(!(com0= *(com+=n)))
1067 break;
1068 np = nv_bfsearch(com0, shp->bltin_tree, &nq, &cp);
1069 }
This skipping is not done if the preliminary b_command() call on
line 1061 (with argc==0) returns zero. This is currently the case
for command -v/-V, so that 'command' is treated as a plain and
regular builtin for those options.
The cause of the bug is that this skipping is even done if
'command' has no arguments. So something like 'foo=bar command' is
treated as simply 'foo=bar', which of course survives.
So the fix is for b_command() to return zero if there are no
arguments. Then b_command() itself needs changing to not error out
on the second/main b_command() call if there are no arguments.
src/cmd/ksh93/bltins/whence.c: b_command():
- When called with argc==0, return a zero offset not just for -v
(X_FLAG) or -V (V_FLAG), but also if there are no arguments left
(!*argv) after parsing options.
- When called with argc>0, do not issue a usage error if there are
no arguments, but instead return status 0 (or, if -v/-V was given,
status 2 which was the status of the previous usage message).
This way, 'command -v $emptyvar' now also works as you'd expect.
BUG 2: 'command -p' sometimes failed after executing certain loops.
src/cmd/ksh93/sh/path.c: defpath_init():
- astconf() returns a pointer to memory that may be overwritten
later, so duplicate the string returned. Backported from ksh2020.
(re: f485fe0f, aa4669ad, <https://github.com/att/ast/issues/959>)
src/cmd/ksh93/tests/builtins.sh:
- Update the test for BUG_CMDSPASGN to check every variant of
'command' (all options and none; invoking/querying all kinds of
command and none) with a preceding assignment. (re: fae8862c)
This also covers bug 2 as 'command -p' was failing on macOS prior
to the fix due to a loop executed earlier in another test.
@JohnoKing writes:
> In emacs mode, using Alt+D or Alt+H with a repeat parameter
> results in the deletion of extra characters. Reproducer:
>
> $ set -o emacs
> $ foo bar delete add # <Ctrl+A> <ESC+3+Alt+D>
> $ d # Should be ' add'
>
> $ foo bar delete add # <ESC+3+Alt+H>
> $ f # Should be 'foo '
>
> [...] this bug also affects the Delete and Arrow keys [...].
> Reproducer:
>
> $ test_string <Ctrl+A> <ESC+3+Delete>
> # This will delete all of 'test', which is four characters
> $ test_string <Ctrl+A> <ESC+4+Right Arrow>
> # This should move the cursor to '_', not 's'
src/cmd/ksh93/edit/emacs.c: ed_emacsread():
- Revert part of 29b11bba: once again set 'count' to
'vt220_save_repeat' instead of adding the value.
- do_escape: If the escape() function (which handles both ESC
repeat counts and commands like ESC d and ESC h) returns a repeat
count, do not use the saved repeat count for v220 sequences.
src/cmd/ksh93/tests/pty.sh:
- Test the four reproducers above.
Fixes: https://github.com/ksh93/ksh/issues/292
This PR corrects #168 for indexed arrays having more than one
level. Turns out ksh was only keeping track of the subscript number
for assignment in lvalue's nosub variable. By saving the actual
subscript reference, the result can be assigned to its proper
destination instead of putting the result into the last looked
value or subscript location.
src/cmd/ksh93/include/streval.h: struct lval:
- Create a new pointer named sub to hold the reference that nosub
describes.
src/cmd/ksh93/sh/arith.c: arith():
- Adjust LOOKUP: for lvalue ARITH_ASSIGNOP operations on indexed
arrays to save the np of the destination subscript for later use.
- Adjust ASSIGN: to act when lvalue's nosub > 0 which happens as
the last step in the arithmetic parsing loop for assignment
operations. Only indexed arrays will have a nosub value > 0. All
others have a nosub of 0 unless they are involved in a unary
operation (++, --) which sets nosub to -1. All said in the
context of assignment operations like (( arr[0][1] += 1 )).
src/cmd/ksh93/sh/streval.c:
- Initialize the new sub pointer to 0.
src/cmd/ksh93/tests/arrays2.sh:
- Created a few multidimensional indexed array tests for assignment
operations like += as an example.
Resolves: https://github.com/ksh93/ksh/issues/168
Following the resolution of Austin Group bug 1393[*] that is set to
be included in the next version of the POSIX standard, the
'command' prefix in POSIX mode (set -o posix) no longer disables
the declaration properties of declaration built-ins.
[*] https://austingroupbugs.net/view.php?id=1393
src/cmd/ksh93/sh/parse.c: lex():
- Skip the 'command' prefix even in POSIX mode so that any
declaration commands prefixed by it are treated as such in xec.c
(sh_exec()).
src/cmd/ksh93/sh/xec.c: sh_exec():
- The foregoing change reintroduced a variant of BUG_CMDSPEXIT: the
shell exits on something like 'command export readonlyvar=foo'.
This now fixes that bug for both POSIX and non-POSIX mode. When
calling nv_setlist() to process true shell assignments, and there
is a 'command' prefix, push a shell context and use sigsetjmp to
intercept any errors in assignments and stop the shell exiting.
src/cmd/ksh93/tests/builtins.sh:
- Borrow the BUG_CMDSPEXIT regression test from modernish and adapt
it for ksh. (I'm the author so yes, I can do this.) Original:
https://github.com/modernish/modernish/blob/ae8fe9c3/lib/modernish/tst/builtin.t#L80-L109
Tab completion in emacs and vi wrongly parses and executes command
substitutions. Example reproducers:
$ $(~)<Tab> # Result:
$ $(~)ksh[1]: /home/johno: cannot execute [Is a directory]
$ $(~ksh)<Tab> # Result:
$ $(~ksh)ksh: /home/johno/GitRepos/KornShell/ksh: cannot execute [Is a directory]
$ $(echo true)<Tab> # Result:
$ /usr/bin/true # or just 'true' -- it's unpredictable
In addition, backtick command substitutions had the following bug:
$ `echo hi`<Tab> # Result:
$ `echo hi`ksh: line 1: BUG_BRACQUOT_test.sh: not found
(where BUG_BRACQUOT_test.sh happens to be lexically the
first-listed file in my ksh development working directory).
There's also a crash associated with this due to an access beyond
buffer boundaries, which is only triggered on some systems (macOS
included).
src/cmd/ksh93/edit/completion.c:
- find_begin():
* When finding the beginning of a command substitution and the
last character is ')', do not increase the character pointer
cp. Increasing it caused the condition 'if(c && c==endchar)' in
the 'default:' block to be true, causing 'return(xp);' to be
executed, which returns a pointer the beginning of the command
substitution to ed_expand() on line 290, so that ed_expand()
eventually executes the command substitution with the
sh_argbuild() call on line 349. After deleting this 'else
cp++', that statement 'if(c && c==endchar) return(xp);' is not
executed and `find_begin()` returns the null pointer, which
avoids anything being executed. Thanks to @JohnoKing:
https://github.com/ksh93/ksh/issues/268#issuecomment-817249164
* Add code for properly skipping over backtick-style command
substitutions, based on the $( ) code.
- ed_expand(): Avoid out[-1] reading one byte to the left of
outbuff by first checking that out>outbuff. Thanks to @JohnoKing
for using ASan to find the location of the crash:
https://github.com/ksh93/ksh/issues/268#issuecomment-825574885
src/cmd/ksh93/tests/pty.sh:
- Test for the bugs detailed above.
Resolves: https://github.com/ksh93/ksh/issues/268
On slower systems it could fail with an arithmetic syntax error
because the output was verified before it had been written.
Also make another test xtrace-proof.
This applies when ksh is compiled with standard malloc.
Apparently, 1024 iterations is not enough on Gentoo Linux i386, at
least not when running the full test suite. The leak tests fail
intermittently and different tests fail each time, but always with
a leak of exactly 36864 bytes for each failing test. So those
failures are clearly spurious. Doubling the number of iterations
seems to make them go away.
src/cmd/ksh93/tests/{basic.sh,builtins.sh,shtests}:
- Redirect error output from the ulimit builtin to silence irrelevant
errors in the regression tests (these errors may occur when a
command such as 'ulimit -t 4' is run before the regression tests).
- Shellquote the error messages from the getconf regression tests.
src/cmd/ksh93/tests/{arrays,io,variables}.sh:
- Backport the ksh2020 regression tests for the following bugs:
https://github.com/att/ast/issues/23https://github.com/att/ast/issues/203https://github.com/att/ast/issues/472https://github.com/att/ast/issues/492
- Minor fix to POSIX mode regression tests in ksh93v-. In ksh93v-,
[[ -o ?posix ]] doesn't return an error (because it's implemented
in the bash mode). However, 'set -o posix' will fail in ksh93v-
if it's not in bash compatibility mode, which causes this test
script to exit prematurely.
src/cmd/ksh93/tests/{basic,pty}.sh:
- Add test for https://github.com/att/ast/issues/1461
- The ksh2020 fix for [ -t 1 ] in non-forking command substitutions
caused the following bug in interactive shells:
$ ( [ -t 1 ]; echo $? )
1 # Always fails
To avoid introducing this bug, this commit adds a regression
test for it.
src/cmd/ksh93/tests/functions.sh:
- Add test for https://github.com/att/ast/issues/1160
Put the test to the start of functions.sh (if it's at the end
of the script, it refuses to fail under ksh2020). Output from
this regression test when run against ksh2020:
functions.sh[46]: eval'ing function dumps function body to
stdout (got $' { eval "bar() { FAILURE; }"; }\n { FAILURE; }')
The regression is:
quoting.sh[189]: expansion of "{q:+'}" not correct when q unset
The failure was that, for unset q, "${q:+'}q${q:+'}" yielded empty
and not 'q'. This is because the single quotes within the double
quotes were erroneously parsed as meaningful.
The originally used ST_QUOTE state table (see data/lexstates.c),
where no quote character has any special meaning, was for avoiding
this problem.
The newly introduced ST_MOD1 state table is a copy of ST_QUOTE
except the ' has been given its special meaning back. We need this
to fix#290, but only for unquoted expansions.
So we need to go back to using ST_QUOTE if the string is quoted
(mp->quote) and we're not parsing a substitution that uses patterns
where quotes are significant (newops, ST_MOD2), i.e., only for
old-style ST_MOD1 operators.
src/cmd/ksh93/sh/macro.c: varsub():
- When the ${var<OP>string} expansion is quoted, and of an old
(S_MOD1) type, then use the ST_QUOTE state table to skip over it
instead of the new ST_MOD1 one.
This fixes the following:
1. Using $RANDOM in a virtual/non-forked subshell no longer
influences the reproducible $RANDOM sequence in the parent
environment.
2. When invoking a subshell $RANDOM is now re-seeded (as mksh and
bash do) so that invocations in repeated subshells (including
forked subshells) longer produce identical sequences by default.
3. Program flow corruption that occurred in scripts on executing
( ( simple_command & ) ).
src/cmd/ksh93/include/variables.h:
- Move 'struct rand' here as it will be needed in subshell.c. Add
rand_seed member to save the pseudorandom generator seed. Remove
the pointer to the shell state as it's redundant.
src/cmd/ksh93/sh/init.c:
- put_rand(): Store given seed in rand_seed while calling srand().
No longer pointlessly limit the number of possible seeds with the
RANDMASK bitmask (that mask is to limit the values to 0-32767,
it should not limit the number of possible sequences to 32768).
- nget_rand(): Instead of using rand(), use rand_r() to update the
random_seed value. This makes it possible to save/restore the
current seed of the pseudorandom generator.
- Add sh_reseed_rand() function that reseeds the pseudorandom
generator by calling srand() with a bitwise-xor combination of
the current PID, the current time with a granularity of 1/10000
seconds, and a sequence number that is increased on each
invocation.
- nv_init(): Set the initial seed using sh_reseed_rand() here
instead of in sh_main(), as this is where the other struct rand
members are initialised.
src/cmd/ksh93/sh/main.c: sh_main():
- Remove the srand() call that was replaced by the sh_reseed_rand()
call in init.c.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Upon entering a virtual subshell, save the current $RANDOM seed
and state, then reseed $RANDOM for the subshell.
- Upon exiting a virtual subshell, restore $RANDOM seed and state
and reseed the generator using srand() with the restored seed.
src/cmd/ksh93/sh/xec.c: sh_exec():
- When optimizing out a subshell that is the last command, still
act like a subshell: reseed $RANDOM and increase ${.sh.subshell}.
- Fix a separate bug discovered while implementing this. Do not
optimize '( simple_command & )' when in a virtual subshell; doing
this causes program flow corruption.
- When optimizing '( simple_command & )', also reseed $RANDOM and
increment ${.sh.subshell}.
src/cmd/ksh93/tests/subshell.sh,
src/cmd/ksh93/tests/variables.sh:
- Add various tests for all of the above.
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
Resolves: https://github.com/ksh93/ksh/issues/285
The following problems remained:
$ var=x; echo ${var:-'{}'}
x}
$ var=; echo ${var:+'{}'}
}
src/cmd/ksh93/sh/macro.c: varsub():
- Use the new ST_MOD1 state table to skip over ${var-'foo'}, etc.
instead of ST_QUOTE. In ST_MOD1 the ' is categorised as S_LIT
which causes the single quotes to be skipped over correctly.
See d087b031 for more info.
src/cmd/ksh93/tests/quoting2.sh:
- Add tests for this remaining bug.
- Make the new test xtrace-proof.
Resolves: https://github.com/ksh93/ksh/issues/290 (again)
src/cmd/ksh93/{bltins/typeset,sh/name,sh/nvtree,sh/nvtype}.c:
- Replace more instances of memcmp with strncmp to fix
heap-buffer-overflow errors when running the regression tests
with ASan enabled.
src/cmd/ksh93/edit/vi.c:
- Fix an invalid dereference of the 'p' pointer to fix a crash in
vi mode when entering a comment in the command history. This
bugfix was backported from ksh2020:
https://github.com/att/ast/issues/798
src/cmd/ksh93/tests/pty.sh:
- Add a regression test for the vi mode crash.
The code contains various checks to see if a subshell needs to
fork, like this one in the ulimit builtin:
if(shp->subshell && !shp->subshare)
sh_subfork();
All checks of this form are fatally broken, as each one of them
causes shared-state command substitutions to ignore parent virtual
subshells.
Currently the only feasible way to fix this is to fork a virtual
subshell before executing a shared-state command substitution in
it. In the long term I think shared-state command substitutions
should probably be redesigned to disassociate them completely from
the virtual subshell mechanism.
src/cmd/ksh93/sh/macro.c: comsubst():
- If we're in a non-subshare virtual subshell, fork it before
entering a type 2 (subshare) command substitution.
src/cmd/ksh93/sh/subshell.c:
- sh_assignok(): Remove subshare fix from 911d6b06 as it's
redundant now that the parent of a subshare is never a virtual
subshell. Go back to not doing anything if the current "subshell"
is a subshare.
- sh_subtracktree(), sh_subfuntree(): Similarly, remove the
now-redundant subshare fixes from 13c57e4b.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Fix a separate bug: only fork a virtual subshell before running a
background job if that "subshell" is not a subshare.
src/cmd/ksh93/tests/subshell.sh:
- Add test for bug fixed in xec.c.
- Add tests for 'ulimit', 'builtin' and 'exec' run in subshare
within subshell -- all commands that use checks of the form
'if(sh.subshell && !sh.subshare) sh_subfork();'.
Resolves: https://github.com/ksh93/ksh/issues/289
src/cmd/ksh93/bltins/typeset.c:
- setall(): Only run sh_assignok() if troot points to the variable
tree. For instance, it's pointless to run it for an alias.
- Remove vestigial SHOPT_BSH code. The ast-open-history repo shows
that earlier SHOPT_BSH code was removed on 2008-06-02 and
2005-05-22. This may have been experimental code for increased
compatibility with the ancient Bourne shell. There was never any
documentation.
This avoids splitting on quoted whitespace when extracting words
from the command history using the emacs M-. or vi _ command.
Example: if the prior command is
$ ls Stairway\ To\ Heaven.mp3
then, M-. in Emacs editing mode (and _ in vi mode) now inserts
Stairway\ To\ Heaven.mp3 instead of Heaven.mp3. The behavior is
similar for 'Stairway To Heaven.mp3' and "Stairway To Heaven.mp3".
src/cmd/ksh93/edit/history.c: hist_word():
- Skip over single-quoted and double-quoted strings and
backslash-escaped characters.
src/cmd/ksh93/tests/pty.sh:
- Add regression test for this feature in vi mode. Since emacs and
vi both use the same code for this, that should be good enough.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
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).
Like tdump() and trestore() before commit 32d1abb1, sh_deparse() fails
to handle process substitutions correctly. This limitation of the shell
deparser is rather minor since it's unused. However, seeing as the
deparser was left in the code base intentionally it should at least
function properly.
src/cmd/ksh93/sh/deparse.c:
- Add a PROCSUBST flag for handling process substitutions in
sh_deparse().
- If we're handling a process substitution, add an ending ')'
without an extra newline.
- Avoid adding an extra ' &' to commands inside of a process
substitution. An extra ' &' is only added if the FAMP and FINT
flags are set, which indicates the command was spawned as a separate
job with '&'.
- Add process substitution handling to 'p_redirect' by calling p_tree()
when encountering a process substitution.
src/cmd/ksh93/bltins/typeset.c:
- Removing the nv_search() call altogether was actually not
neccessary, I was just searching the wrong tree: instead of
sh.fun_base, simply search the current sh.fun_tree which has a
view to all the layered parent subshell copes. It is not going to
find it in the current subshell tree but will find it in one of
the parent trees if it exists. The cost of an unnecessary dummy
is negligible, but so is the cost of this search, and doing it is
more correct.
src/cmd/ksh93/bltins/whence.c:
- The previous commit that fixed 'unset -f' in virtual subshells left
one bug. The type builtin (or 'whence -v') could still find the unset
function in virtual subshells:
$ foo() { echo foo; }
$ (unset -f foo; type foo)
foo is an undefined function
To fix this bug, avoid detecting functions in the whence builtin
unless they have the NV_FUNCTION flag.
src/cmd/ksh93/tests/subshell.sh:
- Add a regression test for using 'type' on a function unset inside of
a virtual subshell.
A bug introduced in the previous commit caused 'unset -f' in a
subshell of a subshell to fail to unset a function created in a
parent subshell. Reproducer:
$ ( f2() { echo WRONG; }; ( unset -f f2; f2 ) )
WRONG
src/cmd/ksh93/bltins/typeset.c: unall():
- Do not nv_search() in sh.fun_base before setting the dummy node
that marks the function as unset in this subshell. That search
only reaches the base tree and not any of its subtrees. Setting
the dummy unconditionally is not harmful; the cost is negligible.
src/cmd/ksh93/tests/subshell.sh:
- Add test for the bug.
This commit implements unsetting functions in virtual subshells,
removing the need for the forking workaround. This is done by
either invalidating the function found in the current subshell
function tree by unsetting its NV_FUNCTION attribute bits (which
will cause sh_exec() to skip it) or, if the function exists in a
parent shell, by creating an empty dummy subshell node in the
current function tree without that attribute.
As a beneficial side effect, it seems that bug 228 (unset -f fails
in forked subshells if a function is defined before forking) is now
also fixed.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/init.c:
- Add sh.fun_base for a saved pointer to the main shell's function
tree for checking when in a subshell, analogous to sh.var_base.
src/cmd/ksh93/bltins/typeset.c: unall():
- Remove the fork workaround.
- When unsetting a function found in the current function tree
(troot) and that tree is not sh.var_base (which checks if we're
in a virtual subshell in a way that handles shared-state command
substitutions correctly), then do not delete the function but
invalidate it by unsetting its NV_FUNCTION attribute bits.
- When unsetting a function not found in the current function tree,
search for it in sh.fun_base and if found, add an empty dummy
node to mask the parent shell environment's function. The dummy
node will not have NV_FUNCTION set, so sh_exec() will skip it.
src/cmd/ksh93/sh/subshell.c:
- sh_subfuntree(): For 'unset -f' to work correctly with
shared-state command substitutions (subshares), this function
needs a fix similar to the one applied to sh_assignok() for
variables in commit 911d6b06. Walk up on the subshells tree until
we find a non-subshare.
- sh_subtracktree(): Apply the same fix for the hash table.
- Remove table_unset() and incorporate an updated version of its
code in sh_subshell(). As of ec888867, this function was only
used to clean up the subshell function table as the alias table
no longer exists.
- sh_subshell():
* Simplify the loop to free the subshell hash table.
* Add table_unset() code, slightly refactored for readability.
Treat dummy nodes now created by unall() separately to avoid a
memory leak; they must be nv_delete()d without passing the
NV_FUNCTION bits. For non-dummy nodes, turn on the NV_FUNCTION
attribute in case they were invalidated by unall(); this is
needed for _nv_unset() to free the function definition.
src/cmd/ksh93/tests/subshell.sh:
- Update the test for multiple levels of subshell functions to test
a subshare as well. While we're add it, add a very similar test
for multiple levels of subshell variables that was missing.
- Add @JohnoKing's reproducer from #228.
src/cmd/ksh93/tests/leaks.sh:
- Add leak tests for unsetting functions in a virtual subshell.
Test both the simple unset case (unall() creates a dummy node)
and the define/unset case (unall() invalidates existing node).
Resolves: https://github.com/ksh93/ksh/issues/228
Noteworthy changes:
- The man pages have been updated to fix a ton of instances of
runaway underlining (this was done with `sed -i 's/\\f5/\\f3/g'`
commands). This commit dramatically increased in size because
of this change.
- The documentation for spawnveg(3) has been extended with
information about its usage of posix_spawn(3) and vfork(2).
- The documentation for tmfmt(3) has been updated with the changes
previously made to the man pages for the printf and date builtins
(though the latter builtin is disabled by default).
- The shell's tracked alias tree (hash table) is now documented in
the shell(3) man page.
- Removed the commented out regression test for an ERRNO variable
as the COMPATIBILITY file states it was removed in ksh93.
There is a TODO note in variables.sh that notes the value of LINENO
is wrong after a virtual subshell. The following script should
print '6', but the bug causes it to print '1' instead:
$ cat /tmp/lineno
#!/bin/ksh
(
unset LINENO
:
)
echo $LINENO
This bug started to occur after the bugfix applied in 7b994b6a.
However, that commit is not where the cause of bug was (when that
bugfix is applied to ksh versions 2008-07-25 through 2012-01-01,
$LINENO works fine). Rather, the cause of this bug was introduced
in 93u+ 2012-02-29. In that version, the mp->nvfun pointer was only
copied from np->nvfun if the variable can be freed from memory.
This is what caused 7b994b6a to break $LINENO in subshells, so to
fix this bug the mp->nvfun and np->nvfun must point to the same
object, even when the variable isn't freed from memory.
src/cmd/ksh93/sh/subshell.c: nv_restore():
- Always copy the np->nvfun pointer to mp->nvfun. To prevent
crashes, the value of np->nvfun->nofree is set to the value given
by the nofree variable, which is set before _nv_unset. See also
commit 7e7f1372, which fixed a crash that happened because
_nv_unset discards the NV_NOFREE flag.
src/cmd/ksh93/tests/variables.sh:
- Remove the workaround for LINENO after a virtual subshell.
- Add a regression test for the value of LINENO when unset in a
virtual subshell, then used after the subshell. Note that before
commit 997ad43b LINENO's value was corrupted after being unset in
a subshell, so the test checks for corruption of the LINENO
variable (in prior commits LINENO was set to '49' because of the
previous bug).
The changes in this commit allow ksh to be built and run with
ASan[*], although for now it only works under vmalloc. Example
command to build ksh with ASan:
$ bin/package make CCFLAGS='-O0 -g -fsanitize=address'
[*] https://en.wikipedia.org/wiki/AddressSanitizer
src/cmd/INIT/mamake.c:
- Fix a few memory leaks in mamake. This doesn't fix all of the
memory leaks ASan complains about (there is one remaining in the
view() function), but it's enough to get ksh to build under ASan.
src/lib/libast/features/map.c,
src/lib/libast/misc/glob.c:
- Rename the ast globbing functions to _ast_glob() and
_ast_globfree(). Without this change the globbing tests fail
under ASan. See: https://github.com/att/ast/commit/2c49eb6e
src/cmd/ksh93/sh/{init,io,nvtree,subshell}.c:
- Fix buffer overflows by using strncmp(3) instead of memcmp(3).
src/cmd/ksh93/sh/name.c:
- Fix another invalid usage of memcmp by using strncmp instead.
This change is also in one of Red Hat's patches:
https://git.centos.org/rpms/ksh/blob/c8s/f/SOURCES/ksh-20120801-nv_open-memcmp.patch
Resolves: https://github.com/ksh93/ksh/issues/230
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
src/cmd/ksh93/README:
- Update compile-time options docuemntation.
- Update build instructions.
- Remove obsolete stuff.
src/cmd/ksh93/SHOPT.sh:
- Remove unused SHOPT_SEVENBIT option. A search in ast-open-history
shows it was removed from include/edit.h on 2001-10-31. You can
still get its effect by changing STRIP from 0377 to 0177 there.
While automagically importing/exporting ksh variable attributes via
the environment is probably a misfeature in general (now disabled
for POSIX standard mode), doing so with the readonly attribute is
particularly problematic. Scripts can take into account the
possibility of importing unwanted attributes by unsetting or
typesetting variables before using them. But there is no way for a
script to get rid of an unwanted imported readonly variable. This
is a possible attack vector with no possible mitigation.
This commit blocks both the import and the export of the readonly
attribute through the environment. I consider it a security fix.
src/cmd/ksh93/sh/init.c: env_import_attributes():
- Clear NV_RDONLY from imported attributes before applying them.
src/cmd/ksh93/sh/name.c: sh_envgen():
- Remove NV_RDONLY from bitmask defining attributes to export.
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>
There were still problems left after the previous commit. On at
least one system (QNX i386), the following regression test crashed:
src/cmd/ksh93/test/subshell.c
900 got=$( { "$SHELL" -c '(cd /; (cd /)); print -r -- "PWD=$PWD"'; } 2>&1 )
A backtrace done on the core dunp pointed to the free() call here:
src/cmd/ksh93/bltins/cd_pwd.c
90 if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot)
91 free(oldpwd);
Analysis: The interaction between $PWD, sh.pwd aka shp->pwd, and
the path_pwd() function is a mess. path_pwd() usually returns a
freeable value, but not always. sh.pwd is sometimes a pointer to
the value of $PWD, but not always (e.g. when you unset PWD or
assign to it). Instead of debugging the exact cause of the crash, I
think it is better to make this work in a more consistent way.
As of this commit:
1. sh.pwd keeps its own copy of the PWD, independently of the PWD
variable. The old value must always be freed immediately before
assigning a new one. This is simple and consistent, reducing the
chance of bugs at negligible cost.
2. The PWD variable is no longer given the NV_NOFREE attribute
because its value no longer points to sh.pwd. It is now a
variable like any other.
src/cmd/ksh93/sh/path.c: path_pwd():
- Do not give PWDNOD the NV_NOFREE attribute.
- Give sh.pwd its own copy of the PWD by strdup'ing PWDNOD's value.
src/cmd/ksh93/bltins/cd_pwd.c: b_cd():
- Since sh.pwd is now consistently freed before giving it a new
value and at no other time, oldpwd must not be freed any longer
and can become a regular non-static variable.
- If the PWD needs reinitialising, call path_pwd() to do it.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Systems with fchdir(2): Always restore the PWD upon exiting a
non-subshare subshell. The check to decide whether or not to
restore it was unsafe: it was not restored if the current PWD
pointer and value was identical to the saved one, but a directory
can be deleted and recreated under the same name.
- Systems without fchdir(2) (if any exist):
. Entry: Fork if the PWD is nonexistent or has no x permission.
. Restore: Only chdir back if the subshell PWD was changed.
That's probably the best we can do. It remains inherently unsafe.
We should probably just require fchdir(2) at some point.
This commit fixes what are hopefully the two final aspects of #153:
1. If the present working directory does not exist (was moved or
deleted) upon entering a virtual subshell, no PWD directory path
is saved. Since restoring the state after exiting a virtual
subshell is contingent on a previous PWD path existing, this
resulted in entire aspects of the virtual subshell, such as the
subshell function tree, not being cleaned up.
2. A separate problem is that 'cd ..' does not update PWD or OLDPWD
when run from a nonexistent directory.
A reproducer exposing both problems is:
$ mkdir test
$ cd test
$ ksh -c '(subfn() { BAD; }; cd ..; echo subPWD==$PWD);
typeset -f subfn; echo mainPWD==$PWD'
subPWD==/usr/local/src/ksh93/ksh/test
subfn() { BAD; };mainPWD==/usr/local/src/ksh93/ksh/test
Expected output:
subPWD==/usr/local/src/ksh93/ksh
mainPWD==/usr/local/src/ksh93/ksh/test
src/cmd/ksh93/bltins/cd_pwd.c:
- If path_pwd() fails to get the PWD (usually it no longer exists),
don't set $OLDPWD to '.' as that is pointless; use $PWD instead.
After cd'ing from a nonexistent directory, 'cd -' *should* fail
and should not be equivalent to 'cd .'.
- Remove a redundant check for (!oldpwd) where it is always set.
- Do not prematurely return without setting PWD or OLDPWD if
pathcanon() fails to canonicalise a nonexistent directory.
Instead, fall back to setting PWD to the result of getcwd(3).
src/cmd/ksh93/sh/subshell.c:
- Minor stylistic adjustment. Some NULL macros sneaked in. This
historic code base does not use them (yet); change to NIL(type*).
- sh_subshell(): Fix logic for determining whether to save/restore
subshell state.
1. When saving, 'if(!comsub || !shp->subshare)' is redundant;
'if(!shp->subshare)' should be enough. If we're not in a
subshare, state should be saved.
2. When restoring, 'if(sp->shpwd)' is just nonsense as there is
no guarantee that the PWD exists upon entering a subshell.
Simply use the same 'if(!shp->subshare)'. Add an extra check
for sp->pwd to avoid a possible segfault. Always restore the
PWD on subshell exit and not only if shp->pwd is set.
- sh_subshell(): Issue fatal errors in libast's "panic" format.
src/cmd/ksh93/tests/builtins.sh:
- Adjust a relevant test to run err_exit() outside of the subshell
so that any error is counted in the main shell.
- Add test for problem 2 described at the top.
src/cmd/ksh93/tests/subshell.sh:
- Add test for problems 1 and 2 based on reproducer above.
Resolves: https://github.com/ksh93/ksh/issues/153
Accessing t->tre.treio for every sh_exec() run is invalid because
't' is of type Shnode_t, which is a union that can contain many
different kinds of structs. As all members of a union occupy the
same address space, only one can be used at a time. Which member is
valid to access depends on the node type sh_exec() was called with.
The invalid access triggered a crash on 32-bit systems when
executing an arithmetic command like ((x=1)).
The t->tre.treio union member should be accessed for a simple
command (case TCOM in sh_exec()). The fix is also needed for
redirections attached to blocks (case TSETIO) in which case the
union member to use is t->fork.forkio.
src/cmd/ksh93/sh/xec.c:
- Add check_exec_optimization() function that checks for all the
conditions where the exec optimisation should not be done. For
redirections we need to loop through the whole list to check for
an IOREWRITE (<>;) one.
- sh_exec(): case TCOM (simple command): Only bother to call
check_exec_optimization() if there are either command arguments
or redirections (IOW: don't bother for bare variable
assignments), so move it to within the if(io||argn) block.
- sh_exec(): case TSETIO: This needs a similar fix. To avoid the
optimization breaking again if the last command is a subshell
with a <>; redirection attached, we need to not only set execflg
to 0 but also clear the SH_NOFORK state bit from the 'flags'
variable which is passed on to the recursive sh_exec() call.
src/cmd/ksh93/tests/io.sh:
- Update and expand tests. Add tests for redirections attached to
simple commands (TCOM) and various kinds of code block (TSETIO).
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
Resolves: https://github.com/ksh93/ksh/issues/278
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
This is the underlying cause for the issue worked around in
3654ee73.
The following explanation refers to the current illumos version of
ksh93 and shows output from illumos' modular debugger:
https://illumos.org/books/dev/debugging.html
Each environment variable (name/value pair) has a linked list of
disciplines attached to it, and at the end of that list there is
optionally a shell context pointer. For example, for the EDITOR
variable:
> ::bp libshell.so.1`put_ed
> ::run
$
$ EDITOR=vim
> ::stack ! head -1
libshell.so.1`put_ed+0x14(e06208, e01c58, 0, dced90)
> e06208::print Namval_t
{
nvname = 0xfffffbffeec40a0e "EDITOR"
nvfun = 0xdced90
nvalue = 0
}
> e06208::print Namval_t nvfun | ::print Namfun_t
{
disc = libshell.so.1`EDITOR_disc
next = libshell.so.1`sh+0x710
}
Here, the EDITOR Namval_t has a discipline stack containing
EDITOR_disc and &Shell_t.nvfun.
The problem arises when a new discipline is pushed onto the stack,
such as when using typeset -u to add an upper-case translation
discipline.
$ typeset -u EDITOR
> e06208::print Namval_t
{
nvname = 0xfffffbffeec40a0e "EDITOR"
nvfun = 0xdced90
nvalue = 0xe0fdb0 "vim"
}
> e06208::print Namval_t nvfun | ::print Namfun_t
{
disc = libshell.so.1`EDITOR_disc
next = 0xdc27a0
}
> e06208::print Namval_t nvfun | ::print Namfun_t next | ::print Namfun_t
{
disc = libshell.so.1`TRANS_disc
next = 0
}
TRANS_disc has been pushed onto the end of the discipline stack,
but the shell handle has been lost.
With this change, the attributes and variables tests pass (this is
on illumos where this change originates).
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:
https://github.com/att/ast/commit/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: https://github.com/att/ast/commit/c073b102
- Simplify EXIT_* and W* macros to use 8 bits.
The usage options test wasn't properly excluding all dtksh builtins,
which was causing the regression tests to fail under dtksh. This commit
adds exclusions for the builtins missed in commit ef4fe41.
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-.
src/cmd/ksh93/tests/readonly.sh:
- Use a 'ulimit --cpu' as a workaround to close down hung processes
that might be caused due to a couple of known bugs (recursion and
type variable function)
Discussion: https://github.com/ksh93/ksh/issues/264
- Adjust tests so xtrace can be used
- Use integer n within for loop
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.
One area where readonly is still ineffective is the local
environment list for a command (preceding assignments) if that
command is not executed using exec(3) after fork(2). Builtin
commands are one example. The following succeeds but should fail:
(readonly v=1; v=2 true) # succeeds, but should fail
If the shell is compiled with SHOPT_SPAWN (the default) then this
also applies to external commands invoked with sh_ntfork():
(readonly v=1; v=2 env) # succeeds if SHOPT_SPAWN
This presents to the user as inconsitent behaviour because external
commands may be fork()ed under certain circumstances but not
others, depending on complex optimisations. One example is:
$ ksh -c 'readonly v=1; v=2 env'
ksh: v: is read only
$ ksh -c 'readonly v=1; v=2 env; :'
(bad: environment list is output, including 'v=2')
In the first command above, where 'v2=env' is the last command in
the -c script, the optimisation skips creating a scope and assigns
the environment list in the current scope.
src/cmd/ksh93/sh/name.c: nv_setlist():
- Add check for readonly. This requires searching for the variable
in the main tree using nv_search() before a locally scoped one is
added using nv_open(). Since nv_search() only works with plain
variable names, temporarily end the string at '='.
src/cmd/ksh93/tests/readonly.sh:
- Add version check and fork the test command substitution subshell
on older versions that would otherwise abort the tests due to the
combination of an excessively low arithmetic recursion tolerance
and a bug that sometimes fails to restore the shell's arithmetic
recursion level.
Since f207cd57, sh_ntfork() is never called if job.jobcontrol is
set (i.e. if job control is active on an interactive shell), so the
code that is only run if job.jobcontrol is set should be removed.
src/cmd/ksh93/sh/xec.c:
- Remove spawnveg() define that is unused as of 7b0e0776.
- sh_exec(): Simplify SHOPT_SPAWN preprocessor logic. As sh_fork()
never returns a negative value, only run the parent<0 check after
running sh_ntfork() -- that check already didn't happen when
compiling ksh with SHOPT_SPAWN disabled.
- sh_ntfork(): Remove signal and terminal handling (with race
condition) that was only run with job.jobcontrol set.
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>
Using the stack makes it impossible for future buffer overflows to
occur. It also simplifies fmttoken() by eliminating the need to
declare a local buffer and pass a pointer to that as an argument.
For info: man src/lib/libast/man/stak.3
src/lib/libast/tm/tmlocale.c:
- Load the locale set by LC_TIME or LC_ALL if it hasn't been loaded
before or if it was loaded previously but isn't the current locale.
src/cmd/ksh93/tests/locale.sh:
- Add a regression test using the nl_NL.UTF-8 and ja_JP.UTF-8 locales.
Fixes: https://github.com/ksh93/ksh/issues/261
fmttoken() needs a minimal char[4] token buffer passed to it.
Originally reported by: Jakub Wilk <jwilk@jwilk.net>
Original bug report: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=879464
The following code lines from fmttoken() yield a n=3 for SYMSEMI as
n=1 from the start, e.g. 'for <>;'.
case SYMSEMI:
if(tok[0]=='<')
tok[n++] = '>';
sym = ';';
break;
default:
sym = 0;
}
tok[n++] = sym;
}
tok[n] = 0;
n[0]='<'
n[1]='>'
n[2]=';'
n[3]=0 # <-- BUFFER overflow as the passed character buffers have a size of 3
src/cmd/ksh93/sh/lex.c:
- DBUG: sh_lex(): Adjust char tokstr[3] to char tokstr[4]
- sh_syntax(): Adjust char tokbuf[3] to char tokbuf[4]
Many of these changes are minor typo fixes. The other changes
(which are mostly compiler warning fixes) are:
NEWS:
- The --globcasedetect shell option works on older Linux kernels
when used with FAT32/VFAT file systems, so remove the note about
it only working with 5.2+ kernels.
src/cmd/ksh93/COMPATIBILITY:
- Update the documentation on function scoping with an addition
from ksh93v- (this does apply to ksh93u+).
src/cmd/ksh93/edit/emacs.c:
- Check for '_AST_ksh_release', not 'AST_ksh_release'.
src/cmd/INIT/mamake.c,
src/cmd/INIT/ratz.c,
src/cmd/INIT/release.c,
src/cmd/builtin/pty.c:
- Add more uses of UNREACHABLE() and noreturn, this time for the
build system and pty.
src/cmd/builtin/pty.c,
src/cmd/builtin/array.c,
src/cmd/ksh93/sh/name.c,
src/cmd/ksh93/sh/nvtype.c,
src/cmd/ksh93/sh/suid_exec.c:
- Fix six -Wunused-variable warnings (the name.c nv_arrayptr()
fixes are also in ksh93v-).
- Remove the unused 'tableval' function to fix a -Wunused-function
warning.
src/cmd/ksh93/sh/lex.c:
- Remove unused 'SHOPT_DOS' code, which isn't enabled anywhere.
https://github.com/att/ast/issues/272#issuecomment-354363112
src/cmd/ksh93/bltins/misc.c,
src/cmd/ksh93/bltins/trap.c,
src/cmd/ksh93/bltins/typeset.c:
- Add dictionary generator function declarations for former
aliases that are now builtins (re: 1fbbeaa1, ef1621c1, 3ba4900e).
- For consistency with the rest of the codebase, use '(void)'
instead of '()' for print_cpu_times.
src/cmd/ksh93/sh/init.c,
src/lib/libast/path/pathshell.c:
- Move the otherwise unused EXE macro to pathshell() and only
search for 'sh.exe' on Windows.
src/cmd/ksh93/sh/xec.c,
src/lib/libast/include/ast.h:
- Add an empty definition for inline when compiling with C89.
This allows the timeval_to_double() function to be inlined.
src/cmd/ksh93/include/shlex.h:
- Remove the unused 'PIPESYM2' macro.
src/cmd/ksh93/tests/pty.sh:
- Add '# err_exit #' to count the regression test added in
commit 113a9392.
src/lib/libast/disc/sfdcdio.c:
- Move diordwr, dioread, diowrite and dioexcept behind
'#ifdef F_DIOINFO' to fix one -Wunused-variable warning and
multiple -Wunused-function warnings (sfdcdio() only uses these
functions when F_DIOINFO is defined).
src/lib/libast/string/fmtdev.c:
- Fix two -Wimplicit-function-declaration warnings on Linux by
including sys/sysmacros.h in fmtdev().
There's an annoying inconsistency in error messages if ksh is
compiled with SHOPT_SPAWN. One way to trigger it:
$ /usr/local/bin/ksh -c '/tmp/nonexistent'
/usr/local/bin/ksh: /tmp/nonexistent: not found
$ /usr/local/bin/ksh -c '/tmp/nonexistent; :'
/usr/local/bin/ksh: /tmp/nonexistent: not found [No such file or directory]
In the first variant, as an optimisation, ksh went straight to
exec'ing the command without forking first. In the second variant,
sh_ntfork() was used.
The first variant is done in path_exec(), path.c, line 1049:
errormsg(SH_DICT,ERROR_exit(ERROR_NOENT),e_found,arg0);
The second one is in sh_ntfork(), xec.c, line 3654:
errormsg(SH_DICT,ERROR_system(ERROR_NOENT),e_found+4);
In both cases, the e_found message is only used if errno==ENOENT,
so the extra '[No such file or directory]' message generated by
ERROR_system() is pointless as that will never change for that
message.
src/cmd/ksh93/sh/xec.c: sh_ntfork():
- Use ERROR_exit() instead of ERROR_system() for the e_found
message to avoid the superfluous addition.
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
This commit fixes BUG_CSUBSTDO, which could break stdout inside of
non-forking command substitutions. The breakage only occurred when
stdout was closed outside of the command substitution and a file
descriptor other than stdout was redirected in the command substitution
(such as stderr). Thanks to the ast-open-history repo, I was able to
identify and backport the bugfix from ksh93v- 2012-08-24.
This backport may fix other bugs as well. On 93v- 2012-08-24 it
fixed the regression below, though it was not triggered on 93u+(m).
src/cmd/ksh93/tests/heredoc.sh
487 print foo > $tmp/foofile
488 x=$( $SHELL 2> /dev/null 'read <<< $(<'"$tmp"'/foofile) 2> /dev/null;print -r "$REPLY"')
489 [[ $x == foo ]] || err_exit '<<< $(<file) not working'
src/cmd/ksh93/sh/io.c: sh_open():
- If the just-opened file descriptor exists in sftable and is
flagged with SF_STRING (as in non-forking command substitutions,
among other situations), then move the file descriptor to a
number >= 10.
src/cmd/ksh93/tests/io.sh:
- Add a regression test for BUG_CSUBSTDO, adapted from the one in
modernish.
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.
$ /usr/local/bin/ksh -c 'readonly v=1; export v'
/usr/local/bin/ksh: export: v: is read only
Every POSIX shell (even zsh, as of 5.8) allows this. So did ksh,
until the referenced commit.
src/cmd/ksh93/bltins/typeset.c: setall():
- Allow setting attributes on a readonly variable if any of
NV_ASSIGN (== NV_NOFREE), NV_EXPORT or NV_RDONLY are the only
flag bits that are set. This allows readonly, export, typeset -r,
typeset -x, and typeset -rx on variable arguments without an
assignment. Note that NV_ASSIGN is set for the first variable
argument even though it is not an assignment, so we must allow
it. The logic (or lack thereof) of that is yet to be worked out.
src/cmd/ksh93/tests/readonly.sh:
- Tests.
Resolves: https://github.com/ksh93/ksh/issues/258
This experiment, the initialisation of which was disabled with '#if
0', defines a bunch of integer type commands as special builtins.
Most are boring as they define variables just like normal integers:
pid_t, size_t, etc.
One is interesting: mode_t is a type that automatically converts
from a octal permission bits (e.g. 755) to a mode string like
u+rwx,g+rw,o+rw. That's not a compelling enough use case to
permanently define a special and immutable builtin though.
stat_t is odd: it takes a file name as an argument and fills the
variable with stat information, but it is base64 encoded binary
data and there doesn't seem to be anything that can parse it.
Anyway, none of this is going to be enabled, so we should get rid.
This is an update to one of Red Hat's patches. The strdup change is
from CentOS:
https://git.centos.org/rpms/ksh/blob/c8s/f/SOURCES/ksh-20120801-annocheck.patch
The reason why gcc (and also clang) optimize out the null check is
because the glibc string.h header gives 's' a nonnull attribute (in
other words, this is a glibc compatibility bug, not a compiler bug).
Clang gives the following informative warning when compiling strdup:
> /home/johno/GitRepos/KornShell/ksh/src/lib/libast/string/strdup.c:66:10: warning: nonnull parameter 's' will evaluate to 'true' on
> return (s && (t = oldof(0, char, n = strlen(s) + 1, 0))) ? (char*)memcpy(t, s, n) : (char*)0;
> ^ ~~
> /usr/include/string.h:172:35: note: declared 'nonnull' here
> __THROW __attribute_malloc__ __nonnull ((1));
> ^
> /usr/include/sys/cdefs.h:303:44: note: expanded from macro '__nonnull'
> # define __nonnull(params) __attribute__ ((__nonnull__ params))
The proper fix is to rename the function in strdup.c to
'_ast_strdup'. This avoids the string.h conflict and fixes the Red
Hat bug. I've also made a similar change to getopt.c, since clang
was throwing a nonnull warning there as well.
src/lib/libast/features/map.c (which generates FEATURE/map which is
indirectly included by everything) is updated to always map getopt
to _ast_getopt and strdup to _ast_strdup.
Renamed: src/cmd/INIT/cc.linux.i386 -> src/cmd/INIT/cc.linux
This ensures that architectures like ARM also use the default Linux
wrapper. This is needed because they may need -D_LARGEFILE64_SOURCE
to compile correctly.
On ARM processors, this fixes at least this regression:
io.sh[243]: long seek not working
Resolves: https://github.com/ksh93/ksh/issues/253
The typecast fix was insufficient, avoiding the crash only when
compiling with optimisation disabled. The real problem is that
put_lineno() was passed a misaligned pointer, and that the value
didn't actually contain a double but a string. The bug occurred
when restoring the LINENO value upon exiting a virtual subshell.
Thanks to Harald van Dijk for figuring out the fix.
src/cmd/ksh93/sh/subshell.c: nv_restore():
- When restoring a special variable as defined by nv_cover(),
do not pass either the np->nvflag bits or NV_NOFREE. Why?
* The np->nvflag bits are not needed. They are also harmful
because they may include the NV_INTEGER bit. This is set
when the value is numeric. However, nv_getval() always
returns the value in string form, converting it if it is
numeric. So the NV_INTEGER flag should never be passed
to nv_putval() when it uses the result of nv_getval().
* According to nval.3, the NV_NOFREE flag stops nv_putval() from
creating a copy of the value. But this should be unnecessary
because the earlier _nv_unset(mp,NV_RDONLY|NV_CLONE) should
ensure there is no previous value. In addition, the NV_NOFREE
flag triggered another bug that caused the value of SECONDS to
be corrupted upon restoring it when exiting a virtual subshell.
- When restoring a regular variable, copy the entire nvalue union
and not just the 'cp' member. In practice this worked because
no current member of the nvalue union is larger than a pointer.
However, there is no guarantee it will stay that way.
src/cmd/ksh93/tests/leaks.sh:
- Add disabled test for a memory leak that was discovered in the
course of dealing with this bug. The fix doesn't introduce or
influence it. It will have to be dealt with later.
src/cmd/ksh93/tests/locale.sh:
- Add test for restoring locale on leaving virtual subshell.
https://github.com/ksh93/ksh/issues/253#issuecomment-815290154
src/cmd/ksh93/tests/variables.sh:
- Test against corruption of SECONDS on leaving virtual subshell.
https://github.com/ksh93/ksh/issues/253#issuecomment-815191052
Co-authored-by: Harald van Dijk <harald@gigawatt.nl>
Progresses: https://github.com/ksh93/ksh/issues/253
On Ubuntu arm7, two variables.sh regression tests crashed with a
bus error (SIGBUS) in init.c on line 720 while testing $LINENO:
707 static void put_lineno(Namval_t* np,const char *val,int flags,Namfun_t *fp)
708 {
709 register long n;
710 Shell_t *shp = sh_getinterp();
711 if(!val)
712 {
713 fp = nv_stack(np, NIL(Namfun_t*));
714 if(fp && !fp->nofree)
715 free((void*)fp);
716 _nv_unset(np,NV_RDONLY);
717 return;
718 }
719 if(flags&NV_INTEGER)
720 n = *(double*)val;
721 else
722 n = sh_arith(shp,val);
723 shp->st.firstline += nget_lineno(np,fp)+1-n;
724 }
Apparently, gcc on arm7 doesn't like the implicit typecast from
double to long.
Those three $LINENO discipline functions are generally a mess of
implicit typecasts between Sfdouble_t, double, long and int.
Line numbers are internally stored as int. The discipline functions
need to use Sfdouble_t for API compatibility.
src/cmd/ksh93/sh/init.c: nget_lineno(), put_lineno(), get_lineno():
- Get rid of unnecessary implicit typecasts by adjusting the types
of local variables.
- Make the typecasts that are done explicit.
Progresses: https://github.com/ksh93/ksh/issues/253
On some systems (such as Ubuntu on ARM), the output of `file`
contains a build hash, such as:
SomeExecutable: ELF 32-bit LSB shared object, ARM, EABI5
version 1 (SYSV), dynamically linked, interpreter
/lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0,
BuildID[sha1]=8934dd61657aac875c190535066466849687a56b,
not stripped
This build hash can contain the string '64', which caused package
to wrongly detect a 64-bit architecture.
bin/package, src/cmd/INIT/package.sh:
- Export LC_ALL=C to ensure 'file' output in English.
- To detect a 64-bit architecture, require the string "64-bit", "64
bit" or "64bit" in 'file' output. The letters 'i' and 't' cannot
occur in a hexadecimal hash, so hopefully that is safe enough. It
is impossible to make this method completely safe, so in the long
term it should be replaced.
Progresses: https://github.com/ksh93/ksh/issues/253
These fixes are applied rather blindly as no one has yet managed to
understand the almost entirely uncommented arrays and variables
handling code (arrays.c, name.c, nvdisc.c, nvtree.c, nvtype.c).
Hopefully we'll figure all that out at some point. In the meantime
these backported fixes appear to work fine, and these bugs impact
the usability of 'enum', so I'm just going to have to violate my
own policy and backport these fixes without understanding them.
Thanks to @JohnoKing for putting in a lot of work tracing these.
Further discussion at: https://github.com/ksh93/ksh/issues/87
src/cmd/ksh93/sh/array.c:
- nv_arraysettype():
* Further simplify the function. After my initial simplification
of it (re: 5491fe97), I don't believe there's actually a need
to save a duplicate copy of the value. Use the pointer returned
by nv_getval() directly to restore the value.
* Cope with a null value (nv_getval() returning a NULL pointer).
This is needed for compatibility with the backported fix in
nvtype.c (below).
- array_putval(): If the array's value pointer (up->cp) is a
pointer to the empty string, it is set to NULL before calling
nv_putv() to prevent an empty string from being deleted. Backport
a fix from 93v- that restores the pointer to the empty string if
the NV_NOFREE attribute is set. Removing it somehow causes these
regressions:
enum.sh[86]: ${array[@]} doesn't yield all values for
associative enum arrays (expected 'green blue blue red
yellow green red orange'; got 'green blue blue yellow
green orange')
enum.sh[94]: unsetting associative enum array does not work
(got 'Color_t -A Colors=([foo]=red [rood]=red)')
enum.sh[116]: assigning first enum element to indexed array
failed (expected 'red red'; got 'BUG BUG')
- nv_associative(): Do not increase the 'nelem' (number of
elements) value of the array's 'header' struct if the array is
associative and of an enum type. The original 93v- fix only
checked for the NV_INTEGER attribute, but backporting that caused
several regressions. Using a debug output command I've determined
that the exact value of 'type' is somehow consistently set to
0x26 if the array is associative and of an enum type, which is
NV_INTEGER | NV_LTOU | NV_RJUST as defined in include/nval.h. I
cannot find where/how that value is determined. In any case this
fix, based on but more specific than the 93v- one, appears to
work fine. Removing it somehow causes this regression:
enum.sh[94]: unsetting associative enum array does not work
(got 'Color_t -A Colors=()')
src/cmd/ksh93/sh/nvtype.c: nv_settype():
- Another fix backported from 93v-. If the variable is an array,
also set the type of element 0 of that array using a call to
nv_arraysettype(). The value may be null. Removing this somehow
causes this regression:
enum.sh[94]: unsetting associative enum array does not work
(got 'Color_t -A Colors=()')
src/cmd/ksh93/tests/enum.sh:
- Add tests for all the bugs fixed here, plus some hypothetical
bugs (e.g., do the same tests for indexed enum type arrays as for
associative enum type arrays, even though indexed enum type
arrays didn't have all the same problems).
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
Resolves: https://github.com/ksh93/ksh/issues/87
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:
https://github.com/ksh93/ast-open-history/commit/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
To set a window title in bash and zsh, the $PS1 prompt can be set
with the title placed between $'\E]0;' and $'\a':
set -o emacs # Or vi mode
typeset -A fmt=(
[start_title]=$'\E]0;'
[end_title]=$'\a'
)
PS1="${fmt[start_title]}$(hostname): $(uname)${fmt[end_title]}\$ "
This also works in ksh unless the shell receives SIGWINCH. With a
$PS1 that sets a window title, the prompt breaks until two
interrupts are received. This is caused by ed_setup() skipping
$'\a' (the bell character) when setting up the e_prompt buffer
which is an edited version of the final line of the PS1 prompt for
use when redrawing the command line.
One fix would be to avoid cutting out the bell character. But if
the prompt contains a bell, we only want the terminal to beep when
a new prompt is printed, and not upon refreshing the command line,
e.g. when receiving SIGWINCH or pressing Ctrl+L.
To avoid the problem, this commit adds code that cuts out sequences
of the form ESC ] <number> ; <text> BELL from the prompt redraw
buffer altogether. They are not needed there because these
sequences will already have taken effect when the full prompt was
printed by io_prompt().
This commit also adds a tweak that should improve the recognition
of other escape sequences to count their length.
src/cmd/ksh93/edit/edit.c: ed_setup():
- When preparing the e_prompt buffer, cut out dtterm/xterm
Operating System Commands that set window/icon title, etc.
See: https://invisible-island.net/xterm/ctlseqs/ctlseqs.html
- When counting the length of escape sequences in that part of PS1,
try to recognize some more types of sequences. These changes are
part of a ksh2020 patch: https://github.com/att/ast/issues/399
src/cmd/ksh93/sh.1:
- Document that any '!' in escape sequences in the PS1 prompt needs
to be changed to '!!'. To avoid breaking compatibility, this
requirement is documented instead of backporting the changes to
io_prompt() from https://github.com/att/ast/issues/399 which try
to remove that requirement for specific escape sequences.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
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 adds an UNREACHABLE() macro that expands to either the
__builtin_unreachable() compiler builtin (for release builds) or
abort(3) (for development builds). This is used to mark code paths
that are never to be reached.
It also adds the 'noreturn' attribute to functions that never
return: path_exec(), sh_done() and sh_syntax(). The UNREACHABLE()
macro is not added after calling these.
The purpose of these is:
* to slightly improve GCC/Clang compiler optimizations;
* to fix a few compiler warnings;
* to add code clarity.
Changes of note:
src/cmd/ksh93/sh/io.c: outexcept():
- Avoid using __builtin_unreachable() here since errormsg can
return despite using ERROR_system(1), as shp->jmplist->mode is
temporarily set to 0. See: https://github.com/att/ast/issues/1336
src/cmd/ksh93/tests/io.sh:
- Add a regression test for the ksh2020 bug referenced above.
src/lib/libast/features/common:
- Detect the existence of either the C11 stdnoreturn.h header or
the GCC noreturn attribute, preferring the former when available.
- Test for the existence of __builtin_unreachable(). Use it for
release builds. On development builds, use abort() instead, which
crahses reliably for debugging when unreachable code is reached.
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 commit adds '/* FALLTHROUGH */' comments to fix many
GCC warnings when compiling with -Wimplicit-fallthrough.
Additionally, the existing fallthrough comments have been
changed for consistency.
src/cmd/ksh93/tests/variables.sh: LC_* error tests:
- Since operating systems validate locale strings differently,
try a few different bad locale strings to find one that makes
setlocale(2) fail, fixing test failures on OpenBSD and Debian.
- Restore warning removed in aed5c6d7, issuing it if none of the
bad locale strings produce a diagnostic.
- Reenable test for diagnostic message disabled in aed5c6d7.
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.
src/cmd/ksh93/bltins/test.c:
- Fix the following compiler warnings from clang:
test.c:554:11: warning: assigning to 'char *' from 'const char []'
discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
e_msg = e_badop;
^ ~~~~~~~
test.c:556:11: warning: assigning to 'char *' from 'const char []'
discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]
e_msg = e_unsupported_op;
^ ~~~~~~~~~~~~~~~~
test.c:560:1: warning: control may reach end of non-void function
[-Wreturn-type]
src/cmd/ksh93/tests/builtins.sh:
- Fix regression test by updating error message text.
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>
src/lib/libast/tm/tminit.c:
- Commit 9f43f8d1, in addition to backporting fixes from ksh93v-, also
backported this bug:
$ printf '%(%Z)T' now
PPT # Should be PDT
Reapply the ksh2020 bugfix to fix the %Z time
format again.
src/cmd/ksh93/tests/builtins.sh:
- Add a regression test so this bug (hopefully) isn't backported from
ksh93v- again).
Every so often, a commit's GitHub CI run throws the following
regression test failure:
sigchld.sh[57]: expected '2 background' -- got '3' (DELAY=0.02)
When I re-run the job, the failure usually goes away.
In 712261c8 the DELAY variable was changed from 0.2 to 0.02 to
speed up the first SIGCHLD test. It's possible the GitHub CI
runners are just too slow or too heavily loaded for that.
src/cmd/ksh93/tests/sigchld.sh:
- Restore 0.2 value for 'float DELAY'.
I grepped for #include changes in all the commits and compared
that to the changes in the Mamfiles. I found 7 commits that don't
update the Mamfiles with the appropriate dependencies while
adding #includes, as I only learned how this works after having
worked with this code for some time.
This commit adds the missing Mamfile updates for the
corresponding #include changes in the following commits:
06e721c3, 65d363fd, 70fc1da7, 79d19458, b1a41311, bb4d6a2e,
db71b3ad, and this commit.
Additionally:
src/lib/libast/comp/setlocale.c:
- Change include errno.h to error.h to use EILSEQ fallback if
needed; remove corresponding #ifdef (re: 4dcf5c50, 71bfe028).
src/cmd/ksh93/Mamfile:
- Fix a broken dependency on libast FEATURE/float (re: 72968eae).
We can't use 'prev' for a file that was not mentioned before in
the same Mamfile, we have to use a 'make'...'done' on the first
mention. Add subdependencies matching those in libast/Mamfile.
src/cmd/ksh93/bltins/print.c:
- Rename the unlisted and misleadingly named SHOPT_ECHOE option
(which disables, not enables, 'echo -e') to SHOPT_NOECHOE.
src/cmd/ksh93/SHOPT.sh:
- Add the SHOPT_NOECHOE and SHOPT_TEST_L compile time options to
the list of SHOPT options. Since there is a probe for TEST_L,
set it to probe (empty) by default. NOECHE is off by default.
src/cmd/ksh93/features/options:
- Small bugfix: Allow SHOPT_TEST_L to be manually enabled on
systems that don't support '$(whence -p test) -l /foo'.
- Add a comment describing the SHOPT_MULTIBYTE feature test and
separate it from the SHOPT_DEVFD test.
This bugfix comes from <https://github.com/att/ast/pull/711>.
Eric Scrivner provided the following explanation for the fix:
> Coverity identified an issue with integer truncation in
> `put_enum`. The function was truncating the return values of
> `strcasecmp` and `strcmp` from an `int` to an `unsigned short`
> when assigning them to the local variable `n`. Since either of
> these methods can return a value that is not in the set `{0, 1,
> -1}` the later check if `n == 0` could spuriously evaluate to
> true. For example, in the case where either function returned
> `-65536`.
> The fix is simply to change `n` from an `unsigned short` to an
> `int` to avoid the possibility of truncation. Since the only
> purpose of `n` is the store the return values of these checks,
> this does not have any side effects.
That bit of code supported bash's redundant 'function foo()'
function declaration syntax (with both the 'function' keyword
and the '()') which is a syntax error on ksh, as it should be.
This allows ksh to be compiled with versions of tcc that define
__dso_handle in libtcc1.a, i.e., versions as of this commit:
https://repo.or.cz/tinycc.git/commit/dd60b20c
Older versions of tcc still fail to compile ksh, although now they
fail after reaching the libdll feature test. I'm not sure if fixing
that is feasible since even if I hack out the failing libdll
feature test, ksh fails to link with a '__dso_handle' error.
src/lib/libast/comp/atexit.c,
src/lib/libast/features/lib,
src/lib/libast/vmalloc/vmexit.c:
- From what I've been able to gather the only OSes with support
for on_exit are Linux and SunOS 4. However, on_exit takes two
arguments, so the macro that defines it as taking one argument
is incorrect. Since Solaris (SunOS 5) no longer has this call
and the macro breaks on Linux, the clean fix is to remove it
(atexit(3) is used instead).
src/lib/libast/include/ast.h:
- When compiling with tcc on FreeBSD, pretend to be gcc 2.95.3
instead of gcc 9.3.0. This stops /usr/include/math.h from
activating gcc 3.0+ math compiler builtins that don't exist on
tcc, while still identifying as gcc which is needed to avoid
other FreeBSD system header breakage.
src/cmd/builtin/Mamfile,
src/cmd/builtin/features/pty,
src/lib/libdll/Mamfile,
src/lib/libdll/features/dll:
- tcc forbids combining the -c compiler flag with -l* linker flags.
Use the -lm flag in the iffe feature tests instead of the
Mamfiles. This avoids iffe combining -lm with the -c flag.
src/lib/libast/vmalloc/malloc.c:
- Fix failure to compile with -D_std_malloc.
This patch is from OpenSUSE:
https://build.opensuse.org/package/view_file/shells/ksh/ksh93-malloc-hook.dif
As it turns out tcc needs this change to build ksh with
-D_std_malloc, so it has been applied.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
Resolves: https://github.com/ksh93/ksh/issues/232
src/lib/libast/features/lib,
src/lib/libast/path/pathicase.c:
- FAT32 file systems on Linux don't support FS_CASEFOLD_FL, which
caused globbing to break. Reproducer using a UEFI boot partition:
$ echo /boot/eF*
/boot/eF*
This is fixed by checking for FAT attributes with ioctl, then
checking for FS_CASEFOLD_FL if that fails.
- The check for FS_CASEFOLD_FL didn't work correctly; I still wasn't
able to get --globcasedetect to work on a case-insensitive ext4
folder. Fix that by adding missing parentheses.
Moving the 'err_exit' and 'warning' alias definitions in the
regression tests to one _common file introduced a bug: they are no
longer expanded at compile time when the tests are run with shcomp,
resulting in a 'command not found' (at best) on trying to execute
one. shcomp requires that the alias definitions need to be present
in the file itself. But that means maintaining 50-odd copies again.
I'd rather add a hack to shtests to avoid this.
src/cmd/ksh93/tests/shtests:
- Before running a test with shcomp, physically concatenate _common
and the test script together into a temporary file, minus the '.'
command that includes _common, and compile that with shcomp.
The NOT_USED() macro is already defined in ast.h (which is included
by shell.h) as an alias of NoP(). So it's better to apply the fix
to NoP() so it takes effect for both verrsions, for libast and ksh.
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.
In various places in libast and libcmd there are preprocessor
fallbacks like this, for systems that don't define all the commonly
used errno value IDs:
#ifndef ENOSYS
#define ENOSYS EINVAL
#endif
and many others. It is better to have these all in one place so
they are not duplicated and we don't risk inconsistencies when
adding new code.
src/lib/libast/include/error.h includes the OS's <errno.h>, so it
is the logical file to move all these fallbacks into.
Quite possibly there is no remotely current system that needs any
of these, but they won't do any harm either.
Most files already use <error.h> directly or indirectly. Four
needed new #include <error.h> directives to use the fallbacks if
needed. The libast Mamfile is updated to make those files depend on
that header.
These are minor fixes I've accumulated over time. The following
changes are somewhat notable:
- Added a missing entry for 'typeset -s' to the man page.
- Add strftime(3) to the 'see also' section. This and the date(1)
addition are meant to add onto the documentation for 'printf %T'.
- Removed the man page the entry for ksh reading $PWD/.profile on
login. That feature was removed in commit aa7713c2.
- Added date(1) to the 'see also' section of the man page.
- Note that the 'hash' command can be used instead of 'alias -t' to
workaround one of the caveats listed in the man page.
- Use an 'out of memory' error message rather than 'out of space'
when memory allocation fails.
- Replaced backticks with quotes in some places for consistency.
- Added missing documentation for the %P date format.
- Added missing documentation for the printf %Q and %p formats
(backported from ksh2020: https://github.com/att/ast/pull/1032).
- The comments that show each builtin's options have been updated.
In 2021, it seems like it's about time to join the 21st century
and officially require fork(2). In practice this was already the
case as the legacy code was unmaintained and didn't compile.
src/cmd/ksh93/edit/edit.c,
src/cmd/ksh93/edit/history.c,
src/cmd/ksh93/sh/deparse.c:
- Remove experimental code protected by '#ifdef future'.
No one is going to do anything with this, it's just clutter.
src/lib/libast/sfio/sfcvt.c:
- In 2021, it might be time to actually start using some C99
features were available. Change two checks for a _c99_in_the_wild
macro to actual checks for C99, enabling the use of fpclassify().
Resolves: https://github.com/ksh93/ksh/issues/219
This removes #ifdefs checking for the existence of
SH_PLUGIN_VERSION (version check for dynamically loaded builtins)
and the SFIO identifiers SF_BUFCONST, SF_CLOSING, SF_APPENDWR,
SF_ATEXIT, all of which are defined by the bundled libast.
While experimenting with #233, a memory segmentation fault occurred.
A search of other emacs issues found a potential matching issue as
described in https://github.com/att/ast/pull/791. Also, a duplicate
PR of https://github.com/att/ast/pull/1489 was submitted. This
commit backports that fix.
src/cmd/ksh93/edit/history.c: hist_word():
- Switch from using strcpy to memmove as the two strings could overlap.
This was failing again on FreeBSD. Replicating the test in a real
session worked as expected.
Apparently, we just cannot rely on external 'vi' utilities playing
well with pty. This test has caused enough trouble. Removed.
I've had ksh crash one too many times when returning to a previous
build directory as I forgot to restore the previously-used CCFLAGS.
bin/package, src/cmd/INIT/package.sh:
- Save each of CC, CCFLAGS, CCLDFLAGS, LDFLAGS, KSH_RELFLAGS on the
first build run. On subsequent runs, compare and refuse to run if
they changed, issuing an informative error message.
- Allow override by exporting FORCE_FLAGS. Don't tell anyone :)
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
Until now, when performing any tilde expansion like ~/foo or
~user/foo, ksh added a placeholder built-in command called
'.sh.tilde', ostensibly with the intention to allow users to
override it with a shell function or custom builtin. The multishell
ksh93 repo <https://github.com/multishell/ksh93/> shows this was
added sometime between 2002-06-28 and 2004-02-29. However, it has
never worked and crashed the shell.
This commit replaces that with something that works. Specific tilde
expansions can now be overridden using .set or .get discipline
functions associated with the .sh.tilde variable (see manual,
Discipline Functions).
For example, you can use either of:
.sh.tilde.set()
{
case ${.sh.value} in
'~tmp') .sh.value=${XDG_RUNTIME_DIR:-${TMPDIR:-/tmp}} ;;
'~doc') .sh.value=~/Documents ;;
'~ksh') .sh.value=/usr/local/src/ksh93/ksh ;;
esac
}
.sh.tilde.get()
{
case ${.sh.tilde} in
'~tmp') .sh.value=${XDG_RUNTIME_DIR:-${TMPDIR:-/tmp}} ;;
'~doc') .sh.value=~/Documents ;;
'~ksh') .sh.value=/usr/local/src/ksh93/ksh ;;
esac
}
src/cmd/ksh93/include/variables.h,
src/cmd/ksh93/data/variables.c:
- Add SH_TILDENOD for a new ${.sh.tilde} predefined variable.
It is initially unset.
src/cmd/ksh93/sh/macro.c:
- sh_btilde(): Removed.
- tilde_expand2(): Rewritten. I started out with the tiny version
of this function from the 2002-06-28 version of ksh. It uses the
stack instead of sfio, which is more efficient. A bugfix for
$HOME == '/' was retrofitted so that ~/foo does not become
//foo instead of /foo. The rest is entirely new code.
To implement the override functionality, it now checks if
${.sh.tilde} has any discipline function associated with it.
If it does, it assigns the tilde expression to ${.sh.tilde} using
nv_putval(), triggering the .set discipline, and then reads it
back using nv_getval(), triggering the .get discipline. The
resulting value is used if it is nonempty and does not still
start with a tilde.
src/cmd/ksh93/bltins/typeset.c,
src/cmd/ksh93/tests/builtins.sh:
- Since ksh no longer adds a dummy '.sh.tilde' builtin, remove the
ad-hoc hack that suppressed it from the output of 'builtin'.
src/cmd/ksh93/tests/tilde.sh:
- Add tests verifying everything I can think of, as well as tests
for bugs found and fixed during this rewrite.
src/cmd/ksh93/tests/pty.sh:
- Add test verifying that the .sh.tilde.set() discipline does not
modify the exit status value ($?) when performing tilde expansion
as part of tab completion.
src/cmd/ksh93/sh.1:
- Instead of "tilde substitution", call the basic mechanism "tilde
expansion", which is the term used everywhere else (including the
1995 Bolsky/Korn ksh book).
- Document the new override feature.
Resolves: https://github.com/ksh93/ksh/issues/217
That patch broke the build on Cygwin, where gcc apparently doesn't
have the required atomic addition/subtraction compiler builtins.
The build fails at link time with those functions not found.
As far as I know, ksh was actually working fine (after @JohnoKing's
gcc workaround in c258a04f), so I'll just revert this for now. If a
need for it is demonstrated later, we'll have to add a feature test
or find some other way to get it working on Cygwin.
The package script was not well behaved with these. When you
pressed Ctrl+C, on some shells (including ksh) both the SIGINT (2)
and EXIT (0) traps are activated, showing a double 'make done'
message. The exit status also wasn't > 128 to indicate a signal.
bin/package, src/cmd/INIT/package.sh:
- Be UNIXly well-behaved. Signals should be passed on after
handling, so when one is caught, make the trap handlers print
their message and then unset both itself and EXIT/0 before
resending the signal to self.
"savxit -= SH_EXITSIG + 128;" may have worked accidentally due to
subsequent bitmasking, but is blatantly wrong . It subtracts 256 +
128 = 384 from the exit status.
Use bitwise logic instead, with an octal literal 0200 instead of
128. This makes more sense in this context.
In ksh93r a crash can occur after switching from emacs mode to vi
mode[*]:
$ ENV=/./dev/null ksh2006 -o emacs
$ echo ${.sh.version}
Version M 1993-12-28 r
$ set -o vi
$ <Esc> <r> <r> # This triggers the memory fault
Commit 129614b9 added the OpenSUSE patch for this crash. This commit
adds the regression test for it.
[*]: https://bugzilla.opensuse.org/show_bug.cgi?id=179917
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>
Sometimes the shell returned to the prompt before bin/package was
finished writing all of its output. The problem was that 'tee',
which is used to write the output to both the terminal and the log
in arch/*lib/package/gen/make.out, hadn't caught up yet.
bin/package, src/cmd/INIT/package.sh:
- Run the build itself in the background and 'tee' in the
foreground. This way, the script will not terminate until 'tee'
is finished. The build's exit status is obtained with 'wait'.
For most numeric types the last provided one wins out. This commit
closes the gap for -F and -i numerics to not be covered up by other
preceding float types. Note: -u for requesting an unsigned float or
integer was considered and decided to be left alone as it stands,
so as to not allow the variable to become an uppercased string if
the requested options ended with a -u. As it stands for a case when
multiple numeric types are requested, a -u option may be applied
after the last numeric type is processed.
Examples:
-EF becomes -F
-Fi becomes -i
-Fu becomes -F
-uF becomes -F
-Fui becomes -i (because isfloat==1, unsigned is not applied)
-Fiu becomes -iu (isfloat is reset and allows unsigned to be set)
src/cmd/ksh93/bltins/typeset.c: b_typeset():
- Reset attribute bit flags for -E and -X when -F is requested by
adding in NV_EXPNOTE to be removed.
- For -i option if a float precedes it, reset isfloat and -E/-F
attribute bit flags.
- Take into account the impact of the shortint flag on floats.
src/cmd/ksh93/tests/attributes.sh:
- Add some validation tests to confirm that, when a -F follows
either -E or -X, -F is used.
- Add some validation tests to confirm that, when -F/E/X precede
a -i, the variable becomes an integer and not a float.
- Add in various tests when -s followed a float.
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/1396https://github.com/att/ast/commit/070d365d
- Add test.
src/cmd/ksh93/COMPATIBILITY:
- Note this change.
src/cmd/ksh93/tests/_common:
- Commit aed5c6d7 renamed the err_exit function,
breaking a few tests in glob.sh that call the function
directly instead of using the alias. Restore the function.
src/cmd/ksh93/tests/builtins.sh:
- The dtksh builtins don't have optget option parsing, so
skip the unrecognized options test for those (this of
course only has relevance when running dtksh against the
regression tests).
src/cmd/ksh93/tests/pty.sh:
- If the vi editor couldn't be found on the $PATH, skip the
regression test that involves it.
src/cmd/ksh93/features/options:
- SHOPT_TEST_L: Use 'env test' instead of '/bin/test' to run
external 'test', as the direct path is unportable. Create a test
symlink and verify the positive case as well as the negative.
- SHOPT_SYSRC: Use if...then..fi instead of ... && ... for the last
test to avoid a non-zero exit status of the script, which outputs
a spurious 'no' result like this:
iffe: test: cross{ ... }end ... no
- Add comments for clarity and to make the SHOPT_* names greppable.
Related: https://github.com/ksh93/ksh/issues/219
src/cmd/ksh93/tests/_common:
- Added. This keeps one common version of 'err_exit', 'warning',
and other init code.
src/cmd/ksh93/tests/*.sh:
- Source _common as a dot script.
- Remove 50-odd, occasionally slightly different, versions of the
common code.
- Some minor tweaks.
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
src/cmd/ksh93/edit/history.c:
- Call sh_close() and sh_fcntl() instead of close(2) and fcntl(2),
updating the shell's file descriptor state.
- Mark files close-on-exec on opening them. The history file should
not remain open if ksh execs another process.
- Another fix for an FD check: < 10 instead of < 2.
src/cmd/ksh93/tests/arrays.sh,
src/cmd/ksh93/tests/arrays2.sh:
- Backport some regression tests from ksh93v- for associative
arrays.
src/cmd/ksh93/tests/basic.sh:
- Add ksh93v- regression tests for background process output in
backtick and shared-state command substitutions as well as
functions used in command substitutions.
- Add regression tests for using EXIT traps in subshells. In
ksh93v- and ksh2020 EXIT traps don't work in forked subshells:
https://github.com/att/ast/issues/1452
- The trap builtin shouldn't segfault after receiving an invalid
signal name. ksh2020 regression:
https://github.com/att/ast/issues/1403
- Add a test to make sure invalid flags don't crash ksh.
ksh2020 regression: https://github.com/att/ast/issues/1284
- Test for an illegal seek error when using the 'join' command with
process substitutions. ksh93v- regression:
https://www.mail-archive.com/ast-users@lists.research.att.com/msg00816.html
src/cmd/ksh93/tests/bracket.sh:
- Add some regression tests from ksh93v- for the -eq test operator.
src/cmd/ksh93/tests/builtins.sh:
- Move the regression test for 'exit' in an interactive shell to
the exit.sh script.
- Test for assignments preceding the command builtin persisting
after an error. ksh2020 regression:
https://github.com/att/ast/issues/1402
- The chmod builtin should modify the permissions of all files
passed to it. ksh2020 regression:
https://github.com/att/ast/issues/949
- Add regression tests for the cd builtin. In ksh93v- 2013-10-10
alpha, using cd on a directory without an execute bit doesn't
cause an error. The test for using cd on a normal file was
backported from ksh93v-.
- Backport a ksh93v- regression test for the exit status
from 'kill %'.
src/cmd/ksh93/tests/case.sh:
- Test for a segfault when ksh handles an invalid character class
in a pattern. ksh2020 regression:
https://github.com/att/ast/issues/1409
src/cmd/ksh93/tests/exit.sh:
- Add regression tests from ksh2020 for the 'exit' builtin:
https://github.com/att/ast/commit/d9491d46
src/cmd/ksh93/tests/io.sh:
- Add a regression test from ksh93v- for a process substitution
hang. This test fails in the 93v- 2013 alpha but succeeds in
the 2014 beta.
src/cmd/ksh93/tests/math.sh:
- 'typeset -s foo=30000' adds garbage to $foo in ksh93u+, ksh93v-
and ksh2020:
$ typeset -s foo=30000
$ echo $foo
5#1430000
This bug was fixed in commit 88a6baa1, but that commit didn't
add a regression test for it.
src/cmd/ksh93/tests/variables.sh:
- Add a regression test for $PS4 incorrectly unsetting
${.sh.subshell}: https://github.com/att/ast/issues/1092
This is an attempt to avoid fairly rare intermittent failures
on the GitHub CI runners. Apparently, they are sometimes so
slow that typeahead can still interfere with a test.
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>
This commit fixes two interrelated problems.
1. The -v unary test/[/[[ operator is documented to test if a
variable is set. However, it always returns true for variable
names with a numeric attribute, even if the variable has not
been given a value. Reproducer:
$ ksh -o nounset -c 'typeset -i n; [[ -v n ]] && echo $n'
ksh: n: parameter not set
That is clearly wrong; 'echo $n' should never be reached and the
error should not occur, and does not occur on mksh or bash.
2. Fixing the previous problem revealed serious breakage in short
integer type variables that was being masked. After applying
that fix and then executing 'typeset -si var=0':
- The conditional assignment expansions ${var=123} and
${var:=123} assigned 123 to var, even though it was set to 0.
- The expansions ${var+s} and ${var:+n} incorrectly acted as if
the variable was unset and empty, respectively.
- '[[ -v var ]]' and 'test -v var' incorrectly returned false.
The problems were caused by a different storage method for short
ints. Their values were stored directly in the 'union Value'
member of the Namval_t struct, instead of allocated on the stack
and referred to by a pointer, as regular integers and all other
types do. This inherently broke nv_isnull() as this leaves no
way to distinguish between a zero value and no value at all.
(I'm also pretty sure it's undefined behaviour in C to check for
a null pointer at the address where a short int is stored.)
The fix is to store short ints like other variables and refer
to them by pointers. The NV_INT16P combined bit mask already
existed for this, but nv_putval() did not yet support it.
src/cmd/ksh93/bltins/test.c: test_unop():
- Fix problem 1. For -v, only check nv_isnull() and do not check
for the NV_INTEGER attribute (which, by the way, is also used
for float variables by combining it with other bits).
See also 5aba0c72 where we recently fixed nv_isnull() to
work properly for all variable types including short ints.
src/cmd/ksh93/sh/name.c: nv_putval():
- Fix problem 2, part 1. Add support for NV_INT16P. The code is
simply copied and adapted from the code for regular integers, a
few lines further on. The regular NV_SHORT code is kept as this
is still used for some special variables like ${.sh.level}.
src/cmd/ksh93/bltins/typeset.c: b_typeset():
- Fix problem 2, part 2. Use NV_INT16P instead of NV_SHORT.
src/cmd/ksh93/tests/attributes.sh:
- Add set/unset/empty/nonempty tests for all numeric types.
src/cmd/ksh93/tests/bracket.sh,
src/cmd/ksh93/tests/comvar.sh:
- Update a couple of existing tests.
- Add test for [[ -v var ]] and [[ -n ${var+s} ]] on unset
and empty variables with many attributes.
src/cmd/ksh93/COMPATIBILITY:
- Add a note detailing the change to test -v.
src/cmd/ksh93/data/builtins.c,
src/cmd/ksh93/sh.1:
- Correct 'typeset -C' documentation. Variables declared as
compound are *not* initially unset, but initially have the empty
compound value. 'typeset' outputs them as:
typeset -C foo=()
and not:
typeset -C foo
and nv_isnull() is never true for them. This may or may not
technically be a bug. I don't think it's worth changing, but
it should at least be documented correctly.
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
Removing the nv_putval() calls also stopped making sure the
NV_NOFREE attribute was set for those variables, causing an invalid
free later on. This caused the funcname.ksh script:
https://gist.github.com/ormaaj/12874b68acd06ee98b59
to crash even more readily than it did before.
Even after this commit there are various crashing bugs left for
that script, all intermittent and with different backtraces and
dependent on the operating system and malloc variant used.
Investigation ongoing at: https://github.com/ksh93/ksh/issues/212
This commit fixes at least three bugs:
1. When issuing 'typeset -p' for unset variables typeset as short
integer, a value of 0 was incorrectly diplayed.
2. ${x=y} and ${x:=y} were still broken for short integer types
(re: 9f2389ed). ${x+set} and ${x:+nonempty} were also broken.
3. A memory fault could occur if typeset -l followed a -s option
with integers. Additonally, now the last -s/-l wins out as the
option to utilize instead of it always being short.
src/cmd/ksh93/include/name.h:
- Fix the nv_isnull() macro by removing the direct exclusion of
short integers from this set/unset test. This breaks few things
(only ${.sh.subshell} and ${.sh.level}, as far as we can tell)
while potentially correcting many aspects of short integer use
(at least bugs 1 and 2 above), as this macro is widely used.
- union Value: add new pid_t *pidp pointer member for PID values
(see further below).
src/cmd/ksh93/bltins/typeset.c: b_typeset():
- To fix bug 3 above, unset the 'shortint' flag and NV_SHORT
attribute bit upon encountering the -l optiobn.
*** To fix ${.sh.subshell} to work with the new nv_isnull():
src/cmd/ksh93/sh/defs.h:
- Add new 'realsubshell' member to the shgd (aka shp->gd) struct
which will be the integer value for ${.sh.subshell}.
src/cmd/ksh93/sh/init.c,
src/cmd/ksh93/data/variables.c:
- Initialize SH_SUBSHELLNOD as a pointer to shgd->realsubshell
instead of using a short value (.s) directly. Using a pointer
allows nv_isnull() to return a positive for ${.sh.subshell} as
a non-null pointer is what it checks for.
- While we're at it, initialize PPIDNOD ($PPID) and SH_PIDNOD
(${.sh.pid}) using the new pdip union member, which is more
correct as they are values of type pid_t.
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Update the ${.sh.subshell} increases/decreases to refer to
shgd->realsubshell (a.k.a. shp->gd->realsubshell).
*** To fix ${.sh.level} after changing nv_isnull():
src/cmd/ksh93/sh/macro.c: varsub():
- Add a specific exception for SH_LEVLNOD to the nv_isnull() test,
so that ${.sh.level} is always considered to be set. Its handling
throughout the code is too complex/special for a simple fix, so
we have to special-case it, at least for now.
*** Regression test additions:
src/cmd/ksh93/tests/attributes.sh:
- Add in missing short integer tests and correct the one that
existed. The -si test now yields 'typeset -x -r -s -i foo'
instead of 'typeset -x -r -s -i foo=0' which brings it in line
with all the others.
- Add in some other -l attribute tests for floats. Note, -lX test
was not added as the size of long double is platform dependent.
src/cmd/ksh93/tests/variables.sh:
- Add tests for ${x=y} and ${x:=y} used on short int variables.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
Symptoms of this bug below. These only seem to occur on Linux and
only if you replace your initial login shell by ksh using 'exec'.
1. An erroneous 'Interrupt' message is printed after stopping the
read builtin in a script. Reproducer:
$ exec arch/*/bin/ksh
$ cat ./reproducer.sh
#!/bin/sh
read foo
$ ./reproducer.sh
^C$ <Enter>
[1] + Interrupt ../reproducer.sh
2. Ctrl+C fails to stop /bin/package make. Reproducer:
$ exec arch/*/bin/ksh
$ mv arch arch.old
$ bin/package make
# Press Ctrl+C multiple times
Analysis: In 41ebb55a, I made an error in changing job_init() to
work correctly on non-interactive shells. This line from before:
552| if(possible = (setpgid(0,job.mypgid)>=0) || errno==EPERM)
was changed to:
555| possible = (setpgid(0,job.mypgid) >= 0);
556| if(sh_isoption(SH_INTERACTIVE) && (possible || errno==EPERM))
That is wrong. Before, 'possible' was set to 1 (true) if setpgid()
either succeeded or failed with EPERM. After, it is only set to 1
if setpgid() succeeds. As a result, job control initialisation is
aborted later on upon a test for non-zero 'possible'.
src/cmd/ksh93/sh/jobs.c: job_init():
- Once again set possible to 1 even if setpgid() fails with EPERM.
Thanks to @JohnoKing for the bug report and reproducers.
Resolves: https://github.com/ksh93/ksh/issues/210
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
Analysis: When a syntax error occurs, the shell performs a
longjmp(3) back to exfile() in main.c on line 417:
415| if(jmpval)
416| {
417| Sfio_t *top;
418| sh_iorestore((void*)shp,0,jmpval);
419| hist_flush(shp->gd->hist_ptr);
420| sfsync(shp->outpool);
The first thing it does is restore the file descriptor state
(sh_iorestore), then it flushes the history file (hist_flush), then
it synchronises sfio's logical stream state with the physical
stream state using (sfsync).
However, the fix applied in e999f6b1 caused sh_iorestore() to sync
all sfio streams unconditionally. So this was done before
hist_flush(), which caused unpredictable behaviour, including
temporary and/or permanent history corruption, as this also synched
shp->outpool before hist_flush() had a chance to do its thing.
The fix is to only call sfsync() in sh_iorestore() if we're
actually about to call ftruncate(2), and not otherwise.
Moral of the story: bug fixes should be as specific as possible to
minimise the risk of side effects.
src/cmd/ksh93/sh/io.c: sh_iorestore():
- Only call sfsync() if we're about to truncate a file.
src/cmd/ksh93/tests/pty.sh:
- Add test.
Thanks to Marc Wilson for reporting the bug and to Johnothan King
for finding the commit that introduced it.
Resolves: https://github.com/ksh93/ksh/issues/209
Relevant: https://github.com/att/ast/issues/61
src/cmd/ksh93/edit/edit.c: ed_read():
- The loop that handles SIGWINCH assumes sfpkrd will return and
set errno to EINTR if ksh is sent SIGWINCH. This only occurs
when select(2) is used to wait for input, so tell sfpkrd to
use select if possible. This is only done if the last argument
given to sfpkrd is '2', which should avoid regressions.
src/lib/libast/sfio/sfpkrd.c: sfpkrd():
- Always use select if the last argument is 2. This allows
sfpkrd() to intercept SIGWINCH when necessary.
Fixes: https://github.com/ksh93/ksh/issues/202
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
That Mac OS X bug workaround is now 23 days shy of the age of
majority, and that bug (symlinks testing as regular files) is
pretty basic, so I'm betting it's fixed by now.
src/lib/libast/include/ast_dir.h:
- Do not disable D_TYPE on macOS.
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.
src/cmd/ksh93/Mamfile:
- regress.c: add missing SH_DICT define for getopt self-doc string,
needed after USAGE_LICENSE macros were removed. (re: ede47996)
src/cmd/ksh93/init.c: sh_init():
- Do not set error_info.exit early in init. This is the function
that is called when an error exits the shell. It defaults to
exit(3). Setting it to sh_exit() early on can cause a crash if an
error is thrown before shell initialisation is fully finished.
So set it at the end of sh_init() instead.
- __regress__: Remove error_info.exit workaround. (re: 506bd2b2)
- Fix SHOPT_P_SUID directive. This is not actually a 0/1 value, so
we should use #ifdef and not #if. If SHOPT_REGRESS is on, it it
set to a function call. (re: 2182ecfa)
src/cmd/ksh93/SHOPT.sh:
- Document that SHOPT_P_SUID cannot be set to 0 to be turned off.
src/cmd/ksh93/tests/basic.sh:
- Fix syntax error (unbalanced single quote) in two -c script
invocations. It only failed to throw a syntax error due to a
problematic hack in ksh that may be removed soon.
See: https://github.com/ksh93/ksh/issues/199
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/io.sh:
- Redirect standard error on two ksh -i invocations to /dev/null
to work around the test hanging on AIX.
src/cmd/ksh93/tests/comvario.sh:
- Remove duplicate copyright header.
- Fix warning format.
src/cmd/ksh93/tests/functions.sh:
- Fix the 'TERM signal sent to last process of function kills the
script' test so that it works on AIX. We cannot rely on grepping
'ps' output as the external 'sleep' command does not show the
command name on AIX. Instead, find it by its parent PID.
src/cmd/ksh93/tests/locale.sh,
src/cmd/ksh93/tests/substring.sh:
- Rewrite the very broken multibyte locale tests (two outright
syntax errors due to unbalanced quotes, and none of the tests
actually worked).
- Since they set LC_ALL, move them to locale.sh.
src/cmd/ksh93/tests/variables.sh:
- Redirect stderr on some 'ulimit -t unlimited' invocations (which
fork subshells as the intended side effect) to /dev/null in case
that exceeds a system-defined limit.
The referenced commit neglected to add checks for strdup() calls.
That calls malloc() as well, and is used a lot.
This commit switches to another strategy: it adds wrapper functions
for all the allocation macros that check if the allocation
succeeded, so those checks don't need to be done manually.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/init.c:
- Add sh_malloc(), sh_realloc(), sh_calloc(), sh_strdup(),
sh_memdup() wrapper functions with success checks. Call nospace()
to error out if allocation fails.
- Update new_of() macro to use sh_malloc().
- Define new sh_newof() macro to replace newof(); it uses
sh_realloc().
All other changed files:
- Replace the relevant calls with the wrappers.
- Remove now-redundant success checks from 18529b88.
- The ERROR_PANIC error message calls are updated to inclusive-or
ERROR_SYSTEM into the exit code argument, so libast's error()
appends the human-readable version of errno in square brackets.
See src/lib/libast/man/error.3
src/cmd/ksh93/edit/history.c:
- Include "defs.h" to get access to the wrappers even if KSHELL is
not defined.
- Since we're here, fix a compile error that occurred with KSHELL
undefined by updating the type definition of hist_fname[] to
match that of history.h.
src/cmd/ksh93/bltins/enum.c:
- To get access to sh_newof(), include "defs.h" instead of
<shell.h> (note that "defs.h" includes <shell.h> itself).
src/cmd/ksh93/Mamfile:
- enum.c: depend on defs.h instead of shell.h.
- enum.o: add an -I. flag in the compiler invocation so that defs.h
can find its subsequent includes.
src/cmd/builtin/pty.c:
- Define one outofmemory() function and call that instead of
repeating the error message call.
- outofmemory() never returns, so remove superfluous exit handling.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
The value of the ${.sh.fun} variable, which is supposed to contain
the name of the function currently being executed, leaks out of the
DEBUG trap if it executes a function. Reproducer:
$ fn() { echo "executing the function"; }
$ trap fn DEBUG
$ trap - DEBUG
executing the function
$ echo ${.sh.fun}
fn
${.sh.fun} should be empty outside the function.
Annalysis:
The sh_debug() function in xec.c, which executes the DEBUG trap
action, contains these lines, which are part of restoring the state
after running the trap action with sh_trap():
nv_putval(SH_PATHNAMENOD,shp->st.filename,NV_NOFREE);
nv_putval(SH_FUNNAMENOD,shp->st.funname,NV_NOFREE);
shp->st = savst;
First the SH_PATHNAMENOD (${.sh.file}) and SH_FUNNAMENOD
(${.sh.fun}) variables get restored from the values in the shell's
scoped information struct (shp->st), but that is done *before*
restoring the parent scope with 'shp->st = savst;'. It should be
done after. Fixing the order is sufficient to fix the bug.
However, I am not convinced that these nv_putval() calls are good
for anything at all. Setting, unsetting, restoring, etc. the
${.sh.fun} and ${.sh.file} variables is already being handled
perfectly well elsewhere in the code for executing functions and
sourcing dot scripts. The DEBUG trap is neither here nor there.
There's no reason for it to get involved with these variables.
I was unable to break anything after simply removing those two
lines. So I strongly suspect this is another case, out of many now,
where a bug in ksh93 is properly fixed by removing some code.
I couldn't get ${.sh.file} to leak similarly -- I think this is
because SH_PATHNAMENOD (and not SH_FUNNOD) is set explicitly in
exfile() in main.c, masking this incorrect restore. It is the only
place where SH_PATHNAMENOD and SH_FUNNOD are not both set.
src/cmd/ksh93/sh/xec.c:
- Remove these two spurious nv_putval() calls.
src/cmd/ksh93/tests/variables.sh:
- Add regression test for leaking ${.sh.fun}.
At init, and then whenever the TERM variable changes, ed_setup()
uses sh_trap() to run the external 'tput' command to get the
current terminal escape sequence for moving up the cursor one line.
A sh_trap() call executes a shell command as if a shell script's
trap action had executed it, so is subject to modes like the
restricted mode. As of 7ff6b73b, we execute tput using its absolute
path (found and hardcoded at compile time) for better
robustness/security. This fails in restricted mode as it does not
allow executing commands by absolute path. But in C, nothing stops
us from turning that off.
src/cmd/ksh93/edit/edit.c: ed_setup():
- Block SIGINT while doing all of the following, so the user can't
interrupt it and escape from restricted mode. Even without that,
it's probably a good idea to do this, so an interrupt doesn't
cause an inconsistent state.
Note that sigblock() and sigrelease() are macros defined in
features/sigfeatures. To get those, we need to include <fault.h>.
- Temporarily turn off SH_RESTRICTED before sh_trap()ping tput to
get the terminal command to move the cursor up one position.
- Avoid potentially using a sequence that was cut off. Only use the
resulting string if its length does not exceed the space reserved
for CURSOR_UP. Otherwise, empty it.
src/cmd/ksh93/Mamfile:
- Add fault.h dependency to edit.c.
src/cmd/ksh93/edit/history.c:
- Fix typos in introductory comment.
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.
This bug was backported along with a fix from 93v-. An inconsistent
state occurred if you caused a file name completion menu to appear
with two TABs (which also puts you in command mode) but then
re-enter insert mode (e.g. with 'a') instead of entering a number.
$ set -o vi
$ cd /
$ bin/p [press TAB twice]
1) pax
2) ps
3) pwd [now type 'a', 'wd', return]
$ bin/pwd
> [PS2 prompt wrongly appears; press return]
/
$
Here's another reproducer, suggesting the problem is a write past
the end of the screen buffer:
$ set -o vi
$ cd /
$ bin/p [press TAB twice]
1) pax
2) ps
3) pwd [press '0', then '$']
$ bin/p [cursor is one too far to the right, past the 'p'!]
[Further operations show random evidence of memory corruption]
Harald van Dijk found the cause (thanks!):
> In vi.c's textmod there is
>
> case '=': /** list file name expansions **/
> ...
> ++last_virt;
> ...
> if(ed_expand(vp->ed,(char*)virtual, &cur_virt, &last_virt, ch, vp->repeat_set?vp->repeat:-1)<0)
> {
> ...
> last_virt = i;
> ...
> }
> else if((c=='=' || (c=='\\'&&virtual[last_virt]=='/')) && !vp->repeat_set)
> {
> ...
> }
> else
> {
> ...
> --last_virt;
> ...
> }
> break;
>
> That middle block does not restore last_virt, and everything goes
> wrong after that. That function used to restore last_virt until
> commit 4cecde1 (#41). The commit message says it was taken from
> ksh93v- and indeed this bug is also present in that version too.
> If I restore the last_virt = i; that was there originally, like
> below, then this bug seems to be fixed. I do not know why it was
> taken out, taking it out does not seem to be necessary to fix the
> original bug.
src/cmd/ksh93/edit/vi.c: textmod():
- Restore the missing restore of last_virt.
src/cmd/ksh93/tests/pty.sh:
- Add test that checks basic completion menu functionality works
and runs modified versions of the two reproducers above.
Resolves: https://github.com/ksh93/ksh/issues/195
src/cmd/ksh93/data/builtins.c:
- sh_optksh[]: Edit descriptions of -c and -s options for clarity.
- sh_set[]: The --rc long name equivalent for -E was documented
wrong, but in any case it does not belong in sh_set[], because
that also shows up in 'set --man' and this invocation-only option
cannot be used with 'set'. Remove it. (Note that all other
invocation options already don't have inline documentation of
their long equivalents. This may or may not be fixed at some
point. It is problematic because they should not be documented in
sh_set[] but there is no other good place for them.)
src/cmd/ksh93/sh.1:
- Generally edit the Invocation section for clarity.
- Document the long invocation option equivalents.
- Remove some nonsense from the -s description: "Shell output,
except for the output of the Special Commands listed above, is
written to file descriptor 2" (which is standard error).
In fact, this option has no influence at all on what is written
to standard error or standard output.
Reproducer:
$ ksh -c 'v=${ PATH=/dev/null; }; echo $PATH; whence ls'
/dev/null
/bin/ls
The PATH=/dev/null assignment should survive the shared-state
command substitution, and does, yet 'ls' is still found.
The variable became inconsistent with the internal pathlist.
This bugfix is from the 93v- beta.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Do not save and restore pathlist for a subshare.
- A few other subshell tweaks from 93v- that made sense:
. reset shp->subdup (bitmask for dups of 1) after saving it
. use e_dot instead of "." for consistency
. retry close(1) if it was interrupted
src/cmd/ksh93/tests/path.sh:
- Add test for this bug.
On some systems (AIX, HP-UX, OpenBSD) the pty tests may hang.
On all systems except Darwin/macOS, FreeBSD and Linux, the pty
tests show one or more regressions. But when I try out the failing
tests manually in a real session, it seems to work fine. So I
suspect pty is broken and not ksh.
src/cmd/ksh93/tests/pty.sh:
- For now, only run the pty tests on Darwin, FreeBSD and Linux.
src/lib/libast/Mamfile:
- tvsleep.c: Add missing error.h dependency (re: 2f7918de).
(unrelated, but just wasn't worth its own commit)
Most of these changes remove unused variables, functions and labels
to fix -Wunused compiler warnings. Somewhat notable changes:
src/cmd/ksh93/bltins/print.c:
- Removed the unused 'neg' variable.
Patch from ksh2020: https://github.com/att/ast/pull/725
src/cmd/ksh93/bltins/sleep.c:
- Initialized ns to fix three -Wsometimes-uninitialized warnings.
src/cmd/ksh93/edit/{emacs,vi}.c:
- Adjust strncpy size to fix two -Wstringop-truncation warnings.
src/cmd/ksh93/include/shell.h:
- The NOT_USED macro caused many -Wunused-value warnings,
so it has been replaced with ksh2020's macro:
https://github.com/att/ast/commit/19d0620a
src/cmd/ksh93/sh/expand.c:
- Removed an unnecessary 'ap = ' since 'ap' is never read
between stakseek and stakfreeze.
src/cmd/ksh93/edit/vi.c: refresh():
- Undef this function's 'w' macro at the end of it to stop it
potentially interfering with future code changes.
src/cmd/ksh93/sh/nvdisc.c,
src/lib/libast/misc/magic.c,
src/lib/libast/regex/regsubexec.c,
src/lib/libast/sfio/sfpool.c,
src/lib/libast/vmalloc/vmbest.c:
- Fixed some indentation to silence -Wmisleading-indentation
warnings.
src/lib/libast/include/ast.h:
- For clang, now only suppress hundreds of -Wparentheses warnings
as well as a few -Wstring-plus-int warnings.
Clang's -Wparentheses warns about things like
if(foo = bar())
which assigns to foo and checks the assigned value.
Clang wants us to change this into
if((foo = bar()))
Clang's -Wstring-plus-int warns about things like
"string"+x
where x is an integer, e.g. "string"+3 represents the string
"ing". Clang wants us to change that to
"string"[3]
The original versions represent a perfectly valid coding style
that was common in the 1980s and 1990s and is not going to change
in this historic code base. (gcc does not complain about these.)
Co-authored-by: Martijn Dekker <martijn@inlv.org>
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.
Huge typeset -L/-R adjustment length values were still causing
crashses on sytems with not enough memory. They should error out
gracefully instead of crashing.
This commit adds out of memory checks to all malloc/calloc/realloc
calls that didn't have them (which is all but two or three).
The stkalloc/stakalloc calls don't need the checks; it has
automatic checking, which is done by passing a pointer to the
outofspace() function to the stakinstall() call in init.c.
src/lib/libast/include/error.h:
- Change the ERROR_PANIC exit status value from ERROR_LEVEL (255)
to 77, which is what it is supposed to be according to the libast
error.3 manual page. Exit statuses > 128 for anything else than
signals are not POSIX compliant and may cause misbehaviour.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/init.c:
- To facilitate consistency, add a simple extern sh_outofmemory()
function that throws an ERROR_PANIC "out of memory".
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/data/builtins.c:
- Remove now-redundant e_nospace[] extern message; it is now only
used in one place so it might as well be a string literal in
sh_outofmemory().
All other changed files:
- Verify the result of all malloc/calloc/realloc calls and call
sh_outofmemory() if they fail.
Additional adjustments to previous commit bdb9974 to correct
crashes when the max size of a justified string is requested.
This commit corrects the following:
Before (Ubuntu 64bit):
$ typeset -L $(((1<<31)-1)) s=h; typeset +p s
Segmentation fault (core dumped)
After:
$ typeset -L $(((1<<31)-1)) s=h; typeset +p s
typeset -L 2147483647 s
src/cmd/ksh93/sh/name.c: nv_putval():
- Alter the variables size, dot, and append from int to unsigned
int to prevent unwanted negative values from being expressed.
- By creating size, dot, and append as unsigned ints; (unsigned)
type casting is avoided.
