In the times(3) fallback for the time keyword (which can be enabled
in xec.c by undefining _lib_getrusage and timeofday), ksh will
print the obtained time incorrectly if TIMEFORMAT is set to use a
precision level of three:
$ TIMEFORMAT=$'\nreal\t%3lR'
$ time sleep .080
real 0m00.008s # Should be '00.080s'
This commit corrects that issue by using 10^precision to get the
correct fractional scaling. Note that the fallback still doesn't
support a true precision level of three (times(3) alone doesn't
support it), so this in effect pads a zero to the end of the output
when the precision level is three.
Additional change to tests/builtins.sh:
- While fixing the above issue I found out that ksh93v- broke
support for passing microseconds to the sleep builtin in the form
of <num>U. I've added a regression test for that bug to ensure it
isn't backported to ksh93u+m by accident.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
If neither gmacs/emacs nor vi are active, the multiline mode should
not be enabled even if the multiline option is on. Doing so can
cause inconsistent behaviour, particularly in multibyte locales
where, if the shell is compiled with SHOPT_RAWONLY (as is default),
the no-editor mode is actually handled by vi.c.
Also, the new --histreedit and --histverify options only work in
the emacs or vi editors, or in no-editor mode when handled by vi.
Which means they cannot ever work if neither emacs or vi were
compiled in (i.e. SHOPT_ESH and SHOPT_VSH were both disabled).
In that case, there's no point in compiling in those options.
Come to think of it, the same applies to the multiline option.
All changed files:
- Update SHOPT_ESH/SHOPT_VSH preprocessor directives as per above.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/include/shell.h:
- Move definitions of history expansion-related options to shell.h,
which is where all the other shell options are defined.
The referenced commit introduced a bug that caused command
substitutions to hang, writing infinite zero bytes, when
redirecting standard output on a built-in comand that forks the
command substitution subshell.
The bug was caused by removing the fork when redirecting standard
output in a non-permanent manner. However, simply reintroducing the
fork causes multiple regressions that we had fixed in the meantime.
Thankfully, it looks like this forking workaround is only necessary
when redirecting the output of built-ins. It appears that moving
workaround from io.c to the built-ins handling code in sh_exec() in
xec.c, right before calling sh_redirect(), allows reintroducing the
forking workaround for non-permanent redirections without causing
other regressions.
It would be better if the underlying cause of the hang were fixed
so the workaround becomes unnecessary, but I don't think that is
going to happen any time soon (AT&T didn't manage, either).
src/cmd/ksh93/sh/io.c: sh_redirect():
- Remove forking workaround for redirecting stdout in a comsub.
src/cmd/ksh93/sh/xec.c: sh_exec(): TCOM: built-ins handling code:
- Reimplement the workaround here.
Resolves: https://github.com/ksh93/ksh/issues/416
On Haiku:
# /bin/cat <(echo hi) # no redirection
cat: /tmp/ksh.f29pd8f: No such file or directory
Whereas this works fine:
# /bin/cat < <(echo hi) # with redirection
hi
# /opt/ast/bin/cat <(echo hi) # no redirection; use built-in
hi
Haiku does not have /dev/fd, so uses the FIFO (named pipe) fallback
mechanism. See also: c3eac977
Analysis: In the TFORK part of sh_exec(), forked branch (child),
the FIFO (sh.fifo) is unlinked immediately after opening it. This
is not a problem if the process substitution is used in combination
with a redirection, but if not, then the FIFO is passed on to the
command as a file name argument. This creates a race condition: ksh
was counting on the external 'cat' command opening the FIFO before
the child could unlink it. Whether that race is won depends on
operating system implementation details. When invoking an external
command on Haiku, the race is lost.
src/cmd/ksh93/sh/xec.c: sh_exec(): TFORK: child branch:
- Delay unlinking the FIFO until after executing the process
substitution, when we're about to exit from the child process.
This commit makes various different improvements to the documentation:
- sh.1: Backported (with changes) mandoc warning fixes from ksh2020
for the ksh93(1) man page: <https://github.com/att/ast/pull/1406>
- Removed unnecessary spaces at the end of lines to fix a few other
mandoc warnings.
- Fixed various typos and capitalization errors in the documentation.
- ANNOUNCE: Document the addition of the ${.sh.pid} variable
(re: 9de65210).
- libast/man/str*: Update the man pages for the libast str* functions
to improve how accurately each function is described.
- ksh93/README: Update regression test/compatibility notes to include
OpenBSD 7.0, FreeBSD 13.0 and WSL running Ubuntu 20.04.
- Change a few places to store the return value from strlen in a
size_t variable rather than signed int.
- comp/setlocale.c: To avoid confusion of two separate variables named
lang, the function local variable has been renamed to langidx.
This commit implements the build fixes required to get ksh running on
Haiku. Note that while ksh does compile, it has a ton of regression test
failures on Haiku.
src/cmd/ksh93/data/signals.c,
src/lib/libast/features/signal.c:
- Add support for the SIGKILLTHR signal, which is supported by BeOS and
Haiku.
- SIGINFO was missing an entry in the libast feature test, so add one
(re: 658bba74).
src/cmd/ksh93/RELEASE:
- Add an entry noting that ksh now compiles on Haiku, albeit with many
regression test failures.
src/cmd/ksh93/{include/terminal.h,sh/path.c}:
- Silence compiler warnings on Haiku.
src/lib/libast/features/mmap:
- The mmap feature test freezes on Haiku, so modify the test to fail
immediately on that OS.
src/lib/libast/misc/signal.c:
- Avoid redefining the signal definition on Haiku to fix a compiler
error.
src/lib/libast/features/nl_types:
- For some reason the nl_item typedef on Haiku doesn't work correctly.
Work around that by creating the nl_item type in the libast nl_types
feature test.
This combines 20 cleanup commits from the dev branch.
All changed files:
- Clean up pointer defererences to sh.
- Remove shp arguments from functions.
Other notable changes:
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/init.c:
- On second thought, get rid of the function version of
sh_getinterp() as libshell ABI compatibility is moot. We've
already been breaking that by reordering the sh struct, so there
is no way it's going to work without recompiling.
src/cmd/ksh93/sh/name.c:
- De-obfuscate the relationship between nv_scan() and scanfilter().
The former just calls the latter as a static function, there's no
need to do that via a function pointer and void* type conversions.
src/cmd/ksh93/bltins/typeset.c,
src/cmd/ksh93/sh/name.c,
src/cmd/ksh93/sh/nvdisc.c:
- 'struct adata' and 'struct tdata', defined as local struct types
in these files, need to have their first three fields in common,
the first being a pointer to sh. This is because scanfilter() in
name.c accesses these fields via a type conversion. So the sh
field needed to be removed in all three at the same time.
TODO: de-obfuscate: good practice definition via a header file.
src/cmd/ksh93/sh/path.c:
- Naming consistency: reserve the path_ function name prefix for
externs and rename statics with that prefix.
- The default path was sometimes referred to as the standard path.
To use one term, rename std_path to defpath and onstdpath() to
ondefpath().
- De-obfuscate SHOPT_PFSH conditional code by only calling
pf_execve() (was path_pfexecve()) if that is compiled in.
src/cmd/ksh93/include/streval.h,
src/cmd/ksh93/sh/streval.c:
- Rename extern strval() to arith_strval() for consistency.
src/cmd/ksh93/sh/string.c:
- Remove outdated/incorrect isxdigit() fallback; '#ifnded isxdigit'
is not a correct test as isxdigit() is specified as a function.
Plus, it's part of C89/C90 which we now require. (re: ac8991e5)
src/cmd/ksh93/sh/suid_exec.c:
- Replace an incorrect reference to shgd->current_pid with
getpid(); it cannot work as (contrary to its misleading directory
placement) suid_exec is an independent libast program with no
link to ksh or libshell at all. However, no one noticed because
this was in fallback code for ancient systems without
setreuid(2). Since that standard function was specified in POSIX
Issue 4 Version 2 from 1994, we should remove that fallback code
sometime as part of another obsolete code cleanup operation to
avoid further bit rot. (re: 843b546c)
src/cmd/ksh93/bltins/print.c: genformat():
- Remove preformat[] which was always empty and had no effect.
src/cmd/ksh93/shell.3:
- Minor copy-edit.
- Remove documentation for nonexistent sh.infile_name. A search
through ast-open-archive[*] reveals this never existed at all.
- Document sh.savexit (== $?).
src/cmd/ksh93/shell.3,
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/init.c:
- Remove sh.gd/shgd; this is now unused and was never documented
or exposed in the shell.h public interface.
- sh_sigcheck() was documented in shell.3 as taking no arguments
whereas in the actual code it took a shp argument. I decided to
go with the documentation.
- That leaves sh_parse() as the only documented function that still
takes an shp argument. I'm just going to go ahead and remove it
for consistency, reverting sh_parse() to its pre-2003 spec.
- Remove undocumented/unused sh_bltin_tree() function which simply
returned sh.bltin_tree.
- Bump SH_VERSION to 20220106.
(nmake makefiles) defined this macro:
__OBSOLETE__ == $("6 months ago":@F=%(%Y0101)T)
This was used to automatically disable code after a period between
6 and 18 months, on 1st Jan of each year, in preprocessor
directives like:
#if __OBSOLETE__ < 20080101
// obsolete code here
#endif
However, when compiling without nmake (as we do), this __OBSOLETE__
macro is not defined at all. And undefined macros evaluate to zero
in arithmetic comparisons, so all that obsolete code has been
getting compiled. Thankfully it doesn't seem to have done any harm,
but all that code was supposed to expire between 2008 and 2014.
src/lib/libast/disc/sfstrtmp.c:
- Removed. Was supposed to be a stub #if __OBSOLETE__ >= 20070101.
src/lib/libast/include/ast.h:
- Remove unused fmtbasell() macro (/* until 2014-01-01 */).
Other changed files:
- Remove __OBSOLETE__d code.
This fixes the use after free issue that caused typeset -m to crash on
older versions of OpenBSD and under ASan. The problem that was causing
the failure was that the ap pointer wasn't set to null after the memory
associated with it was freed. This commit backports a bugfix from
ksh93v- 2013-06-28 that sets ap to null before freeing the associated
memory and adds a check that makes sure ap is still a valid pointer
before calling array_unscope().
tests/types.sh changes:
- Avoid redirecting stderr to /dev/null, as this test shouldn't print
anything to stderr.
- Apply error message improvement from
https://github.com/ksh93/ksh/issues/231#issue-834252084.
tests/arrays.sh change:
- Apply error message improvement from
https://github.com/ksh93/ksh/issues/229#issue-834240645 (re: 7c7fde75).
Resolves: https://github.com/ksh93/ksh/issues/231
This backports a ksh2020 fix for an ASan heap-use-after-free error in
arrays.sh. The arrays regression tests were failing under ASan because
the ap pointer was used after the memory allocated to it was freed by
_nv_unset(). ksh2020 commit:
f1e5119e31
This upstreams the patch to init.c that is necessary to build dtksh
(graphical extensions for CDE, see <https://cdesktopenv.sf.net/>).
It has no effect when building regular ksh. Upstreaming it avoids
the need to keep updating it when changes to init.c are made.
As observed previously (see 3654ee73, 7e6bbf85, 79d19458), the ksh
93u+ codebase on which we rebased development was in a transition:
AT&T evidently wanted to make it possible to have several shell
interpreter states in the same process, which in theory would have
made it possible to start a complete new shell (not just a
subshell) without forking a new process.
This required transitioning from accessing the 'sh' state struct
directly to accessing it via pointers (usually but not always
called 'shp'), introducing a lot of bug-prone passing around of
those pointers via function arguments and other state structs.
Some of the original 'sh' struct was separated into a 'struct
shared' called 'shgd' a.k.a. 'sh.gd' (global data) instead; these
were global state variables that were going to be shared between
the different main shell environments sharing a process. Yet, for
some reason, that struct was allocated dynamically once at init
time, requiring yet another pointer to access it. <shrug>
None of this ever worked, because that transition was incomplete.
It was much further along in the ksh 93v- beta, but I don't think
it actually worked there either (not very much really did). So,
starting a new shell has always required starting a new process.
So, now that it's clear what they were trying to do, should we try
to make it work? I'm going to go with a firm "no" on that question.
Even non-forking (virtual) subshells, something quite a bit less
ambitious, were already an unmitigated nightmare of bugs. In 93u+m
we fixed a load of bugs related to those, but I'm sure there are
still many left. At the very least there are multiple memory leaks.
I think the ambition to go even further and have complete shells
running separate programs share a process, particularly given the
brittle and buggy state of the existing codebase, is evidence that
the AT&T team, in the final years, had well and truly lost the
ability to think "wait a minute, aren't we in over our heads here,
and why are we doing this again? Is this *actually* a feasible and
useful idea?"
In my view, having entirely separate programs share a process is a
*terrible*, horrible, no-good idea that takes us back to the bad
old days before Unix, when kernels and CPUs were unable to enforce
any memory access restrictions. Programmers are imperfect. If
you're going to run a new program, you need the kernel to enforce
the separation between programs, or you're just asking for memory
corruption and security holes. And that separation is enforced by
starting a new program in a new process. That's what processes are
*for*. And if you need *that* to be radically performance-optimised
then you're probably doing it wrong anyway.
(By the way, I would still argue the same for subshells, even after
we fixed many bugs in virtual subshells. But forking all subshells
would in fact cause many scripts to slow down, and the community
would surely revolt. <sigh> Maybe I should make it a shell option
instead, so scripts can 'set -o subfork' for reliability.)
It is also unclear how they were going to make something like
'ulimit' work, which can only work in a separate process. There
was no sign of a mechanism to fork a separate program's shell
mid-execution like there is for subshells (sh_subfork()).
Anyway... I had already changed some code here and there to access
the sh state struct directly, but as of this commit I'm beginning
to properly undo this exercise in pointlessness. From now on, we're
exercising pointerlessness instead.
I'll do this in stages to make any problems introduced more
traceable. Stage 0 restores the full 'sh' state struct to its
former static glory and reverts 'shgd' as a separate entity.
src/cmd/ksh93/sh/defs.c,
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/include/shell.h
src/cmd/ksh93/Mamfile::
- Move 'struct sh_scoped' and 'struct limits' from defs.h to
shell.h as the sh struct will need their complete definitions.
- Get rid of 'struct shared' (shgd) in defs.h; its members are
folded back into their original place, the main Shell_t struct
(sh) in shell.h. There are no name conflicts.
- Get rid of the _SH_PRIVATE macro in defs.h. The members it
defines are now defined normally in the main Shell_t struct (sh)
in shell.h.
- To make this possible, move <history.h> and "fault.h" includes
from defs.h to shell.h and update the Mamfile accordingly.
- Turn sh_getinterp() and shgd into macros that resolve to (&sh).
This will allow the compiler to optimise out many pointer
dereferences already.
- Keep extern sh_getinterp() for libshell ABI compatibility.
src/cmd/ksh93/sh/init.c:
- sh_init(): Do not calloc (sh_newof) the sh or shgd structs.
- sh_getinterp(): Keep function for libshell ABI compat.
This commit backports a bugfix from ksh2020 to fix an ASan
heap-buffer-overflow error in one of the regression tests. See:
c57f7398https://github.com/att/ast/issues/1261
This explanation comes from the linked issue:
> The poplevel() in this block of code is called when lp->lexd.lex_max
> is zero:
> bd94eb56/src/cmd/ksh93/sh/lex.c (L921-L925)
> Since poplevel() first decrements lp->lexd.lex_max then uses it as
> an index into lp->lexd.lex_match this causes the word before the
> start of that buffer to be accessed. The buffer is allocated here:
> bd94eb56/src/cmd/ksh93/sh/lex.c (L2210-L2218)
src/cmd/ksh93/sh/lex.c:
- Avoid calling poplevel() twice when handling syntax errors.
Depending on the OS, the heredoc.sh regression tests, and possibly
others, still crashed with the -x option (xtrace) on.
Analysis: The lexer crashes in lex_advance(). Something has caused
an inconsistent lexer state, and it happened earlier on, so the
backtrace is useless for figuring out where that happened.
But I think I've found it. It's the sh_mactry() call here:
src/cmd/ksh93/sh/xec.c, lines 2800 to 2807 in f7213f03
2800: if(!(cp=nv_getval(sh_scoped(shp,PS4NOD))))
2801: cp = "+ ";
2802: else
2803: {
2804: sh_offoption(SH_XTRACE);
2805: cp = sh_mactry(shp,cp);
2806: sh_onoption(SH_XTRACE);
2807: }
sh_mactry() needs to parse the contents of $PS4 to perform
expansions and command substitutions in it, which involves the
lexer. If that happens in a here-document, the lexer is in the C
function call stack, in the middle of parsing the here-document.
Result: inconsistent lexer state. Solution: save and restore lexer
state in sh_mactry().
After this commit, all regression tests should pass with the
'-x'/'--xtrace' option in use, with no errors or crashes.
Note for backporters: this fix depends both on on d7cada7b and on
the consistency fix for the Lex_t type's size applied in a7ed5d9f.
src/cmd/ksh93/include/shlex.h:
- Cosmetic fix: remove a copied & pasted backslash. (re: a7ed5d9f)
src/cmd/ksh93/sh/macro.c: sh_mactry():
- Save and restore the lexer state before letting sh_mactrim()
indirectly parse and execute code.
src/cmd/ksh93/tests/*.sh:
- Turn off xtrace in various command substitutions that contain
2>&1 redirections, so that the xtrace output is not caught by
the command substitutions, causing tests to fail incorrectly.
- Turn off xtrace for a few code blocks with 2>&1 redirections,
stopping xtrace output from being written to standard output.
Resolves: https://github.com/ksh93/ksh/issues/306 (again)
On some systems (at least Linux and macOS):
1. Run on a command line: t=$(sleep 10|while :; do :; done)
2. Press Ctrl+C in the first 10 seconds.
3. Execute any other command substitution. The shell crashes.
Analysis: Something in the job_wait() call in the sh_subshell()
restore routine may be interrupted by a signal such as SIGINT on
Linux and macOS. Exactly what that interruptible thing is remains
to be determined. In any case, since job_wait() was invoked after
sh_popcontext(), interrupting it caused the sh_subshell() restore
routine to be aborted, resulting in an inconsistent state of the
shell. The fix is to sh_popcontext() at a later stage instead.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Rename struct checkpt buff to checkpoint because it's clearer.
- Move the sh_popcontext() call to near the end, just after
decreasing the subshell level counters and restoring the global
subshell data struct to its parent. This seems like a logical
place for it and could allow other things to be interrupted, too.
- Get rid of the if(shp->subshell) because it is known that the
value is > 0 at this point.
- The short exit routine run if the subshell forked now needs a new
sh_popcontext() call, because this is handled before restoring
the virtual subshell state.
- While we're here, do a little more detransitioning from all those
pointless shp pointers.
Fixes: https://github.com/ksh93/ksh/issues/397
The sh_close() function fails to set errno to EBADF when passed a
negative (invalid) file descriptor. This commit fixes the issue
by setting errno if the file descriptor is a negative value
(backported from ksh93v- 2012-08-24).
This takes another step towards cleaning up the build system. We
now do not even pretend to be theoretically compatible with
pre-1989 K&R C compilers or with C++ compilers. In practice, this
had already been broken for many years due to bit rot.
Commit 46593a89 already removed the license handling enormity that
depended on proto, so now we can cleanly remove it altogether. But
we do need to leave some backwards compatibility stubs to keep the
build system compatible with older AST code; it should remain
possible to build older ksh versions with the current build system
(the bin/ and src/cmd/INIT/ directories) for testing purposes.
So as of now there is no more __MANGLE__d rubbish in your generated
header files. This is only about a quarter of a century overdue...
This commit also includes a huge amount of code cleanup to remove
thousands of unused K&R C fallbacks and other cruft, particularly
in libast. This code base should now be a little easier to
understand for people who are familiar with a modern(ish) C
standard.
ratz is now also removed; this was a standalone and simplified 2005
version of gunzip. As of 6137b99a, none of our code uses it, even
theoretically. And the real g(un)zip is now everywhere.
src/cmd/INIT/proto.c, src/cmd/INIT/ratz.c:
- Removed.
COPYRIGHT:
- Remove zlib license; this only applied to ratz.
bin/package, src/cmd/INIT/package.sh:
- Related cleanups.
- Unset LC_ALL before invoking a new shell, respecting the user's
locale again and avoiding multibyte character corruption on the
command line.
src/cmd/INIT/proto.sh:
- Add stub for backwards compatibility with Mamfiles that depend on
proto. It does nothing but pass input without modification and is
now installed as the new arch/*/bin/proto by src/cmd/INIT/Mamfile.
src/cmd/INIT/iffe.sh:
- Ignore the proto-related -e (--package) and -p (--prototyped)
options; keep parsing them for backwards compatibility.
- Trim the macros passed to every test to their standard C
versions, removing K&R C and C++ versions. These are now
considered to be for backwards compatibility only.
src/cmd/INIT/iffe.tst:
- Remove proto(1) mangling code.
By the way, iffe can be regression-tested as follows:
$ bin/package use # set up environment in a child shell
$ regress src/cmd/INIT/iffe.tst
$ exit # leave package environment
src/cmd/INIT/make.probe, src/cmd/INIT/probe.win32:
- Remove code to handle C++.
src/lib/libast/features/common:
- As in iffe.sh above, trim macros designed for compatibility with
C++ and ancient C compilers to their standard C versions and
comment that they are for backwards compatibility with AST code.
This is needed to keep all the old ast and ksh code compiling.
src/cmd/ksh93/sh/init.c,
src/cmd/ksh93/sh/name.c:
- Clarify libshell ABI compatibility function versions of macros.
A "proto workaround" comment in the original code mislead me into
thinking this had something to do with the removed proto(1), but
it's unrelated. Call the workaround macro BYPASS_MACRO instead.
src/cmd/ksh93/include/defs.h:
- sh_sigcheck() macro: allow &sh as an argument: parenthesise shp.
src/cmd/ksh93/sh/nvtype.c:
- Remove unused nv_mkstruct() function. (re: d0a5cab1)
**/features/*:
- Remove obsolete iffe 'set prototyped' option.
**/Mamfile:
- Remove all references to the ast/prototyped.h header.
- Remove all use of the proto command. Simply copy instead.
*** 850-ish source files: ***
- Remove all '#pragma prototyped' directives.
- Remove all C++ compat code conditional upon defined(__cplusplus).
- Remove all use of the _ARG_ macro, which on standard C expands to
its argument:
#define _ARG_(x) x
(on K&R C, it expanded to nothing)
- Remove all use of _BEGIN_EXTERNS_ and _END_EXTERNS_ macros (empty
on standard C; this was for C++ compatibility)
- Reduce all #if __STD_C (standard code) #else (K&R code) #endif
blocks to the standard code only, without use of the macro.
- Same for _STD_ macro which seems to have had the same function.
- Change all instances of 'Void_t' to standard 'void'.
- sh/args.c: A process substitution run in a profile script may print
its PID as if it was a command spawned with '&'. Reproducer:
$ cat /tmp/env
true >(false)
$ ENV=/tmp/env ksh
[1] 730227
$
This bug is fixed by turning off the SH_PROFILE state while running
a process substitution.
- sh/subshell.c: The SH_INTERACTIVE fix in 3525535e renders the extra
check for SH_PROFILE redundant, so it has been removed.
- tests/io.sh: Update the procsub PIDs test to also check the result
after using process substitution in a profile script.
Reproducer: run vi in a subshell:
$ (vi)
vi opens; now press Ctrl+Z to suspend. The output is as expected:
[2] + Stopped (vi)
…but the exit status is 18 (SIGTSTP's signal number) instead of 0.
Now do:
$ fg
(vi)
$
The exit status is 18 again, vi is not resumed, and the job is
lost. You have to find vi's pid manually using ps and kill it.
Forking all non-command substitution subshells invoked from the
interactive main shell is the only reliable and effective fix I've
found. I've tried to fork the subshell conditionally in every other
remotely plausible place I can think of in fault.c and xec.c, but I
can't get anything to work properly. If anyone can get this to work
without forking as much (or at all), please do submit a patch or PR
that supersedes this fix.
At least subshells of subshells don't need to fork, so the
performance impact can be limited. Plus, it's not as if most people
need maximum speed on the interactive command line. Scripts
(including login/profile scripts) are not affected at all.
Command substitutions can be handled differently. My testing shows
that all shells except ksh93 simply block SIGTSTP (the ^Z signal)
while they run. We should do the same, so they don't need to fork.
NOTE for any backporters: the subshell.c and fault.c changes depend
on commits 35b02626 and 48ba6964 to work correctly.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- If the interactive shell state bit is on, then before executing
the subshell's code:
- for command substitutions, block SIGTSTP;
- for other subshells, fork.
- For command substitutions, release SIGTSTP if the interactive
shell state bit was on upon invoking the subshell.
src/cmd/ksh93/sh/fault.c:
- Instead of checking for a virtual subshell, check the shell's
interactive state bit to decide whether to handle SIGTSTP, as
that is only turned on in the interactive main shell.
src/cmd/ksh93/sh/main.c: sh_main():
- To avoid bugs, ignore SIGTSTP while running profile scripts.
Blocking it doesn't work because delaying it until after
sigrelease() will cause a crash. Thanks to @JohnoKing for this.
- While we're here, prevent a possible overflow of the 'beenhere'
static char variable by only incrementing it once.
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
Resolves: https://github.com/ksh93/ksh/issues/390
Reproducer:
$ (sleep 1& echo done)
done
$ (eval "echo hi"; sleep 1& echo done)
hi
[1] 30587
done
No job control output should be printed for a background process
invoked from a subshell, not even after 'eval'.
The cause: sh_parse() turns on the shell's interactive state bit
(sh_state(SH_INTERACTIVE)) if the interactive shell option is on.
This is incorrect. The parser should have no involvement with shell
interactivity in principle because that's not its domain.
Not only that, the parser may need to run in a subshell, e.g. when
executing traps or 'eval' commands (as above). By definition, a
subshell can never be interactive.
We already fixed many bugs related to job control and the shell's
interactive state. Even if these two lines previously papered over
some breakage, I can't find any now after simply removing them. If
any is found later, then it'll need to be fixed properly instead.
Related: https://github.com/ksh93/ksh/issues/390
With a better understanding of the code 1.5 years later, the
special-casing for IFS introduced in that commit seems like a hack.
The problem was not that the IFS node always exists but that it is
always considered to have a 'get' discipline function. Variables
with a 'get' discipline are considered set. This makes sense for
all variables except IFS.
The nv_isnull() macro is used to check if a variable is set. It
calls nv_hasget() to determine if the variable has a 'get'
discipline. So a better fix is for nv_hasget() always to return
false for IFS.
src/cmd/ksh93/bltins/test.c, src/cmd/ksh93/sh/macro.c:
- Remove special-casing for IFS.
src/cmd/ksh93/sh/nvdisc.c: nv_hasget():
- Always return false for IFS, taking local scope into account.
Strings compared in [[ with the > and < operators should be compared
lexically. This does not work when the strings are single digits, as
the parser interprets it as a syntax error:
$ [[ 10<2 ]] # 10 lexically sorts before 2
$ echo $?
0
$ [[ 1<2 ]]
/usr/bin/ksh: syntax error: `<' unexpected
$ echo $?
3
src/cmd/ksh93/sh/lex.c:
- Don't interpret numbers next to > and < as a redirection while
inside of [[. This bugfix was backported from ksh93v- 2014-06-25.
src/cmd/ksh93/tests/bracket.sh:
- Add regression tests for the > and < operators.
New version. I'm pretty sure the problems that forced me to revert
it earlier are fixed.
This commit mitigates the effects of the hack explained in the
referenced commit so that dummy built-in command nodes added by the
parser for declaration/assignment purposes do not leak out into the
execution level, except in a relatively harmless corner case.
Something like
if false; then
typeset -T Foo_t=(integer -i bar)
fi
will no longer leave a broken dummy Foo_t declaration command. The
same applies to declaration commands created with enum.
The corner case remaining is:
$ ksh -c 'false && enum E_t=(a b c); E_t -a x=(b b a c)'
ksh: E_t: not found
Since the 'enum' command is not executed, this should have thrown
a syntax error on the 'E_t -a' declaration:
ksh: syntax error at line 1: `(' unexpected
This is because the -c script is parsed entirely before being
executed, so E_t is recognised as a declaration built-in at parse
time. However, the 'not found' error shows that it was successfully
eliminated at execution time, so the inconsistent state will no
longer persist.
This fix now allows another fix to be effective as well: since
built-ins do not know about virtual subshells, fork a virtual
subshell into a real subshell before adding any built-ins.
src/cmd/ksh93/sh/parse.c:
- Add a pair of functions, dcl_hactivate() and dcl_dehacktivate(),
that (de)activate an internal declaration built-ins tree into
which check_typedef() can pre-add dummy type declaration command
nodes. A viewpath from the main built-ins tree to this internal
tree is added, unifying the two for search purposes and causing
new nodes to be added to the internal tree. When parsing is done,
we close that viewpath. This hides those pre-added nodes at
execution time. Since the parser is sometimes called recursively
(e.g. for command substitutions), keep track of this and only
activate and deactivate at the first level.
(Fixed compared to previous version of this commit: calling
dcl_dehacktivate() when the recursion level is already zero is
now a harmless no-op. Since this only occurs in error handling
conditions, who cares.)
- We also need to catch errors. This is done by setting libast's
error_info.exit variable to a dcl_exit() function that tidies up
and then passes control to the original (usually sh_exit()).
(Fixed compared to previous version of this commit: dcl_exit()
immediately deactivates the hack, no matter the recursion level,
and restores the regular sh_exit(). This is the right thing to
do when we're in the process of erroring out.)
- sh_cmd(): This is the most central function in the parser. You'd
think it was sh_parse(), but $(modern)-form command substitutions
use sh_dolparen() instead. Both call sh_cmd(). So let's simply
add a dcl_hacktivate() call at the beginning and a
dcl_deactivate() call at the end.
- assign(): This function calls path_search(), which among many
other things executes an FPATH search, which may execute
arbitrary code at parse time (!!!). So, regardless of recursion
level, forcibly dehacktivate() to avoid those ugly parser side
effects returning in that context.
src/cmd/ksh93/bltins/enum.c: b_enum():
- Fork a virtual subshell before adding a built-in.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Fork a virtual subshell when detecting typeset's -T option.
Improves fix to https://github.com/ksh93/ksh/issues/256
The killpg(getpgrp(),SIGINT) call added to ed_getchar() in that
commit caused the interactive shell to exit on ^C even if SIGINT is
being ignored. We cannot revert or remove that call without
breaking job control. This commit applies a new fix instead.
Reproducers fixed by this commit:
SIGINT ignored by child:
$ PS1='childshell$ ' ksh
childshell$ trap '' INT
childshell$ (press Ctrl+C)
$
SIGINT ignored by parent:
$ (trap '' INT; ENV=/./dev/null PS1='childshell$ ' ksh)
childshell$ (press Ctrl+C)
$
SIGINT ignored by parent, trapped in child:
$ (trap '' INT; ENV=/./dev/null PS1='childshell$ ' ksh)
childshell$ trap 'echo test' INT
childshell$ (press Ctrl+C)
$
I've experimentally determined that, under these conditions, the
SFIO stream error state is set to 256 == 0400 == SH_EXITSIG.
src/cmd/ksh93/sh/main.c: exfile():
- On EOF or error, do not return (exiting the shell) if the shell
state is interactive and if sferror(iop)==SH_EXITSIG.
- Refactor that block a little to make the new check fit in nicely.
src/cmd/ksh93/tests/pty.sh:
- Test the above three reproducers.
Fixes: https://github.com/ksh93/ksh/issues/343
This commit fixes an issue with how ksh was obtaining the value of
NGROUPS_MAX. On some systems this setting can be changed (e.g., on
illumos adding 'set ngroups_max=32' to /etc/system then rebooting
changes NGROUPS_MAX from 16 to 32). Ksh was using NGROUPS_MAX with
the assumption it's a static value, which could cause issues on
systems where it isn't static. This bugfix is inspired by the one
from <b1362c3a5>, although it
has been expanded a bit to account for OPEN_MAX as well.
src/cmd/ksh93/sh/init.c, src/lib/libcmd/fds.c:
- Rename the getconf() macro to astconf_long() and move it to ast.h
to prevent redundancy. Other sections of the code have been
modified to use this macro for astconf() to account for
dynamic settings.
- An equivalent macro for unsigned long values (astconf_ulong) has
been added.
- Prefer sysconf(3) where available. It has better performance as it
returns a numeric value directly instead of via string
conversion.
- The astconf_long and astconf_ulong macros have been documented in
the ast(3) man page.
Turns out there is a bona fide, honest-to-goodness use case for
matching '.' and '..' in globbing after all. It's when globbing is
used as the backend mechanism for file name completion in
interactive shell editors. A tab invisibly adds a * at the end of
the word to the left of your cursor and the resulting pattern is
expanded. In 5312a59d, this broke for '.' and '..'.
Typing '.' followed by two tabs should result in a menu that
includes './' and '../'. Typing '..' followed by a tab should
result in '../', (or a menu that includes it if there are files
with names starting with '..'). This is the behaviour in 93u+ and
we should maintain this.
To restore this functionality without reintroducing the harmful
behaviour fixed in the referenced commits, we should special-case
this, allowing '.' and '..' to match only for file name completion.
src/lib/libast/include/glob.h:
- Fix an inaccurate comment: the GLOB_COMPLETE flag is used for
command completion, not file name completion. This is very clear
from reading the path_expand() function in sh/expand.c.
- Add new GLOB_FCOMPLETE flag for file name completion.
src/lib/libast/misc/glob.c:
- Adapt flags mask to fit the new flag.
- glob_dir(): If GLOB_FCOMPLETE is passed, allow '.' and '..' to
match even if expanded from a pattern.
- Clarify the fix from aad74597 with an extended comment based on
<https://github.com/ksh93/ksh/issues/146#issuecomment-790991990>.
src/cmd/ksh93/sh/expand.c: path_expand():
- If we're in the SH_FCOMPLETE (file name completion) state, then
pass the new GLOB_FCOMPLETE flag to AST glob(3).
Fixes: https://github.com/ksh93/ksh/issues/372
Thanks to @fbrau for the bug report.
This commit adds onto <https://github.com/ksh93/ksh/pull/353> by porting
over two additional improvements to the shell linter:
1) The changes in the aforementioned pull request were merged into
illumos-gate with an additional change.[*] The illumos revision of
the patch improved the warning for (( $foo = $? )) to specify '$foo'
causes the warning.[**] Example:
$ ksh -n -c '(( $? != $bar ))'
ksh: warning: line 1: in '(( $? != $bar ))', using '$' as in '$bar' is slower and can introduce rounding errors
While I was porting the illumos patch I did notice one problem. The
string it uses from paramsub() skips over the initial '{' in
'${var}', resulting in the warning printing '$var}' instead:
$ ksh -n -c '(( ${.sh.pid} != $$ ))'
... in '(( ${.sh.pid} != $$ ))', using '$' as in '$.sh.pid}' is slower ...
This was fixed by including the missing '{' in the string returned by
paramsub for ${var} variables.
2) In ksh93v-, parsing x=$((expr)) with the shell linter will cause ksh
to warn the user x=$((expr)) is slower than ((x=expr)). This
improvement has been backported with a modified warning:
# Result from this commit
$ ksh -n -c 'x=$((1 + 2))'
ksh: warning: line 1: x=$((1 + 2)) is slower than ((x=1 + 2))
# Result from ksh93v-
$ ksh93v -n -c 'x=$((1 + 2))'
ksh93v: warning: line 1: ((x=1 + 2)) is more efficient than x=$((1 + 2))
Minor note: the ksh93v- patch had an invalid use of memcmp; this
version of the patch uses strncmp instead.
References:
be548e87bc65722363_22fdf8e7/
*** Crash 1: ***
ksh crashed if the PS1 prompt contains one or more command
substitutions and you enter a multi-line command substitution
on the command line, then interrupt while on the PS2 prompt.
$ ENV=/./dev/null /usr/local/bin/ksh -o emacs
$ PS1='$(echo foo) $(echo bar) $(echo baz) ! % '
foo bar baz 16999 % echo $(
> true <-- here, press Ctrl+C instead of Return
Memory fault
The crash occurred due to a corrupted lexer state while trying to
display the PS1 prompt.
Analysis: My fix for the crashing bug with Ctrl+C in commit
3023d53b is incorrect and only worked accidentally. sh_fault() is
not the right place to reset the lexer state because, when we press
Ctrl+C on a PS2 prompt, ksh had been waiting for input to finish
lexing a multi-line command, so sh_lex() and other lexer functions
are on the function call stack and will be returned to.
src/cmd/ksh93/sh/fault.c: sh_fault():
- Remove incorrect SIGINT fix.
src/cmd/ksh93/sh/io.c: io_prompt():
- Reset the lexer state immediately before printing every PS1
prompt. Even in situations where this is redundant it should be
perfectly safe, the overhead is negligible, and it resolves this
crash. It may pre-empt other problems as well.
*** Crash 2: ***
If an INT trap is set, and you start entering a multi-line command
substitution, then press Ctrl+C on the PS2 prompt to trigger the
crash, the lexer state is corrupted because the lexer is invoked to
eval the trap action. A crash then occurs on entering the final ')'
of the command substitution.
$ trap 'echo TRAPPED' INT
$ echo $(
> trueTRAPPED <-- press Ctrl+C to output "TRAPPED"
> )
Memory fault
Technically, as SIGINT is trapped, it should not interrupt, so ksh
should execute the trap, then continue with the PS2 prompt to let
the user finish inputting the command. But I have been unsuccessful
in many different attempts to make this work properly. I managed to
get multi-line command substitutions to lex correctly by saving and
restoring the lexer state, but command substitutions were still
corrupted at the parser and/or execution level and I have not
managed to trace the cause of that.
My testing showed that all other shells interrupt the PS2 prompt
and return to PS1 when the user presses Ctrl+C, even if SIGINT is
trapped. I think that is a reasonable alternative, and it is
something I managed to make work.
src/cmd/ksh93/sh/fault.c: sh_chktrap():
- Immediately after invoking sh_trap() to run a trap action, check
if we're in a PS2 prompt (sh.nextprompt == 2). If so, assume the
lexer state is now overwritten. Closing the fcin stream with
fcclose() seems to reliably force the lexer to stop doing
anything else. Then we can just reset the prompt to PS1 and
invoke sh_exit() to start new command line, which will now reset
the lexer state as per above.
ksh crashed if you pressed Ctrl+C or Ctrl+D on a PS2 prompt while
you haven't finished entering a $(command substitution). It
corrupts subsequent command substitutions. Sometimes the situation
recovers, sometimes the shell crashes.
Simple crash reproducer:
$ PS1="\$(echo foo) \$(echo bar) \$(echo baz) > "
foo bar baz > echo $( <-- now press Ctrl+D
> ksh: syntax error: `(' unmatched
Memory fault
The same happens with Ctrl+C, minus the syntax error message.
The problem is that the lexer state becomes inconsistent when the
lexer is interrupted in the middle of reading a command
substitution of the form $( ... ). This is tracked in the
'lexd.dolparen' variable in the lexer state struct.
Resetting that variable is sufficient to fix this issue. However,
in this commit I prefer to just reinitialise the lexer state
completely to pre-empt any other possible issues. Whether there was
a syntax error or the user pressed Ctrl+C, we just interrupted all
lexing and parsing, so the lexer *should* restart from scratch.
src/cmd/ksh93/sh/fault.c: sh_fault():
- If the shell is in an interactive state (e.g. not a subshell) and
SIGINT was received, reinitialise the lexer state. This fixes the
crash with Ctrl+C.
src/cmd/ksh93/sh/lex.c: sh_syntax():
- When handling a syntax error, reset the lexer state. This fixes
the crash with Ctrl+D.
NEWS:
- Also add the forgotten item for the previous fix (re: 2322f939).
This reverts c0334e32, thereby restoring 936a1939.
After the fixes in 0a343244 and a2bc49be, the tilde expansion
disciplines work nicely, so they can come back to the 1.0 branch.
This should fix various crashes that remain, at least:
* when running a PS2 discipline at parse time
* when pressing Ctrl+C on a PS2 prompt
* when a special builtin within a discipline throws an error within
a virtual subshell
src/cmd/ksh93/sh/nvdisc.c:
- In both assign() which handles .set disciplines and lookup()
which handles .get disciplines, to stop errors in discipline
functions from wreaking havoc:
- Save, reinitialise and restore the lexer state in case the
discipline is run at parse time. This happens with PS2; I'm not
currently aware of other contexts but that doesn't mean there
aren't any or that there won't be any. Plus, I determined by
experimenting that doing this here seems to be the only way to
make it work reliably. Thankfully the overhead is low.
- Check the topfd redirection state and run sh_iorestore() if
needed. Without this, if a special builtin with a redirection
throws an error in a discipline function, its redirection(s)
remain permanent. For example, 'trap --bad-option 2>/dev/null'
in a PS2.get() discipline would kill standard error, including
all your prompts.
src/cmd/ksh93/sh/io.c: io_prompt():
- Before getting the value of the PS2 prompt, save the stack state
and restore it after. This stops a PS2.get discipline function
from corrupting a command substitution that the user is typing.
Doing this in assign()/lookup() is ineffective, so do it here.
Fixes: https://github.com/ksh93/ksh/issues/347
Once upon a time it might have been possible to build certain parts
of ksh, such as the emacs and vi editors and possibly even the
name/value library (nval(3)) as independent libraries. But given
the depressing amount of bit rot in the code that we inherited, I
am certain that disabling either of these macros had been resulting
in a broken build for many years before AT&T abandoned this code
base. These are certainly not going to be useful now.
Meanwhile the KSHELL macro got in the way of me today, because the
Mamfile did not define it for all the .c files, but some headers
declared some functionality conditionally upon that macro. So
including <io.h> in, e.g., nvdisc.c did not declare the same
functions as including that header in files with KSHELL defined.
This inconsistency is now gone as well, for various files.
I'm currently working on making it possible once again to build
libshell as a dynamic library; that should be good enough. And that
never involved disabling either of these macros.
List of changes:
- Fixed some -Wuninitialized warnings and removed some unused variables.
- Removed the unused extern for B_login (re: d8eba9d1).
- The libcmd builtins and the vmalloc memfatal function now handle
memory errors with 'ERROR_SYSTEM|ERROR_PANIC' for consistency with how
ksh itself handles out of memory errors.
- Added usage of UNREACHABLE() where it was missing from error handling.
- Extend many variables from short to int to prevent overflows (most
variables involve file descriptors).
- Backported a ksh2020 patch to fix unused value Coverity issues
(https://github.com/att/ast/pull/740).
- Note in src/cmd/ksh93/README that ksh compiles with Cygwin on
Windows 10 and Windows 11, albeit with many test failures.
- Add comments to detail some sections of code. Extensive list of
commits related to this change:
ca2443b5, 7e7f1372, 2db9953a, 7003aba4, 6f50ff64, b1a41311,
222515bf, a0dcdeea, 0aa9e03f, 61437b27, 352e68da, 88e8fa67,
bc8b36fa, 6e515f1d, 017d088c, 035a4cb3, 588a1ff7, 6d63b57d,
a2f13c19, 794d1c86, ab98ec65, 1026006d
- Removed a lot of dead ifdef code.
- edit/emacs.c: Hide an assignment to avoid a -Wunused warning. (See
also https://github.com/att/ast/pull/753, which removed the assignment
because ksh2020 removed the !SHOPT_MULTIBYTE code.)
- sh/nvdisc.c: The sh_newof macro cannot return a null pointer because
it will instead cause the shell to exit if memory cannot be allocated.
That makes the if statement here a no-op, so remove it.
- sh/xec.c: Fixed one unused variable warning in sh_funscope().
- sh/xec.c: Remove a fallthrough comment added in commit ed478ab7
because the TFORK code doesn't fall through (GCC also produces no
-Wimplicit-fallthrough warning here).
- data/builtins.c: The cd and pwd man pages state that these builtins
default to -P if PATH_RESOLVE is 'physical', which isn't accurate:
$ /opt/ast/bin/getconf PATH_RESOLVE
physical
$ mkdir /tmp/dir; ln -s /tmp/dir /tmp/sym
$ cd /tmp/sym
$ pwd
/tmp/sym
$ cd -P /tmp/sym
$ pwd
/tmp/dir
The behavior described by these man pages isn't specified in the ksh
man page or by POSIX, so to avoid changing these builtin's behavior
the inaccurate PATH_RESOLVE information has been removed.
- Mamfiles: Preserve multi-line errors by quoting the $x variable.
This fix was backported from 93v-.
(See also <a7e9cc82>.)
- sh/subshell.c: Remove set but not used sp->errcontext variable.
When a global EXIT trap is set, and a ksh-style function exits with
a status > 256 that could have been the result of a signal, then
the shell incorrectly issues that signal to itself. Depending on
the signal, this causes ksh to terminate itself ungracefully:
$ cat /tmp/exit267
trap 'echo OK' EXIT # This trap triggers the crash
function foo { return 267; }
foo
$ bash /tmp/exit267
OK
$ ksh-3aee10d7 /tmp/exit267
OK
$ ksh /tmp/exit267
Memory fault(coredump)
On most systems, status 267 corresponds to SIGSEGV. The reported
memory fault is not real; it results from ksh incorrectly killing
itself with that signal.
The problem is caused by two factors:
1. As of 93u+ 2012-08-01, ksh explicitly allows 'return' to use an
exit status corresponding to a signal (from 257 to end of signal
range). The rest of the integer range is trunctated to 8 bits.
This is contrary to both 'man ksh' and 'return --man' which both
say it's always truncated to 8 bits. Plus, combined with point 2
below, this new behaviour is nonsensical, as 'return' has no
business actually generating signals. However, a couple of
regression tests now depend on this, as may some scripts.
2. When a ksh-style function does not handle a signal, the signal
is passed down to the parent environment and ksh does this by
reissuing the signal to its own process after leaving the
function scope. However, it does this by checking the exit
status, which is very bad practice as there is no guarantee
that an exit status corresponding to a signal was in fact
produced by a signal, particularly after they changed the
behaviour of 'return' per 1 above.
This commit fixes both issues. It also takes a proper decision on
allowable 'return' exit status arguments. Since 93u+ was released
nearly a decade ago and some scripts may now rely on being able to
pass certain exit statuses out of the 8-bit range, we should not
disallow this now. But neither should we be half-hearted in
allowing only some arbitrary selection of 9-bit statuses; 'return'
values categorically should have nothing to do with signals, so
this is no basis for limiting them. We're now allowing the full
unsigned integer range, which is usually 32 bits. This is like zsh,
and may create some interesting possibilities for scripts.
Just don't forget that $? will still lose all but its 8 least
significant bits when leaving the current (sub)shell environment.
src/cmd/ksh93/sh/xec.c: sh_funscope():
- Fix passing down unhandled signals from interrupted ksh functions
(jumpval==SH_JMPFUN) to the parent environment. Do not pay any
attention to the exit status. Instead, use sh.lastsig (a.k.a.
shp->lastsig). It is set by sh_fault() in fault.c for just this
purpose and contains the last signal handled for the current
command. It is reset in sh_exec() before running any new command.
So if it contains a signal, that is the one that interrupted the
ksh function, so it's the correct one to pass down. (Further
evidence: sh_subshell() was already using this in the same way.)
src/cmd/ksh93/bltins/cflow.c: b_return():
- Allow any signed int return value when invoked as and behaving
like 'return'.
- Add warning if a passed value is out of int range. Set the exit
status to 128 in that case; int overflow is undefined behaviour
in C and we want consistent behaviour across platforms. It should
be safe enough to check if the long and int values are equal.
- Refactor for clarity.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- If a function returns with a status out of the 8 bit range in a
virtual subshell, this status could be passed down to the parent
shell in full. However, if the subshell forks, then the kernel
will enforce an 8-bit exit status. That is inconsistent. Scripts
should not be able to tell the difference between forked and
non-forked subshells, so artificially enforce that limit here.
Other changed files:
- Documentation updates and copy-edits.
- Update an AT&T functions.sh regress test to allow arbitrary
integer return values for functions.
- Add regression tests based in part on @JohnoKing's reproducers.
- Rework some vaguely related regression tests to fail gracefully.
Thanks to Johnothan King for the report and the testing.
Fixes: https://github.com/ksh93/ksh/issues/364
This patch fixes the crashes experienced when a discipline function
exited because of a signal or an error from a special builtin. The
crashes were caused by ksh entering an inconsistent state after
performing a longjmp away from the assign() and lookup() functions in
nvdisc.c. Fixing the crash requires entering a new context, then setting
a nonlocal goto with sigsetjmp(3). Any longjmps that happen while
running the discipline function will go back to assign/lookup, allowing
ksh to do a proper cleanup afterwards.
Resolves: https://github.com/ksh93/ksh/issues/346
If a bug is ever introduced that causes a [[ ... ]] operator to be
unhandled by the linter, we should at least avoid writing random
memory contents to standard error. In non-release builds, let's
abort() so the problem can be more easily backtraced.
This commit ports over two improvements to the shell linter from
illumos (original patch written by Andy Fiddaman). Links to the
relevant bug reports and the original patch:
https://www.illumos.org/issues/13601https://www.illumos.org/issues/13631c7b656fc71
The first improvement is to the lint warning for arithmetic
operators in [[ ... ]]. The ksh linter now suggests the correct
equivalent operator to use in ((...)). Example:
$ ksh -nc '[[ 30 -gt 25 ]]'
# Original warning
warning: line 1: -gt within [[ ... ]] obsolete, use ((...))
# New warning
warning: line 1: [[ ... -gt ... ]] obsolete, use ((... > ...))
The second improvement pertains to variable expansion in arithmetic
expressions. The ksh linter now suggests referencing variable names
directly:
$ ksh -nc 'integer foo=40; (($foo < 50 ))'
# Old warning
warning: line 1: variable expansion makes arithmetic evaluation less efficient
# New warning
warning: line 1: in '(($foo < 50))', using '$' is slower and can introduce rounding errors
src/cmd/ksh93/{data/lexstates,sh/lex,sh/parse}.c:
- Port the improved shell lint warnings from illumos to ksh93u+m.
- The original checks for arithmetic operators involved a bunch of
if statements with inefficient calls to strcmp(3). These were
replaced with a more efficient switch statement that avoids
strcmp.
This change fixes a crash that can occur after setting a KEYBD trap
then inputting a multi-line command substitution. The crash is
similar to issue #347, but it's easier to reproduce since it
doesn't require you to setup a kshrc file. Reproducer for the
crash:
$ ENV=/./dev/null ksh
$ trap : KEYBD
$ : $(
> true)
Memory fault(coredump)
The bugfix was backported (with considerable changes) from ksh93v-
2013-10-08. The crash was first reported on the old mailing list:
https://www.mail-archive.com/ast-users@lists.research.att.com/msg00313.html
src/cmd/ksh93/{include/shlex.h,sh/lex.c}:
- To fix this properly, we need sizeof(Lex_t) to work as expected
in edit.c, but that is thwarted by the _SHLEX_PRIVATE macro in
lex.c which shlex.h uses to add private structs to the Lex_t type
in lex.c only. So get rid of that _SHLEX_PRIVATE macro and make
those members part of the centrally defined struct, renaming them
to make it clear they're considered private to lex.c.
src/cmd/ksh93/edit/edit.c:
- Now that we can get its size, save and restore the shell lexing
context when a KEYBD trap is present.
src/cmd/ksh93/tests/pty.sh:
- Add a regression test for the KEYBD trap crash.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This bug was first reported in <https://www.illumos.org/issues/7694>.
The time keyword currently overrides the errexit shell option,
allowing failing scripts to continue after an error:
$ cat 1.sh
#!/bin/sh
time false # This should cause the script to exit
echo FAILURE
true
$ ksh -o errexit 1.sh
real 0m0.00s
user 0m0.00s
sys 0m0.00s
FAILURE
src/cmd/ksh93/sh/xec.c:
- When the time keyword runs a command, pass the errexit state flag
to the sh_exec call. This state flag is required for ksh to exit
when a command fails while the errexit option is on.
src/cmd/ksh93/tests/basic.sh:
- Add a regression test based on the reproducer.
This reverts commit 2b9cbbbc8e.
This is not ready for prime time. Crashses when running a $PS2
discipline function. This needs fixing and more testing in
development before making it into the 1.0 branch. In the meantime,
that terrible problem with types is back, sorry about that.
This commit mitigates the effects of the hack explained in the
referenced commit so that dummy built-in command nodes added by the
parser for declaration/assignment purposes do not leak out into the
execution level, except in a relatively harmless corner case.
Something like
if false; then
typeset -T Foo_t=(integer -i bar)
fi
will no longer leave a broken dummy Foo_t declaration command. The
same applies to declaration commands created with enum.
The corner case remaining is:
$ ksh -c 'false && enum E_t=(a b c); E_t -a x=(b b a c)'
ksh: E_t: not found
Since the 'enum' command is not executed, this should have thrown
a syntax error on the 'E_t -a' declaration:
ksh: syntax error at line 1: `(' unexpected
This is because the -c script is parsed entirely before being
executed, so E_t is recognised as a declaration built-in at parse
time. However, the 'not found' error shows that it was successfully
eliminated at execution time, so the inconsistent state will no
longer persist.
This fix now allows another fix to be effective as well: since
built-ins do not know about virtual subshells, fork a virtual
subshell into a real subshell before adding any built-ins.
src/cmd/ksh93/sh/parse.c:
- Add a pair of functions, dcl_hactivate() and dcl_dehacktivate(),
that (de)activate an internal declaration built-ins tree into
which check_typedef() can pre-add dummy type declaration command
nodes. A viewpath from the main built-ins tree to this internal
tree is added, unifying the two for search purposes and causing
new nodes to be added to the internal tree. When parsing is done,
we close that viewpath. This hides those pre-added nodes at
execution time. Since the parser is sometimes called recursively
(e.g. for command substitutions), keep track of this and only
activate and deactivate at the first level.
- We also need to catch errors. This is done by setting libast's
error_info.exit variable to a dcl_exit() function that tidies up
and then passes control to the original (usually sh_exit()).
- sh_cmd(): This is the most central function in the parser. You'd
think it was sh_parse(), but $(modern)-form command substitutions
use sh_dolparen() instead. Both call sh_cmd(). So let's simply
add a dcl_hacktivate() call at the beginning and a
dcl_deactivate() call at the end.
- assign(): This function calls path_search(), which among many
other things executes an FATH search, which may execute arbitrary
code at parse time (!!!). So, regardless of recursion level,
forcibly dehacktivate() to avoid those ugly parser side effects
returning in that context.
src/cmd/ksh93/bltins/enum.c: b_enum():
- Fork a virtual subshell before adding a built-in.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Fork a virtual subshell when detecting typeset's -T option.
Improves fix to https://github.com/ksh93/ksh/issues/256
Symptom:
$ ksh -c 'command enum -i P_t=(a b); P_t -A v=([f]=b); typeset -p v'
ksh: syntax error at line 1: `(' unexpected
Expected: no syntax error, and output of 'P_t -A v=([f]=b)'.
src/cmd/ksh93/sh/parse.c: check_typedef():
- For enum, skip over any possible 'command' prefixes before
pre-parsing options with optget (or, technically, skip anything
else that might come before 'enum', though I don't think anything
else is possible).
- The sh_addbuiltin() call at the end to pre-add the builtin
obtained the node pointer to the built-in and the node flags from
the parser tree. This did not work if a 'command' prefix was
present. However, we don't actually need this. For parsing
purposes, the BLT_DCL flag for a declaration built-in is
sufficient; this is what gets the parser to accept
assignment-arguments including parentheses. So just apply that.
In addition, let's point it to an actual dummy built-in, 'true'
(SYSTRUE), so that if a user does run something like 'if false;
then enum Foo_t=(...); fi', the leaked Foo_t dummy at least won't
do anything (not even crash).
There is quite a bit of no-op code in the job_hup() function due
to conditions that always test false. This commit removes that code
and clarifies the rest, making the purpose of this function clear.
job_hup() (before 62cf88d0: job_terminate()) is called via
job_walk() by sh_done() in fault.c to issue SIGHUP, the "hang up"
signal, to every background job's process group when the current
session is ungracefully disconnected. (One way to trigger such a
disconnection is to forcibly terminate a ssh session by typing '~.'
on a new prompt.)
The bug that Solaris patch 260-22964338 fixed is that ksh then
killed all non-disowned jobs' process groups without considering
that ksh still remembers a job even when all its processes are
finished (have the P_DONE flag). In that condition, the process
group ID may well be reused by another process by now, so it is
dangerous to killpg() it; we risk killing unrelated processes!
This is *not* a hypothetical problem; the Solaris patch exists
because this happened to a Solaris customer. However, the bug
exists on all operating systems. It's rarely triggered but serious,
and it's more likely to occur on heavy workloads that re-use
process/group IDs a lot. And it's on every currently released
non-Solaris version of ksh93. Eesh.
src/cmd/ksh93/sh/jobs.c:
src/cmd/ksh93/include/jobs.h:
- Remove job_terminate() which was unused as of 62cf88d0.
It could have been fixed instead of replaced. Oh well.
- Refactor job_hup():
- Remove code that will never be executed because, at
those points, it is known that pw->p_pgrp != 0.
- Simplify the loop that checks that there is at least
one non-P_DONE process so it doesn't need a flag.
For documentation purposes, below is a reproducer for the bug
before the Solaris patch. It is rather involved.
1. Compile the C program below (cpid).
2. In one terminal, 'ssh localhost'.
3. Within the ssh session:
- 'exec -a-ksh /path/to/buggy/ksh' to get a ksh login shell.
- 'sleep 1 &' and let it finish. Note down the reported PID.
That is the one we will reuse. Let's say 26650.
4. In another terminal, run: ./cpid 26650 (the PID from the
previous step). Now wait until it says "PID 26650 is ready"; it
has now succeeded at re-using that PID, and will just sit there.
This process will never voluntarily terminate. If we have the
bug, the termination of this process will be the symptom.
5. In the first terminal, forcibly terminate the ssh session by
typing, on a new prompt: ~. (tilde, dot). This triggers the
buggy routine to issue SIGHUP to all of ksh's background jobs.
6. In the second terminal, the bug is reproduced if cpid has been
terminated, reporting 'waitpid return 26650, status 0x0001', so
ksh just killed this process that it had nothing to do with.
(Note that status 0x0001 refers to being killed by signal 1
which is SIGHUP.)
cpid.c follows (written by George Lijo, tweaked by me):
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <sys/wait.h>
int main(int argc, char *argv[])
{
pid_t pid, rpid, opid;
int i, status, npid;
if (argc != 2)
{
fprintf(stderr, "Usage: cpid <PID to re-use>\n");
exit(1);
}
rpid = atoi(argv[1]);
opid = getpid();
for (;;)
{
if ((pid = fork()) == 0)
{
setpgrp();
pause();
_exit(0);
}
if (pid == rpid)
break;
kill(pid, SIGKILL);
waitpid(pid, NULL, 0);
if (opid < rpid && pid > rpid)
printf("Cannot create PID %d\n", rpid);
opid = pid;
}
printf("PID %d is ready\n", pid);
i = waitpid(pid, &status, 0);
printf("waitpid return %d, status 0x%4.4x\n", i, status);
return status;
}
Since the arithmetic recursion level only becomes incorrect when
an error interrupts the arithmetic subsystem, and all such error
messages call sh_exit(), it should be good enough to reset it
there, so we don't need to do that for nearly every sh_exec() run.
This function adds the NV_ADD flag to its 'flags' variable for
nv_serach() calls subject to some checks. However, every call that
uses that variable explicitly turns off the NV_ADD bit again.
A search in the ast-open-history repo reveals that this check
briefly made a difference between versions 2010-06-25 and
2010-08-11, but it's been a complete no-op ever since.
src/cmd/ksh93/sh/arith.c: scope():
- Remove no-op code.
- Resolve the constant expressions involving the 'flags' variable,
get rid of the variable, and just indicate the flag bitmasks
directly in the nv_search() calls.
- Detangle and split up the excessively long 'if' construct.
No change in behaviour.
Previously noticed by Kurtis Rader for ksh2020:
d5ce3b05
Defining a .sh.tilde.get or .sh.tilde.set discipline function to
extend tilde expansion works well as long as the discipline
function doesn't get interrupted (e.g. with Crtl+C) or produce an
error message. Either of those will cause the shell to become
unstable and crash.
This feature is now removed from the 1.0 branch as it is not ready
for prime time. It can return to a release branch if/when we manage
to fix it on the master branch.
Related: https://github.com/ksh93/ksh/issues/346
In 93v-/ksh2020, namespace defs in any function are a syntax error.
This commit blocks namespace defs for ksh functions only, at the
execution level. This follows some of AT&T original intention while
working around some of the known bugs with namespaces.
Related: https://github.com/ksh93/ksh/issues/325
