src/lib/libast/features/standards:
- Do not emit #defines for the typ u_long test which is only used
as a heuristic in subsequent tests in this file. (Note that 'set'
can set and unset any iffe command-line --option at runtime.)
- Remove definition of _ISOC99_SOURCE macro. This is another old
GNU thing; feature_test_macros(7) says invoking the compiler with
the option -std=c99 has the same effect. But modern GCC has C11
with GNU extensions as the default, which is fine. If a
particular standard is desired, pass a -std=... flag in $CC.
src/cmd/ksh93/features/rlimits:
- Remove overlooked Linux *64* types/functions hackery.
After defining standards macros it caused a build failure
on at least one version of Void Linux (but not 5.15.14_1).
Thanks to @JohnoKing for the report.
src/cmd/ksh93/sh/subshell.c,
src/lib/libdll/dllnext.c:
- Remove now-redundant local definitions of _GNU_SOURCE and
__EXTENSIONS__ macros.
src/cmd/ksh93/tests/builtins.sh:
- Fix broken sed invocation (re: 41829efa).
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 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.
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
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
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 commit fixes an issue I found in the subshell $RANDOM
reseeding code.
The main issue is a performance regression in the shbench fibonacci
benchmark, introduced in commit af6a32d1. Performance dropped in
this benchmark because $RANDOM is always reseeded and restored,
even when it's never used in a subshell. Performance results from
before and after this performance fix (results are on Linux with
CC=gcc and CCFLAGS='-O2 -D_std_malloc'):
$ ./shbench -b bench/fibonacci.ksh -l 100 ./ksh-0f06a2e ./ksh-af6a32d ./ksh-f31e368 ./ksh-randfix
benchmarking ./ksh-0f06a2e, ./ksh-af6a32d, ./ksh-f31e368, ./ksh-randfix ...
*** fibonacci.ksh ***
# ./ksh-0f06a2e # Recent version of ksh93u+m
# ./ksh-af6a32d # Commit that introduced the regression
# ./ksh-f31e368 # Commit without the regression
# ./ksh-randfix # Ksh93u+m with this patch applied
-------------------------------------------------------------------------------------------------
name ./ksh-0f06a2e ./ksh-af6a32d ./ksh-f31e368 ./ksh-randfix
-------------------------------------------------------------------------------------------------
fibonacci.ksh 0.481 [0.459-0.515] 0.472 [0.455-0.504] 0.396 [0.380-0.442] 0.407 [0.385-0.439]
-------------------------------------------------------------------------------------------------
src/cmd/ksh93/include/variables.h,
src/cmd/ksh93/sh/{init,subshell}.c:
- Rather than reseed $RANDOM every time a subshell is created, add
a sh_save_rand_seed() function that does this only when the
$RANDOM variable is used in a subshell. This function is called
by the $RANDOM discipline functions nget_rand() and put_rand().
As a minor optimization, sh_save_rand_seed doesn't reseed if it's
called from put_rand().
- Because $RANDOM may have a seed of zero (i.e., RANDOM=0),
sp->rand_seed isn't enough to tell if $RANDOM has been reseeded.
Add sp->rand_state for this purpose.
- sh_subshell(): Only restore the former $RANDOM seed and state if
it is necessary to prevent a subshell leak.
src/cmd/ksh93/tests/variables.sh:
- Add two regression tests for bugs I ran into while making this
patch.
In C/POSIX arithmetic, a leading 0 denotes an octal number, e.g.
010 == 8. But this is not a desirable feature as it can cause
problems with processing things like dates with a leading zero.
In ksh, you should use 8#10 instead ("10" with base 8).
It would be tolerable if ksh at least implemented it consistently.
But AT&T made an incredible mess of it. For anyone who is not
intimately familiar with ksh internals, it is inscrutable where
arithmetic evaluation special-cases a leading 0 and where it
doesn't. Here are just some of the surprises/inconsistencies:
1. The AT&T maintainers tried to honour a leading 0 inside of
((...)) and $((...)) and not for arithmetic contexts outside it,
but even that inconsistency was never quite consistent.
2. Since 2010-12-12, $((x)) and $(($x)) are different:
$ /bin/ksh -c 'x=010; echo $((x)) $(($x))'
10 8
That's a clear violation of both POSIX and the principle of
least astonishment. $((x)) and $(($x)) should be the same in
all cases.
3. 'let' with '-o letoctal' acts in this bizarre way:
$ set -o letoctal; x=010; let "y1=$x" "y2=010"; echo $y1 $y2
10 8
That's right, 'let y=$x' is different from 'let y=010' even
when $x contains the same string value '010'! This violates
established shell grammar on the most basic level.
This commit introduces consistency. By default, ksh now acts like
mksh and zsh: the octal leading zero is disabled in all arithmetic
contexts equally. In POSIX mode, it is enabled equally.
The one exception is the 'let' built-in, where this can still be
controlled independently with the letoctal option as before (but,
because letoctal is synched with posix when switching that on/off,
it's consistent by default).
We're also removing the hackery that causes variable expansions for
the 'let' builtin to be quietly altered, so that 'x=010; let y=$x'
now does the same as 'let y=010' even with letoctal on.
Various files:
- Get rid of now-redundant sh.inarith (shp->inarith) flag, as we're
no longer distinguishing between being inside or outside ((...)).
src/cmd/ksh93/sh/arith.c:
- arith(): Let disabling POSIX octal constants by skipping leading
zeros depend on either the letoctal option being off (if we're
running the "let" built-in") or the posix option being off.
- sh_strnum(): Preset a base of 10 for strtonll(3) depending on the
posix or letoctal option being off, not on the sh.inarith flag.
src/cmd/ksh93/include/argnod.h,
src/cmd/ksh93/sh/args.c,
src/cmd/ksh93/sh/macro.c:
- Remove astonishing hackery that violated shell grammar for 'let'.
src/cmd/ksh93/sh/name.c (nv_getnum()),
src/cmd/ksh93/sh/nvdisc.c (nv_getn()):
- Remove loops for skipping leading zeroes that included a broken
check for justify/zerofill attributes, thereby fixing this bug:
$ typeset -Z x=0x15; echo $((x))
-ksh: x15: parameter not set
Even if this code wasn't redundant before, it is now: sh_arith()
is called immediately after the removed code and it ignores
leading zeroes via sh_strnum() and strtonll(3).
Resolves: https://github.com/ksh93/ksh/issues/334
Bug 1: POSIX requires numbers used as arguments for all the %d,
%u... in printf to be interpreted as in the C language, so
printf '%d\n' 010
should output 8 when the posix option is on. However, it outputs 10.
This bug was introduced as a side effect of a change introduced in
the 2012-02-07 version of ksh 93u+m, which caused the recognition
of leading-zero numbers as octal in arithmetic expressions to be
disabled outside ((...)) and $((...)). However, POSIX requires
leading-zero octal numbers to be recognised for printf, too.
The change in question introduced a sh.arith flag that is set while
we're processing a POSIX arithmetic expression, i.e., one that
recognises leading-zero octal numbers.
Bug 2: Said flag is not reset in a command substitution used within
an arithmetic expression. A command substitution should be a
completely new context, so the following should both output 10:
$ ksh -c 'integer x; x=010; echo $x'
10 # ok; it's outside ((…)) so octals are not recognised
$ ksh -c 'echo $(( $(integer x; x=010; echo $x) ))'
8 # bad; $(comsub) should create new non-((…)) context
src/cmd/ksh93/bltins/print.c: extend():
- For the u, d, i, o, x, and X conversion modifiers, set the POSIX
arithmetic context flag before calling sh_strnum() to convert the
argument. This fixes bug 1.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- When invoking a command substitution, save and unset the POSIX
arithmetic context flag. Restore it at the end. This fixes bug 2.
Reported-by: @stephane-chazelas
Resolves: https://github.com/ksh93/ksh/issues/326
ksh93 currently has three command substitution mechanisms:
- type 1: old-style backtick comsubs that use a pipe;
- type 3: $(modern) comsubs that use a temp file, currently with
fallback to a pipe if a temp file cannot be created;
- type 2: ${ shared-state; } comsubs; same as type 3, but shares
state with parent environment.
Type 1 is buggy. There are at least two reproducers that make it
hang. The Red Hat patch applied in 4ce486a7 fixed a hang in
backtick comsubs but reintroduced another hang that was fixed in
ksh 93v-. So far, no one has succeeded in making pipe-based comsubs
work properly.
But, modern (type 3) comsubs use temp files. How does it make any
sense to have two different command substitution mechanisms at the
execution level? The specified functionality between backtick and
modern command substitutions is exactly the same; the difference
*should* be purely syntactic.
So this commit removes the type 1 comsub code at the execution
level, treating them all like type 3 (or 2). As a result, the
related bugs vanish while the regression tests all pass.
The only side effect that I can find is that the behaviour of bug
https://github.com/ksh93/ksh/issues/124 changes for backtick
comsubs. But it's broken either way, so that's neutral.
So this commit can now be added to my growing list of ksh93 issues
fixed by simply removing code.
src/cmd/ksh93/sh/xec.c:
- Remove special code for type 1 comsubs from iousepipe(),
sh_iounpipe(), sh_exec() and _sh_fork().
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/subshell.c:
- Remove pipe support from sh_subtmpfile(). This also removes the
use of a pipe as a fallback for $(modern) comsubs. Instead, panic
and error out if temp file creation fails. If the shell cannot
create a temporary file, there are fatal system problems anyway
and a script should not continue.
- No longer pass comsub type to sh_subtmpfile().
All other changes:
- Update sh_subtmpfile() calls.
src/cmd/ksh93/tests/subshell.sh:
- Add two regression tests based on reproducers from bug reports.
Resolves: https://github.com/ksh93/ksh/issues/305
Resolves: https://github.com/ksh93/ksh/issues/316
This fixes the following:
1. Using $RANDOM in a virtual/non-forked subshell no longer
influences the reproducible $RANDOM sequence in the parent
environment.
2. When invoking a subshell $RANDOM is now re-seeded (as mksh and
bash do) so that invocations in repeated subshells (including
forked subshells) longer produce identical sequences by default.
3. Program flow corruption that occurred in scripts on executing
( ( simple_command & ) ).
src/cmd/ksh93/include/variables.h:
- Move 'struct rand' here as it will be needed in subshell.c. Add
rand_seed member to save the pseudorandom generator seed. Remove
the pointer to the shell state as it's redundant.
src/cmd/ksh93/sh/init.c:
- put_rand(): Store given seed in rand_seed while calling srand().
No longer pointlessly limit the number of possible seeds with the
RANDMASK bitmask (that mask is to limit the values to 0-32767,
it should not limit the number of possible sequences to 32768).
- nget_rand(): Instead of using rand(), use rand_r() to update the
random_seed value. This makes it possible to save/restore the
current seed of the pseudorandom generator.
- Add sh_reseed_rand() function that reseeds the pseudorandom
generator by calling srand() with a bitwise-xor combination of
the current PID, the current time with a granularity of 1/10000
seconds, and a sequence number that is increased on each
invocation.
- nv_init(): Set the initial seed using sh_reseed_rand() here
instead of in sh_main(), as this is where the other struct rand
members are initialised.
src/cmd/ksh93/sh/main.c: sh_main():
- Remove the srand() call that was replaced by the sh_reseed_rand()
call in init.c.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Upon entering a virtual subshell, save the current $RANDOM seed
and state, then reseed $RANDOM for the subshell.
- Upon exiting a virtual subshell, restore $RANDOM seed and state
and reseed the generator using srand() with the restored seed.
src/cmd/ksh93/sh/xec.c: sh_exec():
- When optimizing out a subshell that is the last command, still
act like a subshell: reseed $RANDOM and increase ${.sh.subshell}.
- Fix a separate bug discovered while implementing this. Do not
optimize '( simple_command & )' when in a virtual subshell; doing
this causes program flow corruption.
- When optimizing '( simple_command & )', also reseed $RANDOM and
increment ${.sh.subshell}.
src/cmd/ksh93/tests/subshell.sh,
src/cmd/ksh93/tests/variables.sh:
- Add various tests for all of the above.
Co-authored-by: Johnothan King <johnothanking@protonmail.com>
Resolves: https://github.com/ksh93/ksh/issues/285
The code contains various checks to see if a subshell needs to
fork, like this one in the ulimit builtin:
if(shp->subshell && !shp->subshare)
sh_subfork();
All checks of this form are fatally broken, as each one of them
causes shared-state command substitutions to ignore parent virtual
subshells.
Currently the only feasible way to fix this is to fork a virtual
subshell before executing a shared-state command substitution in
it. In the long term I think shared-state command substitutions
should probably be redesigned to disassociate them completely from
the virtual subshell mechanism.
src/cmd/ksh93/sh/macro.c: comsubst():
- If we're in a non-subshare virtual subshell, fork it before
entering a type 2 (subshare) command substitution.
src/cmd/ksh93/sh/subshell.c:
- sh_assignok(): Remove subshare fix from 911d6b06 as it's
redundant now that the parent of a subshare is never a virtual
subshell. Go back to not doing anything if the current "subshell"
is a subshare.
- sh_subtracktree(), sh_subfuntree(): Similarly, remove the
now-redundant subshare fixes from 13c57e4b.
src/cmd/ksh93/sh/xec.c: sh_exec():
- Fix a separate bug: only fork a virtual subshell before running a
background job if that "subshell" is not a subshare.
src/cmd/ksh93/tests/subshell.sh:
- Add test for bug fixed in xec.c.
- Add tests for 'ulimit', 'builtin' and 'exec' run in subshare
within subshell -- all commands that use checks of the form
'if(sh.subshell && !sh.subshare) sh_subfork();'.
Resolves: https://github.com/ksh93/ksh/issues/289
This commit implements unsetting functions in virtual subshells,
removing the need for the forking workaround. This is done by
either invalidating the function found in the current subshell
function tree by unsetting its NV_FUNCTION attribute bits (which
will cause sh_exec() to skip it) or, if the function exists in a
parent shell, by creating an empty dummy subshell node in the
current function tree without that attribute.
As a beneficial side effect, it seems that bug 228 (unset -f fails
in forked subshells if a function is defined before forking) is now
also fixed.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/init.c:
- Add sh.fun_base for a saved pointer to the main shell's function
tree for checking when in a subshell, analogous to sh.var_base.
src/cmd/ksh93/bltins/typeset.c: unall():
- Remove the fork workaround.
- When unsetting a function found in the current function tree
(troot) and that tree is not sh.var_base (which checks if we're
in a virtual subshell in a way that handles shared-state command
substitutions correctly), then do not delete the function but
invalidate it by unsetting its NV_FUNCTION attribute bits.
- When unsetting a function not found in the current function tree,
search for it in sh.fun_base and if found, add an empty dummy
node to mask the parent shell environment's function. The dummy
node will not have NV_FUNCTION set, so sh_exec() will skip it.
src/cmd/ksh93/sh/subshell.c:
- sh_subfuntree(): For 'unset -f' to work correctly with
shared-state command substitutions (subshares), this function
needs a fix similar to the one applied to sh_assignok() for
variables in commit 911d6b06. Walk up on the subshells tree until
we find a non-subshare.
- sh_subtracktree(): Apply the same fix for the hash table.
- Remove table_unset() and incorporate an updated version of its
code in sh_subshell(). As of ec888867, this function was only
used to clean up the subshell function table as the alias table
no longer exists.
- sh_subshell():
* Simplify the loop to free the subshell hash table.
* Add table_unset() code, slightly refactored for readability.
Treat dummy nodes now created by unall() separately to avoid a
memory leak; they must be nv_delete()d without passing the
NV_FUNCTION bits. For non-dummy nodes, turn on the NV_FUNCTION
attribute in case they were invalidated by unall(); this is
needed for _nv_unset() to free the function definition.
src/cmd/ksh93/tests/subshell.sh:
- Update the test for multiple levels of subshell functions to test
a subshare as well. While we're add it, add a very similar test
for multiple levels of subshell variables that was missing.
- Add @JohnoKing's reproducer from #228.
src/cmd/ksh93/tests/leaks.sh:
- Add leak tests for unsetting functions in a virtual subshell.
Test both the simple unset case (unall() creates a dummy node)
and the define/unset case (unall() invalidates existing node).
Resolves: https://github.com/ksh93/ksh/issues/228
There is a TODO note in variables.sh that notes the value of LINENO
is wrong after a virtual subshell. The following script should
print '6', but the bug causes it to print '1' instead:
$ cat /tmp/lineno
#!/bin/ksh
(
unset LINENO
:
)
echo $LINENO
This bug started to occur after the bugfix applied in 7b994b6a.
However, that commit is not where the cause of bug was (when that
bugfix is applied to ksh versions 2008-07-25 through 2012-01-01,
$LINENO works fine). Rather, the cause of this bug was introduced
in 93u+ 2012-02-29. In that version, the mp->nvfun pointer was only
copied from np->nvfun if the variable can be freed from memory.
This is what caused 7b994b6a to break $LINENO in subshells, so to
fix this bug the mp->nvfun and np->nvfun must point to the same
object, even when the variable isn't freed from memory.
src/cmd/ksh93/sh/subshell.c: nv_restore():
- Always copy the np->nvfun pointer to mp->nvfun. To prevent
crashes, the value of np->nvfun->nofree is set to the value given
by the nofree variable, which is set before _nv_unset. See also
commit 7e7f1372, which fixed a crash that happened because
_nv_unset discards the NV_NOFREE flag.
src/cmd/ksh93/tests/variables.sh:
- Remove the workaround for LINENO after a virtual subshell.
- Add a regression test for the value of LINENO when unset in a
virtual subshell, then used after the subshell. Note that before
commit 997ad43b LINENO's value was corrupted after being unset in
a subshell, so the test checks for corruption of the LINENO
variable (in prior commits LINENO was set to '49' because of the
previous bug).
The changes in this commit allow ksh to be built and run with
ASan[*], although for now it only works under vmalloc. Example
command to build ksh with ASan:
$ bin/package make CCFLAGS='-O0 -g -fsanitize=address'
[*] https://en.wikipedia.org/wiki/AddressSanitizer
src/cmd/INIT/mamake.c:
- Fix a few memory leaks in mamake. This doesn't fix all of the
memory leaks ASan complains about (there is one remaining in the
view() function), but it's enough to get ksh to build under ASan.
src/lib/libast/features/map.c,
src/lib/libast/misc/glob.c:
- Rename the ast globbing functions to _ast_glob() and
_ast_globfree(). Without this change the globbing tests fail
under ASan. See: 2c49eb6e
src/cmd/ksh93/sh/{init,io,nvtree,subshell}.c:
- Fix buffer overflows by using strncmp(3) instead of memcmp(3).
src/cmd/ksh93/sh/name.c:
- Fix another invalid usage of memcmp by using strncmp instead.
This change is also in one of Red Hat's patches:
https://git.centos.org/rpms/ksh/blob/c8s/f/SOURCES/ksh-20120801-nv_open-memcmp.patch
Resolves: https://github.com/ksh93/ksh/issues/230
There were still problems left after the previous commit. On at
least one system (QNX i386), the following regression test crashed:
src/cmd/ksh93/test/subshell.c
900 got=$( { "$SHELL" -c '(cd /; (cd /)); print -r -- "PWD=$PWD"'; } 2>&1 )
A backtrace done on the core dunp pointed to the free() call here:
src/cmd/ksh93/bltins/cd_pwd.c
90 if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot)
91 free(oldpwd);
Analysis: The interaction between $PWD, sh.pwd aka shp->pwd, and
the path_pwd() function is a mess. path_pwd() usually returns a
freeable value, but not always. sh.pwd is sometimes a pointer to
the value of $PWD, but not always (e.g. when you unset PWD or
assign to it). Instead of debugging the exact cause of the crash, I
think it is better to make this work in a more consistent way.
As of this commit:
1. sh.pwd keeps its own copy of the PWD, independently of the PWD
variable. The old value must always be freed immediately before
assigning a new one. This is simple and consistent, reducing the
chance of bugs at negligible cost.
2. The PWD variable is no longer given the NV_NOFREE attribute
because its value no longer points to sh.pwd. It is now a
variable like any other.
src/cmd/ksh93/sh/path.c: path_pwd():
- Do not give PWDNOD the NV_NOFREE attribute.
- Give sh.pwd its own copy of the PWD by strdup'ing PWDNOD's value.
src/cmd/ksh93/bltins/cd_pwd.c: b_cd():
- Since sh.pwd is now consistently freed before giving it a new
value and at no other time, oldpwd must not be freed any longer
and can become a regular non-static variable.
- If the PWD needs reinitialising, call path_pwd() to do it.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Systems with fchdir(2): Always restore the PWD upon exiting a
non-subshare subshell. The check to decide whether or not to
restore it was unsafe: it was not restored if the current PWD
pointer and value was identical to the saved one, but a directory
can be deleted and recreated under the same name.
- Systems without fchdir(2) (if any exist):
. Entry: Fork if the PWD is nonexistent or has no x permission.
. Restore: Only chdir back if the subshell PWD was changed.
That's probably the best we can do. It remains inherently unsafe.
We should probably just require fchdir(2) at some point.
This commit fixes what are hopefully the two final aspects of #153:
1. If the present working directory does not exist (was moved or
deleted) upon entering a virtual subshell, no PWD directory path
is saved. Since restoring the state after exiting a virtual
subshell is contingent on a previous PWD path existing, this
resulted in entire aspects of the virtual subshell, such as the
subshell function tree, not being cleaned up.
2. A separate problem is that 'cd ..' does not update PWD or OLDPWD
when run from a nonexistent directory.
A reproducer exposing both problems is:
$ mkdir test
$ cd test
$ ksh -c '(subfn() { BAD; }; cd ..; echo subPWD==$PWD);
typeset -f subfn; echo mainPWD==$PWD'
subPWD==/usr/local/src/ksh93/ksh/test
subfn() { BAD; };mainPWD==/usr/local/src/ksh93/ksh/test
Expected output:
subPWD==/usr/local/src/ksh93/ksh
mainPWD==/usr/local/src/ksh93/ksh/test
src/cmd/ksh93/bltins/cd_pwd.c:
- If path_pwd() fails to get the PWD (usually it no longer exists),
don't set $OLDPWD to '.' as that is pointless; use $PWD instead.
After cd'ing from a nonexistent directory, 'cd -' *should* fail
and should not be equivalent to 'cd .'.
- Remove a redundant check for (!oldpwd) where it is always set.
- Do not prematurely return without setting PWD or OLDPWD if
pathcanon() fails to canonicalise a nonexistent directory.
Instead, fall back to setting PWD to the result of getcwd(3).
src/cmd/ksh93/sh/subshell.c:
- Minor stylistic adjustment. Some NULL macros sneaked in. This
historic code base does not use them (yet); change to NIL(type*).
- sh_subshell(): Fix logic for determining whether to save/restore
subshell state.
1. When saving, 'if(!comsub || !shp->subshare)' is redundant;
'if(!shp->subshare)' should be enough. If we're not in a
subshare, state should be saved.
2. When restoring, 'if(sp->shpwd)' is just nonsense as there is
no guarantee that the PWD exists upon entering a subshell.
Simply use the same 'if(!shp->subshare)'. Add an extra check
for sp->pwd to avoid a possible segfault. Always restore the
PWD on subshell exit and not only if shp->pwd is set.
- sh_subshell(): Issue fatal errors in libast's "panic" format.
src/cmd/ksh93/tests/builtins.sh:
- Adjust a relevant test to run err_exit() outside of the subshell
so that any error is counted in the main shell.
- Add test for problem 2 described at the top.
src/cmd/ksh93/tests/subshell.sh:
- Add test for problems 1 and 2 based on reproducer above.
Resolves: https://github.com/ksh93/ksh/issues/153
Many of these changes are minor typo fixes. The other changes
(which are mostly compiler warning fixes) are:
NEWS:
- The --globcasedetect shell option works on older Linux kernels
when used with FAT32/VFAT file systems, so remove the note about
it only working with 5.2+ kernels.
src/cmd/ksh93/COMPATIBILITY:
- Update the documentation on function scoping with an addition
from ksh93v- (this does apply to ksh93u+).
src/cmd/ksh93/edit/emacs.c:
- Check for '_AST_ksh_release', not 'AST_ksh_release'.
src/cmd/INIT/mamake.c,
src/cmd/INIT/ratz.c,
src/cmd/INIT/release.c,
src/cmd/builtin/pty.c:
- Add more uses of UNREACHABLE() and noreturn, this time for the
build system and pty.
src/cmd/builtin/pty.c,
src/cmd/builtin/array.c,
src/cmd/ksh93/sh/name.c,
src/cmd/ksh93/sh/nvtype.c,
src/cmd/ksh93/sh/suid_exec.c:
- Fix six -Wunused-variable warnings (the name.c nv_arrayptr()
fixes are also in ksh93v-).
- Remove the unused 'tableval' function to fix a -Wunused-function
warning.
src/cmd/ksh93/sh/lex.c:
- Remove unused 'SHOPT_DOS' code, which isn't enabled anywhere.
https://github.com/att/ast/issues/272#issuecomment-354363112
src/cmd/ksh93/bltins/misc.c,
src/cmd/ksh93/bltins/trap.c,
src/cmd/ksh93/bltins/typeset.c:
- Add dictionary generator function declarations for former
aliases that are now builtins (re: 1fbbeaa1, ef1621c1, 3ba4900e).
- For consistency with the rest of the codebase, use '(void)'
instead of '()' for print_cpu_times.
src/cmd/ksh93/sh/init.c,
src/lib/libast/path/pathshell.c:
- Move the otherwise unused EXE macro to pathshell() and only
search for 'sh.exe' on Windows.
src/cmd/ksh93/sh/xec.c,
src/lib/libast/include/ast.h:
- Add an empty definition for inline when compiling with C89.
This allows the timeval_to_double() function to be inlined.
src/cmd/ksh93/include/shlex.h:
- Remove the unused 'PIPESYM2' macro.
src/cmd/ksh93/tests/pty.sh:
- Add '# err_exit #' to count the regression test added in
commit 113a9392.
src/lib/libast/disc/sfdcdio.c:
- Move diordwr, dioread, diowrite and dioexcept behind
'#ifdef F_DIOINFO' to fix one -Wunused-variable warning and
multiple -Wunused-function warnings (sfdcdio() only uses these
functions when F_DIOINFO is defined).
src/lib/libast/string/fmtdev.c:
- Fix two -Wimplicit-function-declaration warnings on Linux by
including sys/sysmacros.h in fmtdev().
The typecast fix was insufficient, avoiding the crash only when
compiling with optimisation disabled. The real problem is that
put_lineno() was passed a misaligned pointer, and that the value
didn't actually contain a double but a string. The bug occurred
when restoring the LINENO value upon exiting a virtual subshell.
Thanks to Harald van Dijk for figuring out the fix.
src/cmd/ksh93/sh/subshell.c: nv_restore():
- When restoring a special variable as defined by nv_cover(),
do not pass either the np->nvflag bits or NV_NOFREE. Why?
* The np->nvflag bits are not needed. They are also harmful
because they may include the NV_INTEGER bit. This is set
when the value is numeric. However, nv_getval() always
returns the value in string form, converting it if it is
numeric. So the NV_INTEGER flag should never be passed
to nv_putval() when it uses the result of nv_getval().
* According to nval.3, the NV_NOFREE flag stops nv_putval() from
creating a copy of the value. But this should be unnecessary
because the earlier _nv_unset(mp,NV_RDONLY|NV_CLONE) should
ensure there is no previous value. In addition, the NV_NOFREE
flag triggered another bug that caused the value of SECONDS to
be corrupted upon restoring it when exiting a virtual subshell.
- When restoring a regular variable, copy the entire nvalue union
and not just the 'cp' member. In practice this worked because
no current member of the nvalue union is larger than a pointer.
However, there is no guarantee it will stay that way.
src/cmd/ksh93/tests/leaks.sh:
- Add disabled test for a memory leak that was discovered in the
course of dealing with this bug. The fix doesn't introduce or
influence it. It will have to be dealt with later.
src/cmd/ksh93/tests/locale.sh:
- Add test for restoring locale on leaving virtual subshell.
https://github.com/ksh93/ksh/issues/253#issuecomment-815290154
src/cmd/ksh93/tests/variables.sh:
- Test against corruption of SECONDS on leaving virtual subshell.
https://github.com/ksh93/ksh/issues/253#issuecomment-815191052
Co-authored-by: Harald van Dijk <harald@gigawatt.nl>
Progresses: https://github.com/ksh93/ksh/issues/253
This commit adds an UNREACHABLE() macro that expands to either the
__builtin_unreachable() compiler builtin (for release builds) or
abort(3) (for development builds). This is used to mark code paths
that are never to be reached.
It also adds the 'noreturn' attribute to functions that never
return: path_exec(), sh_done() and sh_syntax(). The UNREACHABLE()
macro is not added after calling these.
The purpose of these is:
* to slightly improve GCC/Clang compiler optimizations;
* to fix a few compiler warnings;
* to add code clarity.
Changes of note:
src/cmd/ksh93/sh/io.c: outexcept():
- Avoid using __builtin_unreachable() here since errormsg can
return despite using ERROR_system(1), as shp->jmplist->mode is
temporarily set to 0. See: https://github.com/att/ast/issues/1336
src/cmd/ksh93/tests/io.sh:
- Add a regression test for the ksh2020 bug referenced above.
src/lib/libast/features/common:
- Detect the existence of either the C11 stdnoreturn.h header or
the GCC noreturn attribute, preferring the former when available.
- Test for the existence of __builtin_unreachable(). Use it for
release builds. On development builds, use abort() instead, which
crahses reliably for debugging when unreachable code is reached.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This commit fixes 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>
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>
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.
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.
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.
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
If I haven't missed anything, this should make the non-interactive
aspects of job control in scripts work as expected, except for the
"<command unknown>" issue in the output of 'bg', 'fg' and 'jobs'
(which is not such a high priority as those commands are really
designed for interactive use).
Plus, I believe I now finally understand what these three are for:
* The job.jobcontrol variable is set to nonzero by job_init() in
jobs.c if, and only if, the shell is interactive *and* managed to
get control of the terminal. Therefore, any changing of terminal
settings (tcsetpgrp(3), tty_set()) should only be done if
job.jobcontrol is nonzero. This commit changes several checks for
sh_isoption(SH_INTERACTIVE) to checks for job.jobcontrol for
better consistency with this.
* The state flag, sh_isstate(SH_MONITOR), determines whether the
bits of job control that are relevant for both scripts and
interactive shells are active, which is mostly making sure that a
background job gets its own process group (setpgid(3)).
* The shell option, sh_isoption(SH_MONITOR), is just that. When the
user turns it on or off, the state flag is synched with it. It
should usually not be directly checked for, as the state may be
temporarily turned off without turning off the option.
Prior discussion:
https://www.mail-archive.com/austin-group-l@opengroup.org/msg06456.html
src/cmd/ksh93/bltins/typeset.c, src/cmd/ksh93/sh/args.c:
- Move synching the SH_MONITOR state flag with the SH_MONITOR
shell option from b_set() (the 'set' builtin) to sh_applyopts()
which is indirectly called from b_set() and is also used when
parsing the shell invocation command line. This ensures -m is
properly enabled in both scenarios.
src/cmd/ksh93/sh/jobs.c:
- job_init(): Do not refuse to initialise job control on
non-interactive shells. Instead, skip everything that should only
be done on interactive shells (i.e., everything to do with the
terminal). This function is now even more of a mess than it was
before, so refactoring may be desirabe at some point.
- job_close(), job_set(), job_reset(), job_wait(): Do not reset the
terminal process group (tcsetpgrp()) if job.jobcontrol isn't on.
src/cmd/ksh93/sh/xec.c:
- sh_exec(): TFORK: For SIGINT handling, check the SH_MONITOR
state flag, not the shell option.
- sh_exec(): TFORK: Do not turn off the SH_MONITOR state flag in
forked children. The non-interactive part of job control should
stay active. Instead, turn off the SH_INTERACTIVE state flag so
we don't get interactive shell behaviour (i.e. job control noise
on the terminal) in forked subshells.
- _sh_fork(), sh_ntfork(): Do not reset the terminal process group
(tcsetpgrp()) if job.jobcontrol isn't on. Do not turn off the
SH_MONITOR state flag in forked children.
src/cmd/ksh93/sh/subshell.c: sh_subfork():
- Do not turn off the monitor option and state in forked subshells.
The non-interactive part of job control should stay active.
src/cmd/ksh93/bltins/misc.c: b_bg():
- Check isstate(SH_MONITOR) instead of sh_isoption(SH_MONITOR) &&
job.jobcontrol before throwing a 'no job control' error.
This fixes a minor bug: fg, bg and disown could quietly fail.
src/cmd/ksh93/tests/jobs.sh:
- Add tests for 'fg' with job control IDs (%%, %1) in scripts.
- Add test checking that a background job launched from a subsell
with job control enabled correctly becomes the leader of its own
process group.
Makes progress on: https://github.com/ksh93/ksh/issues/119
Another longstanding whopper of a bug in basic ksh93 functionality:
run a ${ shared-state; } command substitution twice and job control
promptly loses track of all your running jobs. New jobs are tracked
again until you run another two shared-state command substitutions.
This is in at least 93t+, 93u-, 93u+, 93v- and ksh2020.
$ sleep 300 &
[1] 56883
$ jobs # OK
[1] + Running sleep 300 &
$ v=${ echo hi1; }
$ jobs # OK
[1] + Running sleep 300 &
$ v=${ echo hi2; }
$ jobs # Nothing!
$ fg
ksh: fg: no such job
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- The current environment number shp->curenv (a.k.a. sh.curenv) was
not being restored if the virtual subshell we're leaving is of
the shared-state command substitution variety as it was wrongly
considered to be part of the environment that didn't need
restoring. This caused it to be out of sync with shp->jobenv
(a.k.a. sh.jobenv) which did get restored from savedcurenv.
Restore both from savedcurenv at the same time for any subshell.
(How these numbers are used exactly remains to be discovered.)
src/cmd/ksh93/tests/jobs.sh:
- Added, with a test for this bug to start it off. There is no
other test script where job control fits, and a lot more related
fixes are anticipated: https://github.com/ksh93/ksh/issues/119
This backports most of the Cdt (container data types) mechanism
from the ksh 93v- beta, based on ground work done by OpenSUSE:
https://build.opensuse.org/package/view_file/shells/ksh/ksh93-dttree-crash.dif
plus adaptations to match ksh 93u+m and an updated manual page
(src/lib/libast/man/cdt.3) added directly from the 93v- sources.
| Thu Dec 20 12:48:02 UTC 2012 - werner@suse.de
|
| - Add ksh93-dttree-crash.dif - Allow empty strings in (dt)trees
| (bnc#795324)
|
| Fri Oct 25 14:07:57 UTC 2013 - werner@suse.de
|
| - Rework patch ksh93-dttree-crash.dif
As usual, precious little information is available because the
OpenSUSE bug report is currently closed to the public:
https://bugzilla.opensuse.org/show_bug.cgi?id=795324
However, a cursory inspection suggests that this code contains
improvements to do with concurrent processing and related
robustness. The new cdt.3 manual page adds a lot about that.
This has been in production use on OpenSUSE for a long time,
so hopefully this will make ksh a little more stable again.
Only one way to find out: let's commit and test this...
BTW, to get a nice manual, use groff and ghostscript's ps2pdf:
$ groff -tman src/lib/libast/man/cdt.3 | ps2pdf - cdt.3.pdf
This commit also further mitigates the problems with restoring an
inaccessible or nonexistent PWD on exiting a virtual subshell.
Harald van Dijk writes:
> On a build of ksh with -fsanitize=undefined to help diagnose
> problems:
>
> $ mkdir deleted
> $ cd deleted
> $ rmdir ../deleted
> $ ksh -c '(cd /; (cd /)); :'
> /home/harald/ksh/src/cmd/ksh93/sh/subshell.c:561:22: runtime
> error: null pointer passed as argument 1, which is declared to
> never be null
> Segmentation fault (core dumped)
>
> Note that it segfaults the same with default compilation flags,
> but it does not print out the useful extra message. The code
> assumes that pwd is non-null and passes it to strcmp without
> checking, but it will be null if the current directory cannot be
> determined, for instance because it has been deleted.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Avoid the null pointer dereference reported above.
src/cmd/ksh93/bltins/cd_pwd.c: b_cd():
- Fork a virtual subshell even on systems with fchdir(2) if the
present working directory tests as inaccessible on invoking 'cd';
it may no longer exist and fchdir would fail to get a handle.
(For the test we have to opendir(3) the full path to the PWD and
not ".", as the latter may succeed even if the PWD is gone.)
src/cmd/ksh93/data/builtins.c:
- Update 'cd' version string.
Fixes: https://github.com/ksh93/ksh/issues/153
Related: https://github.com/ksh93/ksh/issues/141
This change is backported from the abandoned ksh 93v- beta.
src/cmd/ksh93/sh/subshell.c: sh_subsavefd():
- Do not subtract 1 from fd, as this would cause a negative shift
operand for stdin (fd==0).
The forking fix implemented in 102868f8 and 9d428f8f, which stops
the main shell's hash table from being cleared if PATH is changed
in a subshell, can cause a significant performance penalty for
certain scripts that do something like
( PATH=... command foo )
in a subshell, especially if done repeatedly. This is because the
hash table is cleared (and hence a subshell forks) even for
temporary PATH assignments preceding commands.
It also just plain doesn't work. For instance:
$ hash -r; (ls) >/dev/null; hash
ls=/bin/ls
Simply running an external command in a subshell caches the path in
the hash table that is shared with a main shell. To remedy this, we
would have to fork the subshell before forking any external
command. And that would be an unacceptable performance regression.
Virtual subshells do not need to fork when changing PATH if they
get their own hash tables. This commit adds these. The code for
alias subshell trees (which was removed in ec888867 because they
were broken and unneeded) provided the beginning of a template for
their implementation.
src/cmd/ksh93/sh/subshell.c:
- struct subshell: Add strack pointer to subshell hash table.
- Add sh_subtracktree(): return pointer to subshell hash table.
- sh_subfuntree(): Refactor a bit for legibility.
- sh_subshell(): Add code for cleaning up subshell hash table.
src/cmd/ksh93/sh/name.c:
- nv_putval(): Remove code to fork a subshell upon resetting PATH.
- nv_rehash(): When in a subshell, invalidate a hash table entry
for a subshell by creating the subshell scope if needed, then
giving that entry the NV_NOALIAS attribute to invalidate it.
src/cmd/ksh93/sh/path.c: path_search():
- To set a tracked alias/hash table entry, use sh_subtracktree()
and pass the HASH_NOSCOPE flag to nv_search() so that any new
entries are added to the current subshell table (if any) and do
not influence any parent scopes.
src/cmd/ksh93/bltins/typeset.c: b_alias():
- b_alias(): For hash table entries, use sh_subtracktree() instead
of forking a subshell. Keep forking for normal aliases.
- setall(): To set a tracked alias/hash table entry, pass the
HASH_NOSCOPE flag to nv_search() so that any new entries are
added to the current subshell table (if any) and do not influence
any parent scopes.
src/cmd/ksh93/sh/init.c: put_restricted():
- Update code for clearing the hash table (when changing $PATH) to
use sh_subtracktree().
src/cmd/ksh93/bltins/cd_pwd.c:
- When invalidating path name bindings to relative paths, use the
subshell hash tree if applicable by calling sh_subtracktree().
- rehash(): Call nv_rehash() instead of _nv_unset()ting the hash
table entry; this is needed to work correctly in subshells.
src/cmd/ksh93/tests/leaks.sh:
- Add leak tests for various PATH-related operations in the main
shell and in a virtual subshell.
- Several pre-existing memory leaks are exposed by the new tests
(I've confirmed these in 93u+). The tests are disabled and marked
TODO for now, as these bugs have not yet been fixed.
src/cmd/ksh93/tests/subshell.sh:
- Update.
Resolves: https://github.com/ksh93/ksh/issues/66
The '-o nolog' option (which prevented function definitions from being
recorded in the history file) was removed a long time ago, leaving
only a stub for backwards compatibility to stop 'set' from erroring
out if the option is set. But some other vestiges remained.
src/cmd/ksh93/sh/path.c,
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Remove a few pointless 'sh_onstate(SH_NOLOG)' statements. As of
93u+ or earlier, this is never checked for anywhere.
src/cmd/ksh93/sh.1:
- They forgot to remove the 'nolog' option documentation here.
Specify that it's obsolete and has no effect.
src/cmd/ksh93/data/builtins.c: sh_set[]:
- Be more concise.
Virtual/non-forking subshells that change the present working
directory (PWD) with 'cd' suffer from a serious race condition. The
PWD is changed within the same process. This means it may not be
possible to change back to the original PWD when exiting the
subshell, as some other process may destroy the PWD or modify its
permissions in the meantime. ksh did not handle this error
condition at all, so, after exiting a subshell that invoked 'cd',
it could silently end up running the script's following command(s)
in the wrong directory. Which might be 'rm -rf *'. So, ouch.
The proper and obvious fix is never to allow a virtual subshell to
change the PWD, as it can never be guaranteed you can return to a
previous directory. If the PWD is changed in a child process, there
is no need to restore it in the parent process, and this whole
problem is avoided. So subshells really should always fork on
encountering a 'cd' command.
But forking is slow. It is not uncommon for scripts to 'cd' in a
subshell that is run repeatedly in a loop.
There is also the issue of custom builtins that can be added to ksh
via shared libraries. In the standard shell language, 'cd' is the
only command that changes the PWD, so we could just make that
command fork the subshell it is run from. But there's no telling
what a custom builtin might do.
So this commit implements a compromise that will not affect
performance unless there is the pathological condition of a PWD
that has been rendered inaccessible in some way:
1. When entering a virtual subshell, if the parent shell's PWD
proves inaccessible upon saving it, the subshell will now fork into
a separate process, avoiding the unrestorable PWD problem.
2. If some attack renders the parent shell's PWD unrestorable
*after* ksh enters a virtual subshell, ksh will now error out when
exiting it. There is nothing else left to do then. Continuing would
mean running arbitrary commands in the wrong PWD.
src/cmd/ksh93/sh/subshell.c:
- Put all the code/variables only needed for fchdir() behind '#if
_lib_fchdir'. This makes it clearer what's what.
(I don't know if there is still any system out there without
fchdir(3); I haven't found any. The chdir(3) fallback version may
be removed later as there is no way to make it remotely secure.)
- Fix the attempt to use the O_PATH mode for open(2) as a fallback
for nonexistent O_SEARCH on Linux. Define _GNU_SOURCE on Linux,
or <fcntl.h> (which is included indirectly) won't define O_PATH.
- Fix use of O_SEARCH. The code was simply wrong, repeating an
open(".",O_RDONLY) instead. Since a nonexistent O_SEARCH is now
redefined as either O_PATH or O_RDONLY, we can simply
open(".",O_SEARCH) and be done with it.
- Fix fatal error handling. Introduce fatal error condition for
failure to fchdir(3) back to the parent's PWD; rename 'duped' to
'fatalerror' and use it for error numbers; save and restore errno
on fatal error so the message will report the cause. (We must
call errormsg() near the end of sh_subshell() to avoid crashes.)
- If open(".",O_SEARCH) was not able get a file descriptor to our
PWD on entry, then call sh_subfork() immediately before running
the subshell commands. (Forking earlier causes a crash.)
- When restoring the PWD, if fchdir(3) fails, do *not* fall back to
chdir(3). We already know the PWD is inaccessible, so if chdir(3)
"succeeds" then, it's very likely to be a substitute injected by
an attacker.
src/cmd/ksh93/bltins/cd_pwd.c:
- If we don't have fchdir(3), then sh_subshell() must fall back to
chdir(2) to restore the PWD. That is highly vulnerable, as a
well-timed rename would allow an attacker to usurp the PWD. We
can't do anything about that if some custom builtin changes the
PWD, but we can at least make 'cd' always fork a subshell, which
slows down ksh but removes the need for the parent shell ever to
restore the PWD. (There is certainly no popular system where this
is relevant and there might not be any such current system.)
This commit adds no regression test because a portable regression
test is not really doable. Different kernels, external /bin/pwd
utilities, etc. all have quite different behaviour under the
pathological condition of an inaccessible PWD, so both the
before-fix and the after-fix behaviour differs. See link below.
Resolves: https://github.com/ksh93/ksh/issues/141
Thanks to Stéphane Chazelas for the bug report.
By definition, subshells are never interactive, so they should
disable behaviour associated with interactive shells even if the
main shell is interactive.
Most visibly, running a background job from a subshell like
( some_command & )
now no longer prints a job ID that you cannot use in the main shell.
This behaviour change matches pdksh/mksh, bash, zsh, dash, et al.
Prior discussion:
https://www.mail-archive.com/austin-group-l@opengroup.org/msg06456.html
(plus the preceding thread)
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Before running the command(s) in the subshell using sh_exec(),
turn off the SH_INTERACTIVE shell state flag. (No need to add
code to restore it as this function already saves and restores
the entire shell state.)
src/cmd/ksh93/bltins/misc.c: b_bg():
- If there is no job control when using 'bg', 'fg' or 'disown',
always print the "no job control" error message and not only if
the shell is in the interactive state. This is also what
pdksh/mksh, bash and zsh do.
Mildly interesting: apparently there was once an idea to implement
shared-state command substitutions as a shell option like 'set -o
subshare'. They were implemented using a new ${ syntax; } instead,
but there is a vestigial SH_SUBSHARE option ID in shell.h plus a
check for it in subshell.c that would cause backtick-style command
substitutions (comsub==1) to share their state. That option isn't
defined in data/options.c so it's impossible for a user to set it.
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/subshell.c:
- Remove SH_SUBSHELL option vestiges.
src/cmd/ksh93/include/defs.h:
- Correct my comment on 'comsub' flag; I was wrong about what the
values meant. 2 is for a shared-state comsub. (re: 4ce486a7)
The ksh-20120801-trapcom.patch patch contains an off-by-one error,
which was also imported into 93u+m. When saving signals:
ceb77b136f/src/cmd/ksh93/sh/subshell.c (L572-L592)
572 if((nsig=shp->st.trapmax)>0 || shp->st.trapcom[0])
573 {
574 ++nsig;
575 savsig = malloc(nsig * sizeof(char*));
576 /*
577 * the data is, usually, modified in code like:
578 * tmp = buf[i]; buf[i] = strdup(tmp); free(tmp);
579 * so shp->st.trapcom needs a "deep copy" to properly save/restore pointers.
580 */
581 for (isig = 0; isig < nsig; ++isig)
582 {
583 if(shp->st.trapcom[isig] == Empty)
584 savsig[isig] = Empty;
585 else if(shp->st.trapcom[isig])
586 savsig[isig] = strdup(shp->st.trapcom[isig]);
587 else
588 savsig[isig] = NULL;
589 }
On line 574, the number of signals 'nsig' is increased by one. That
increase is permanent, so the 'for' loop on line 581 tries to save
one signal state too many.
The increase was a holdout from the ksh93 code from before the
patch. After the patch, it is not required; it is fine to malloc as
many records as there are trapcom elements to save. So it should
simply be removed. xec.c has the same code to save trap states for
ksh functions, and the same applies.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Don't increase nsig.
src/cmd/ksh93/sh/xec.c: sh_funscope():
- Same.
src/cmd/ksh93/tests/signal.sh:
- Add test.
Contrary to the RH bug report, this is yet another bug with
virtual/non-forked subshells and has nothing to do with functions.
If a signal is ignored (empty trap) in the main shell while any
trap (empty or not) is set on the same signal in a subshell, a
crash eventually occurred upon restoring state when leaving the
subshell.
Original patch:
642af4d6/f/ksh-20120801-trapcom.patch
Prior discussion:
https://bugzilla.redhat.com/1117404
Paulo Andrade wrote there:
> The problem is that the sh_subshell function was saving pointers
> that could change, and when restoring, bad things would happen.
[...]
> The only comment I added:
> /* contents of shp->st.trapcom may change */
> may be a bit misleading, the "bad" save/restore already knows it,
> probably I should have added a better description telling that the
> data is, usually, modified in code like:
>
> tmp = buf[i]; buf[i] = strdup(tmp); free(tmp);
>
> so the shp->st.trapcom needs a "deep copy", as done in the
> patch, to properly save/restore pointers.
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- sh_subshell(), sh_funscope(): Make *savsig/*savstak into a
**savsig array. Use strdup(3) to save the data and get known
pointers that will not change. Free these upon restore.
- Change the comment from the patch as Paulo wished he had done.
src/cmd/ksh93/tests/subshell.sh:
- Test 2500 times. This should trigger the crash most of the time.
Another Red Hat patch. "Prior to this update, the result of a
command substitution was lost if a file descriptor used for the
substitution was previously explicitly closed. With this update,
ksh no longer reuses file descriptors that were closed during the
execution of a command substitution. Now, command substitutions
work as expected in the described situation."
Prior discussion:
https://bugzilla.redhat.com/1116072
Original patch:
642af4d6/f/ksh-20140929-safefd.patch
src/cmd/ksh93/include/io.h,
src/cmd/ksh93/sh/io.c:
- Add sh_iosafefd() function to get a file descriptor that is not
in use or otherwise occupied (including marked as closed).
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Use that function to obtain a safe FD upon restoring state when
exiting a command substitution. I don't really know the how and
why -- all that I/O magic is still beyond me and the code is
uncommented as usual.
src/cmd/ksh93/tests/subshell.sh:
- Add regression test from the reproducer in the bug, reduced to
the minimum necessary.
Another Red Hat patch of a patch. With the new comsub mechanism,
functions could sometimes return the wrong exit status when invoked
from a command substitution.
Original patch:
642af4d6/f/ksh-20120801-fununset.patch
I have determined that the extra setexit() in the Red Hat patch,
which copies the current exit status to $?, is not needed, as the
code for running functions already sets $? on termination. I've
added extra regression tests to prove this.
By the way, the setexit() macro is defined like this in defs.h:
#define exitset() (sh.savexit=sh.exitval)
That's more evidence (see also 3654ee73) that it does not
matter whether you address the shell's status struct via a
pointer. That macro is used in places that use shp pointers.
But, that aside...
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- When waiting within a command substitution for a forked process
to end, save & restore sh.exitval (the exit status of the command
currently being run) so that job_wait() cannot override it.
src/cmd/ksh93/tests/functions.sh:
- Add tests based in part on the reproducer from rhbz#1116508.
The new command substitution mechanism imported in 970069a6 from
Red Hat patches introduced this bug: backtick-style command
substitutions hang when processing about 117KiB of data or more.
It is fixed by another Red Hat patch:
642af4d6/f/ksh-20140415-hokaido.patch
It saves the value of the shp->comsub flag so that it is set to 2
(usually meaning new-style $(comsubs)) in two specific cases even
when processing backtick comsubs. This stops the sh_subtmpfile()
function in subshell.c from creating a /tmp file. However, I think
that approach is quite ugly, so I'm taking a slightly different one
that has the same effect.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/sh/subshell.c:
- Redefine sh_subtmpfile() to pass the comsub flag as an argument.
(Remove the shp pointer argument, which is redundant; a pointer
to the shell state can easily be obtained in the function.)
src/cmd/ksh93/sh/xec.c: sh_exec():
- Apply the Red Hat fix by passing flag 2 to sh_subtmpfile().
src/cmd/ksh93/tests/subshell.sh:
- Move regress test from ce68e1be from basic.sh to here; this is
the place for command substitution tests as they are subshells.
- Add regress test for this bug.
All other changed files:
- Update sh_subtmpfile() calls to pass on the shp->comsub flag.
Now that we have ${.sh.pid} a.k.a. shgd->current_pid, which is
updated using getpid() whenever forking a new process, there is no
need for anything else to ever call getpid(); we can use the stored
value instead. There were a lot of these syscalls kicking around,
some of them in performance-sensitive places.
The following lists only changes *other* than changing getpid() to
shgd->currentpid.
src/cmd/ksh93/include/defs.h:
- Comments: clarify what shgd->{pid,ppid,current_pid} are for.
src/cmd/ksh93/sh/main.c,
src/cmd/ksh93/sh/init.c:
- On reinit for a new script, update shgd->{pid,ppid,current_pid}
in the sh_reinit() function itself instead of calling sh_reinit()
from sh_main() and then updating those immediately after that
call. It just makes more sense this way. Nothing else ever calls
sh_reinit() so there are no side effects.
src/cmd/ksh93/sh/xec.c: _sh_fork():
- Update shgd->current_pid in the child early, so that the rest of
the function can use it instead of calling getpid() again.
- Remove reassignment of SH_PIDNOD->nvalue.lp value pointer to
shgd->current_pid (which makes ${.sh.pid} work in the shell).
It's constant and was already set on init.
This imports another fix from Red Hat/Fedora. Original patch:
642af4d6/f/ksh-20120801-crash.patch
src/cmd/ksh93/include/jobs.h,
src/cmd/ksh93/sh/jobs.c,
src/cmd/ksh93/sh/subshell.c,
src/cmd/ksh93/sh/xec.c:
- Import the Red Hat fix with these differences:
- Rename the 'hack1_waitall' variable to 'bktick_waitall' and add
a comment describing what it's for.
- Remove unused 'pipefail' variable.
src/cmd/ksh93/tests/basic.sh:
- Regression test from reproducer given in the Red Hat bug report.
- Add special handling to SIGKILL it, as it might freeze hard.
When ksh was compiled with SHOPT_SPAWN (the default), any command
substitution embedded in a here-document returned an empty string.
The bug was also present in 93u+ 2012-08-01 (although not in every
case as some systems compile it without SHOPT_SPAWN).
This fixes it by applying a slightly edited combination of two Red
Hat patches (the second containing a fix for the first), which
backport a new command substitution mechanism from the abandoned
ksh 93v- beta version. The originals are:
642af4d6/f/ksh-20120801-macro.patch642af4d6/f/ksh-20120801-fd2lost.patch
src/cmd/ksh93/include/io.h:
- The iopipe() function from xec.c is now needed in sh_subshell()
(subshell.c), so rename it to sh_iounpipe() and declare it as an
extern here. The 93v- beta did it as well. (The Red Hat patch did
this without renaming it.)
src/cmd/ksh93/sh/xec.c:
- Backport new versions of iousepipe() and sh_iounpipe() from ksh
93v-. New 'type' flaggery is introduced to distinguish between
different command substitution conditions. What all that means
remains to be determined.
- sh_exec(): I made one change to the Red Hat patch myself: if in a
subshell and the type flags FAMP (for "ampersand" as in '&' as in
background job) and TFORK are set, continue to call sh_subfork()
to fork the subshell unconditionally, instead of only if we're in
a command substitution connected to an unseekable file. Maybe the
latter works for the 93v- code, but on 93u+(m) it causes a couple
of regressions, which are fixed by my change:
signal.sh[273]: subshell ignoring signal does not send signal to parent
signal.sh[276]: subshell catching signal does not send signal to parent
Details: https://github.com/ksh93/ksh/issues/104#issuecomment-696341902
src/cmd/ksh93/sh/macro.c,
src/cmd/ksh93/sh/subshell.c:
- Updates that go with those new functions.
Fixes: https://github.com/ksh93/ksh/issues/104
Affects: https://github.com/ksh93/ksh/issues/124
A memory leak occurred when typeset was used in a function called
from within a command substitution. This fix was backported from
the 93v- beta by Red Hat on 22 Jan 2014. Source:
642af4d6/f/ksh-20120801-memlik3.patch
src/cmd/ksh93/include/name.h,
src/cmd/ksh93/sh/subshell.c:
- Replace the nv_subsaved() function by the version from ksh 93v-.
This version frees a table from memory if the NV_TABLE flag is
passed in the new second parameter, a bitmask for flags (which
was oddly named 'table'; I've renamed it to 'flags').
src/cmd/ksh93/sh/name.c:
- nv_delete(): When calling nv_subsaved(), pass on the NV_TABLE
flag if given.
- table_unset(): Call nv_delete() with the NV_TABLE flag.
src/cmd/ksh93/tests/leaks.sh:
- Add test based on the reproducer provided in Red Hat bug 1036470.
