*.getn discipline functions cause .sh.value to have a float type
for arithmetic expressions that get the value of foo, avoiding the
problem of having to convert between floats and strings (e.g.
rounding errors). There is no corresponding .setn discipline.
A search in the ast-open-archive repo reveals that the getn
discipline was quietly added in version 2009-08-21 93t+, with not
even a mention in the RELEASE file.
The one available mention on the internet is this old thread:
https://www.mail-archive.com/ast-users@research.att.com/msg00601.html
Apparently a setn discipline *was* planned, but never implemented.
getn discipline functions may also crash in several ways. I've been
unsuccessful at solving all the crashes, particularly as one of
them is intermittent. This should not be in the 1.0 release.
Further discussion: https://github.com/ksh93/ksh/issues/435
This now allows our little make utility to run when compiled with
AddressSanitizer on gcc on Linux; it no longer aborts the build on
exit with a complaint about (very small) memory leaks.
The change in .github/workflows/ci.yml (use 'bin/package test' to
run the iffe regression tests as well) caused the GitHub workflow
to fail immediately with a syntax error. This is because iffe.tst
is a ksh93 script, but the runner does not have a ksh93 installed
as a system package.
As of e08ca80d, package prefers a known-good system shell over the
newly compiled shell to stop builds from failing when I break ksh,
making it convenient to fix it. But that change should not apply to
the regression tests; they should use the newly compiled shell.
src/cmd/INIT/package.sh, bin/package:
- Set KEEP_SHELL to 2 when given a SHELL=* argument.
- Before running the regression tests, override the known-good
shell with the compiled one if $KEEP_SHELL < 2, ensuring that all
tests, including the iffe ones, are run with the compiled shell.
This allows 'bin/package test' to run if the known-good shell is
not a ksh93, while testing that the compiled shell successfully
runs iffe. (re: e08ca80d)
- Standardise 'test' command use: -a and -o are deprecated.
- Remove some more unused cruft. (re: 6137b99a)
src/cmd/ksh93/Mamfile:
- Do not override SHELL when running shtests; this is now set
correctly in 'bin/package test'.
src/cmd/INIT/rt.sh:
- Removed. This regression test output filter was only used with
nmake, which we deleted. (re: 2940b3f5, 6cc2f6a0, aa601a39)
When varsub(), which "handles $param, ${param}, and ${param op
word}", handles arrays, it obtains an 'ap' array pointer once using
nv_arrayptr(np). In several locations it calls nv_putsub() which
may call array_grow() which invalidates that pointer. But the
pointer is never updated. So whenver an array grows, there is a
use-after-free problem, easily caught by AddresSanitizer.
This commit makes sure the 'ap' pointer, if non-null, is refreshed
whenever nv_putsub() may previously have been called.
Notable changes:
.github/workflows/ci.yml:
- Run 'bin/package test' on the github runner so we test iffe too.
src/cmd/ksh93/sh/subshell.c:
- sh_assignok was usually called like 'np = sh_assignok(np,0)'. But
the function never changes np, it just returns the np value
passed to it, so the assignment is pointless and that function
can be changed to a void.
src/cmd/ksh93/sh/fault.c: sh_fault():
- Remove check for sh.subshell after sh_isstate(SH_INTERACTIVE). As
of 48ba6964, it is never set in subshells.
When running an external command while trapping Ctrl+C via SIGINT,
and set -b is on, then a spurious Done job control message is
printed. No background job was executed.
$ trap 'ls' INT
$ set -b
$ <Ctrl+C>[file listing follows]
[1] + Done set -b
In jobs.c (487-493), job_reap() calls job_list() to list a running
or completed background job, passing the JOB_NFLAG bit to only
print jobs with the P_NOTIFY flag. But the 'ls' in the trap is not
a background job. So it is getting the P_NOTIFY flag by mistake.
In fact all processes get the P_NOTIFY flag by default when they
terminate. Somehow the shell normally does not follow a code path
that calls job_list() for foreground processes, but does when
running one from a trap. I have not yet figured out how that works.
What I do know is that there is no reason why non-background
processes should ever have the P_NOTIFY flag set on termination,
because those should never print any 'Done' messages. And we seem
to have a handy P_BG flag that is set for background processes; we
can check for this before setting P_NOTIFY. The only thing is that
flag is only compiled in if SHOPT_BGX is enabled, as it was added
to support that functionality.
For some reason I am unable to reproduce the bug in a pty session,
so there is no pty.sh regression test.
src/cmd/ksh93/sh/jobs.c:
- Rename misleadingly named P_FG flag to P_MOVED2FG; this flag is
not set for all foreground processes but only for processes moved
to the foreground by job_switch(), called by the fg command.
- Compile in the P_BG flag even when SHOPT_BGX is not enabled. We
need to set this flag to check for a background job.
- job_reap(): Do not set the P_NOTIFY flag for all terminated
processes, but only for those with P_BG set.
src/cmd/ksh93/sh/xec.c: sh_fork():
- Also pass special argument 1 for background job to job_post() if
SHOPT_BGX is not enabled. This is what gets it to set P_BG.
- Simplify 5 lines of convoluted code into 1.
Resolves: https://github.com/ksh93/ksh/issues/481
Switching the function scope to a parent scope by assigning to
.sh.level (SH_LEVELNOD) leaves the shell in an inconsistent state,
causing invalid-free and/or use-after-free bugs. The intention of
.sh.level was always to temporarily switch scopes inside a DEBUG
trap, so this commit minimises the pitfalls and instability by
imposing some sensible limitations:
1. .sh.level is now a read-only variable except while executing a
DEBUG trap;
2. while it's writeable, attempts to unset .sh.level or to change
its attributes are ignored;
3. attempts to set a discipline function for .sh.level are ignored;
4. it is an error to set a level < 0 or > the current scope.
Even more crashing bugs are fixed by simplifiying the handling and
initialisation of .sh.level and by exempting it completely from
virtual subshell scoping (to which it's irrelevant).
TODO: one thing remains: scope corruption and use-after-free happen
when using the '.' command inside a DEBUG trap with ${.sh.level}
changed. Behaviour same as before this commit. To be investigated.
All changed files:
- Consistently use the int16_t type for level values as that is the
type of its non-pointer storage in SH_LEVELNOD.
- Update .sh.level by using an update_sh_level() macro that assigns
directly to the node value, then restores the scope if needed.
- To eliminate implicit typecasts, use the same int16_t type (the
type used by short ints such as SH_LEVELNOD) for all variables
containing a function and/or dot script level.
src/cmd/ksh93/include/variables.h:
- Add update_sh_level() macro.
src/cmd/ksh93/include/name.h,
src/cmd/ksh93/sh/macro.c:
- Add a nv_nonptr() macro that checks attributes for a non-pointer
value -- currently only signed or unsigned short integer value,
accessed via the 's' member of 'union Value' (e.g. np->nvalue.s).
- nv_isnull(): To avoid undefined behaviour, check for attributes
indicating a non-pointer value before accessing the nvalue.cp
pointer (re: 5aba0c72).
- varsub(): In the set/unset check, remove the now-redundant
exception for SH_LEVELNOD.
src/cmd/ksh93/data/variables.c,
src/cmd/ksh93/sh/init.c:
- shtab_variables[]: Make .sh.level a read-only short integer.
- sh_inittree(): To avoid undefined behaviour, do not assign to the
'union Value' char pointer if the attribute indicates a non-
pointer short integer value. Instead, the table value is ignored.
src/cmd/ksh93/sh/subshell.c: sh_assignok():
- Never create a subshell scope for SH_LEVELNOD.
src/cmd/ksh93/sh/xec.c:
- Get rid of 'struct Level' and its maxlevel member. This was only
used in put_level() to check for an out of range assignment, but
this can be trivially done by checking sh.fn_depth+sh.dot_depth.
- This in turn allows further simplification that reduces init for
.sh.level to a single nv_disc() call in sh_debug(), so get rid of
init_level().
- put_level(): Throw a "level out of range" error if assigned a
wrong level.
- sh_debug():
- Turn off the NV_RDONLY (read-only) attribute for SH_LEVELNOD
while executing the DEBUG trap.
- Restore the current scope when trap execution is finished.
- sh_funct(): Remove all .sh.level handling. POSIX functions (and
dot scripts) already handle it in b_dot_cmd(), so sh_funct(),
which is used by both, is the wrong place to do it.
- sh_funscope(): Update .sh.level for ksh syntax functions here
instead. Also, do not bother to initialise its discipline here,
as it can now only be changed in a DEBUG trap.
src/cmd/ksh93/bltins/typeset.c: setall():
- When it's not read-only, ignore all attribute changes for
.sh.level, as changing the attributes would crash the shell.
src/cmd/ksh93/sh/nvdisc.c: nv_setdisc():
- Ignore all attempts to set a discipline function for .sh.level,
as doing this would crash the shell.
src/cmd/ksh93/bltins/misc.c: b_dot_cmd():
- Bug fix: also update .sh.level when quitting a dot script.
src/cmd/ksh93/sh/name.c:
- _nv_unset():
- To avoid an inconsistent state, ignore all attempts to unset
.sh.level.
- To avoid undefined behaviour, do not zero np->nvalue.cp if
attributes for np indicate a non-pointer value (the actual bit
value of a null pointer is not defined by the standard, so
there is no guarantee that zeroing .cp will zero .s).
- sh_setscope(): For consistency, always set error_info.id (the
command name for error messages) to the new scope's cmdname.
Previously this was only done for two calls of this function.
- nv_name(): Fix a crashing bug by checking that np->nvname is a
non-null pointer before dereferencing it.
src/cmd/ksh93/include/nval.h:
- The NV_UINT16P macro (which is unsigned NV_INT16P) had a typo in
it, which went unnoticed for many years because it's not directly
used (though its bit flags are set and used indirectly). Let's
fix it anyway and keep it for completeness' sake.
The lexer use 256-byte state tables (see data/lexstates.c), one
byte per possible value for the (unsigned) char type. But the sp
variable used as an index to a state table in loops like this...
while((n = state[*sp++]) == 0)
;
...is a char*, a pointer to a char. The C standard does not define
if the char type is signed or not (!). On clang and gcc, it is
signed. That means that, whenever a single-byte, high-bit (> 127)
character is encountered, the value wraps around to negative, and a
read occurs outside of the actual state table, causing potentially
incorrect behaviour or a crash.
src/cmd/ksh93/sh/lex.c:
- endword(): Make sp and three related variables explicitly
unsigned char pointers. This requires a bunch of annoying
typecasts to stop compilers complaining; so be it.
- To avoid even more typecasts, make stack_shift() follow suit.
- Reorder variable declarations for legibility.
num_clone handles simple numeric values of all types but not array
nodes. Calling it for an array node caused the arith.sh regression
test below to crash in num_clone() with a buffer overflow when ksh
is compiled with AddressSanitizer. The array has the NV_INTEGER
attribute because it is an array of numeric values, but that
doesn't mean the array node itself holds a number.
After this, all the arith.sh tests pass with AddressSanitizer.
The failing test in arith.c was this one, introduced in d50d3d7c:
got=$(
typeset -r -A -i ro_arr=([a]=10 [b]=20 [c]=30)
set +x
for ((i=0; i<loopcount; i++)); do
( ((ro_arr[i+1] += 5)) )
done 2>&1
)
[[ $got == *recursion* ]] && err_exit "recursion level not reset on readonly error (subshell)"
Reproducer: Compile a ksh with AddressSanitizer. In that ksh, edit
the last command line with 'fc', insert an empty line at the start,
and save. Now use the up-arrow to retrieve the empty line. Ksh
aborts on history.c line 1011 as hist_copy() tries to read before
the beginning of the buffer pointed to by s1.
src/cmd/ksh93/edit/history.c: hist_copy():
- Verify that the s1 pointer was increased from the original s1
before trying to read the character *(s1-1).
The race is between '$cat |&' and 'kill $pid'. In between, there
are only a variable assignment and two buffered writes, so there
is nothing that waits for the external 'cat' to finish forking,
execve'ing and initialising -- meaning there is no guarantee it is
ready to catch SIGTERM. This explains the hang; 'cat' misses the
signal, continues to initialise, and simply waits for more input.
src/cmd/ksh93/tests/coprocess.sh:
- Actually read from the /bin/cat coprocess and verify that it
works. This has the beneficial side effect of ensuring it is
fully loaded and initialised before SIGTERMing it.
Resolves: https://github.com/ksh93/ksh/issues/132
Reproducer:
$ x=([x]=1 [y)
-ksh: syntax error: `)' unexpected
$ [[ -z $x ]]
-ksh: [[ -z ]]: not found
Any '[[' command following that syntax error will fail similarly;
the whole of it (after variable expansion) is incorrectly looked up
as a command name. The syntax error must be generated by an
associative array assignment (with or without an explicit typeset
-A) with at least one valid assignment element followed by an
invalid assignment element starting with '[' but not containing
']='.
This seems to be another bug that is in every ksh93 version ever.
I've confirmed that ksh 1993-12-28 s+ and ksh2020 fail identically.
Presumably, so does everything in between.
Analysis:
The syntax error function, sh_syntax(), calls lexopen() in mode 0
to reset the lexer state. There is a variable that isn't getting
reset there though it should be. Using systematic elimination I
found that the variable that needs to be reset is lp->assignok (set
"when name=value is legal"). If it is set, '[[' is not processed.
src/cmd/ksh93/sh/lex.c: lexopen():
- Reset 'assignok' in the lexer state (regardless of mode).
- In the mode 0 total lexer state reinit, several members of lexd
(struct _shlex_pvt_lexdata_) were not getting reset; just memset
the whole thing to zero.
Note for backporters: this change requires commit da97587e to
be correct. That commit took the stack size and pointer (lex_max
and *lex_match) out of this struct; those should not be reset!
Resolves: https://github.com/ksh93/ksh/issues/486
Dynamically loadable built-ins do not work well with a statically
linked ksh; they cannot use ksh's statically linked copies of
libast and libshell, so they would need to bring their own, but
multiple copies of those don't play well together. So dynamically
loaded built-ins cannot interface with the shell. Only non-AST,
non-SFIO built-ins are possible. Which is something that perhaps
five people in the world know how to do as this is not documented
anywhere (hint: your built-in needs the BLT_NOSFIO attribute to use
stdio without problems). And those five people are also able to
compile their own ksh with SHOPT_DYNAMIC reenabled.
Plus, the SHOPT_DYNAMIC code causes strange $PATH search
regressions on a few systems. The cause of that bug has eluded me
so far, but disabling this is effectively a fix on those systems.
src/cmd/ksh93/SHOPT.sh:
- Turn off SHOPT_DYNAMIC by default.
src/cmd/ksh93/data/builtins.c:
- Do not compile in irrelevant sh_optbuiltin[] (builtin --man)
documentation if SHOPT_DYNAMIC is disabled.
The SHOPT_MULTIBYTE compile-time option did not make much sense as
disabling it only disabled multibyte support for ksh/libshell, not
libast or libcmd built-in commands. This commit allows disabling
multibyte support for the entire codebase by defining the macro
AST_NOMULTIBYTE (e.g. via CCFLAGS). This slightly speeds up the
code and makes an optimised binary about 5% smaller.
src/lib/libast/include/ast.h:
- Add non-multibyte fallback versions of the multibyte macros that
are used if AST_NOMULTIBYTE is defined. This should cause most
multibyte handling to be automatically optimised out everywhere.
- Reformat the multibyte macros for legibility.
- Similify mbchar() and and mbsize() macros by defining them in
terms of mbnchar() and mbnsize(), eliminating code duplication.
- Correct non-multibyte fallback of mbwidth(). For consistent
behaviour, control characters and out-of-range values should
return -1 as they do for UTF-8. The fallback is now the same as
default_wcwidth() in src/lib/libast/comp/setlocale.c.
src/lib/libast/comp/setlocale.c:
- If AST_NOMULTIBYTE is defined, do not compile in the debug and
UTF-8 locale conversion functions, including several large
conversion tables. Define their fallback macros as 0 as these are
used as function pointers.
src/cmd/ksh93/SHOPT.sh,
src/cmd/ksh93/Mamfile:
- Change the SHOPT_MULTIBYTE default to empty, indicating "probe".
- Synchronise SHOPT_MULTIBYTE with !AST_NOMULTIBYTE by default.
src/cmd/ksh93/include/defs.h:
- When SHOPT_MULTIBYTE is zero but AST_NOMULTIBYTE is not non-zero,
then enable AST_NOMULTIBYTE here to use the ast.h non-multibyte
fallbacks for ksh. When this is done, the effect is that
multibyte is optimized out for ksh only, as before.
- Remove previous fallback for disabling multibyte (re: c2cb0eae).
src/cmd/ksh93/include/lexstates.h,
src/cmd/ksh93/sh/lex.c:
- Define SETLEN() macro to assign to LEN (i.e. _Fcin.fclen) for
multibyte only and do not assign to it directly. With no
SHOPT_MULTIBYTE, define that macro as empty. This allows removing
multiple '#if SHOPT_MULTIBYTE' directives from lex.c, as that
code will all be optimised out automatically if it's disabled.
src/cmd/ksh93/include/national.h,
src/cmd/ksh93/sh/string.c:
- Fix flagrantly incorrect non-multibyte fallback for sh_strchr().
The latter returns an integer offset (-1 if not found), whereas
strchr(3) returns a char pointer (NULL if not found). Incorporate
the fallback into the function for correct handling instead of
falling back to strchr(3) directly.
src/cmd/ksh93/sh/macro.c:
- lastchar() optimisation: avoid function call if SHOPT_MULTIBYTE
is enabled but we're not actually in a multibyte locale.
src/cmd/ksh93/sh/name.c:
- Use ja_size() even with SHOPT_MULTIBYTE disabled (re: 2182ecfa).
Though no regression tests failed, the non-multibyte fallback for
typeset -L/-R/-Z length calculation was probably not quite
correct as ja_size() does more. The ast.h change to mbwidth()
ensures correct behaviour for non-multibyte locales.
src/cmd/ksh93/tests/shtests:
- Since its value in SHOPT.sh is now empty by default, add a quick
feature test (for the length of the UTF-8 character 'é') to check
if SHOPT_MULTIBYTE needs to be enabled for the regression tests.
inout() initialises its 'token' variable to the value of
lexp->token. So when inout() returns upon finding a process
subtitution, lexp->token is known to contain either IPROCSYM or
OPROCSYM; simple() can check for that instead, making a global flag
unnecessary.
The fact that inout() calls itself recursively to process multiple
redirections does not influence this because the recursive call is
done right before returning from the current call.
Reproducer:
$ fn=([foo_key]=foo_val [bar_key])
-ksh: [bar_key]: not found
Expected output:
-ksh: syntax error: `[bar_key]' unexpected
As soon as one correct associative array assignment element has
been processed, a subsequent one, starting with '[' but not
containing ']=', is incorrectly seen as a command to execute.
If a command '[bar_key]' existed on $PATH, it would have been run.
src/cmd/ksh93/sh/parse.c: simple():
- In the syntax check for associative array assignments, don't just
check for an initial '[' but also verify the presence of ']='.
Thanks to @JohnoKing for finding this bug.
Resolves: https://github.com/ksh93/ksh/issues/427
Grammatically, redirections may occur anywhere within a command
line and are removed after processing them, whereas a process
substitution (<(commandlist) or >(commandlist)) is replaced by a
file name which should be treated as just another simple word.
So the following should not be a syntax error:
$ cat </dev/null <(true)
-ksh: syntax error: `)' unexpected
$ cat </dev/null >(true)
-ksh: syntax error: `)' unexpected
$ cat >/dev/null <(true)
-ksh: syntax error: `)' unexpected
$ cat >/dev/null >(true)
-ksh: syntax error: `)' unexpected
This bug is in every ksh93 version.
The problem is in the parser (parse.c). The process substitution is
misparsed as a redirection due to inout() recursively parsing
multiple redirections without recognising process substitutions.
inout() is mistaking '<(' for '<' and '>(' for '>', which explains
the incorrect syntax error.
This also causes the following to fail to detect a syntax error:
$ cat >&1 <(README.md
[the contents of README.md are shown]
...and other syntax errors detected in the wrong spot, for example:
$ { true; } <(echo wrong)
-ksh: syntax error: `wrong' unexpected
which should be:
-ksh: syntax error: `<(' unexpected
src/cmd/ksh93/sh/parse.c:
- Add global inout_found_procsub flag.
- inout(): On encountering a process substitution, set this flag
and return, otherwise clear the flag.
- simple(): After calling inout(), check this flag and, if set,
jump back to where process substitutions are parsed.
Resolves: https://github.com/ksh93/ksh/issues/418
Notable changes:
- sh/timers.c: Rename timerdel() to sh_timerdel() for consistency;
we had timerdel() but sh_timeradd().
- include/shell.h: Remove unused sh struct members:
- sh.lastpath (this was still being saved/restored in several
places, but not actually used since 2008-06-02 ksh93t-)
- sh.lastbase (unused since 2000-10-31 ksh93k)
- sh.inpool (unused since libcoshell was removed in 3613da42)
- sh/xec.c: sh_exec(): Add comments for the various command types.
For some reason that I can't be bothered to investigate, gcc 3.3 on
my museum-grade PowerMac G5 running Mac OS X 10.3 doesn't like -lm
when compiling the output{...}end blocks in features/pty and
features/dll. The compilation fails silently. But some other
systems require the -lm for it to work.
Thankfully iffe has a syntax for trying a test repeatedly with
different flags (a.k.a. prerequisites). From 'iffe --man':
- Prereq grouping mark; prereqs before the first - are
passed to all feature tests. Subsequent groups are
attempted in left-to-right order until the first
successful group is found.
src/cmd/builtin/features/pty,
src/lib/libdll/features/dll:
- Try compiling the output{...}end block once with -lm and, if that
fails, try once without -lm before erroring out.
We try to stay compatibile with C90. Turns out that repeating a
typedef is valid only from C11 onwards, as a feature taken from
C++. So I goofed and broke the build on old or strict compilers.
src/cmd/ksh93/include/{name,shell}.h:
- union Value: Since we will now once again have to typecast to use
nvalue.bfp in any case, just make it a void pointer; that is how
pointers that require typecasts are handled in every other case.
- Since the funptr() macro needs a typecast to Shbltin_f which is
defined in libast's shcmd.h, move this macro to shell.h which
(unlike name.h) includes that header.
src/cmd/ksh93/sh/{init,nvdisc}.c:
- Typecast to void* when assigning to *->nvalue.bfp.
src/lib/libast/include/shcmd.h:
- Use shell_h_defined (introduced in 4491bc6a) and defs_h_defined
to check if ksh's shell.h or defs.h were included before shcmd.h,
instead of random macros defined by them; much clearer.
The 'break' and 'continue' flow control commands use three int
variables in the scoped sh.st struct:
sh.st.execbrk: nonzero if 'break' or 'continue' are used
sh.st.breakcnt: number of levels to 'break'/'continue'
(negative if 'continue')
sh.st.loopcnt: loop level counter for 'break'/'continue'
Reading the code that sets and uses these (in bltins/cflow.c and
sh/xec.c) makes it fairly obvious that the sh.st.execbrk flag is
redundant; it is zero if no 'break' or 'continue' should happen,
but the same is true for sh.st.breakcnt.
This commit simplifies the code by removing sh.st.execbrk.
It also adds some comments clarifying the use of the other two.
Trivia: the ancient "Version 06/03/86a" ksh source code was
recently discovered. It uses global execbrk, breakcnt and loopcnt
variables with the same redundancy. More evidence that the AT&T
team always lacked a question-everything department...
https://minnie.tuhs.org/pipermail/tuhs/2020-December/022640.htmlhttps://github.com/weiss/original-bsd/blob/master/local/toolchest/ksh/sh/builtin.chttps://github.com/weiss/original-bsd/blob/master/local/toolchest/ksh/sh/xec.c
Reproducer:
$ ksh -c 'trap "echo OK" TERM; (kill -s TERM $$)'
Actual output: none
Expected output: OK
The bug is only triggered if 'kill' is executed from a subshell
that is optimised out due to being the last command in the script.
src/cmd/ksh93/sh/xec.c: sh_exec(): case TPAR:
- Instead of only checking for EXIT and ERR traps, do not avoid
creating a virtual subshell if there are any traps (except DEBUG,
SIGKILL, SIGSTOP); for this, use the sh.st.trapdontexec flag
introduced in 40245e08.
Reproducer (on macOS/*BSD where SIGUSR1 has signal number 30):
$ ksh -c '(sh -c '\''kill -s USR1 $$'\''); echo $?'
ksh: 54220: User signal 1
30
Expected output for $?: 286, not 30. The signal is not reflected in
the 9th bit of the exit status.
This bug was introduced for virtual subshells in b3050769 but
exists in every ksh93 version for real (forked) subshells:
$ ksh -c '(ulimit -t unlimited; trap : EXIT; \
sh -c '\''kill -s USR1 $$'\''); echo $?'
ksh: 54267: User signal 1
30
(As of d6c9821c, a dummy trap is needed to trigger the bug, or it
will be masked by the exec optimization for the sh invocation.)
This is caused by the exit status being masked to 8 bits when a
subshell terminates. For a real subshell, this is inevitable as the
kernel does this. As of b3050769, virtual subshells behave in a
manner consistent with real subshells in this regard.
However, for both virtual and real subshells, if its last command
was terminated by a signal, then that should still be reflected in
the 9th bit of ksh's exit stauts.
The root of the problem is that ksh simply cannot rely internally
on the 9th bit of the exit status to determine if a command exited
due to a signal. The 9th bit may be trimmed by a subshell or may be
set by 'return' without a signal being involved. This commit fixes
it by introducing a separate flag which will be a reliable
indicator of this.
src/cmd/ksh93/include/shell.h:
- Add sh.chldexitsig flag (set if the last command was a child
process that exited due to a signal).
src/cmd/ksh93/sh/jobs.c: job_wait():
- When the last child process exited due to a signal, not only set
the 9th (SH_EXITSIG) bit of sh.exitval but also sh.chldexitsig.
src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Fix the virtual subshell reproducer above. After trimming the
exit status to 8 bit, set the 9th bit if sh.chldexitsig is set.
This needs to be done in two places: one that runs in the parent
process after sh_subfork() and one for the regular virtual
subshell exit.
src/cmd/ksh93/sh/fault.c:
- sh_trap(): Save and restore sh.chldexitsig so that this fix does
not get deactivated if a trap is set.
- sh_done():
- Fix the real subshell reproducer above. When the last command
of a real subshell is a child process that exited due to a
signal (i.e., if (sh.chldexitsig && sh.realsubshell)), then
activate the code to pass down the signal to the parent
process. Since there is no way to pass a 9-bit exit status to a
parent process, this is the only way to ensure a correct exit
status in the parent shell environment.
- When exiting the main shell, use sh.chldexitsig and not the
unreliable SH_EXITSIG bit to determine if the 8th bit needs to
be set for a portable exit status indicating its last command
exited due to a signal.
When a user invokes 'ksh +o emacs' they should not land in the
emacs line editor, as they explicitly asked not to have it. So
default to no editor mode in that case.
The fault.c and edit.c changes in this commit were inspired by
changes in the 93v- beta but take a slightly different approach:
mainly, the code to update $COLUMNS and $LINES is put in its own
function instead of duplicated in sh_chktrap() and ed_setup().
src/cmd/ksh93/sh/fault.c:
- Move code to update $COLUMNS and $LINES to a new
sh_update_columns_lines() function so it can be reused.
- Fix compile error on systems without SIGWINCH.
src/cmd/ksh93/edit/edit.c:
- ed_setup(): Call sh_update_columns_lines() instead of issuing
SIGWINCH to self.
- Change two sh_fault(SIGINT) calls to issuing the signal to the
current process using kill(2). sh_fault() is now never called
directly (as in the 93v- beta).
src/cmd/ksh93/sh/main.c: sh_main():
- On non-interactive, call sh_update_columns_lines() and set the
signal handler for SIGWINCH to sh_fault(), causing $COLUMNS and
$LINES to be kept up to date when the terminal window is resized
(this is handled elsewhere for interactive shells). This change
makes ksh act like mksh, bash and zsh. (Previously, ksh required
setting a dummy SIGWINCH trap to get auto-updated $COLUMNS and
$LINES in scripts, as this set the SIGWINCH signal handler to
sh_fault(). This persisted even after unsetting the trap again,
so that was inconsistent behaviour.)
src/cmd/ksh93/include/shell.h:
- Don't define sh.winch on systems without SIGWINCH.
src/cmd/ksh93/sh.1:
- Update and tweak the COLUMNS and LINES variable documentation.
- Move them up to the section of variables that are set by the
shell (which AT&T should have done before).
@stephane-chazelas reports:
> A very weird issue:
>
> To reproduce on GNU/Linux (here as superuser)
>
> # truncate -s10M file
> # export DEV="$(losetup -f --show file)"
> # ksh -c 'exec 3<> "$DEV" 3>#((0))' # fine
> # ksh -c 'exec 1<> file 1>#((0))' # fine
> # ksh -c 'exec 1<> "$DEV" 1>#((0))'
> ksh: 0: invalid seek offset
>
> Any seek offset is considered "invalid" as long as the file is a
> block device and the fd is 0, 1 or 2. It's fine for fds above 2
> and it's fine with any fd for regular files.
Apparently, block devices are not seekable with sfio. In io.c there
is specific code to avoid using sfio's sfseek(3) if there is no
sfio stream in sh.sftable[] for the file descriptor in question:
1398: Sfio_t *sp = sh.sftable[fn];
[...]
1420: if(sp)
1421: {
1422: off=sfseek(sp, off, SEEK_SET);
1423: sfsync(sp);
1424: }
1425: else
1426: off=lseek(fn, off, SEEK_SET);
For file descriptors 0, 1 or 2 (stdin/stdout/stderr), there is a
sh.sftable[] stream by default, and it is marked as not seekable.
This makes it return -1 in these lines in sfseek.c, even if the
system call called via SFSK() succeeds:
89: if(f->extent < 0)
90: { /* let system call set errno */
91: (void)SFSK(f,(Sfoff_t)0,SEEK_CUR,f->disc);
92: return (Sfoff_t)(-1);
93: }
...which explains the strange behaviour.
src/lib/libast/sfio/sfseek.c: sfseek():
- Allow for the possibility that the fallback system call might
succeed: let it handle both errno and the return value.
Resolves: https://github.com/ksh93/ksh/issues/318
Notable changes:
- Change a bunch of uses of memcmp(3) to compare strings (which can
cause buffer read overflows) to strncmp(3).
- src/cmd/ksh93/include/name.h: Eliminate redundant Nambfp_f type.
Replace with Shbltin_f type from libast's shcmd.h. Since that is
not guaranteed to be included here, repeat the type definition
here without fully defining the struct (which is valid in C).
Lexical levels are stored in a dynamically grown array of int values
grown by the stack_grow function. The pointer lex_match and the
maximum index lex_max are part of the lexer state struct that is now
saved and restored in various places -- see e.g. 37044047, a2bc49be.
If the stack needs to be grown, it is reallocated in stack_grow()
using sh_realloc(). If that happens between saving and restoring the
lexer state, then an outdated pointer is restored, and crash.
src/cmd/ksh93/include/shlex.h,
src/cmd/ksh93/sh/lex.c:
- Take lex_match and lex_max out of the lexer state struct and make
them separate static variables.
src/cmd/ksh93/edit/edit.c:
- While we're at it, save and restore the lexer state in a way that
is saner than the 93v- beta approach (re: 37044047) as well as
more readable. Instead of permanently allocating memory, use a
local variable to save the struct. Save/restore directly around
the sh_trap() call that actually needs this done.
Resolves: https://github.com/ksh93/ksh/issues/482
Reproducers:
$ ksh -c 'typeset -a arr=( ( (a $(($(echo 1) + 1)) c)1))'
ksh: echo: arr[0]._AST_FEATURES=CONFORMANCE - ast UNIVERSE - ucb: cannot be an array
ksh: [1]=1: invalid variable name
$ ksh -c 'typeset -a arr=( (a $(($(echo 1) + 1)) c)1)'
ksh: echo: arr._AST_FEATURES=CONFORMANCE - ast UNIVERSE - ucb: is not an identifier
ksh: [1]=1: invalid variable name
src/cmd/ksh93/sh/name.c: sh_setenviron():
- Save and clear the current compound assignment prefix (sh.prefix)
while assigning to the _AST_FEATURES variable.
It's undocumented, it's broken and can crash the shell, and it's
unclear if it can ever be fixed. So with a 1.0 release (hopefully)
not very far off, it's time to remove it from the 1.0 branch.
Related: https://github.com/ksh93/ksh/issues/422
src/cmd/ksh93/include/name.h:
- Include the ususally-wanted (Shbltin_f) typecast in funptr().
Various files:
- Change several direct foo->nvalue.bfp usages to funptr(np).
- Reduce explicit typecasts after the name.h change.
- To determine if we are (or just were) running a certain built-in
command, instead of comparing sh.bltindata.bnode with a builtin
table node, use sh.bltinfun to directly compare the builtin's
function; this is more readable and efficient.
The xargs-like functionality of 'command -x' was still failing with
E2BIG in cases or on systems where the environment variables list
is very large. For instance, on a default NixOS installation it's
about 50k by default (absurd; *each* process carries this weight).
This commit tweaks the feature test and introduces a runtime
fallback if it still fails.
POSIX: "The number of bytes available for the new process' combined
argument and environment lists is {ARG_MAX}. It is implementation-
defined whether null terminators, pointers, and/or any alignment
bytes are included in this total."
https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
More recommended reading:
https://mina86.com/2021/the-real-arg-max-part-1/https://mina86.com/2021/the-real-arg-max-part-2/
So, operating systems are free to consume ARG_MAX space in whatever
bizarre way they want, and may even come up with more innovative
ways to waste buffer space in future. <sigh>
command_xargs() allows for the possibility of adding a certain
number of extra bytes per argument to account for pointers and
whatnot. As of this commit, we still start off from the value that
was determined by the _arg_extrabytes test in features/externs, but
path_spawn() will now increase that number at runtime and retry if
E2BIG still occurs. Hopefully this makes it future-proof.
src/cmd/ksh93/features/externs:
- Rename generated ARG_EXTRA_BYTES macro to _arg_extrabytes for
better naming consistency with other iffe feature tests.
- Tweaks to avoid detecting 9 extra bytes instead of 8 on some
versions of 64-bit Linux (it needs the size of a 64 bit pointer).
- Show the result in the iffe output.
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/init.c:
- Do not store getconf(CONF_ARG_MAX) at init time; on Linux, this
value may be changed dynamically (via ulimit -s), so it must be
re-obtained on every use.
src/cmd/ksh93/sh/path.c:
- command_xargs():
- Use a static global variable for the number of extra bytes per
argument. Initialise it with the results of the feature test.
This allows increasing it at runtime if an OS does something
weird causing an E2BIG failure.
- Abort instead of return if command_xargs() is called with
sh.xargmin < 0; this should never happen.
- To allow retrying without crashing, restore saved args before
returning -1.
- Leave more generous space for the environment -- half the size
of the existing environment. This was experimentally determined
to be needed to keep Linux and macOS happy.
- Instead of crashing, return with E2BIG if there is too little
space to run.
- Get rid of unnecessary (void*) typecasts; we no longer pretend
to be compatible with C++ (re: a34e8319).
- Remove a couple of dead 'if(saveargs) free(saveargs);'
statements; at those points, saveargs is known to be NULL.
- Return -2 instead of -1 when retrying would be pointless.
- path_spawn():
- When command_xargs() returns -1 and the error is E2BIG,
increase the number of extra bytes by the size of a char*
pointer and try again. Give up if adding bytes the size of 8
char* pointers fails.
src/cmd/ksh93/sh/xec.c: check_exec_optimization():
- Do not use this optimization if we are running 'command -x';
I noticed some instances of the PATH search yielding incorrect
results if we do. TODO: work this out at some point.
Reproducer:
trap : USR1
while :; do kill -s USR1 $$ || exit; done &
while :; do : >/dev/null; done
It can take between a fraction of a second and a few minutes, but
eventually it will fail like this:
$ ksh foo
foo[3]: /dev/null: cannot create
kill: 77946: no such process
It fails similarly with "cannot open" if </dev/null is used instead
of >/dev/null.
This is the same problem as in the referenced commit, except when
handling traps -- so the same fix is required in sh_fault().
Researching #483 revealed several instances in coprocess cleanup
where sh_close() is called with a file descriptor of -1 (which
flags that the pipe is already closed). As of feeb62d1, this sets
errno to EBADF. While the race condition triggered by this was
fixed in 411481eb, it's still better to avoid it.
This re-applies most of the changes reverted in 80767b1f.
Reproducer for a race condition:
typeset -i i
cat=${ command -pv cat; }
for((i=0;i<20000;i++))
do printf '\r%d ' "$i"
ksh |&
cop=$!
print -p $'print hello | '$cat$'\nprint '$exp
read -t 1 -p
read -t 1 -p
redirect 5<&p 6>&p 5<&- 6>&-
{ sleep .4; kill $cop; } &
spy=$!
if wait $cop 2>/dev/null # "/dev/null: cannot create"?
then kill $spy
else kill $spy; exit # boom
fi
wait
done
Result on my system. The time it takes to fail varies a lot:
$ arch/*/bin/ksh foo
717 foo[13]: /dev/null: cannot create [Bad file descriptor]
$
Analysis:
errno may get a value during SIGCHLD processing. This may cause the
functions called by the signal interrupt to call sh_close() with a
negative file descriptor, giving errno the EBADF value as this loop
in sh_open() is interrupted:
ksh/src/cmd/ksh93/sh/io.c in 94fc983
851: while((fd = open(path, flags, mode)) < 0)
852: if(errno!=EINTR || sh.trapnote)
853: return(-1);
Commit feeb62d1 caused sh_close() to set errno to EBADF if it gets
called with a negative file descriptor. In the cleanup of
coprocesses during the SIGCHLD interrupt handling, there are
several locations where sh_close() may be called with a negative
file descriptor, which now sets errno to EBADF. If SIGCHLD is
handled between open() being interrupted and the check for
errno!=EINTR, sh_open() will return -1 instead of retrying, causing
sh_redirect() to issue the "cannot create" error message.
SIGCHLD is issued whenever a process (such as the coprocess in the
reproducer) terminates. job_reap() is called (via job_waitsafe())
to handle this. That function does not always restore errno, which
allowed this race condition to happen.
src/cmd/ksh93/sh/jobs.c: job_reap():
- Always save/restore errno.
Resolves: https://github.com/ksh93/ksh/issues/483
This reverts commit a72ba2cf59.
It's broken:
test coprocess begins at 2022-06-20+02:34:59
coprocess.sh[198]: FAIL: traps when reading from cat coprocess not working
coprocess.sh[198]: FAIL: traps when reading from /bin/cat coprocess not working
test coprocess failed at 2022-06-20+02:35:01 with exit code 2 [ 32 tests 2 errors ]
Reopens: https://github.com/ksh93/ksh/issues/483
Intermittent regression test failure:
test coprocess begins at 2022-02-08+23:41:52
/usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/coprocess.sh[116]: /dev/null: cannot create [Bad file descriptor]
coprocess.sh[118]: FAIL: /bin/cat coprocess hung after 'exec 5<&p 6>&p; exec 5<&- 6>&-'
test coprocess failed at 2022-02-08+23:41:56 with exit code 1 [ 33 tests 1 error ]
The coprocess didn't hang; the 2>/dev/null redirection in the
regression test somehow failed.
Running the regression test in a loop 10000 times is a fairly
reliable way of reproducing the failure on my system. This has
allowed me to find the commit that introduced it. It is: feeb62d1
How odd. That commit merely sets errno to EBADF if sh_close() is
called with a file descriptor < 0. I don't understand how this
causes a race condition.
But calling abort() in the location of the feeb62d1 patch did allow
me to trace all the sh_close() calls with a negative file
descriptor. Those shouldn't be happening in any case.
All those sh_close() calls were related to co-processes, which that
regression test also tests. A co-process pipe that is not in use is
set to -1 so we need to avoid calling sh_close() if that happened.
This commit does that (as well as making some consistency tweaks)
and it reliably makes the race condition go away on my system. The
"why" of this remains a mystery as the only difference is that
errno is not set to EBADF in these situations, and both
sh_redirect() and sh_open() explicitly set errno to 0.
Resolves: https://github.com/ksh93/ksh/issues/483
Notable changes:
src/cmd/ksh93/bltins/trap.c: b_trap():
- Disable the unadvertised 'trap + SIG' feature in POSIX mode; it's
not compatible as '+' is a legitimate command name.
src/cmd/ksh93/data/builtins.c:
- Give "pwd", "alarm" and "times" the BLT_ENV flag for better
performance. There is no need for those to be stoppable.
src/cmd/ksh93/sh/xec.c:
- sh_eval(): Remove a "temporary tksh hack" and associated
sh.fn_reset flag.
- sh_exec():
- Remove now-used 'unpipe' flag and one remaining dead check for
it (re: a2196f94, 4b22fd5d).
- Fix unnecessary and confusing reuse of the 'type' variable.
src/lib/libast/comp/conf.sh:
- trap: Use 'rm -rf' instead of 'rm -f' to remove temp executables;
on macOS, they may include *.dSYM directories.
Part of the *subshell_data/*sp struct:
92: int nofork;
In subshell.c, sh_subshell(), before entering a virtual subshell:
628: if(!(sp->nofork = sh_state(SH_NOFORK)))
629: sh_onstate(SH_NOFORK);
...and upon restoring when leaving the subshell:
696: if(!sp->nofork)
697: sh_offstate(SH_NOFORK);
This code is clearly nonsense, because 'sh_state(SH_NOFORK)' is
a static expression that always has the value 1. This can be seen
by looking at the definitions in defs.h:
194: #define sh_state(x) ( 1<<(x))
...and in shell.h:
68: #define SH_NOFORK 0 /* set when fork not necessary */
So, sh_state(SH_NOFORK) == 1<<(0) == 1, meaning sp->nofork is
always 0, meaning the statements conditional on it are never
executed. We can get rid of all of this; it's dead code.
In subshell.c on line 628, perhaps sh_isstate(SH_NOFORK) was meant
instead of sh_state(SH_NOFORK) -- but that state flag is supposed
to signal that forking is not necessary when running an external
command, that we can potentially 'exec' it directly if it's the
last command in the subshell -- which makes no sense at all when
being in a virtual subshell which does not have its own process.
This dead code was added in version 2008-09-21 ksh93t.
The subsequent 'flags |= sh_state(SH_NOFORK)' (subshell.c line 630)
probably makes more sense, since this sets execflg in sh_exec(),
which is also used for optimising redirections (avoids bothering to
store previous state if it's the last command in the subshell).
This commit supersedes @lijog's Solaris patch 280-23332860 (see
17ebfbf6) as this is a more general fix that makes the patch
redundant. Of course its associated regression tests stay.
Reproducer script:
trap 'echo SIGUSR1 received' USR1
sh -c 'kill -s USR1 $PPID'
Run as a normal script.
Expected behaviour: prints "SIGUSR1 received"
Actual behaviour: the shell invoking the script terminates. Oops.
As of 6701bb30, ksh again allows an exec-without-fork optimisation
for the last command in a script. So the 'sh' command gets the same
PID as the script, therefore its parent PID ($PPID) is the invoking
script and not the script itself, which has been overwritten in
working memory. This shows that, if there are traps set, the exec
optimisation is incorrect as the expected process is not signalled.
While 6701bb30 reintroduced this problem for scripts, this has
always been an issue for certain other situations: forked command
substitutions, background subshells, and -c option argument
scripts. This commit fixes it in all those cases.
In sh_exec(), case TFORK, the optimisation (flagged in no_fork) was
only blocked for SIGINT and for the EXIT and ERR pseudosignals.
That is wrong. It should be blocked for all signal and pseudosignal
traps, except DEBUG (which is run before the command) and SIGKILL
and SIGSTOP (which cannot be trapped).
(I've also tested the behaviour of other shells. One shell, mksh,
never does an exec optimisation, even if no traps are set. I don't
know if that is intentional or not. I suppose it is possible that a
script might expect to receive a signal without trapping it first,
and they could conceivably be affected the same way by this exec
optimisation. But the ash variants (e.g. Busybox ash, dash, FreeBSD
sh), as well as bash, yash and zsh, all do act like this, so the
behaviour is very widespread. This commit makes ksh act like them.)
Multiple files:
- Remove the sh.errtrap, sh.exittrap and sh.end_fn flags and their
associated code from the superseded Solaris patch.
src/cmd/ksh93/include/shell.h:
- Add a scoped sh.st.trapdontexec flag for sh_exec() to disable
exec-without-fork optimisations. It should be in the sh.st scope
struct because it needs to be reset in subshell scopes.
src/cmd/ksh93/bltins/trap.c: b_trap():
- Set sh.st.trapdontexec if any trap is set and non-empty (an empty
trap means ignore the signal, which is inherited by an exec'd
process, so the optimisation is fine in that case).
- Only clear sh.st.trapdontexec if we're not in a ksh function
scope; unlike subshells, ksh functions fall back to parent traps
if they don't trap a signal themselves, so a ksh function's
parent traps also need to disable the exec optimisation.
src/cmd/ksh93/sh/fault.c: sh_sigreset():
- Introduce a new -1 mode for sh_funscope() to use, which acts like
mode 0 except it does not clear sh.st.trapdontexec. This avoids
clearing sh.st.trapdontexec for ksh functions scopes (see above).
- Otherwise, clear sh.st.trapdontexec whenever traps are reset.
src/cmd/ksh93/sh/xec.c: check_exec_optimization():
- Consolidate all the exec optimisation logic into this function,
including the logic from the no_fork flag in sh_exec()/TFORK.
- In the former no_fork flag logic, replace the three checks for
SIGINT, ERR and EXIT traps with a single check for the
sh.st.trapdontexec flag.
The exec optimization only happened in background subshells and not
in regular subshells when they had forked via sh_subfork(), which
makes little sense.
src/cmd/ksh93/sh/xec.c: sh_exec: case TLST:
- A subshell is executed as a list of commands which is TLST. If
the shell had not forked at the beginning of the subshell, the
sh_state(SH_FORKED) flag was not passed on to recursive sh_exec()
invocations, and a sh_subfork() event did not change this. To fix
this, re-check for the SH_FORKED state and pass that bit on to
the recursive sh_exec() invocation if set (note that sh_isstate()
returns a bitmask and not a boolean value).
src/cmd/ksh93/sh/subshell.c: sh_subfork():
- Remove redundant sh_onstate(SH_FORKED); this is already done in
sh_fork() which this function calls.
Reproducer script:
tempfile=/tmp/out2.$$.$RANDOM
bintrue=$(whence -p true)
for opt in monitor pipefail
do
(
set +x -o "$opt"
(
sleep .05
echo "ok $opt" >&2
) 2>$tempfile | "$bintrue"
) &
wait
cat "$tempfile"
rm -f "$tempfile"
done
Expected output:
ok monitor
ok pipefail
Actual output:
(none)
The 'monitor' and 'pipefail' options are supposed to make the shell
wait for the all commands in the pipeline to terminate and not only
the last component, regardless of whether the pipe between the
component commands is still open. In the failing reproducer, the
dummy external true command is subject to an exec optimization, so
it replaces the subshell instead of forking a new process. This is
incorrect, as the shell is no longer around to wait for the
left-hand part of the pipeline, so it continues in the background
without being waited for. Since it writes to standard error after
.05 seconds (after the pipe is closed), the 'cat' command reliably
finds the temp file empty. Without the sleep this would be a race
condition with unpredictable results.
Interestingly, this bug is only triggered for a (background
subshell)& and not for a forked (regular subshell). Which means the
exec optimization is not done for a forked regular subshell, though
there is no reason not to. That will be fixed in the next commit.
src/cmd/ksh93/sh/xec.c: sh_exec():
- case TFORK: Never allow an exec optimization if we're running a
command in a multi-command pipeline (pipejob is set) and the
shell needs to wait for all pipeline commands, i.e.: either the
time keyword is in use, the SH_MONITOR state is active, or the
SH_PIPEFAIL option is on.
- case TFIL: Fix the logic for setting job.waitall for the
non-SH_PIPEFAIL case. Do not 'or' in the boolean value but assign
it, and include the SH_TIMING (time keyword in use) state too.
- case TTIME: After that fix in case TFIL, we don't need to bother
setting job.waitall explicitly here.
src/cmd/ksh93/sh.1:
- Add missing documentation for the conditions where the shell
waits for all pipeline components (time, -o monitor/pipefail).
Resolves: https://github.com/ksh93/ksh/issues/449
$ arch/*/bin/ksh -xc 'typeset -a a=(1 2 3); \
(typeset -A a; typeset -p a); typeset -p a'
typeset -A a=()
typeset -a a=(1 2 3)
The associative array in the subshell is empty, so the conversion
failed. So far, I have been unsuccessful at fixing this in the
array and/or virtual subshell code (a patch that fixes it there
would still be more than welcome).
As usual, real subshells work correctly, so this commit adds
another forking workaround. The use case is rare and specific
enough that I have no performance concerns.
src/cmd/ksh93/bltins/typeset.c: setall():
- Fork a virtual subshell if we're actually converting a variable
to an associative array, i.e.: the NV_ARRAY (-A, associative
array) attribute was passed, there are no assignments (sh.envlist
is NULL), and the variable is not unset.
src/cmd/ksh93/tests/arith.sh:
- Fix the "Array subscript quoting test" tests that should not have
been passing and that correctly failed after this fix; they used
'typeset -A' without an assignment in a subshell, assuming it was
unset in the parent shell, which it wasn't.
Resolves: https://github.com/ksh93/ksh/issues/409
When the funstaks() function deletes a stack, other code could
still reference that stack's pointer, at least if a script's DEBUG
trap changed the function context by assigning to ${.sh.level}.
This crashed @ormaaj's funcname.ksh script in certain contexts, at
least when run as a dot script or in a virtual subshell.
This allows that script to run in all contexts by making
funstaks(s) set the stack pointer to NULL after deleting the stack
and making various other points in the code check for a null
pointer before dereferencing it.
This may not be the most elegant fix but (in my testing) it does
work, even when compiling ksh with AddressSanitiser.
Thanks to @JohnoKing for help researching this problem.
Resolves: https://github.com/ksh93/ksh/issues/212
$ ksh -c '(sleep 1 & echo ${.sh.stats.forks}); :'
2
The shell forks twice. A virtual subshell that launches a
background job forks first before forking the background job.
As discussed in the P.S. in e3d7bf1d, this is caused by the
following code in sh_exec() in xec.c (as since changed in 4ce486a7,
e40aaa8a, 88a1f3d6, and a2196f94):
1505: if((type&(FAMP|TFORK))==(FAMP|TFORK))
1506: {
1507: if(sh.comsub && !(sh.fdstatus[1]&IONOSEEK))
1508: unpipe = iousepipe();
1509: if(!sh.subshare)
1510: sh_subfork();
1511: }
This smells like an ugly workaround. It forks a virtual subshell
whenever a background job is launched within it, whether there is
an actual reason to do this or not. Researching the ksh93-history
repo reveals that this workaround had already gone through many
different versions before we restarted ksh development.
In the e3d7bf1d P.S. discussion, four regression test failures were
caused by removing this workaround. I recently tried it again and
two of the failures were already gone. (Testing which commits fix
it would require recompiling every 93u+m commit with the workaround
patched out and I can't be bothered.) Two were left: the two
signal.sh failures. The last commit (50db00e1) fixed those as well.
So I believe we no longer need this workaround, which never did
make very much sense. Removing it significantly improves the
performance of ksh when launching background jobs from subshells.
Ksh handles local traps in virtual subshells the same way as local
traps in ksh-style shell functions, which can cause incorrect
operation.
Reproducer script:
trap 'echo "parent shell trap"; exit 0' USR1
(trap 'echo "subshell trap"' USR1; kill -USR1 $$)
echo wrong
Output on every ksh93 version: 'wrong'
Output on every other shell: 'parent shell trap'
The ksh93 output is wrong because $$ is the PID of the main shell,
therefore 'kill -USR1 $$' from a subshell needs to issue SIGUSR1 to
the main shell and trigger the 'echo SIGUSR1' trap.
This is an inevitable consequence of processing signals in a
virtual subshell. Signals are a process-wide property, but a
virtual subshell and the parent shell share the same process.
Therefore it is not possible to distinguish between the parent
shell and subshell trap.
This means virtual subshells are fundamentally incompatible with
receiving signals. No workaround can make this work properly.
Ksh could either assume the signal needs to be caught by the
subshell trap (wrong in this case, but right in others) or by the
parent shell trap. But it does neither and just gives up and does
nothing, which I suppose is the least bad way of doing it wrong.
As another example, consider a subshell that traps a signal, then
passes its own process ID (as of 9de65210, that's ${.sh.pid}) to
another process to say "here is where to signal me". A virtual
subshell will send it the PID that it shares with the the parent
shell. Even if a virtual subshell receives the signal correctly, it
may fork mid-execution afterwards, depending on the commands that
it runs (and this varies by implementation as we fix or work around
bugs). So its PID could be invalidated at any time.
Forking a virtual subshell at the time of trapping a signal is the
only way to ensure a persistent PID and correct operation.
src/cmd/ksh93/bltins/trap.c: b_trap():
- Fork when trapping (or ignoring) a signal in a virtual subshell.
(There's no need to fork when trapping a pseudosignal.)
src/cmd/ksh93/tests/signal.sh:
- Add tests. These are simplified versions of tests already there,
which issued 'kill' as a background job. Currently, running a
background job causes a virtual subshell to fork before forking
the 'kill' background job (or *any* background job, see e3d7bf1d)
-- an ugly partial workaround that I believe just became
redundant and which I will remove in the next commit.
