ksh aborts when I have typo in an option

[kdudka]

Description of problem:
ksh aborts when I have typo in an option.

Ksh version:
2020.0.0-alpha1-1-g7d7bba3e

How reproducible:
easily

Steps to reproduce:

1. ksh --verson

Actual results:

kdudka@f29 ~/git/ast/build % ./src/cmd/ksh93/ksh --version
  version         sh (AT&T Research) 2020.0.0-alpha1-1-g7d7bba3e
kdudka@f29 ~/git/ast/build % ./src/cmd/ksh93/ksh --verson 
ksh: verson: bad option(s)
Usage: ksh [ options ] [arg ...]
../src/cmd/ksh93/sh/fault.c:457: failed assertion 'pp'
zsh: abort (core dumped)  ./src/cmd/ksh93/ksh --verson

Expected results:

kdudka@f29 ~/fedora/ksh % ksh --verson       
ksh: verson: bad option(s)
Usage: ksh [ options ] [arg ...]

Additional info:
I get the expected output with ksh 93u+.

[kusalananda]

I'm noticing that as long as the --version option is specified as a correctly spelt prefix (--v, --ver etc.), there is no failed assertion. The failed assertion only occurs when the option name is not a correctly spelt prefix of the full option name. The same goes for e.g. --help (--h and --he works, but --hep does not).

[kdudka]

@kusalananda Because --ver is recognized as abbreviated --version, not as bad option. This is a well-known feature of getopt_long() -- see e.g. http://refspecs.linuxbase.org/LSB_3.1.0/LSB-generic/LSB-generic/baselib-getopt-long-3.html

[kusalananda]

ksh does not use getopt_long() (there's at least no references to a call to that function in the sources) and I'm not on Linux.

This is the backtrace:

#0  thrkill () at -:3
#1  0x00000b78fb12ff7e in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
#2  0x00000b7651a74a0f in __assert (e=0xb76519e9078 "pp", file=0xb76519e93ce "../src/cmd/ksh93/sh/fault.c",
    line=457) at ast_assert.h:31
#3  0x00000b7651a73eda in sh_exit (shp=0xb7651b96278, xno=2) at ../src/cmd/ksh93/sh/fault.c:457
#4  0x00000b7651a4f265 in no_shell_context_sh_exit (exit_val=2) at ../src/cmd/ksh93/sh/init.c:1212
#5  0x00000b7651b4678a in errorv (id=0x0, level=4, ap=0x7f7ffffd6430) at ../src/lib/libast/misc/error.c:399
#6  0x00000b7651b46d1f in errormsg (dictionary=0xb76519f046e "libshell", level=2052)
    at ../src/lib/libast/misc/errormsg.c:38
#7  0x00000b7651a78663 in sh_argopts (argc=2, argv=0x7f7ffffd69a8, context=0xb7651b96278)
    at ../src/cmd/ksh93/sh/args.c:430
#8  0x00000b7651a4f9df in sh_init (argc=2, argv=0x7f7ffffd69a8, userinit=0) at ../src/cmd/ksh93/sh/init.c:1343
#9  0x00000b7651a34503 in sh_main (ac=2, av=0x7f7ffffd69a8, userinit=0) at ../src/cmd/ksh93/sh/main.c:114
#10 0x00000b7651a333fc in main (argc=2, argv=0x7f7ffffd69a8) at ../src/cmd/ksh93/sh/pmain.c:41

[siteshwar]

It was introduced by 3c27860.

@krader1961 Can yo take a look at it ?

[krader1961]

Turns out that sh_exit() can be called, indirectly via errormsg(), early in the ksh startup before all its state has been initialized. Meaning that the shp->jmplist var is still NULL. I'll add this as a regression test and move the assert.

[krader1961]

TBD is whether Coverity Scan issues a new warning with the moved assert(). This issue is a good example for how the existing code is badly structured. Tools like Coverity Scan wouldn't warn about dereferencing a NULL pointer after explicitly testing if it was NULL if the code was better structured.

[kdudka]

@kusalananda Sorry for the confusion. I did not want to say that ksh uses getopt_long(). I just wanted to highlight that there is a fundamental difference between bad option and abbreviated option. The long option abbreviation is a common feature that many command-line tools support (no matter how they implement it internally).

[kdudka]

@krader1961 Thank you for fixing it!

As for the original Coverity report, I think it was a true positive but the fix was wrong. Instead of removing the first check for NULL, you should have added an additional check for NULL before the second dereference.

Note that Coverity CID#279531 says nothing to anybody from outside (and will tell nothing to you 10 years from now). I think it is a better practice to paste the exact report, preferably using the GCC-like format.

[krader1961]

@kdudka If you take the original implementation at face value, and assume it is correct, then it is in fact an error for the nested, second, dereference of pp to occur if it is NULL. So predicating that dereference on checking that pp is not NULL is actually wrong. That is, changing it to if (pp && ...) would paper over a bug and potentially introduce a different bug. That is the crux of the problem Coverity Scan identified. And similar issues are present throughout the code. I've suppressed most of those via more targeted assert(ptr) statements immediately prior to the dereference that assumes the pointer is not NULL.

My mistake in this instance was concluding the original code was incorrect in assuming that sh_exit() would never be called with shp->jmplist set to NULL simply because no existing unit tests ever resulted in it being called with with a NULL value. And the structure of the code suggested the if (pp && pp->...) test was left over from a time when it could be NULL.