Skip to content

Commit

Permalink
optget(3): Fix crash on unexpected long option value (re: 9fcd023)
Browse files Browse the repository at this point in the history
@dnewhall reports:
> Found this while testing a very long optstring that had a typo.
>
> For the following snippet of code in opttest.ksh:
>
>	optstring="f(file)"
>	while getopts "$optstring" opt
>	do
>	    print -- "$opt: $OPTARG"
>	done
>
> If you accidentally add a parameter using =, you get a
> segmentation fault:
>
>	$ opttest.ksh --file=foo
>	Segmentation fault (core dumped)
>
> Obviously, calling with --file foo doesn't break it.
>
> (The correct optstring was supposed to be "f:(file)", which works.)

The bug is worse; the crash is also triggered for a correct options
string, when an option-argument is given for a long option that
doesn't expect one. For example, set --default=foo, ksh
--posix=foo, /opt/ast/bin/cp --preserve=foo all crash.

Analysis: In the referenced commit I incorrectly assumed that
nothing ever calles optnumber() with a third argument of NULL (int
*e, a pointer through which to pass back the value of errno before
restoring it), so that commit removed the check for e==NULL from
optget.c:4145. However, in optget.c:5242, there is an optnumber()
call I overlooked that does pass a third argument of NULL.

src/lib/libast/misc/optget.c: optnum():
- Restore the check for non-NULL e before dereferencing e.

Resolves: #794
  • Loading branch information
McDutchie committed Nov 14, 2024
1 parent 94e94a0 commit 336eb13
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 4 deletions.
6 changes: 6 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ This documents significant changes in the dev branch of ksh 93u+m.
For full details, see the git log at: https://github.com/ksh93/ksh
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.

2024-11-14:

- Fixed a crash in option parsing upon encountering an option value to a
long option that doesn't accept one (e.g., 'set --state=foo' crashed).
Bug introduced on 2024-08-01.

2024-11-02:

- Fixed: assigning to TMOUT in a virtual subshell was rendering further
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.1.0-alpha" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2024-11-03" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-11-14" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
19 changes: 19 additions & 0 deletions src/cmd/ksh93/tests/builtins.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1729,5 +1729,24 @@ case $? in
*) err_exit "broken test" ;;
esac
# ======
# https://github.com/ksh93/ksh/issues/794
exp=$'issue794: --file: value not expected\n?: '
got=$(set +x; { "$SHELL" -c '
optstring="f(file)"
while getopts "$optstring" opt
do
print -- "$opt: $OPTARG"
done
' issue794 --file=foo; } 2>&1)
[[ e=$? -eq 0 && $got == "$exp" ]] || err_exit "crash on unexpected option value" \
"(expected status 0, $(printf %q "$exp");" \
"got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"), $(printf %q "$got"))"
got=$(set +x; { (ulimit -c 0; set --state=foo); } 2>&1)
let "(e=$?) == 2" || err_exit "crash on unexpected option value" \
"(got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"), $(printf %q "$got"))"
# ======
exit $((Errors<125?Errors:125))
10 changes: 10 additions & 0 deletions src/cmd/ksh93/tests/libcmd.sh
Original file line number Diff line number Diff line change
Expand Up @@ -905,5 +905,15 @@ if builtin getconf 2> /dev/null; then
[[ $exp == "$got" ]] || err_exit "'getconf -n' doesn't match names correctly" \
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
fi

# ======
# https://github.com/ksh93/ksh/issues/794

if builtin cp 2>/dev/null; then
got=$(set +x; { (ulimit -c 0; cp --preserve=foo); } 2>&1)
let "(e=$?) == 2" || err_exit "crash on unexpected option value" \
"(got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"), $(printf %q "$got"))"
fi

# ======
exit $((Errors<125?Errors:125))
5 changes: 2 additions & 3 deletions src/lib/libast/misc/optget.c
Original file line number Diff line number Diff line change
Expand Up @@ -4122,8 +4122,6 @@ optusage(const char* opts)
* i.e., it looks octal but isn't, to meet
* POSIX Utility Argument Syntax -- use
* 0x.* or <base>#* for alternate bases
*
* NOTE: none of the pointer arguments may be NULL
*/

static intmax_t
Expand All @@ -4142,7 +4140,8 @@ optnumber(const char* s, char** t, int* e)
errno = 0;
n = strtonll(s, t, &lastbase, 0);
}
*e = errno;
if (e)
*e = errno;
errno = oerrno;
return n;
}
Expand Down

0 comments on commit 336eb13

Please sign in to comment.