Skip to content

Commit

Permalink
parse.c: fix use-after-free probs related to funstaks()
Browse files Browse the repository at this point in the history
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: #212
  • Loading branch information
McDutchie committed Jun 14, 2022
1 parent 2dfdc4f commit 7e317c5
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 9 deletions.
6 changes: 5 additions & 1 deletion src/cmd/ksh93/sh/name.c
Original file line number Diff line number Diff line change
Expand Up @@ -2501,7 +2501,11 @@ void _nv_unset(register Namval_t *np,int flags)
}
dtclose(rp->sdict);
}
stakdelete(slp->slptr);
if(slp->slptr)
{
stakdelete(slp->slptr);
slp->slptr = NIL(Stak_t*);
}
free((void*)np->nvalue.ip);
np->nvalue.ip = 0;
}
Expand Down
15 changes: 11 additions & 4 deletions src/cmd/ksh93/sh/parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -527,10 +527,16 @@ void sh_funstaks(register struct slnod *slp,int flag)
if(slp->slchild)
sh_funstaks(slp->slchild,flag);
slp = slp->slnext;
if(flag<=0)
stakdelete(slpold->slptr);
else
staklink(slpold->slptr);
if(slpold->slptr)
{
if(flag<=0)
{
stakdelete(slpold->slptr);
slpold->slptr = NIL(Stak_t*);
}
else
staklink(slpold->slptr);
}
}
}
/*
Expand Down Expand Up @@ -953,6 +959,7 @@ static Shnode_t *funct(Lex_t *lexp)
{
sh.st.staklist = slp->slnext;
stakdelete(slp->slptr);
slp->slptr = NIL(Stak_t*);
}
siglongjmp(*sh.jmplist,jmpval);
}
Expand Down
18 changes: 14 additions & 4 deletions src/cmd/ksh93/sh/xec.c
Original file line number Diff line number Diff line change
Expand Up @@ -1427,7 +1427,8 @@ int sh_exec(register const Shnode_t *t, int flags)
/* increase refcnt for unset */
slp = (struct slnod*)np->nvenv;
sh_funstaks(slp->slchild,1);
staklink(slp->slptr);
if(slp->slptr)
staklink(slp->slptr);
if(nq)
{
Namval_t *mp=0;
Expand Down Expand Up @@ -1475,7 +1476,11 @@ int sh_exec(register const Shnode_t *t, int flags)
if(nq)
unset_instance(nq,&node,&nr,mode);
sh_funstaks(slp->slchild,-1);
stakdelete(slp->slptr);
if(slp->slptr)
{
stakdelete(slp->slptr);
slp->slptr = NIL(Stak_t*);
}
if(jmpval > SH_JMPFUN || (io && jmpval > SH_JMPIO))
siglongjmp(*sh.jmplist,jmpval);
goto setexit;
Expand Down Expand Up @@ -2486,7 +2491,11 @@ int sh_exec(register const Shnode_t *t, int flags)
struct Ufunction *rp = np->nvalue.rp;
slp = (struct slnod*)np->nvenv;
sh_funstaks(slp->slchild,-1);
stakdelete(slp->slptr);
if(slp->slptr)
{
stakdelete(slp->slptr);
slp->slptr = NIL(Stak_t*);
}
if(rp->sdict)
{
Namval_t *mp, *nq;
Expand Down Expand Up @@ -2522,7 +2531,8 @@ int sh_exec(register const Shnode_t *t, int flags)
struct comnod *ac = t->funct.functargs;
slp = t->funct.functstak;
sh_funstaks(slp->slchild,1);
staklink(slp->slptr);
if(slp->slptr)
staklink(slp->slptr);
np->nvenv = (char*)slp;
nv_funtree(np) = (int*)(t->funct.functtre);
np->nvalue.rp->hoffset = t->funct.functloc;
Expand Down
95 changes: 95 additions & 0 deletions src/cmd/ksh93/tests/functions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1400,5 +1400,100 @@ else
unset exp
fi

# ======
# funcname.ksh crashed
# https://github.com/ksh93/ksh/issues/212
cat >$tmp/funcname.ksh <<-'EOF'
# tweaked version of funname.ksh by Daniel Douglas
# https://gist.github.com/ormaaj/12874b68acd06ee98b59
# Used by permission: "Consider all my gists MIT / do whatever."
# https://github.com/ksh93/ksh/issues/212#issuecomment-807915937
# run in subshell for additional regression testing
(
IFS='-' # element separator for "$*" etc., incl. "${FUNCNAME[*]}"
typeset -a FUNCNAME
function FUNCNAME.get {
nameref self=${.sh.name}
if (( .sh.subscript < .sh.level )); then
trap "(( .sh.level -= .sh.subscript + 1 )); eval '(( .sh.level = ${.sh.level} ))' \; _=\${.sh.fun}" DEBUG
trap - DEBUG;
fi
(( .sh.subscript < .sh.level - 2 )) && self[.sh.subscript + 1]=
}
function f {
if (($1 < 10)); then
print -r -- "${FUNCNAME[*]}"
g $(($1 + 1))
fi
print -r -- "${FUNCNAME[*]}"
}
function g {
if (($1 < 10)); then
print -r -- "${FUNCNAME[*]}"
f $(($1 + 1))
fi
print -r -- "${FUNCNAME[*]}"
}
#typeset -ft FUNCNAME.get f g
f 0
)
: do not optimize out the subshell
EOF
exp='f
g-f
f-g-f
g-f-g-f
f-g-f-g-f
g-f-g-f-g-f
f-g-f-g-f-g-f
g-f-g-f-g-f-g-f
f-g-f-g-f-g-f-g-f
g-f-g-f-g-f-g-f-g-f
f-g-f-g-f-g-f-g-f-g-f
g-f-g-f-g-f-g-f-g-f-
f-g-f-g-f-g-f-g-f--
g-f-g-f-g-f-g-f---
f-g-f-g-f-g-f----
g-f-g-f-g-f-----
f-g-f-g-f------
g-f-g-f-------
f-g-f--------
g-f---------
f----------'
got=$(set +x; { "$SHELL" funcname.ksh ;} 2>&1)
[[ $got == "$exp" ]] || err_exit 'funcname.ksh crash (direct run)' \
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
# dotting it slightly changes the output
exp='f-
g-f-
f-g-f-
g-f-g-f-
f-g-f-g-f-
g-f-g-f-g-f-
f-g-f-g-f-g-f-
g-f-g-f-g-f-g-f-
f-g-f-g-f-g-f-g-f-
g-f-g-f-g-f-g-f-g-f-
f-g-f-g-f-g-f-g-f-g-f-
g-f-g-f-g-f-g-f-g-f--
f-g-f-g-f-g-f-g-f---
g-f-g-f-g-f-g-f----
f-g-f-g-f-g-f-----
g-f-g-f-g-f------
f-g-f-g-f-------
g-f-g-f--------
f-g-f---------
g-f----------
f-----------'
got=$(set +x; { "$SHELL" -c '. ./funcname.ksh' ;} 2>&1)
[[ $got == "$exp" ]] || err_exit 'funcname.ksh crash (dot)' \
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"

# ======
exit $((Errors<125?Errors:125))

0 comments on commit 7e317c5

Please sign in to comment.