error message for missing argument segfaults (musl libc)

[ncopa]

running any of: smartctl -l, smartctl -t, smartctl -P, smartctl -v, smartctl -b and smartctl -f results in segfault on Alpine Linux (using musl libc).

Backtrace:

Program received signal SIGSEGV, Segmentation fault.
parse_options (print_type_only=@0x7fffffffa556: false, nvmeopts=..., scsiopts=..., ataopts=..., 
    type=@0x7fffffffa538: 0x0, argv=0x7fffffffe668, argc=2) at smartctl.cpp:1173
1173	      if (arg[1] == '-' && optchar != 'h') {

Downstream report: ​https://gitlab.alpinelinux.org/alpine/aports/-/issues/15948

[ncopa]

This fixes it:

diff --git a/smartctl.cpp b/smartctl.cpp
index da97640..41a6e0a 100644
--- a/smartctl.cpp
+++ b/smartctl.cpp
@@ -1168,7 +1168,7 @@ static int parse_options(int argc, char** argv, const char * & type,
       printing_is_off = false;
       printslogan();
       // Point arg to the argument in which this option was found.
-      arg = argv[optind-1];
+      arg = argv[optind-2];
       // Check whether the option is a long option that doesn't map to -h.
       if (arg[1] == '-' && optchar != 'h') {
         // Iff optopt holds a valid option then argument must be missing.

[Christian Franke]

Thanks for the report. Please check whether smartd is also affected, for example with smartd -l.

The related code is 21+ years old, see r369. I don't remember any similar problem report.

The fix does not work with getopt_long() variants from glibc and Open/NetBSD. The latter is also used by Cygwin and Mingw-w64 CRT. All of these set optind = 2 on smartctl -l such that arg correctly points to -l.

The fix suggests that musl libc sets optind = 3. This either means that smartctl relies on an undocumented corner case or that there is a bug in musl libc itself. I'm not sure.

Perhaps we could get rid of this legacy arg check. Leaving ticket open as undecided for now.

[Alex Samorukov]

I can confirm that:

/ # apk add smartmontools
fetch http://dl-cdn.alpinelinux.org/alpine/v3.12/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.12/community/x86_64/APKINDEX.tar.gz
(1/3) Installing libgcc (9.3.0-r2)
(2/3) Installing libstdc++ (9.3.0-r2)
(3/3) Installing smartmontools (7.1-r2)
Executing busybox-1.31.1-r19.trigger
OK: 9 MiB in 17 packages
/ # smartctl -l
smartctl 7.1 2019-12-30 r5022 [x86_64-linux-5.10.47-linuxkit] (local build)
Copyright (C) 2002-19, Bruce Allen, Christian Franke, www.smartmontools.org

Segmentation fault
/ # cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.12.3
PRETTY_NAME="Alpine Linux v3.12"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://bugs.alpinelinux.org/"

Probably we do not have a lot of such reports because alpine is mostly popular in containers and not bone servers.

[Christian Franke]

I could not reproduce the problem with a quick stand-alone test of musl-1.2.5/src/misc/getopt_long.c. It sets optind = 2 on missing arguments as expected.

Which musl version is used on the affected systems?

[Alex Samorukov]

Some data from gdb with smartctl -l

(gdb) print optind
$37 = 3
(gdb) print argc
$38 = 2
(gdb) print argv[0]
$39 = 0x7fff95e8cf12 "/smartmontools-7.5/smartctl"
(gdb) print argv[1]
$40 = 0x7fff95e8cf2e "-l"
(gdb) print argv[2]
$41 = 0x0

I think we can change the logic to check if optind > argc and use argc instead, but we need to check if it works correctly.

[Alex Samorukov]

I was trying different versions (using docker and run alpine), now it is:

/smartmontools-7.5 # apk list --installed|grep -i musl
musl-dbg-1.2.2-r9 x86_64 {musl} (MIT) [installed]
musl-1.2.2-r9 x86_64 {musl} (MIT) [installed]
musl-utils-1.2.2-r9 x86_64 {musl} (MIT BSD GPL2+) [installed]
musl-dev-1.2.2-r9 x86_64 {musl} (MIT) [installed]

[Alex Samorukov]

Also issue confirmed with alpine:edge which uses musl-1.2.5

/ # smartctl -l
smartctl 7.4 2023-08-01 r5530 [x86_64-linux-5.10.47-linuxkit] (local build)
Copyright (C) 2002-23, Bruce Allen, Christian Franke, www.smartmontools.org

Segmentation fault
/ # apk list --installed|grep musl
musl-1.2.5-r0 x86_64 {musl} (MIT) [installed]
musl-utils-1.2.5-r0 x86_64 {musl} (MIT AND BSD-2-Clause AND GPL-2.0-or-later) [installed]

[Christian Franke]

I could not reproduce the problem ...

I take that back as I missed that getopt_long() calls getopt() for short options. This actually returns optind = argc + 1 if the argument of the last option is missing.

This is IMO a bug as optind should never point behind the argv[argc+1] array.

[Alex Samorukov]

Posix spec is not very strict about it, it tells that getopt should update optind after option is processed, however, there is no promise that it should be pointed to valid arg. So i think if we will add logic min(optind, argc) it will not make any issues.

[Alex Samorukov]

​https://github.com/JuliaLang/julia/pull/31946 looks like a very similar issue from another project

[Alex Samorukov]

ah, yes

If the option was the last character in the string pointed to by an element of argv, then optarg shall contain the next element of argv, and optind shall be incremented by 2. If the resulting value of optind is greater than argc, this indicates a missing option-argument, and getopt() shall return an error indication.

[Alex Samorukov]

probably something like that should always work:

arg = optind > argc ? argv[argc-1] : argv[optind-1];

[Christian Franke]

Proposed fix for smartctl.cpp and smartd.cpp:

-      arg = argv[optind-1];
+      arg = argv[optind <= argc ? optind - 1 : argc - 1];

[Alex Samorukov]

lgtm :)

[Christian Franke]

r5613.