Skip to content

Commit 4d68815

Browse files
committed
Further robustify sh_reinit() (re: 232b7bf, ebfe3b6, 4e979ec)
When ksh executes a script without a #! path (note that the AT&T team had a real disliking for #! paths), ksh forks and goes through a quick reinitialisation procedure. This is much faster than invoking a fully new shell but should have the same effect if it all works well. Unfortunately it's not worked all that well so far. Even after recent improvements (see referenced commits) I've been finding corner case problems. FYI, running a script without #! basically goes like this: * in path_spawn(), execve() fails with ENOEXEC because the file is not a binary executable and does not start with #! * this triggers 'case ENOEXEC:' which: * forks ksh * calls exscript() * exscript() cleans up & calls siglongjmp(*sh.jmplist,SH_JMPSCRIPT) * SH_JMPSCRIPT is the highest longjmp value, so *all* the previous sigsetjmp/sh_pushcontext calls are unwinded in reverse order, triggering all sorts of cleanup, state restoration, removal of local scopes, etc. * eventually, this lands us at the top sigsetjmp in sh_main() * sh_main() calls sh_reinit(), then resumes as if the shell had just been started This commit makes the following interrelated changes for the correct functioning of this procedure: 1. exscript() now exports the environment into a dedicated Stk_t buffer and sets environ[] to that. 2. Instead of re-using existing variables, sh_reinit() deletes everything and reinits all name-value trees from scratch, then re-imports the environment from environ[]. 3. Variable values that were imported from the environment are no longer treated specially with an NV_IMPORT attribute and the np->nvenv pointer to their value in environ[], fixing at least one crash.[*1] Details of the changes follow: src/cmd/ksh93/sh/path.c: - exscript(): Generate a new environ[] by activating a dedicated AST stack that will not be overwritten before calling sh_envgen(). This will allow sh_reinit() to delete all variables and then reimport the environment. The exporting must be done here, before siglongjmp, otherwise locally scoped exported variables won't be included (siglongjmp with SH_JMPSCRIPT triggers cleanup of all scopes). src/cmd/ksh93/sh/init.c: - sh_reinit(): Largely rewritten as follows. - Reset shell options first. This has the beneficial side effect of unsetting SH_RESTRICTED which interferes with unsetting certain variables, like PATH. - Remove workarounds for FPATH, SHLVL and tilde expansion disciplines; these will not be needed now. - Properly unset and delete all functions and built-ins. Since we now unset a function before deleting it, this should now free up their memory. (See nvdisc.c below for a change allowing removal of special built-ins.) - Properly unset all variables (which includes any associated discipline functions). Incorporate here the needed logic from sh_envnolocal() in name.c; most of it is unneeded (that function was previously used to cleanup local variables but has not been used for that for decades). So sh_envnolocal() is now unused. - Delete variables in a separate pass after unsetting variables and unsetting and deleting functions; this avoids use-after- free problems as well as possible "no parent" problems with namespace variables (e.g., .rc.status in our new kshrc.sh). - After all that, close and free up all function, alias, tracked alias, type and variable trees. - Free the contiguous built-in node space and the Init_t init context (with all the special variable discipline pointers). - Call nv_init (previously only called from sh_init) to reinitialise all of the above name-value stuff from scratch. It's the only way to be sure. - Re-import the environment as stored by exscript() above. - env_init(): - Per item 3 above and footnote 1 below, no longer set NV_IMPORT attribute and no longer point np->nvenv to the item in environ. - POSIX says, for 'environ': "Any application that directly modifies the pointers to which the environ variable points has undefined behavior."[*2] Yet, env_init() is indeed juggling the environ[] pointers to deal with variables that cannot be imported because their names are invalid (because they still need to be saved to be passed on to child processes). Replace the current approach with one where those env vars get allocated on the heap, pointed to by sh.save_env and counted by sh.save_env_n (renamed from sh.nenv). This only needs to be done once as ksh cannot use or change these variables. src/cmd/ksh93/sh/name.c: - sh_envgen(): Update to match env_init() change above. - pushnam() (called by sh_envgen()): Remove NV_IMPORT attribute check as per above and never get the value from the nvenv pointer -- simply always use nv_getval(). As of this change, the NV_IMPORT attribute is unused. The next commit will remove it and do related cleanups. - staknam(): is only called if value!=NULL, so remove that 'if'. - sh_envnolocal(): Removed. src/cmd/ksh93/sh/nvdisc.c: - assign(): Remove a check for the SH_INIT state bit that avoids freeing functions during sh_reinit(). This works fine now. - sh_addbuiltin(): Allow sh_reinit() to delete special builtins by checking for the SH_INIT state bit before throwing an error. src/cmd/ksh93/sh/nvtree.c: - outval(): Add a workaround for a use-after-free, introduced by the changes above, that occurred in the types.sh tests for #!-less scripts (types.sh:675-722). The use-after-free occurred here (abridged ASan trace follows; line numbers are current as of this commit): ==30849==ERROR: AddressSanitizer: heap-use-after-free [...] #0 in dttree dttree.c:393 ksh-community#1 in sh_reinit init.c:1637 ksh-community#2 in sh_main main.c:136 [...] The pointer was freed in the same loop via nv_delete() in outval: #0 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:[...]) ksh-community#1 in nv_delete name.c:1318 ksh-community#2 in outval nvtree.c:731 ksh-community#3 in genvalue nvtree.c:905 ksh-community#4 in walk_tree nvtree.c:1042 ksh-community#5 in put_tree nvtree.c:1108 ksh-community#6 in nv_putv nvdisc.c:144 ksh-community#7 in _nv_unset name.c:2437 ksh-community#8 in sh_reinit init.c:1645 ksh-community#9 in sh_main main.c:136 [...] So, what happened was that the nv_delete() call on name.c:1318 (eventually resulting from the _nv_unset call on init.c:1645) freed the node pointed to by np, so that the next loop iteration crashed on line 1637 as the dtnext() macro now gets a freed np. Now, why on earth should _nv_unset() *ever* indirectly call nv_delete()? That's a question for another day; I suspect it may be a bug, or it may be needed for compound variables for some reason. For now, I'm adding a workaround: simply avoid calling nv_delete() if the SH_INIT state bit is on, indicating sh_reinit() is in the call stack. This allows the variables unset loop in sh_reinit() to continue without crashing. sh_reinit() handles deletion later anyway. src/cmd/ksh93/sh/main.c: - sh_main(): remove zeroing of sh.fun_depth and sh.dot_depth; these are known to be 0, coming from either sh_init() or sh_reinit(). ________ [*1] This NV_IMPORT/nvenv usage is a redundant holdout from ancient ksh code; the imported value is easily available as a normal shell variable value via nv_getval(). Plus, the nvenv pointer is overloaded with too many other purposes: so far I've discovered it's used for pointers to subarrays of arrays (multidimentional arrays), compound variables, builtins, and other things. This mess caused at least one crash in set_instance() (xec.c) due to incorrectly using that nvenv pointer. The current kshrc script triggers this. Reproducer: $ export PS1 $ bin/package use «0»26:…/src/ksh93/ksh[dev] $ typeset +x PS1 ...and crash. That is now fixed. [*2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/environ.html
1 parent a45a0eb commit 4d68815

