ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
192 stars 32 forks source link

Regression: ksh segfaults when unsetting SHLVL then exec'ing #788

Closed vmihalko closed 1 month ago

vmihalko commented 2 months ago

What happens?

If the user unsets the SHLVL variable and later replaces the shell by executing a command, ksh will segfault.

Reproducer

# unset -v SHLVL
# exec bash
Segmentation fault (core dumped)

Reason

The reason for this is that the SHLVL variable is getting decremented without any guard making sure it's still in the environment, see: https://github.com/ksh93/ksh/blob/7170ac01c4f4603586661761ba685d2d3c2d69bc/src/cmd/ksh93/bltins/misc.c#L145-L147

Fix

I tested the following simple fix, and the segmentation fault no longer occurs.

diff --git a/src/cmd/ksh93/bltins/misc.c b/src/cmd/ksh93/bltins/misc.c
index cb7e883b..0e09b9ea 100644
--- a/src/cmd/ksh93/bltins/misc.c
+++ b/src/cmd/ksh93/bltins/misc.c
@@ -143,7 +143,7 @@ int    b_exec(int argc,char *argv[], Shbltin_t *context)
                if(job_close() < 0)
                        return 1;
                /* if the main shell is about to be replaced, decrease SHLVL to cancel out a subsequent increase */
-               if(!sh.realsubshell)
+               if(!sh.realsubshell && SHLVL->nvalue.ip)
                        (*SHLVL->nvalue.ip)--;
                sh_onstate(SH_EXEC);
                if(sh.subshell && !sh.subshare)

Should I use some other way to test if SHLVL is set, or is this OK, and I can create a PR?

posguy99 commented 2 months ago

Why is SHLVL unsettable? Shouldn't that generate some sort of error?

Edit... NVM, the man page actually refers to a case where it's not in the environment, so it's on purpose.

McDutchie commented 1 month ago

Thanks for the report, @vmihalko. I prefer to fix it in a slightly different way. Commit coming up.