Subject: Re: bug#28859: Segmentation fault with NULL pointer dereference in 'stty'

From: Pádraig Brady <[email-masked]>
Date: Tue, 17 Oct 2017 00:37:10 -0700

On 16/10/17 10:49, Jim Meyering wrote:
> On Mon, Oct 16, 2017 at 2:30 AM, Pádraig Brady <[email-masked]> wrote:
>> On 15/10/17 18:07, Jaeseung Choi wrote:
>>> Dear GNU team,
>>>
>>> While testing coreutils for a research purpose, we found the following
>>> crash in 'stty'. Running stty with the command-line "stty eol -F AA"
>>> raises a crash as below. We did not change any terminal setting, and
>>> believe the bug is irrelevant from any specific terminal
>>> configuration.
>>>
>>> jason@ubuntu:~$ tar -xf coreutils-8.28.tar.xz
>>> jason@ubuntu:~$ cd coreutils-8.28/
>>> jason@ubuntu:~/coreutils-8.28$ mkdir obj
>>> jason@ubuntu:~/coreutils-8.28$ cd obj
>>> jason@ubuntu:~/coreutils-8.28/obj$ ../configure --disable-nls && make
>>> ...
>>> jason@ubuntu:~/coreutils-8.28/obj$ gdb ./src/stty -q
>>> Reading symbols from ./src/stty...done.
>>> (gdb) run eol -F AA
>>> Starting program: /home/jason/coreutils-8.28/obj/src/stty eol -F AA
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> set_control_char (info=0x40a6f8 <control_info+120>, info=0x40a6f8
>>> <control_info+120>, mode=0x6103c0 <check_mode>, arg=0x0) at
>>> ../src/stty.c:1695
>>> 1695      else if (arg[0] == '\0' || arg[1] == '\0')
>>> (gdb) x/i $rip
>>> => 0x40387a <apply_settings+746>:       movzbl (%rbx),%r14d
>>> (gdb) info reg rbx
>>> rbx            0x0      0
>>> (gdb)
>>>
>>> We could reproduce the bug in coreutils from version 8.27 to 8.28.
>>> Also, the bug was reproducible in both Ubuntu 16.04 and Debian 9.1.
>>> But the stty program pre-built in Debian 9.1 did not crash because
>>> currently 8.26 version is installed in Debian.
>>
>> This is actually an old bug which you can reproduce with -F /dev/tty.
>> The attached should fix it up.
> 
> Thank you!
> If it's not too hard to determine, would you please mention in the log
> the commit that introduced the bug?

Updated patch attached. I mistakenly thought getopt would
permute the argv so NULLs were at the end.  The attached
caters for NULLs interspersed in the argv[].

cheers,
Pádraig.

>From 9402e1a21fa4e6939fb978e41efabf532d344c66 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Mon, 16 Oct 2017 02:17:34 -0700
Subject: [PATCH] stty: fix processing of options when -F is specified

This was a latent issue that became significant with
the addition of the -F option in FILEUTILS-3_16n-56-ge46a424

* src/stty.c (apply_settings): Refactor argument checking
to a function macro.  Augment the argument check to ignore
NULLed out arguments (already processed -F).
* NEWS: Mention the fix.
* tests/misc/stty-invalid.sh: Add a test case.
Fixes https://bugs.gnu.org/28859
---
 NEWS                       |  3 +++
 src/stty.c                 | 43 +++++++++++++------------------------------
 tests/misc/stty-invalid.sh |  9 +++++++++
 3 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/NEWS b/NEWS
index 2878b70..9b0dc6c 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   to attempt to hide the original length of the file name.
   [bug introduced in coreutils-8.28]
 
+  stty no longer crashes when processing settings with -F also specified.
+  [bug introduced in fileutils-4.0]
+
 ** Build-related
 
   Default man pages are now distributed which are used if perl is