File tree

12 files changed

+199
-232
lines changed

12 files changed

+199
-232
lines changed

NEWS

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,14 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
22
For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
33
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.
44

5+
2024-02-29:
6+
7+
- Improved the robustness of the routines handling the execution by ksh of a
8+
script without an initial #! path. More memory is freed up after ksh forks.
9+
10+
- Fixed a crash that could occur after 'typeset +x var' when var was imported
11+
from the environment and then a discipline function was set for it.
12+
513
2024-02-22:
614

715
- Fixed a crash that occurred when starting ksh with no TERM variable in the

src/cmd/ksh93/include/defs.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,6 @@ extern char *sh_checkid(char*,char*);
115115
extern void sh_chktrap(void);
116116
extern int sh_debug(const char*,const char*,const char*,char *const[],int);
117117
extern char **sh_envgen(void);
118-
extern void sh_envnolocal(Namval_t*,void*);
119118
extern Sfdouble_t sh_arith(const char*);
120119
extern void *sh_arithcomp(char*);
121120
extern pid_t sh_fork(int,int*);

src/cmd/ksh93/include/shell.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,8 @@ struct Shell_s
341341
int xargmin;
342342
int xargmax;
343343
int xargexit;
344-
int nenv;
344+
int save_env_n; /* number of saved pointers to environment variables with invalid names */
345+
char **save_env; /* saved pointers to environment variables with invalid names */
345346
mode_t mask;
346347
void *init_context;
347348
void *mac_context;
@@ -425,7 +426,7 @@ extern Libcomp_t *liblist;
425426

426427
extern void sh_subfork(void);
427428
extern Shell_t *sh_init(int,char*[],Shinit_f);
428-
extern int sh_reinit(char*[]);
429+
extern void sh_reinit(void);
429430
extern int sh_eval(Sfio_t*,int);
430431
extern void sh_delay(double,int);
431432
extern void *sh_parse(Sfio_t*,int);

src/cmd/ksh93/include/version.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818

1919
#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
2020
#define SH_RELEASE_SVER "1.0.9-beta" /* semantic version number: https://semver.org */
21-
#define SH_RELEASE_DATE "2024-02-22" /* must be in this format for $((.sh.version)) */
21+
#define SH_RELEASE_DATE "2024-02-29" /* must be in this format for $((.sh.version)) */
2222
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK
2323

2424
/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */

0 commit comments

Comments
 (0)