On 16/10/17 10:49, Jim Meyering wrote: > On Mon, Oct 16, 2017 at 2:30 AM, Pádraig Brady <p...@draigbrady.com> 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?= <p...@draigbrady.com> 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
