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

getopts segfaults on malformed arguments #794

Closed dnewhall closed 2 weeks ago

dnewhall commented 2 weeks ago

Found this while testing a very long optstring that had a typo.

For the following snippet of code in opttest.ksh:

#!/usr/bin/env 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.

This is in 93u+m/1.0.10 2024-08-01 running on Ubuntu 22.04.

(The correct optstring was supposed to be "f:(file)", which works.)

McDutchie commented 2 weeks ago

Thanks for the report.

Call stack trace:

$ arch/darwin.arm64-64/bin/ksh issue794.sh --file=foo
AddressSanitizer:DEADLYSIGNAL
=================================================================
==21565==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000100583d08 bp 0x00016fafa650 sp 0x00016fafa480 T0)
==21565==The signal is caused by a UNKNOWN memory access.
==21565==Hint: address points to the zero page.
    #0 0x100583d08 in optnumber optget.c:4145
    #1 0x10057da70 in optget optget.c:5242
    #2 0x100307088 in b_getopts getopts.c:120
    #3 0x1004cbe30 in sh_exec xec.c:1271
    #4 0x1004d565c in sh_exec xec.c:2179
    #5 0x10034fe94 in exfile main.c:595
    #6 0x10034c770 in sh_main main.c:348
    #7 0x1002f8fa0 in main pmain.c:41
    #8 0x100a45088 in start+0x204 (dyld:arm64e+0x5088)
McDutchie commented 2 weeks ago

I introduced this bug in 9fcd023ed6f87f588a7a2dbd743f882a76cbcf6f. Oops.

In optget.c:4145, I removed the check for a non-null e pointer before dereferencing it, but optget.c:5242 calls optnumber with e==NULL.

This patch restores that check and should fix the bug.

diff --git a/src/lib/libast/misc/optget.c b/src/lib/libast/misc/optget.c
index 02f1ad678..2c8b5b808 100644
--- a/src/lib/libast/misc/optget.c
+++ b/src/lib/libast/misc/optget.c
@@ -4142,7 +4142,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;
 }
McDutchie commented 2 weeks ago

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.

I should release the next version with the fix ASAP.