stty: ensure no side effects from invalid options

* src/stty.c (apply_settings): A new function refactored
from main() that is used to both check and apply options.
(main): Call apply_settings before we open the device,
so all validation is done before interacting with a device.
* NEWS: Mention the improvement.
* tests/misc/stty.sh: Add a test case.

3 files changed, 209 insertions, 162 deletions

diff --git a/NEWS b/NEWS
index 9ee0c58fc..9e0aaf437 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,11 @@ GNU coreutils NEWS                                    -*- outline -*-
   depending on the size of the first file processed.
   [bug introduced in coreutils-8.24]
 
+** Improvements
+
+  stty now validates arguments before interacting with the device,
+  ensuring there are no side effects to specifying an invalid option.
+
 
 * Noteworthy changes in release 8.26 (2016-11-30) [stable]
 
diff --git a/src/stty.c b/src/stty.c
index f8aefff62..4f911a006 100644
--- a/src/stty.c
+++ b/src/stty.c
@@ -1077,143 +1077,21 @@ settings, CHAR is taken literally, or coded as in ^c, 0x37, 0177 or\n\
   exit (status);
 }
 
-int
-main (int argc, char **argv)
-{
-  /* Initialize to all zeroes so there is no risk memcmp will report a
-     spurious difference in an uninitialized portion of the structure.  */
-  static struct termios mode;
-
-  enum output_type output_type;
-  int optc;
-  int argi = 0;
-  int opti = 1;
-  bool require_set_attr;
-  bool speed_was_set _GL_UNUSED;
-  bool verbose_output;
-  bool recoverable_output;
-  int k;
-  bool noargs = true;
-  char *file_name = NULL;
-  const char *device_name;
-
-  initialize_main (&argc, &argv);
-  set_program_name (argv[0]);
-  setlocale (LC_ALL, "");
-  bindtextdomain (PACKAGE, LOCALEDIR);
-  textdomain (PACKAGE);
-
-  atexit (close_stdout);
-
-  output_type = changed;
-  verbose_output = false;
-  recoverable_output = false;
-
-  /* Don't print error messages for unrecognized options.  */
-  opterr = 0;
-
-  /* If any new options are ever added to stty, the short options MUST
-     NOT allow any ambiguity with the stty settings.  For example, the
-     stty setting "-gagFork" would not be feasible, since it will be
-     parsed as "-g -a -g -F ork".  If you change anything about how
-     stty parses options, be sure it still works with combinations of
-     short and long options, --, POSIXLY_CORRECT, etc.  */
-
-  while ((optc = getopt_long (argc - argi, argv + argi, "-agF:",
-                              longopts, NULL))
-         != -1)
-    {
-      switch (optc)
-        {
-        case 'a':
-          verbose_output = true;
-          output_type = all;
-          break;
-
-        case 'g':
-          recoverable_output = true;
-          output_type = recoverable;
-          break;
-
-        case 'F':
-          if (file_name)
-            die (EXIT_FAILURE, 0, _("only one device may be specified"));
-          file_name = optarg;
-          break;
-
-        case_GETOPT_HELP_CHAR;
-
-        case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
 
-        default:
-          /* Consider "drain" as an option rather than a setting,
-             to support: alias stty='stty -drain'  etc.  */
-          if (! STREQ (argv[argi + opti], "-drain")
-              && ! STREQ (argv[argi + opti], "drain"))
-            noargs = false;
+/* Apply specified settings to MODE, and update
+   SPEED_WAS_SET and REQUIRE_SET_ATTR as required.
+   If CHECKING is true, this function doesn't interact
+   with a device, and only validates specified settings.  */
 
-          /* Skip the argument containing this unrecognized option;
-             the 2nd pass will analyze it.  */
-          argi += opti;
-
-          /* Restart getopt_long from the first unskipped argument.  */
-          opti = 1;
-          optind = 0;
-
-          break;
-        }
-
-      /* Clear fully-parsed arguments, so they don't confuse the 2nd pass.  */
-      while (opti < optind)
-        argv[argi + opti++] = NULL;
-    }
-
-  /* Specifying both -a and -g gets an error.  */
-  if (verbose_output && recoverable_output)
-    die (EXIT_FAILURE, 0,
-         _("the options for verbose and stty-readable output styles are\n"
-           "mutually exclusive"));
-
-  /* Specifying any other arguments with -a or -g gets an error.  */
-  if (!noargs && (verbose_output || recoverable_output))
-    die (EXIT_FAILURE, 0,
-         _("when specifying an output style, modes may not be set"));
-
-  /* FIXME: it'd be better not to open the file until we've verified
-     that all arguments are valid.  Otherwise, we could end up doing
-     only some of the requested operations and then failing, probably
-     leaving things in an undesirable state.  */
-
-  if (file_name)
-    {
-      int fdflags;
-      device_name = file_name;
-      if (fd_reopen (STDIN_FILENO, device_name, O_RDONLY | O_NONBLOCK, 0) < 0)
-        die (EXIT_FAILURE, errno, "%s", quotef (device_name));
-      if ((fdflags = fcntl (STDIN_FILENO, F_GETFL)) == -1
-          || fcntl (STDIN_FILENO, F_SETFL, fdflags & ~O_NONBLOCK) < 0)
-        die (EXIT_FAILURE, errno, _("%s: couldn't reset non-blocking mode"),
-             quotef (device_name));
-    }
-  else
-    device_name = _("standard input");
-
-  if (tcgetattr (STDIN_FILENO, &mode))
-    die (EXIT_FAILURE, errno, "%s", quotef (device_name));
-
-  if (verbose_output || recoverable_output || noargs)
-    {
-      max_col = screen_columns ();
-      current_col = 0;
-      display_settings (output_type, &mode, device_name);
-      return EXIT_SUCCESS;
-    }
-
-  speed_was_set = false;
-  require_set_attr = false;
-  for (k = 1; k < argc; k++)
+static void
+apply_settings (bool checking, const char *device_name,
+                char * const *settings, int n_settings,
+                struct termios *mode, bool *speed_was_set,
+                bool *require_set_attr)
+{
+  for (int k = 1; k < n_settings; k++)
     {
-      char const *arg = argv[k];
+      char const *arg = settings[k];
       bool match_found = false;
       bool not_set_attr = false;
       bool reversed = false;
@@ -1238,8 +1116,8 @@ main (int argc, char **argv)
             {
               if ((mode_info[i].flags & NO_SETATTR) == 0)
                 {
-                  match_found = set_mode (&mode_info[i], reversed, &mode);
-                  require_set_attr = true;
+                  match_found = set_mode (&mode_info[i], reversed, mode);
+                  *require_set_attr = true;
                 }
               else
                 match_found = not_set_attr = true;
@@ -1257,15 +1135,15 @@ main (int argc, char **argv)
             {
               if (STREQ (arg, control_info[i].name))
                 {
-                  if (k == argc - 1)
+                  if (k == n_settings - 1)
                     {
                       error (0, 0, _("missing argument to %s"), quote (arg));
                       usage (EXIT_FAILURE);
                     }
                   match_found = true;
                   ++k;
-                  set_control_char (&control_info[i], argv[k], &mode);
-                  require_set_attr = true;
+                  set_control_char (&control_info[i], settings[k], mode);
+                  *require_set_attr = true;
                   break;
                 }
             }
@@ -1274,27 +1152,31 @@ main (int argc, char **argv)
         {
           if (STREQ (arg, "ispeed"))
             {
-              if (k == argc - 1)
+              if (k == n_settings - 1)
                 {
                   error (0, 0, _("missing argument to %s"), quote (arg));
                   usage (EXIT_FAILURE);
                 }
               ++k;
-              set_speed (input_speed, argv[k], &mode);
-              speed_was_set = true;
-              require_set_attr = true;
+              if (checking)
+                continue;
+              set_speed (input_speed, settings[k], mode);
+              *speed_was_set = true;
+              *require_set_attr = true;
             }
           else if (STREQ (arg, "ospeed"))
             {
-              if (k == argc - 1)
+              if (k == n_settings - 1)
                 {
                   error (0, 0, _("missing argument to %s"), quote (arg));
                   usage (EXIT_FAILURE);
                 }
               ++k;
-              set_speed (output_speed, argv[k], &mode);
-              speed_was_set = true;
-              require_set_attr = true;
+              if (checking)
+                continue;
+              set_speed (output_speed, settings[k], mode);
+              *speed_was_set = true;
+              *require_set_attr = true;
             }
 #ifdef TIOCEXT
           /* This is the BSD interface to "extproc".
@@ -1303,6 +1185,9 @@ main (int argc, char **argv)
             {
               int val = ! reversed;
 
+              if (checking)
+                continue;
+
               if (ioctl (STDIN_FILENO, TIOCEXT, &val) != 0)
                 {
                   die (EXIT_FAILURE, errno, _("%s: error setting %s"),
@@ -1313,29 +1198,35 @@ main (int argc, char **argv)
 #ifdef TIOCGWINSZ
           else if (STREQ (arg, "rows"))
             {
-              if (k == argc - 1)
+              if (k == n_settings - 1)
                 {
                   error (0, 0, _("missing argument to %s"), quote (arg));
                   usage (EXIT_FAILURE);
                 }
               ++k;
-              set_window_size (integer_arg (argv[k], INT_MAX), -1,
+              if (checking)
+                continue;
+              set_window_size (integer_arg (settings[k], INT_MAX), -1,
                                device_name);
             }
           else if (STREQ (arg, "cols")
                    || STREQ (arg, "columns"))
             {
-              if (k == argc - 1)
+              if (k == n_settings - 1)
                 {
                   error (0, 0, _("missing argument to %s"), quote (arg));
                   usage (EXIT_FAILURE);
                 }
               ++k;
-              set_window_size (-1, integer_arg (argv[k], INT_MAX),
+              if (checking)
+                continue;
+              set_window_size (-1, integer_arg (settings[k], INT_MAX),
                                device_name);
             }
           else if (STREQ (arg, "size"))
             {
+              if (checking)
+                continue;
               max_col = screen_columns ();
               current_col = 0;
               display_window_size (false, device_name);
@@ -1345,40 +1236,183 @@ main (int argc, char **argv)
           else if (STREQ (arg, "line"))
             {
               unsigned long int value;
-              if (k == argc - 1)
+              if (k == n_settings - 1)
                 {
                   error (0, 0, _("missing argument to %s"), quote (arg));
                   usage (EXIT_FAILURE);
                 }
               ++k;
-              mode.c_line = value = integer_arg (argv[k], ULONG_MAX);
-              if (mode.c_line != value)
-                error (0, 0, _("invalid line discipline %s"), quote (argv[k]));
-              require_set_attr = true;
+              mode->c_line = value = integer_arg (settings[k], ULONG_MAX);
+              if (mode->c_line != value)
+                error (0, 0, _("invalid line discipline %s"),
+                       quote (settings[k]));
+              *require_set_attr = true;
             }
 #endif
           else if (STREQ (arg, "speed"))
             {
+              if (checking)
+                continue;
               max_col = screen_columns ();
-              display_speed (&mode, false);
+              display_speed (mode, false);
             }
           else if (string_to_baud (arg) != (speed_t) -1)
             {
-              set_speed (both_speeds, arg, &mode);
-              speed_was_set = true;
-              require_set_attr = true;
+              if (checking)
+                continue;
+              set_speed (both_speeds, arg, mode);
+              *speed_was_set = true;
+              *require_set_attr = true;
             }
           else
             {
-              if (! recover_mode (arg, &mode))
+              if (! recover_mode (arg, mode))
                 {
                   error (0, 0, _("invalid argument %s"), quote (arg));
                   usage (EXIT_FAILURE);
                 }
-              require_set_attr = true;
+              *require_set_attr = true;
             }
         }
     }
+}
+
+int
+main (int argc, char **argv)
+{
+  /* Initialize to all zeroes so there is no risk memcmp will report a
+     spurious difference in an uninitialized portion of the structure.  */
+  static struct termios mode;
+
+  enum output_type output_type;
+  int optc;
+  int argi = 0;
+  int opti = 1;
+  bool require_set_attr;
+  bool speed_was_set _GL_UNUSED;
+  bool verbose_output;
+  bool recoverable_output;
+  bool noargs = true;
+  char *file_name = NULL;
+  const char *device_name;
+
+  initialize_main (&argc, &argv);
+  set_program_name (argv[0]);
+  setlocale (LC_ALL, "");
+  bindtextdomain (PACKAGE, LOCALEDIR);
+  textdomain (PACKAGE);
+
+  atexit (close_stdout);
+
+  output_type = changed;
+  verbose_output = false;
+  recoverable_output = false;
+
+  /* Don't print error messages for unrecognized options.  */
+  opterr = 0;
+
+  /* If any new options are ever added to stty, the short options MUST
+     NOT allow any ambiguity with the stty settings.  For example, the
+     stty setting "-gagFork" would not be feasible, since it will be
+     parsed as "-g -a -g -F ork".  If you change anything about how
+     stty parses options, be sure it still works with combinations of
+     short and long options, --, POSIXLY_CORRECT, etc.  */
+
+  while ((optc = getopt_long (argc - argi, argv + argi, "-agF:",
+                              longopts, NULL))
+         != -1)
+    {
+      switch (optc)
+        {
+        case 'a':
+          verbose_output = true;
+          output_type = all;
+          break;
+
+        case 'g':
+          recoverable_output = true;
+          output_type = recoverable;
+          break;
+
+        case 'F':
+          if (file_name)
+            die (EXIT_FAILURE, 0, _("only one device may be specified"));
+          file_name = optarg;
+          break;
+
+        case_GETOPT_HELP_CHAR;
+
+        case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
+
+        default:
+          /* Consider "drain" as an option rather than a setting,
+             to support: alias stty='stty -drain'  etc.  */
+          if (! STREQ (argv[argi + opti], "-drain")
+              && ! STREQ (argv[argi + opti], "drain"))
+            noargs = false;
+
+          /* Skip the argument containing this unrecognized option;
+             the 2nd pass will analyze it.  */
+          argi += opti;
+
+          /* Restart getopt_long from the first unskipped argument.  */
+          opti = 1;
+          optind = 0;
+
+          break;
+        }
+
+      /* Clear fully-parsed arguments, so they don't confuse the 2nd pass.  */
+      while (opti < optind)
+        argv[argi + opti++] = NULL;
+    }
+
+  /* Specifying both -a and -g gets an error.  */
+  if (verbose_output && recoverable_output)
+    die (EXIT_FAILURE, 0,
+         _("the options for verbose and stty-readable output styles are\n"
+           "mutually exclusive"));
+
+  /* Specifying any other arguments with -a or -g gets an error.  */
+  if (!noargs && (verbose_output || recoverable_output))
+    die (EXIT_FAILURE, 0,
+         _("when specifying an output style, modes may not be set"));
+
+  device_name = file_name ? file_name : _("standard input");
+
+  if (!noargs && !verbose_output && !recoverable_output)
+    {
+      static struct termios check_mode;
+      apply_settings (/* checking= */ true, device_name, argv, argc,
+                      &check_mode, &speed_was_set, &require_set_attr);
+    }
+
+  if (file_name)
+    {
+      int fdflags;
+      if (fd_reopen (STDIN_FILENO, device_name, O_RDONLY | O_NONBLOCK, 0) < 0)
+        die (EXIT_FAILURE, errno, "%s", quotef (device_name));
+      if ((fdflags = fcntl (STDIN_FILENO, F_GETFL)) == -1
+          || fcntl (STDIN_FILENO, F_SETFL, fdflags & ~O_NONBLOCK) < 0)
+        die (EXIT_FAILURE, errno, _("%s: couldn't reset non-blocking mode"),
+             quotef (device_name));
+    }
+
+  if (tcgetattr (STDIN_FILENO, &mode))
+    die (EXIT_FAILURE, errno, "%s", quotef (device_name));
+
+  if (verbose_output || recoverable_output || noargs)
+    {
+      max_col = screen_columns ();
+      current_col = 0;
+      display_settings (output_type, &mode, device_name);
+      return EXIT_SUCCESS;
+    }
+
+  speed_was_set = false;
+  require_set_attr = false;
+  apply_settings (/* checking= */ false, device_name, argv, argc,
+                  &mode, &speed_was_set, &require_set_attr);
 
   if (require_set_attr)
     {
diff --git a/tests/misc/stty.sh b/tests/misc/stty.sh
index f6200e715..e549adb65 100755
--- a/tests/misc/stty.sh
+++ b/tests/misc/stty.sh
@@ -22,6 +22,7 @@ print_ver_ stty
 
 require_controlling_input_terminal_
 require_trap_signame_
+require_strace_ ioctl
 
 trap '' TTOU # Ignore SIGTTOU
 
@@ -81,4 +82,11 @@ done
 
 stty $(cat $saved_state)
 
+# Ensure we validate options before accessing the device
+strace -o log1 -e ioctl stty --version || fail=1
+n_ioctl1=$(wc -l < log1) || framework_failure_
+returns_ 1 strace -o log2 -e ioctl stty -blahblah || fail=1
+n_ioctl2=$(wc -l < log2) || framework_failure_
+test "$n_ioctl1" = "$n_ioctl2" || fail=1
+
 Exit $fail