This small commit replaces one instance of memcmp with strncmp to
fix one of the buffer overflows that causes 'typeset -p .sh.type'
to crash (see also https://github.com/ksh93/ksh/issues/456).
This commit backports all of the relevant .sh.match bugfixes from
ksh93v-. Most of the .sh.match rewrite is from versions 2012-08-24
and 2012-10-04, with patches from later releases of 93v- and
ksh2020 also applied. Note that there are still some remaining bugs
in .sh.match, although now the total count of .sh.match bugs should
be less that before.
These are the relevant changes in the ksh93v- changelog that were
backported:
12-08-07 .sh.match no longer gets set for patterns in PS4 during
set -x.
12-08-10 Rewrote .sh.match expansions fixing several bugs and
improving performance.
12-08-22 .sh.match now handles subpatterns that had no matches with
${var//pattern} correctly.
12-08-21 A bug in setting .sh.match after ${var//pattern/string}
when string is empty has been fixed.
12-08-21 A bug in setting .sh.match after [[ string == pattern ]]
has been fixed.
12-08-31 A bug that could cause a core dump after
typeset -m var=.sh.match has been fixed.
12-09-10 Fixed a bug in typeset -m the .sh.match is being renamed.
12-09-07 Fixed a bug in .sh.match code that coud cause the shell
to quitely
13-02-21 The 12-01-16 bug fix prevented .sh.match from being used
in the replacement string. The previous code was restored
and a different fix which prevented .sh.match from being
computed for nested replacement has been used instead.
13-05-28 Fixed two bug for typeset -c and typeset -m for variable
.sh.match.
Changes:
- The SHOPT_2DMATCH option has been removed. This was already the
default behavior previously, and now it's documented in the man
page.
- init.c: Backported the sh_setmatch() rewrite from 93v- 2012-08-24
and 2012-10-04.
- Backported the libast 93v- strngrpmatch() function, as the
.sh.match rewrite requires this API.
- Backported the sh_match regression tests from ksh93v-, with many
other sh_match tests backported from ksh2020. Much of the sh_match
script is based on code from Roland Mainz:
https://marc.info/?l=ast-developers&m=134606574109162&w=2https://marc.info/?l=ast-developers&m=134490505607093
- tests/{substring,treemove}.sh: Backported other relevant .sh.match
fixes, with tests added to the substring and treemove test scripts.
- tests/types.sh: One of the (now reverted) memory leak bugfixes
introduced a CI test failure in this script, so for that test the
error message has been improved.
- string/strmatch.c: The original ksh93v- code for the strngrpmatch()
changes introduced a crash that could occur because strlen would
be used on a null pointer. This has been fixed by avoiding strlen
if the string is null.
One nice side effect of these changes is a considerable performance
improvement in the shbench[1] gsub benchmark (results from 20
iterations with CCFLAGS=-Os):
--------------------------------------------------
name /tmp/ksh-current /tmp/ksh-matchfixes
--------------------------------------------------
gsub.ksh 0.883 [0.822-0.959] 0.457 [0.442-0.505]
--------------------------------------------------
Despite all of the many fixes and improvements in the backported
93v- .sh.match code, there are a few remaining bugs:
- .sh.match is printed with a default [0] subscript (see also
https://github.com/ksh93/ksh/issues/308#issuecomment-1025016088):
$ arch/*/bin/ksh -c 'echo ${!.sh.match}'
.sh.match[0]
This bug appears to have been introduced by the changes from
ksh93v- 2012-08-24.
- The wrong variable name is given for 'parameter not set' errors
(from https://marc.info/?l=ast-developers&m=134489094602596):
$ arch/*/bin/ksh -u
$ x=1234
$ true "${x//~(X)([012])|([345])/}"
$ compound co
$ typeset -m co.array=.sh.match
$ printf "%q\n" "${co.array[2][0]}"
arch/linux.i386-64/bin/ksh: co.array[2][(null)]: parameter not set
- .sh.match leaks out of subshells. Further information and a
reproducer can be found here:
https://marc.info/?l=ast-developers&m=136292897330187
[1]: https://github.com/ksh-community/shbench
When executing a script without a hashbang path like #!/bin/ksh,
ksh forks itself, longjmps back to sh_main(), and then (among other
things) calling sh_reinit() which is the function that tries to
reinitialise as much of the shell as it can. This is its way of
ensuring the child script is run in ksh and not some other shell.
However, this appraoch is incredibly buggy. Among other things,
changes in built-in commands and custom type definitions survived
the reinitialisation, "exporting" variables didn't work properly,
and the hash table and ${.sh.stats} weren't reset. As a result,
depending on what the invoking script did, the invoked script could
easily fail or malfunction.
It is not actually possible to reinitialise the shell correctly,
because some of the shell state is in locally scoped static
variables that cannot simply be reinitialised. There are probably
huge memory leaks with this approach as well. At some point, all
this is going to need a total redesign. Clearly, the only reliable
way involves execve(2) and a start from scratch.
For now though, this seems to fix the known bugs at least. I'm sure
there are more to be discovered.
This commit makes another change: instead of the -h/trackall option
(which has been a no-op for decades), the posix option is now
inherited by the child script. Since there is no hashbang path from
which to decide whether the shell should run in POSIX mode nor not,
the best guess is probably the invoking script's setting.
src/cmd/ksh93/sh/init.c: sh_reinit():
- Keep the SH_INIT state on during the entire procedure.
- Delete remaining non-exported, non-default variables.
- Remove attributes from exported variables. In POSIX mode, remove
all attributes; otherwise, only remove readonly.
- Unset discipline function pointers for variables.
- Delete all custom types.
- Delete all functions and built-ins, then reinitialise the built-ins
table from scatch.
- Free the alias values before clearing the alias table.
- Same with hash table entries (tracked aliases).
- Reset statistics.
- Inherit SH_POSIX instead of SH_TRACKALL.
- Call user init function last, not somewhere in the middle.
src/cmd/ksh93/sh/name.c: sh_envnolocal():
- Be sure to preserve the export attribute of kept variables.
Resolves: https://github.com/ksh93/ksh/issues/350
This bugfix was backported from ksh93v- 2012-10-04. The bug fixed
by this change is one that causes 'typeset -p' to omit the -C flag
when listing compound arrays belonging to a type:
$ typeset -T Foo_t=(compound -a bar)
$ Foo_t baz
$ typeset -p baz.bar
typeset -a baz.bar='' # This should be 'typeset -C -a'
src/cmd/ksh93/sh/nvtype.c:
- Backport change from 93v- 2012-10-04 that sets the array nvalue to
a pointer named Null (which is "") in nv_mktype(), then to Empty
in fixnode().
- Change the Null name from the 93v- code to AltEmpty to avoid
misleading code readers into thinking that it's a null pointer.
src/cmd/ksh93/tests/types.sh:
- Backport the relevant 93v- changes to the types regression tests.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
In ksh93v- 2012-10-04 the following bugfix is noted in the changelog
(this fix was most likely part of ksh93v- 2012-09-27, although that
version is not archived anywhere):
12-09-21 A bug in which the output of a two dimensional sparse
indexed array would cause the second subscript be treated
as an associative array when read back in has been fixed.
Elements that are sparse indexed arrays now are prefixed
type "typeset -a".
Below is a before and after of this change:
# Before
$ typeset -a foo[1][2]=bar
$ typeset -p foo
typeset -a foo=([1]=([2]=bar) )
# After
$ typeset -a foo[1][2]=bar
$ typeset -p foo
typeset -a foo=(typeset -a [1]=([2]=bar) )
src/cmd/ksh93/sh/*.c:
- Backport changes from ksh93v- to print 'typeset -a' before sparse
indexed arrays and properly handle 'typeset -a' in reinput
commands from 'typeset -p'.
src/cmd/ksh93/tests:
- Add two regression tests to arrays.sh for this change.
- Update the existing regression tests for compatibility with the
new printed typeset output.
Special builtins are undeleteable for a reason. But 'enum' and
'typeset -T' allow overriding them, causing an inconsistent state.
@JohnoKing writes:
| The behavior is rather buggy, as it appears to successfully
| override normal builtins but fails to delete the special
| builtins, leading to scenarios where both the original builtin
| and type are run:
|
| $ typeset -T eval=(typeset BAD; typeset TYPE) # This should have failed
| $ eval foo=BAD
| /usr/bin/ksh: eval: line 1: foo: not found
| $ enum trap=(BAD TYPE) # This also should have failed
| $ trap foo=BAD
| /usr/bin/ksh: trap: condition(s) required
| $ enum umask=(BAD TYPE)
| $ umask foo=BAD
| $ echo $foo
| BAD
|
| # Examples of general bugginess
| $ trap bar=TYPE
| /usr/bin/ksh: trap: condition(s) required
| $ echo $bar
| TYPE
| $ eval var=TYPE
| /usr/bin/ksh: eval: line 1: var: not found
| $ echo $var
| TYPE
This commit fixes the following:
The 'enum' and 'typeset -T' commands are no longer allowed to
override and replace special built-in commands, except for type
definition commands previously created by these commands; these
are already (dis)allowed elsewhere.
A command like 'typeset -T foo_t' without any assignments no longer
creates an incompletely defined 'foo_t' built-in comamnd. Instead,
it is now silently ignored for backwards compatibility. This did
have a regression test checking for it, but I'm changing it because
that's just not a valid use case. An incomplete type definition
command does nothing useful and only crashes the shell when run.
src/cmd/ksh93/bltins/enum.c: b_enum():
- Do not allow overriding non-type special built-ins.
src/cmd/ksh93/sh/name.c: nv_setlist():
- Do not allow 'typeset -T' to override non-type special built-ins.
To avoid an inconsistent state, this must be checked for while
processing the assignments list before typeset is really invoked.
src/cmd/ksh93/bltins_typeset.c: b_typeset():
- Only create a type command if sh.envlist is set, i.e., if some
shell assignment(s) were passed to the 'typeset -T' command.
Progresses: https://github.com/ksh93/ksh/issues/350
Building ksh with the tcc (tinycc) compiler failed as of glibc
commit 5d98a7da. The NEWS file in that commit adds:
+* When _DYNAMIC_STACK_SIZE_SOURCE or _GNU_SOURCE are defined,
+ PTHREAD_STACK_MIN is no longer constant and is redefined to
+ sysconf(_SC_THREAD_STACK_MIN).
The tcc build failure seeminly had nothing to do with that --
however, deleting the PTHREAD_STACK_MIN entry and its dependent
THREAD_STACK_MIN entry from conf.tab fixes the build failure.
For reasons unknown, gcc didn't have a problem with it. However,
these config identifiers aren't used anywhere in the ast codebase
(including the full ast-open-history repo) so it should be fine to
just get rid of them; ksh is not and will not be threaded.
NOTE: To build ksh with tcc, you need to build the latest tcc code
from <https://repo.or.cz/tinycc>. The tcc release packages in OS
distributions are typically too old and will not work.
Resolves: https://github.com/ksh93/ksh/issues/437
Thanks to @JohnoKing for the report.
$ unset foo
$ echo ${foo[42]=bar}
(empty line)
Instead of the empty line, 'bar' was expected. As foo[42] was
unset, the conditional assignment should have worked.
$ unset foo
$ : ${foo[42]?error: unset}
(no output)
The expansion should have thrown an error with the given message.
This bug was introduced in ksh 93t 2008-10-01. Thanks to @JohnoKing
for finding the breaking change.
Analysis: The problem was experimenally determined to be in in the
following lines of nv_putsub(). If the array member is unset (i.e.
null), the value is set to the empty string instead:
src/cmd/ksh93/sh/array.c
1250: else
1251: ap->val[size].cp = Empty;
It makes some sense: if there is a value (even an empty one), the
variable is set and these expansions should behave accordingly.
Sure enough, deleting these lines fixes the bug, but at the expense
of introducing a lot of other array-related regressions. So we need
a way to special-case the affected expansions.
Where to do this? If we replace line 1251 with an abort(3) call, we
get this stack trace:
0 libsystem_kernel.dylib __pthread_kill + 10
1 libsystem_pthread.dylib pthread_kill + 284
2 libsystem_c.dylib abort + 127
3 ksh nv_putsub + 1411 (array.c:1255)
4 ksh nv_endsubscript + 940 (array.c:1547)
5 ksh nv_create + 4732 (name.c:1066)
6 ksh nv_open + 1951 (name.c:1425)
7 ksh varsub + 4934 (macro.c:1322)
[rest omitted]
The special-casing needs to be done on line 1250 of array.c, but
flagged in varsub() which processes these expansions. So, varsub()
calls nv_open() calls nv_create() calls nv_endsubscript() calls
nv_putsub(). That's a fairly deep call stack, so passing an extra
flag argument does not seem doable. I did try an approach using a
couple of new bit flags passed via these functions' flags and mode
parameters, but the way this code base uses bit flags is so
intricate, it does not seem to be possible to add or change
anything without unwanted side effects in all sorts of places.
So the only fix I can think of adds yet another global flag
variable for a very special case. It's ugly, but it works.
An elegant fix would probably involve a fairly comprehensive
redesign, which is simply not going to happen.
src/cmd/ksh93/include/shell.h:
- Add global sh.cond_expan flag.
src/cmd/ksh93/sh/array.c: nv_putsub():
- Do not set value to empty string if sh.cond_expan is set.
src/cmd/ksh93/sh/macro.c: varsub():
- Set sh.cond_expan flag while calling nv_open() for one of the
affected expansions.
- Minor refactoring for legibility and to make the fix fit better.
- SSOT: Instead of repeating string "REPLY", use the node's nvname.
- Do not pointlessly add an extra 0 byte when saving id for error
message; sfstruse() already adds this.
Thanks to @oguz-ismail for the bug report.
Resolves: https://github.com/ksh93/ksh/issues/383
Variables with a dot in their name, such as those declared in
namespace { ... } blocks, are usually stored in a separate tree
with their actual names not containing any dots. But under some
circumstances, including at least direct assignment of a
non-preexisting dot variable, dot variables are stored in the main
sh.var_tree with names actually containing dots. With allexport
active, those could end up exported to the environment. This bug
was also present in previous release versions of ksh.
src/cmd/ksh93/sh/name.c: pushnam():
- Check for a dot in the name before pushing a variable to export.
This fix was backported from ksh 93v- 2012-10-04.
src/cmd/ksh93/sh/nvtree.c: nv_outnode():
- If the array is supposed to be empty, do not continue. This
avoids outputting a nonexistent [0]= element for empty arrays.
Resolves: https://github.com/ksh93/ksh/issues/420
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This commit adds a fix for the trap command, backported from a fork
of ksh2020: https://github.com/l0stman/ksh/commit/2033375f
src/cmd/ksh93/sh/jobs.c: job_chldtrap():
- Fixed a use after free bug in the for loop. The string pointed to
by sh.st.trapcom[SIGCHLD] may be freed from memory after
sh_trap(), so it must be reobtained each time sh_trap() is called
from within the for loop.
All variables that are assigned a value should be exported while
the allexport shell option is enabled. This works in most cases,
but variables assigned to with ${var:=foo} or $((var=123)) aren't
exported while allexport is on.
src/cmd/ksh93/sh/name.c:
- nv_putval(): This is the central assignment function; all forms
of variable assignment end up here. So this is the best place
to check for SH_ALLEXPORT and turn on the export attribute.
- nv_setlist(): Remove allexport handling, now redundant.
src/cmd/ksh93/bltins/read.c: sh_readline():
- Remove allexport handling, now redundant.
src/cmd/ksh93/sh/main.c: sh_main():
- nv_putval() is used to initialize PS4 and IFS using nv_putval();
this is after an -a/--allexport specified on the ksh command
line has been processed, so temporarily turn that off.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
This commit implements support for the glibc 2.35
posix_spawn_file_actions_addtcsetpgrp_np(3) extension[2][3],
updating spawnveg(3) to use the new function for setting the
terminal group. This was done with the intention of improving
performance in interactive shells without reintroducing previous
race conditions[4][5].
[1]: https://sourceware.org/pipermail/libc-alpha/2022-February/136040.html
[2]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934
[3]: https://sourceware.org/git/?p=glibc.git;a=commit;h=6289d28d
[4]: https://github.com/ksh93/ksh/issues/79
[5]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html
src/cmd/ksh93/sh/path.c:
- Tell spawnveg(3) to set the terminal process group when launching
a child process in an interactive shell.
src/cmd/ksh93/sh/xec.c:
- If posix_spawn_file_actions_addtcsetpgrp_np(3) is available,
allow use of spawnveg(3) (via sh_ntfork()) even with job control
active.
- sh_ntfork(): Reimplement most of the SIGTSTP handling code
removed in commit 66c37202.
src/lib/libast/comp/spawnveg.c,
src/lib/libast/misc/procopen.c,
src/lib/libast/features/sys:
- Add support for posix_spawn_file_actions_addtcsetpgrp_np(3).
- Allow spawnveg to set the terminal process group when pgid == 0.
This was necessary to avoid race conditions when using the new
function.
src/lib/libast/features/lib:
- Detect posix_spawn_file_actions_addtcsetpgrp_np(3).
- Do not detect an OS spawnveg(3). With the API changes to spawnveg
in this pull request ksh probably can't use the OS's spawnveg
function anymore. (That's assuming anything else even provides a
spawnveg function to begin with, which is unlikely.)
src/lib/libast/features/api,
src/cmd/ksh93/include/defs.h:
- Bump libast version (20220101 => 20220201) due to the spawnveg(3)
API change.
src/lib/libast/man/spawnveg.3:
- Document the changes to spawnveg(3) in the corresponding man
page. Currently, it will only use the new tcfd argument if
posix_spawn_file_actions_addtcsetpgrp_np(3) is supported. This
could also be implemented for the fork(2) fallback, but for now
I've avoided changing that since actually using it in the fork
code would likely require a lot of hackery to avoid attempting
tcsetpgrp with vfork (the behavior of tcsetpgrp after vfork is
not portable) and would only benefit systems that don't have
posix_spawn and vfork (I can't recall any off the top of my head
that would fall under that category).
- Updated the man page to account for spawnveg's change in
behavior.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
They were still pointing to the master branch. They should point to
this 1.0 branch (also, master was renamed to dev on 2nd Jan, so the
links were not only wrong but broken).
If the VISUAL or EDITOR environment variable is not set to a value
matching *[Vv][Ii]* or *macs* at initialisation time, then ksh does
not turn on any line editor.
This is user-hostile. New users on Unix-like systems typically have
a simple editor like nano preconfigured as their default, or may
not have the VISUAL or EDITOR variable set at all. So if they try
ksh, they find themselves without basic functionality such as arrow
keys and probably go straight back to bash.
The emacs line editor is by far the most widely used, especially
among new users, so ksh should default to that. Most other shells
already do this.
src/cmd/ksh93/sh/main.c: sh_main():
- On an interactive shell, if on editor was turned on based on
$VISUAL or $EDITOR, turn on emacs before reading input.
Reproducer:
$ ksh -c 'bash -c '\''kill -s INT $$'\''; echo "$?, continuing"'
Expected result: output "258, continuing"; exit status 0.
Actual result: no output; exit status 258. The child process sent
SIGINT only to itself and not to the process group, so the parent
script was wrongly interrupted.
Every shell except ksh93 produces the expected result. ksh93 also
gave the expected result before version 2008-01-31 93s+, which
introduced the code below.
Analysis: The problem is in these lines of code in xec.c,
sh_exec(), TFORK case, parent branch of fork:
1649: if(!sh_isstate(SH_MONITOR))
1650: {
1651: if(!(sh.sigflag[SIGINT]&(SH_SIGFAULT|SH_SIGOFF)))
1652: sh_sigtrap(SIGINT);
1653: sh.trapnote |= SH_SIGIGNORE;
1654: }
[...pipe and I/O handling, wait for command to finish...]
1667: if(!sh_isstate(SH_MONITOR))
1668: {
1669: sh.trapnote &= ~SH_SIGIGNORE;
1700: if(sh.exitval == (SH_EXITSIG|SIGINT))
1701: kill(sh.current_pid,SIGINT);
1702: }
When a user presses Ctrl+C, SIGINT is sent to the entire process
group. If job control is fully off (i.e., !sh_isstate(SH_MONITOR)),
then the process group includes the parent script. Therefore, in a
script such as
$ ksh -c 'bash -c '\''read x'\''; echo "$?, continuing"'
when the user presses Ctrl+C while bash waits for 'read x' input,
the parent ksh script should be interrupted as well.
Now, the code above ignores SIGINT while bash is running. (This is
done using special-casing in sh_fault() to handle that SH_SIGIGNORE
flag for SIGINT.) So, when Ctrl+C interrupts the process group, the
parent script is not getting interrupted as it should.
To compensate for that, the code then detects, using sh.exitval
(the child process' exit status), whether the child process was
killed by SIGINT. If so, it simply assumes that the signal was
meant for the process group including the parent script, so it
reissues SIGINT to itself after unignoring it.
But, as we can see from the broken reproducer above, that
assumption is not valid. Scripts are perfectly free to send SIGINT
to themselves only, and that must work as expected.
src/cmd/ksh93/sh/xec.c: sh_exec(): TFORK: parent branch:
- Instead of ignoring SIGINT, sigblock() it, which delays handling
the signal until sigrelease(). (Note that these are macros
defined in src/cmd/ksh93/features/sigfeatures according to OS
capabilities.)
- This makes reissuing SIGINT redundant, so delete that, which
fixes the bug.
src/cmd/ksh93/sh/fault.c:
- Nothing now sets the SH_SIGIGNORE flag in sh.trapnote, so remove
special-casing added in 2008-01-31 93s+.
Add extra key bindings to the emacs and vi modes
This patch adds the following key bindings to the emacs and vi
editing modes:
- Support for Home key sequences ^[[1~ and ^[[7~ as well as End key
sequences ^[[4~ and ^[[8~.
- Support for arrow key sequences ^[OA, ^[OB, ^[OC and ^[OD.
- Support for the following keyboard shortcuts (if the platform
supports the expected escape sequence):
- Ctrl-Left Arrow: Go back one word
- Alt-Left Arrow: Go back one word (Not supported on Haiku)
- Ctrl-Right Arrow: Go forward one word
- Alt-Right Arrow: Go forward one word (Not supported on Haiku)
- Ctrl-G: Cancel reverse search
- Ctrl-Delete: Delete next word (Not supported on Haiku)
- Added a key binding for the Insert key, which differs in the
emacs and vi editing modes:
- In emacs mode, Insert escapes the next character.
- In vi mode, Insert will switch the editor to insert mode (like
in vim).
src/cmd/ksh93/edit/{emacs,vi}.c:
- Add support for the <M-Left> and <M-Right> sequences. Like in
bash and mksh, these shortcuts move the cursor one word backward
or forward (like the <Ctrl-Left> and <Ctrl-Right> shortcuts).
- Only attempt to process these shortcuts if the escape sequence
begins with $'\E[1;'.
src/cmd/ksh93/edit/vi.c:
- If the shell isn't doing a reverse search, insert the bell
character when Ctrl+G is input.
- Add the Ctrl-Delete shortcut as an alias of 'dw'. Calling
ed_ungetchar twice does not work for 'dw', so Ctrl-Delete was
implemented by using a vp->del_word variable.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
Currently, running ksh under ASan without the ASAN_OPTIONS variable
set to 'detect_leaks=0' usually ends with ASan complaining about a
memory leak in defpathinit() (this leak doesn't grow in a loop, so
no regression test was added to leaks.sh). Reproducer:
$ ENV=/dev/null arch/*/bin/ksh
$ cp -?
cp: invalid option -- '?'
Try 'cp --help' for more information.
$ exit
=================================================================
==225132==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 85 byte(s) in 1 object(s) allocated from:
#0 0x7f6dab42d459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
#1 0x5647b77fe144 in sh_calloc /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/init.c:265
#2 0x5647b788fea9 in path_addcomp /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1567
#3 0x5647b78911ed in path_addpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:1705
#4 0x5647b7888e82 in defpathinit /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:442
#5 0x5647b78869f3 in ondefpath /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/path.c:67
--- cut ---
SUMMARY: AddressSanitizer: 174 byte(s) leaked in 2 allocation(s).
Analysis: The previous code was leaking memory because
defpathinit() returns a pointer from path_addpath(), which has
memory allocated to it in path_addcomp(). This is the code ASan
traced as having allocated memory:
442: return(path_addpath((Pathcomp_t*)0,(defpath),PATH_PATH));
In path_addpath():
1705: first = path_addcomp(first,old,cp,type);
[...]
1729: return(first);
In path_addcomp():
1567: pp = sh_newof((Pathcomp_t*)0,Pathcomp_t,1,len+1);
The ondefpath() function doesn't save a reference to the pointer
returned by defpathinit(), which causes the memory leak:
66: if(!defpath)
67: defpathinit();
The changes in this commit avoid this problem by setting the
defpath variable without also calling path_addpath().
src/cmd/ksh93/sh/path.c:
- Move the code for allocating defpath from defpathinit() into its
own dedicated function called std_path(). This function is called
by defpathinit() and ondefpath() to obtain the current string
stored in the defpath variable. This bugfix is adapted from a
fork of ksh2020: https://github.com/l0stman/ksh/commit/db5c83a6
The array_grow() function calculates the size by multiplying with
sizeof(union Value*), where sizeof(union Value) was clearly meant.
In practice, these are the same size on most (or maybe even all)
systems, as no current member of union Value is larger than a
pointer -- see name.h. But it's still wrong.
Opening the match stack with the STK_SMALL flag causes the stk code
to allocate memory in blocks of 64*sizeof(char*) instead of
1024*sizeof(char*). This caused a significant slowdown which was
exposed by the extglob.ksh module of shbench. Thanks to @JohnoKing
for noticing and reporting the problem.
src/lib/libast/regex/regcomp.c: regcomp():
- Remove STK_SMALL from the stkopen() option bit flags.
Resolves: https://github.com/ksh93/ksh/issues/440
Notable changes:
src/cmd/ksh93/*.c:
- Get rid of all the dtuserdata(FOO,&sh,1) calls backported in
cc492752. These set pointers to sh in Cdt objects. As of
b590a9f1, the code does not use any pointers to sh, so these are
superfluous.
src/cmd/ksh93/data/builtins.c,
src/cmd/ksh93/sh.1:
- As of ksh 93l 2001-06-01, the -h/trackall option has no effect at
all, so trim its documentation.
src/lib/libast/man/stk.3,
src/lib/libast/man/stak.3:
- Correct the documentation on what the ST(A)K_SMALL option bit
actually does based on a reading of the code.
- Document the STK_NULL option bit.
README.md,
src/cmd/ksh93/README:
- Add a note that -fdiagnostics-color=always will break the build.
Ref.: https://github.com/ksh93/ksh/issues/379
src/lib/libast/Mamfile:
- Remove a 'rm -f astmath' command -- a file that is never created.
But on Cygwin this removes astmath.exe, which *is* used. As a
result, executing it failed on Cygwin, so the system incorrectly
detected that Cygwin needs -lm for math functions.
This commit implements support for POSIX_SPAWN_SETSID in
spawnveg(3). The fork/vfork fallback for spawnveg already attempts
to use setsid in the manner described by the man page, so the
posix_spawn implementation should also do so.
src/lib/libast/comp/spawnveg.c:
- Add support for POSIX_SPAWN_SETSID to the posix_spawn version of
spawnveg.
- Minor extra: Remove dead code that's never used (the
_lib_posix_spawn < 2 code block is inside of _lib_posix_spawn >
1, plus when it's manually enabled by changing the previous #if
condition you'll find it causes many regression test failures (at
least on OpenBSD)).
src/lib/libast/man/spawnveg.3:
- Document that spawnveg cannot make the new process a session
leader if the operating system doesn't support POSIX_SPAWN_SETSID
and the new process was created using posix_spawn.
'eval' suffers from the same bug. Reproducer:
$ eval vi
then suspend vi, then try to resume it -- the same as in the
reproducer shown in the previous commit.
src/cmd/ksh93/bltins/misc.c: b_eval():
- Same fix. Do *not* turn off SH_MONITOR.
This fixes yet another whopper of a bug in job control. And it's
been in every version of ksh93 back to 1995, the beginning of
ast-open-archive. <sigh>
This is also bug number 23 that is fixed by simply removing code.
Reproducer:
1. Run vi, or another suspendable program, from a dot script or
POSIX function (ksh handles them the same way). So either:
$ echo vi >v
$ . ./v
or:
$ v() { vi; }
$ v
2. Suspend vi by typing Ctrl+Z.
3. Not one but two jobs are registered:
$ jobs -l
[2] + 85513 Stopped . ./v
[1] - 85512 Stopped . ./v
4. 'fg' does not work on either of them, just printing the job
command name but not resuming the editor. The second job
disappears from the table after 'fg'.
$ fg %2
. ./v
$ fg %2
ksh: fg: no such job
$ fg %1
. ./v
$ fg %1
. ./v
Either way, you're stuck with an unresumable vi that you have to
'kill -9' manually.
src/cmd/ksh93/sh/xec.c: sh_exec(): TFORK:
- Do *not* turn off the SH_MONITOR state flag (which tells ksh to
keep track of jobs) when running an external command from a
profile script or dot script/POSIX function. It's obvious that
this results in an inconsistent job control state as ksh will not
update it when the external command is suspended. The purpose of
this nonsense is surely lost to history as it's been there since
1995 or before. My testing says that removing it doesn't break
anything. If that turns out to be wrong, then the breakage will
have to be fixed in a correct way instead.
On my machine, the build system has been intermittently rebuilding
a sizeable part of libast for no apparent reason. I think I've
finally tracked down the cause: occasionally, the lctab.c file,
generated by port/lcgen.c, randomly changes, triggering said
recompile.
Diff of the latest instance on my system:
--- lctab.c.save 2022-01-28 03:22:47.000000000 +0000
+++ arch/darwin.i386-64/src/lib/libast/lctab.c 2022-01-28 03:30:01.000000000 +0000
@@ -2146,7 +2146,7 @@
#endif
0,0,0,
},
-{"no","norway",LC_primary,
+{"no","norway",0,
#ifdef CTRY_NORWAY
CTRY_NORWAY,
#else
In the port/lc.tab input file, "norway" does *not* have the
"primary" tag, unlike e.g. "sweden" or "united-kingdom". So that
LC_primary value did not belong in the generated code.
A look at the port/lcgen.c code shows that it's using uninitialised
memory. The newof() macro uses malloc, which does not initialise
the memory blocks it allocates:
131:#define newof(p,t,n,x) ((t*)malloc(sizeof(t)*(n)+(x)))
This is then used as follows:
403: case TERRITORY:
404: if (!(tp = newof(0, Territory_t, 1, s - b + 1)))
[...]
444: if (!strcmp(b, "primary"))
445: tp->primary = 1;
The flag is not set to zero if the string does not match, so it's
left uninitialised. Perhaps there are more such problems, but
instead of spending time trying to find them, I'll fix newof().
src/lib/libast/port/lcgen.c:
- In the newof() macro, call calloc(3) instead of malloc(3),
ensuring that all allocated memory is initialised to zero.
posix_spawn(2) was never used as a result of this error as the test
failed to compile, with most systems falling back to vfork(2).
src/lib/libast/features/lib: tst lib_posix_spawn:
- Fix parentheses goof.
Attempting to use array subscript expansion with variables that
aren't set currently causes a spurious syntax error (in ksh93u+ and
older commits the reproducer crashes):
$ ksh -c 'echo ${foo[${bar}..${baz}]}' # Shouldn't print anything
ksh: : arithmetic syntax error
src/cmd/ksh93/sh/macro.c:
- Backport a parser bugfix from ksh93v- 2012-08-24 that avoids
setting mp->dotdot until the copyto() function's loop is
finished.
src/cmd/ksh93/tests/arrays.sh:
- Add regression tests for this bug.
On Cygwin, ksh does not execute scripts without a #! path in a fork
of the ksh process as it does on other systems. Reproducer (run
from ksh):
$ cat test.sh
echo "${BASH_VERSION:-not bash}"
echo "${.sh.version}"
$ chmod +x test.sh
$ ./test.sh
4.4.12(3)-release
./test.sh: line 2: ${.sh.version}: bad substitution
The script was executed in bash instead of ksh.
After this fix, the output on Cygwin is like ksh on other systems:
not bash
Version AJM 93u+m/1.1.0-alpha+dev 2022-01-26
This also fixes a number of regression test failures, as quite a
few tests create and execute temp scripts without a hashbang path.
Analysis: On Cygwin, execve(2) happily executes shell scripts
without a #! path with /bin/sh (which is bash --posix). However,
ksh relies on execve(2) executing binaries or #! only, as it uses
an ENOEXEC failure to decide whether to fork and execute a #!-less
shell script with a reinitialized copy of itself via exscript().
src/cmd/ksh93/sh/path.c: path_spawn():
- Look at the magic first two bytes of the file; if it is "MZ"
(Mark Zbikowski, originator of the .exe format) or "#!", continue
as normal, otherwise simulate an ENOEXEC failure from execve(2)
which will cause ksh to fall back on #!-less script execution.
Notable changes:
src/cmd/ksh93/include/fault.h:
- Get rid of the superflous sh pointer argument in the
sh_pushcontext() and sh_popcontext() macros. (re: 2d3ec8b6)
src/cmd/ksh93/tests/io.sh:
- Tweak a process substitution test to allow up to a second for
unused process substitution processes to disappear. On the Alpine
Linux console (at least the musl libc version), this is needed to
avoid a test failure as long as no GUI is active. As soon as you
start X11, this phenomenon disappears, even on the console. Very
strange, but also very probably not ksh's fault.
src/cmd/ksh93/tests/shtests:
- Instead of just SIGCONT and SIGPIPE, set all signals to default,
just to be sure to avoid spurious test failures due to signals
that were ignored on entry. (It made no difference to the
aforementioned Alpine Linux test failure, so ignored signals had
nothing to do with that -- but still a good idea.)
.github/workflows/ci.yml:
- On the GitHub CI runs, when testing with SHOPTs disabled, disable
SHOPT_SPAWN as well, which tests that everything still works
correctly with the regular fork(2) method.
COPYRIGHT:
- Remove duplicate of BSD license.
src/cmd/ksh93/sh.1:
- Add a new section on history expansion mostly adapted from the
"History substitution" section from the tcsh(1) man page. This
has the standard BSD license which is already in the COPYRIGHT
file. Inapplicable stuff was removed, some new stuff added.
src/cmd/ksh93/edit/hexpand.c,
src/cmd/ksh93/sh/io.c:
- Set '#' as the default history comment character as on bash;
no longer disable it by default.
- Add the 'a' modifier as a synonym for 'g', as on bash.
- Remove pointless static keyword from np variable; since the
value from previous calls is never used it can just be local.
- Use NV_NOADD flag with nv_open() to avoid pointlessly creating
the node if the variable doesn't exist yet.
- Fix a bug in history expansion where the 'p' modifier had no
effect if the 'histverify' option is on.
Reproducer:
$ set -H -o histv
$ true a b c
$ !!:p
$ true a b c▁ <= instead of printed, the line is re-edited
Expected:
$ set -H -o histv
$ true a b c
$ !!:p
true a b c
$ ▁
This is fixed by making 'p' remove the HIST_EVENT bit from the
returned flag bits in hist_expand(), leaving only the HIST_PRINT
flag, then adding special handling for this case to slowread()
in io.c (print the line, then instead of executing it, continue
and read the next line).
Reproducer:
$ set -o histexpand
$ echo foo !#^:h !#^:&
/usr/local/bin/ksh: :&: no previous substitution
ksh(80822,0x10bc2a5c0) malloc: *** error for object 0x10a13bae3: pointer being freed was not allocated
ksh(80822,0x10bc2a5c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort
Analysis: In hist_expand(), the 'cc' variable has two functions:
it holds a pointer to a malloc'ed copy of the current line, and is
also used as a temporary pointer with functions like strchr().
After that temporary use, it is set to NULL again, because the
'done:' routine checks if it non-NULL to decide whether to free the
pointer. But if an error occurs, the function may jump straight to
'done' without first setting cc to NULL if it had been used as a
temporary pointer. It then tries to free an unallocated pointer.
src/cmd/ksh93/edit/hexpand.c: hist_expand():
- Eliminate this bad practice by using a separate variable for
temporary pointer purposes.
(I was unable to reproduce the crash in a pty regression test,
though it is consistently reproducible in a real interactive
session. So I haven't added that test.)
A 2008 blog post by Finnbar P. Murphy is the only documentation
on these facilities that is available to date. Thankfully, Finnbarr
has graciously granted me permission to use all his ksh93-related
blog posts for ksh 93u+m under the same license as ksh.
Since SHOPT_ACCT (disabled by default) is essentially an older and
more primitive version of SHOPT_AUDIT (enabled by default), we
should probably remove the former in a future release.
src/cmd/ksh93/README-AUDIT.md:
- Added.
src/cmd/ksh93/data/variables.sh: shtab_variables[]:
- Remove unused "CSWIDTH" entry. All use of it (including the
matching CSWIDTHNOD macro) was removed in version 2003-04-22.
src/cmd/ksh93/tests/variables.sh:
- For the tests on the shtab_variables[] variables, read the
variable names straight from variables.c instead of synching
the list in the test script, which would surely be forgotten.
src/cmd/ksh93/tests/*.sh:
- Fix a number of mistaken tries to count errors from a subshell.
- Fix miscellaneous minor breakage and typos.
So far all ksh versions accept event numbers referring to
nonexistent history events in history expansion (-H/-o histexpand),
e.g. !9999 is accepted even if the history file has no item 9999.
These expansions seem to show random content from the history file,
sometimes including binary data. Of course an "event not found"
error should have been thrown instead.
hist_expand() in hexpand.c calls hist_seek() (from history.c)
without any bounds checking except verifying the history event
number is greater than zero. This commit adds a bounds check
to hist_seek() itself as it's called from three other places
in history.c, so perhaps this fixes a few other bugs as well.
src/cmd/ksh93/edit/history.c: hist_seek():
- Use the hist_min() and hist_max() macros provided in history.h
to check bounds. Note that hist_max() yields the number of the
command line currently being entered, so the maximum for seeking
purposes is actually its result minus 1.
History expansion currently crashes under ASan due to a buffer
overflow. Reproducer:
$ set -H
$ !!:s/old/new/
Explanation from <https://github.com/att/ast/issues/1369>:
> The problem is the code assumes the buffer allocated for a string
> stream is zero initialized. But the SFIO code uses malloc() to
> allocate the buffer and does not explicitly initialize it with
> memset(). That it works at all, even without ASAN enabled, is
> purely accidental. It will fail if that malloc() returns a block
> that had been previously allocated, used, and freed. Under ASAN
> the buffer is initialized (at least on my system) to a sequence
> of 0xBE bytes. So the strdup() happily tries to duplicate a
> string that is the size of that buffer and fails when it reads
> past the end of the buffer looking for the terminating zero byte.
src/cmd/ksh93/edit/hexpand.c:
- Backport ksh2020 bugfix that avoids assuming the string stream
has been initialized to zeros:
https://github.com/att/ast/commit/cf16bcca
(minus the incorrect change to the static wm variable).
FreeBSD defines an SF_SYNC macro in sys/socket.h that conflicts
with sfio's SF_SYNC discipline, at best rendering it ineffective.
src/lib/libast/sfio/sfhdr.h:
- Temporarily undef __BSD_VISIBLE while including <sys/socket.h>
to hide the BSD extension with the conflicting definition.
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).
The more notable ones are:
src/lib/libast/features/standards:
- Do not redefine _GNU_SOURCE and _FILE_OFFSET_BITS if already
defined from $CCFLAGS. Thanks to @hyanias for the heads-up.
(re: 289dd46c)
src/cmd/ksh93/data/builtins.c,
src/cmd/ksh93/include/shell.h,
src/cmd/ksh93/sh/args.c,
src/cmd/ksh93/sh/name.c:
- Remove -T test code activation option. It was basically unused.
The only thing it did was intentionally introduce a memory leak
in table_unset() if the 4th bit in the option argument was set.
A search in ast-open-history reveals a few more trivial test uses
that were later deleted, but nothing interesting.
src/cmd/ksh93/tests/{basic,path}.sh:
- Skip a couple of tests on AIX avoid hangs, at least one of which
is not ksh's fault. Thanks to @HansH111 for the report.
src/cmd/ksh93/tests/builtins.sh:
- Change one awk use to a more portable sed invocation to placate
systems with ancient awk commands, such as AIX. (re: de795e1f)
Turns out that the standards macros set by features/standards (such
as _GNU_SOURCE for Linux or _DARWIN_SOURCE for macOS) were still
*not* included for most C source files! Instead, they were
selectively included for some files only, sometimes via
FEATURE/standards (the output of features/standards), sometimes
via ast_standards.h which is copied from FEATURE/standards.
Consequently, there were still inconsistencies in the system header
interfaces exposed on Linux, macOS, Solaris, et al. It's no wonder
it sometimes took so much hackery to keep everything building.
Of course, making this consistent had to break things somewhere.
Breakage occurred on 32-bit Linux due to a lot of ugly hackery
involving direct use of internal GNU types like off64_t and
functions like fseek64(). This is now all removed and they are
activated by setting the appropriate feature macro instead, so
these types and functions can be used with their standard names
(off_t, fseek, etc.)
Before committing I've tested these changes on the following
i386/x86_64 systems: Linux (glibc 32 and 64 bit, musl libc 64 bit),
Solaris (32 and 64 bit), illumos (32 and 64 bit), FreeBSD (64 bit),
macOS (64 bit), Cygwin (32 bit), and Haiku (64 bit).
(Note: ast_standards.h is copied from FEATURE/standards, whereas
ast_common.h is copied from FEATURE/common.)
src/lib/libast/include/ast_std.h,
src/lib/libast/stdio/stdhdr.h:
- Include <ast_standards.h> first. This should cause all the AST
and dependent code (such as ksh) to get the standards macros.
src/lib/libast/features/standards:
- For GNU (glibc), #define _FILE_OFFSET_BITS 64 to get large file
support with 64-bit offsets.
- Stop GNU and Cygwin <string.h> form defining the GNU version of
basename(3); on Cygwin, that declaration conflicts with the AST
version (and with POSIX) by using a const char* argument instead
of char*. It is deactivated by defining the macro 'basename' (as
'basename'); this causes GNU string.h to consider it to be
already defined by the standard libgen.h header.
All other changed files:
- Remove direct use of *64* types and functions and a lot of
related hackery.
This commit adds a wrapper for the AIX ar command that uses the
-X64 flag to avoid build errors on that platform.
Resolves: https://github.com/ksh93/ksh/issues/385
Commit 24fc1bbc broke the build on Cygwin in comp/setlocale.c by no
longer defining _GNU_SOURCE on that system in features/standards.
This caused wcwidth() to be hidden by wchar.h though it was
detected in the libraries.
src/lib/libast/features/standards:
- Detect Cygwin along with GNU as a system on which to define
_GNU_SOURCE.
- Add wcwidth() compilation as an extra heuristic to the BSD,
SunOS, Darwin and GNU/Cygwin tests. (Since it's specified as an
optional (X/Open) feature, it should not be tested for in the
generic fallbacks.)
These are minor things I accumulated over the last month or so.
Notable changes:
src/lib/libast/features/api,
src/lib/libast/misc/state.c,
src/lib/libast/comp/conf.tab,
src/cmd/ksh93/include/defs.h:
- Bump internal libast version to 20220101L. We've made a few
additions to the API, at least pathicase (see 71934570, ca3ec200)
and astconf_long (see c2ac69b2), so this should have been done
already. This also updates '/opt/ast/bin/getconf _AST_VERSION'.
- Use AST_VERSION instead of outdated _AST_VERSION.
- In state.c, use AST_VERSION instead of hardcoding the version.
src/cmd/ksh93/sh/xec.c:
- Remove 'restorefd' variable, unused as of 42becab6.
- Remove 'cmdrecurse' function and SH_RUNPROG macro; this was once
used by a few libcmd commands, but ast-open-archive reveals it's
unused as of ast 1999-12-25.
src/cmd/ksh93/sh/*.c:
- Where available, use e_dot instead of "." for consistency; it is
defined as an extern so we might as well use it.
src/cmd/ksh93/tests/*.sh:
- When reporting signal names in fails, include the SIG prefix.
- Fix a broken process hang test in subshell.sh.
src/lib/libast/man/sfdisc.3:
- Removed. The interfaces described here never made it out of AT&T;
they do not exist in any libast version in ast-open-archive.
Resolves: https://github.com/ksh93/ksh/issues/426
In the times(3) fallback for the time keyword (which can be enabled
in xec.c by undefining _lib_getrusage and timeofday), ksh will
print the obtained time incorrectly if TIMEFORMAT is set to use a
precision level of three:
$ TIMEFORMAT=$'\nreal\t%3lR'
$ time sleep .080
real 0m00.008s # Should be '00.080s'
This commit corrects that issue by using 10^precision to get the
correct fractional scaling. Note that the fallback still doesn't
support a true precision level of three (times(3) alone doesn't
support it), so this in effect pads a zero to the end of the output
when the precision level is three.
Additional change to tests/builtins.sh:
- While fixing the above issue I found out that ksh93v- broke
support for passing microseconds to the sleep builtin in the form
of <num>U. I've added a regression test for that bug to ensure it
isn't backported to ksh93u+m by accident.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
The code used to fork subshells when creating/changing aliases will
always fork, even when the alias tree isn't changed:
$ echo $(unalias --man 2> /dev/null; echo $$ ${.sh.pid})
375097 375107
$ alias foo=bar; echo $(alias -p foo; echo $$ ${.sh.pid})
alias foo=bar 375097 375110
This is a bit inefficient, so this commit avoids forking a subshell
unless at least one change is made to the alias table.
src/cmd/ksh93/bltins/typeset.c:
- b_alias(), b_unalias(): Remove sh_subfork() calls.
- setall(): When creating an alias (name contains '='), fork a
virtual subshell before calling nv_open() to add the node.
- unall():
- When unsetting all aliases (-a), fork subshell before dtclear().
- When unsetting one alias, fork subshell before nv_delete().
- Move sh_pushcontext() and sh_popcontext() expansions so that
sh_subfork() is not in between them, as that would cause
program flow corruption or a crash.
Co-authored-by: Martijn Dekker <martijn@inlv.org>
If neither gmacs/emacs nor vi are active, the multiline mode should
not be enabled even if the multiline option is on. Doing so can
cause inconsistent behaviour, particularly in multibyte locales
where, if the shell is compiled with SHOPT_RAWONLY (as is default),
the no-editor mode is actually handled by vi.c.
Also, the new --histreedit and --histverify options only work in
the emacs or vi editors, or in no-editor mode when handled by vi.
Which means they cannot ever work if neither emacs or vi were
compiled in (i.e. SHOPT_ESH and SHOPT_VSH were both disabled).
In that case, there's no point in compiling in those options.
Come to think of it, the same applies to the multiline option.
All changed files:
- Update SHOPT_ESH/SHOPT_VSH preprocessor directives as per above.
src/cmd/ksh93/include/defs.h,
src/cmd/ksh93/include/shell.h:
- Move definitions of history expansion-related options to shell.h,
which is where all the other shell options are defined.
This adds two long-form shell options that modify history expansion
(-H/--histexpand). If --histreedit is on and a history expansion
fails, the command line is reloaded into the next prompt's edit
buffer, allowing corrections. If --histverify is on, the results of
a history expansion are not immediately executed but instead loaded
into the next prompt's edit buffer, allowing further changes.
SH_HISTREEDIT and SH_HISTVERIFY were already handled all along in
slowread() in io.c and via 'reedit' arguments to functions called
there, but could not be turned on as they were only ever exposed as
shopt options in the removed bash compatibility mode (in theory
only, as it failed to compile). I had overlooked them until now.
So, since the code is there, let's expose these options through the
normal long options interface. They're working fine, and activating
them actually makes history expansion tolerable to use.
src/cmd/ksh93/data/options.c:
- Make these options available as "histreedit" and "histverify".
src/cmd/ksh93/data/builtins.c,
src/cmd/ksh93/sh.1:
- Document the "new" options.
src/cmd/ksh93/include/defs.h:
- Remove unused SH_HISTAPPEND and SH_HISTORY2 macros which were
part of the removed bash compatibility code. Note that ksh does
not need a histappend option as it never overwrites the history
file (in the bash mode, this shopt option was a no-op).
The referenced commit introduced a bug that caused command
substitutions to hang, writing infinite zero bytes, when
redirecting standard output on a built-in comand that forks the
command substitution subshell.
The bug was caused by removing the fork when redirecting standard
output in a non-permanent manner. However, simply reintroducing the
fork causes multiple regressions that we had fixed in the meantime.
Thankfully, it looks like this forking workaround is only necessary
when redirecting the output of built-ins. It appears that moving
workaround from io.c to the built-ins handling code in sh_exec() in
xec.c, right before calling sh_redirect(), allows reintroducing the
forking workaround for non-permanent redirections without causing
other regressions.
It would be better if the underlying cause of the hang were fixed
so the workaround becomes unnecessary, but I don't think that is
going to happen any time soon (AT&T didn't manage, either).
src/cmd/ksh93/sh/io.c: sh_redirect():
- Remove forking workaround for redirecting stdout in a comsub.
src/cmd/ksh93/sh/xec.c: sh_exec(): TCOM: built-ins handling code:
- Reimplement the workaround here.
Resolves: https://github.com/ksh93/ksh/issues/416
