Subject: bug#25398: closed (Re: bug#25398: stty: bug or feature?)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 09 Jan 2017 00:33:02 +0000

Your bug report

#25398: stty: bug or feature?

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
25398: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=25398
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#25398: stty: bug or feature?
From: Pádraig Brady <[email-masked]>
To: Dave B <[email-masked]>, [email-masked]
Date: Mon, 9 Jan 2017 00:31:59 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 08/01/17 20:08, Pádraig Brady wrote:
> On 08/01/17 18:14, Dave B wrote:
>> Would it be possible "one day" for stty to parse the command line args,
>> and only open the port when all the specified parameters are actually in
>> force?
> 
> That would be better. It's even mentioned as a FIXME:
> https://github.com/coreutils/coreutils/blob/229431d/src/stty.c#L1182-L1185

Attached patch does that.

cheers,
Pádraig

>From 9c0a3a27f70bbb27e839404571922b0f8f0d48da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Mon, 9 Jan 2017 00:07:42 +0000
Subject: [PATCH] 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.
---
 NEWS               |   5 +
 src/stty.c         | 358 +++++++++++++++++++++++++++++------------------------
 tests/misc/stty.sh |   8 ++
 3 files changed, 209 insertions(+), 162 deletions(-)

diff --git a/NEWS b/NEWS
index 9ee0c58..9e0aaf4 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 f8aefff..4f911a0 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 f6200e7..e549adb 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
-- 
2.5.5

Subject: stty: bug or feature?
From: Dave B <[email-masked]>
To: [email-masked]
Date: Sun, 8 Jan 2017 18:14:48 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi.

While arranging to automate the startup and shutdown of my ham radio
station, for safe remote control use, attempting to do as much as
possible with native Linux commands and tools, I found stty has a rather
annoying "feature", or even a bug..

This is on Linux Mint, 17.2 with the Cinamon desktop environment.  Mint
18.x seems to have more serious issues, but I'm not sure if it's the OS,
PC or serial hardware on that system, hence reverting to a 17.x system.

Anyway.

After some hours experimenting, and a lot of Googling, I can see that
many others have come across the same issue, in respect to using Linux
to program and control Arduino devices, where more often than not, the
devices are reset at inapropriate times.

This is all related to the way a serial port DTR control line is
handled.   Arduino's use it as a reset line, my radio uses it as a
"Transmit" command line.

In essence..

a bash script in the form...

#!/bin/bash

stty -F /dev/ttyUSB0 57600 -hupcl crtscts cs8 -cstopb -parity
echo -n 'EX0270001;' > /dev/ttyUSB0
sleep 1s
echo -n 'PS0;' > /dev/ttyUSB0

Would once the port is open, send the command to prevent the radio from
going into transmit, wait for 1s for that to process, then send the
command to power off.   This is does..

But...

Other software that is used to do the real remote control work, PRIOR to
me wanting to shut it all down with that script, obviously sets the
serial port so that the DTR line can be used...

So, when the above script is run, DTR is immediately asserted, DESPITE
"-hupcl" being specified.  (If I try using -cdtrdsr that results
in:-     stty: invalid argument ‘-cdtrdsr’  )

Would it be possible "one day" for stty to parse the command line args,
and only open the port when all the specified parameters are actualy in
force?

Plus, is the cdtrdsr parameter actually allowed to b negated, as
specified in the man pages.

I and many have found, that if you "do something" with the port,
specifying -hucpl, as the system boots, though at that time DTR is
briefly asserted, for subsequent invocations it is not.   Unless, a
third party program or tool sets it to be used again.

Regards.

Dave Baxter.