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.
This solves another intermittent crash that happened upon
processing SIGWINCH in the emacs editor. See also: 7ff6b73b
I found this bug while testing ksh 93u+m on OpenBSD. Due to its
pervasive security hardening, this system crashes a program
reliably where others crash it intermittently, which is invaluable.
src/cmd/ksh93/sh/jobs.c: job_reap():
- The pw pointer is not ever given a value if the loop breaks on
line 318-319, but it is used unconditionally on lines 464-470,
Initialise the pointer to null on function entry and do not call
job_list() and job_unpost() if the pointer is still null.
It's amazing what can happen when you compile ksh using standard
malloc (i.e. with AST vmalloc disabled) on OpenBSD. Its security
hardening provokes crashes that reveal decades-old unsolved bugs.
This one is an attempt to access one byte before the beginning of
the command line buffer when the cursor is at the beginning of it.
On this system configuration, it provoked an instant crash whenever
you moved the cursor back to the beginning of the command line,
e.g. with ^A or the cursor keys.
src/cmd/ksh93/edit/emacs.c: draw():
- Check that the cursor is actually past the first position of the
command line buffer before trying to read the position
immediately before it. If not, zero the value.
The following caused an infinite loop:
v=${ exec >/dev/tty; }
v=${ redirect >/dev/tty; }
Even the original authors didn't figure out how to 'exec >foo' or
'redirect >foo' inside a non-forking command substitution, so they
fork it by calling sh_subfork(). If we delete that call, even
normal command substitutions enter into that infinite loop. But of
course a shared-state comsub can never fork as it would no longer
share its state. Without a solution to make this work without
forking, an error message is the only sensible thing left to do.
src/cmd/ksh93/sh/io.c: sh_redirect():
- If we're redirecting standard output (1), the redirection is
permanent as in 'exec'/'redirect' (flag==2), and we're in a
subshare, then error out.
Resolves: https://github.com/ksh93/ksh/issues/128
POSIX warns about "unset PWD" leading to unspecified behavior from
the pwd util, which we then use.
bin/package, src/cmd/INIT/package.sh:
- Determine if the shell has $PWD with a feature test. Only unset
PWD if it does not (the old Bourne shell).
- Clean up 'case' flow.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
ksh crashed in various different and operating system-dependent
ways when attempting to create or apply justification strings
using typeset -L/-R/-Z, especially if large sizes are used.
The crashes had two immediate causes:
- In nv_newattr(), when applying justification attributes, a buffer
was allocated for the justified string that was exactly 8 bytes
longer than the original string. Any larger justification string
caused a buffer overflow (!!!).
- In nv_putval(), when applying existing attributes to a new value,
the corresponding memmove() either did not zero-terminate the
justified string (if the original string was longer than the
justified string) or could read memory past the original string
(if the original string was shorter than the justified string).
Both scenarios can cause a crash.
This commit fixes other minor issues as well, such as a mysterious
8 extra bytes allocated by several malloc/realloc calls. This may
have been some naive attempt to paper over the above bugs. It seems
no one can make any other kind of sense of it.
A readjustment bug with zero-filling was also fixed.
src/cmd/ksh93/sh/name.c:
- nv_putval():
. Get rid of the magical +8 bytes for malloc and realloc. Just
allocate one extra byte for the terminating zero.
. Fix the memmove operation to use strncpy instead, so that
buffer overflows are avoided in both scenarios described above.
Also make it conditional upon a size adjustment actually
happening (i.e. if 'dot' is nonzero).
. Mild refactoring: combine two 'if(sp)' blocks into one;
declare variables only used there locally for legibility.
- nv_newattr():
* Replace the fatally broken "let's allocate string length + 8
bytes no matter the size of the adjustment" routine with a new
one based on work by @hyenias (see comments in #142). It is
efficient with memory, taking into account numeric types,
growing strings, and shrinking strings.
* Fix zero-filling in readjustment after changing the initial
size of a -Z attribute. If the number was zero, all zeros were
still skipped, leaving an empty string.
Thanks to @hyenias for originally identifying this breakage and
laying the groundwork for fixing nv_newattr(), and to @lijog for
the crash analysis that revealed the key to the nv_putval() fix.
Resolves: https://github.com/ksh93/ksh/issues/142
Resolves: https://github.com/ksh93/ksh/issues/181
So now we know what that faulty check for shp->indebug in sh_trap()
was meant to do: it was meant to pass down the trap handler's exit
status, via sh_debug(), down to sh_exec() (xec.c) so that it could
then skip the execution of the next command if the trap's exit
status is 2, as documented in the manual page. As of d00b4b39, exit
status 2 was not passed down, so this stopped working.
This commit reinstates that functionality, but without the exit
status bug in command substitutions caused by the old way.
src/cmd/ksh93/sh/fault.c: sh_trap():
- Save the trap's exit status before restoring the parent
envionment's exit status. Make this saved exit status the return
value of the function. (This does not break anything, AFAICT; the
majority of sh_trap() calls ignore the return value, and the few
that don't ignore it seem to expect it to return exactly this.)
src/cmd/ksh93/sh/xec.c: sh_exec():
- The sh_trap() fix has one side effect: whereas the exit status of
a skipped command was always 2 (as per the trap handler), now it
is always 0, because it gets reset in sh_exec() but no command is
executed. That is probably not a desirable change in behaviour,
so let's fix that here instead: set sh.exitval to 2 when skipping
commands.
src/cmd/ksh93/sh.1:
- Document that ${.sh.command} shell-quotes its arguments for use
by 'eval' and such. This fact was not documented anywhere, AFAIK.
src/cmd/ksh93/shell.3:
- Document that $? (exit status) is made local to trap handlers.
- Document that sh_trap() returns the trap handler's exit status.
src/cmd/ksh93/tests/basic.sh:
- Add test for this bug.
- Add a missing test for the exit status 255 functionality (if a
DEBUG trap handler yields this exit status and we're executing a
function or dot script, a return is triggered).
Fixes: https://github.com/ksh93/ksh/issues/187
Many of the errors fixed in this commit are word repetitions
such as 'the the' and minor spelling errors. One formatting
error in the ksh man page has also been fixed.
On SVR4 platforms, select is a sometimes erroneous wrapper around
the poll system call, so call poll directly for efficiency purposes if
it implies no loss in precision.
See, e.g., http://bugs.motifzone.net/long_list.cgi?buglist=129 .
src/lib/libast/features/tvlib:
- For systems lacking nanosleep, test whether select is truly more
precise than poll.
src/lib/libast/tm/tvsleep.c:
- Verify that tv argument is not null.
- Immediately return if asked to sleep for a duration of zero.
- Immediately initialize timespec in the nanosleep case.
- Revise variable names to use Apps Hungarian.
- Use poll instead of select when there is no loss in precision.
- Check for overflow in the poll case.
- Improve comments.
- Revise arithmetic to work with unsigned types, rather than
casting to long.
- Do not return non-zero if we have slept for a sufficient
time.
In the 93v- beta, they add a newline instead of a space.
This has fewer side effects as final newlines get stripped.
It's still a hack and it would still be nice to have a real fix,
but it seems even the AT&T guys couldn't come up with one.
src/cmd/ksh93/sh/macro.c:
- To somehow avoid a memory leak involving alias substitution,
append a linefeed instead of a space to the comsub buffer.
src/cmd/ksh93/tests/subshell.sh:
- Add test for minor regression caused by the RedHat version.
Compilers like GCC are capable of optimizing away calls like
pow(1,inf), which caused the IEEE compliance feature test within
libast to incorrectly succeed on platforms with non-IEEE behavior.
This is arguably a bug within GCC, as floating point optimizations
should never alter the behavior of code unless IEEE compliance is
explicitly disabled via a flag like -ffast-math. Programs in which
only some calls to pow are optimized away are liable to severely
malfunction.
Thanks to Martijn Dekker for pointing this issue out and the kind
operators of polarhome.com for permitting me gratis use of their
Unix systems.
src/lib/libast/comp/omitted.c:
- Add IEEE compliant functions that wrap powf, pow, and powl.
src/lib/libast/features/float:
- Look for powf, pow, and powl in the C library.
- For compilers that do the right thing, like the native toolchains
of Solaris and UnixWare, use lightweight macros to wrap the pow
functions.
- Use a volatile function pointer through which to access the C
library's pow function in an attempt to defeat code optimization.
- For these overzealous compilers, define pow to _ast_pow so that
the same technique can be used within the above functions.
This commit fixes a bug in the 'read' built-in: it did not properly
skip over multibyte characters. The bug never affects UTF-8 locales
because all UTF-8 bytes have the high-order bit set. But Shift-JIS
characters may include a byte corresponding to the ASCII backslash
character, which cauased buggy behaviour when using 'read' without
the '-r' option that disables backslash escape processing.
It also makes the regression tests compatible with Shift-JIS
locales. They failed with syntax errors.
src/cmd/ksh93/bltins/read.c:
- Use the multibyte macros when skipping over word characters.
Based on a patch from the old ast-developers mailing list:
https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01848.html
src/cmd/ksh93/include/defs.h:
- Be a bit smarter about causing the compiler to optimise out
multibyte code when SHOPT_MULTIBYTE is disabled. See the updated
comment for details.
src/cmd/ksh93/tests/locale.sh:
- Put all the supported locales in an array for future tests.
- Add test for the 'read' bug. Include it in a loop that tests
64 SHIFT-JIS character combinations. Only one fails on old ksh:
the one where the final byte corresponds to the ASCII backslash.
It doesn't hurt to test all the others anyway.
src/cmd/ksh93/tests/basic.sh,
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/quoting2.sh:
- Fix syntax errors that occurred in SHIFT-JIS locales as the
parser was processing literal UTF-8 characters. Not executing
that code is not enough; we need to make sure it never gets
parsed as well. This is done by wrapping the commands containing
literal UTF-8 strings in an 'eval' command as a single-quoted
operand.
.github/workflows/ci.yml:
- Run the tests in the ja_JP.SJIS locale instead of ja_JP.UTF-8.
UTF-8 is already covered by the nl_NL.UTF-8 test run; that should
be good enough.
This commit introduced the following bug, which is worse than the
one that commit fixed: it became impossible to alter the size of an
existing justified string attribute.
Thanks to @hyenias for catching this bug:
https://github.com/ksh93/ksh/issues/142#issuecomment-780931533
$ unset s; typeset -L 100 s=h; typeset +p s; typeset -L 5 s; typeset +p s
typeset -L 100 s
typeset -L 100 s
Expected output:
typeset -L 100 s
typeset -L 5 s
src/cmd/ksh93/sh/name.c:
- Revert.
src/cmd/ksh93/tests/attributes.sh:
- Revert: re-disable tests for minor attribute output regressions.
- Add a test for this bug and potential similar bugs.
A ${ shared-state command substitution; } (internally called
subshare) is documented to share its state with the parent shell
environment, so all changes made within the command substitution
survive outside of it. However, when it is run within a
virtual/non-forked subshell, variables that are not already local
to that subshell will leak out of it into the grandparent state.
Reproducer:
$ ksh -c '( v=${ bug=BAD; } ); echo "$bug"'
BAD
If the variable pre-exists in the subshell, the bug does not occur:
$ ksh -c '( bug=BAD1; v=${ bug=BAD2; } ); echo "$bug"'
(empty line, as expected)
The problem is that the sh_assignok() function, which is
responsible for variable scoping in virtual subshells, does not
ever bother to create a virtual subshell scope for a subshare.
That is an error if a subshare's parent (or higher-up ancestor)
environment is a virtual subshell, because a scope needs to be
created in that parent environment if none exists.
To make this bugfix possible, first we need to get something out of
the way. nv_restore() temporarily sets the subshell's pointer to
the preesnt working directory, shpwd, to null. This causes
sh_assignok() to assume that the subshell is a subshare (because
subshares don't store their own PWD) and refuse to create a scope.
However, nv_restore() sets it to null for a different purpose: to
temporarily disable scoping for *all* virtual subshells, making
restoring possible. This is a good illustration of why it's often
not a good idea to use the same variable for unrelated purposes.
src/cmd/ksh93/sh/subshell.c:
- Add a global static subshell_noscope flag variable to replace the
misuse of sh.shpwd described above.
- sh_assignok():
. Check subshell_noscope instead of shpwd to see if scope
creation is disabled. This makes it possible to distinguish
between restoring scope and handling subshares.
. If the current environment is a subshare that is in a virtual
subshell, create a scope in the parent subshell. This is done
by temporarily making the parent virtual subshell the current
subshell (by setting the global subshell_data pointer to it)
and calling sh_assignok() again, recursively.
- nv_restore(): To disable subshell scope creation while restoring,
set subshell_noscope instead of saving and unsetting sh.shpwd.
src/cmd/ksh93/tests/subshell.sh:
- Add tests. I like tests. Tests are good.
Fixes: https://github.com/ksh93/ksh/issues/143
This commit fixes the following:
1. Emacs mode ignores --nobackslashctrl (re: 24598fed) when in
reverse search.
2. When entering more than one backslash, emacs reverse search mode
deletes multiple backslashes after pressing backspace once.
Reproducer:
$ set --emacs --nobackslashctrl
$ <Ctrl+R> \\\\<Backspace>
3. Except when in reverse search, the backslash fails to escape a
subsequent interrupt character (^C). Reproducer:
$ set --emacs --backslashctrl
$ teststring \<Ctrl+C>
src/cmd/ksh93/edit/emacs.c:
- Disable escaping backslashes in emacs reverse search if
'nobackslashctrl' is enabled.
- Fix the buggy behavior of backslashes in emacs reverse
search by processing backslashes in a loop.
src/cmd/ksh93/tests/pty.sh:
- Add regression tests.
src/cmd/ksh93/sh.1:
- Fix a minor documentation error (^C is the usual interrupt
character, not ^?).
Co-authored-by: Martijn Dekker <martijn@inlv.org>
'case x in esac' should be syntactically correct, but was an error:
$ ksh -c 'case x in esac'
ksh: syntax error at line 1: `case' unmatched
Inserting a newline was a workaround:
$ ksh -c $'case x in\nesac'
(no output)
The problem was that the 'esac' reserved word was not being
recognised if it immediately followed the 'in' reserved word.
src/cmd/ksh93/sh/lex.c: sh_lex():
- Do not turn off recognition of reserved words after 'in' if we're
in a 'case' construct; only do this for 'for' and 'select'.
src/cmd/ksh93/tests/case.sh:
- Add seven regression test for correct recognition of 'esac'.
Only two failed on ksh93. The rest is to catch future bugs.
Fixes: https://github.com/ksh93/ksh/issues/177
This commit fixes the functionality of Alt+D and Alt+H in emacs mode.
These keyboard shortcuts are intended to work on whole words, but
after commit 13c3fb21 their functionality was reduced to deleting only
singular letters:
$ Test word <Alt+H> # This should delete 'word', not just 'd'.
$ Foo <Alt+B> <Alt+D> # This should delete 'Foo', not just 'F'.
Man page entries for reference:
M-d Delete current word.
M-^H (Meta-backspace) Delete previous word.
M-h Delete previous word.
src/cmd/ksh93/edit/emacs.c:
- 'count' cannot be overridden when handling Alt+D or Alt+H,
so add the total number of repetitions to count (the number of
repetitions can't be negative).
- If 'count' is a negative number, set it to one before adding the
number of repetitions.