diff --git a/src/stty.c b/src/stty.c
index 48aac59..1a5c1e9 100644
--- a/src/stty.c
+++ b/src/stty.c
@@ -1089,6 +1089,13 @@ apply_settings (bool checking, const char *device_name,
                 struct termios *mode, bool *speed_was_set,
                 bool *require_set_attr)
 {
+#define check_argument(arg) \
+  if (k == n_settings - 1 || ! settings[k+1]) \
+    { \
+      error (0, 0, _("missing argument to %s"), quote (arg)); \
+      usage (EXIT_FAILURE); \
+    }
+
   for (int k = 1; k < n_settings; k++)
     {
       char const *arg = settings[k];
@@ -1135,11 +1142,7 @@ apply_settings (bool checking, const char *device_name,
             {
               if (STREQ (arg, control_info[i].name))
                 {
-                  if (k == n_settings - 1)
-                    {
-                      error (0, 0, _("missing argument to %s"), quote (arg));
-                      usage (EXIT_FAILURE);
-                    }
+                  check_argument (arg);
                   match_found = true;
                   ++k;
                   set_control_char (&control_info[i], settings[k], mode);
@@ -1152,11 +1155,7 @@ apply_settings (bool checking, const char *device_name,
         {
           if (STREQ (arg, "ispeed"))
             {
-              if (k == n_settings - 1)
-                {
-                  error (0, 0, _("missing argument to %s"), quote (arg));
-                  usage (EXIT_FAILURE);
-                }
+              check_argument (arg);
               ++k;
               if (checking)
                 continue;
@@ -1166,11 +1165,7 @@ apply_settings (bool checking, const char *device_name,
             }
           else if (STREQ (arg, "ospeed"))
             {
-              if (k == n_settings - 1)
-                {
-                  error (0, 0, _("missing argument to %s"), quote (arg));
-                  usage (EXIT_FAILURE);
-                }
+              check_argument (arg);
               ++k;
               if (checking)
                 continue;
@@ -1198,11 +1193,7 @@ apply_settings (bool checking, const char *device_name,
 #ifdef TIOCGWINSZ
           else if (STREQ (arg, "rows"))
             {
-              if (k == n_settings - 1)
-                {
-                  error (0, 0, _("missing argument to %s"), quote (arg));
-                  usage (EXIT_FAILURE);
-                }
+              check_argument (arg);
               ++k;
               if (checking)
                 continue;
@@ -1212,11 +1203,7 @@ apply_settings (bool checking, const char *device_name,
           else if (STREQ (arg, "cols")
                    || STREQ (arg, "columns"))
             {
-              if (k == n_settings - 1)
-                {
-                  error (0, 0, _("missing argument to %s"), quote (arg));
-                  usage (EXIT_FAILURE);
-                }
+              check_argument (arg);
               ++k;
               if (checking)
                 continue;
@@ -1236,11 +1223,7 @@ apply_settings (bool checking, const char *device_name,
           else if (STREQ (arg, "line"))
             {
               unsigned long int value;
-              if (k == n_settings - 1)
-                {
-                  error (0, 0, _("missing argument to %s"), quote (arg));
-                  usage (EXIT_FAILURE);
-                }
+              check_argument (arg);
               ++k;
               mode->c_line = value = integer_arg (settings[k], ULONG_MAX);
               if (mode->c_line != value)
diff --git a/tests/misc/stty-invalid.sh b/tests/misc/stty-invalid.sh
index 06186e9..5509b65 100755
--- a/tests/misc/stty-invalid.sh
+++ b/tests/misc/stty-invalid.sh
@@ -41,6 +41,15 @@ returns_ 1 stty $(echo $saved_state |sed 's/^[^:]*:/'$hex_2_64:/) \
 returns_ 1 stty $(echo $saved_state |sed 's/:[0-9a-f]*$/:'$hex_2_64/) \
   2>/dev/null || fail=1
 
+# From coreutils 5.3.0 to 8.28, the following would crash
+# due to incorrect argument handling.
+if tty -s </dev/tty; then
+  returns_ 1 stty eol -F /dev/tty || fail=1
+  returns_ 1 stty -F /dev/tty eol || fail=1
+  returns_ 1 stty -F/dev/tty eol || fail=1
+  returns_ 1 stty eol -F/dev/tty eol || fail=1
+fi
+
 # Just in case either of the above mistakenly succeeds (and changes
 # the state of our tty), try to restore the initial state.
 stty $saved_state || fail=1
-- 
2.9.3