Opened 3 months ago

Closed 3 months ago

#1830 closed defect (fixed)

error message for missing argument segfaults (musl libc)

Reported by: ncopa Owned by: Christian Franke
Priority: major Milestone: Release 7.5
Component: all Version:
Keywords: Cc:

Description

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

Change History (16)

comment:1 by ncopa, 3 months ago

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.

comment:2 by Christian Franke, 3 months ago

Milestone: undecided
Priority: minormajor

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.

comment:3 by Christian Franke, 3 months ago

Summary: error message for missing argument segfaultserror message for missing argument segfaults (musl libc)

comment:4 by Alex Samorukov, 3 months ago

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.

comment:5 by Christian Franke, 3 months ago

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?

Last edited 3 months ago by Christian Franke (previous) (diff)

comment:6 by Alex Samorukov, 3 months ago

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.

comment:7 by Alex Samorukov, 3 months ago

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]

comment:8 by Alex Samorukov, 3 months ago

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]

comment:9 by Christian Franke, 3 months ago

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.

Last edited 3 months ago by Christian Franke (previous) (diff)

comment:10 by Alex Samorukov, 3 months ago

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.

Last edited 3 months ago by Alex Samorukov (previous) (diff)

comment:11 by Alex Samorukov, 3 months ago

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

comment:12 by Alex Samorukov, 3 months ago

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.

Last edited 3 months ago by Alex Samorukov (previous) (diff)

comment:13 by Alex Samorukov, 3 months ago

probably something like that should always work:

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

P.S. same apply for smartd, with a fix above working the same.

Last edited 3 months ago by Alex Samorukov (previous) (diff)

comment:14 by Christian Franke, 3 months ago

Milestone: undecidedRelease 7.5
Owner: set to Christian Franke
Status: newaccepted

Proposed fix for smartctl.cpp and smartd.cpp:

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

comment:15 by Alex Samorukov, 3 months ago

lgtm :)

comment:16 by Christian Franke, 3 months ago

Resolution: fixed
Status: acceptedclosed
Note: See TracTickets for help on using tickets.