On 31/08/2022 00:22, Pádraig Brady wrote:
* src/stty.c (apply_settings): Validate [io]speed arguments
against the internal accepted set.
(set_speed): Check the cfset[io]speed() return value so
that we validate against the system supported set.
When testing this I noticed that errors would be
printed on Linux on the first change of [io]speed,
but not on subsequent invocations.
I added the ---debug option to help investigate the
difference between returned and expected mode bits.
That led me to the conclusion that using memcmp()
across the whole mode structure is a layering violation,
which was already seen to fail on Solaris when setting speeds,
and seen to fail on Linux also.
Instead the attached patch uses documented interfaces
(already used anyway in stty), to inspect the returned mode.
This also simplifies the code as we don't need to
handle edge cases.
This was independently identified as an issue in
https://bugs.debian.org/1019468
cheers,
Pádraig
From 760998a7898bbf21208e127a5250108035dc0dd7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Sun, 11 Sep 2022 15:37:35 +0100
Subject: [PATCH] stty: fix false warnings from [io]speed settings
* src/stty.c (eq_mode): A new function to compare
equivalence of two modes.
(main): Use eq_mode() rather than memcmp() to compare
two modes. Also use stack variables rather than implicitly
initialized static variables. Also remove all uses of
the SPEED_WAS_SET hack since we now more robustly compare modes.
* NEWS: Update the [io]speed fix entry.
Reported at https://bugs.debian.org/1019468
---
NEWS | 5 ++--
src/stty.c | 87 ++++++++++++++++++++++--------------------------------
2 files changed, 39 insertions(+), 53 deletions(-)
diff --git a/NEWS b/NEWS
index db5150824..d1c400e67 100644
--- a/NEWS
+++ b/NEWS
@@ -27,8 +27,9 @@ GNU coreutils NEWS -*- outline -*-
[bug introduced 1999-05-02 and only partly fixed in coreutils-8.14]
stty ispeed and ospeed options no longer accept and silently ignore
- invalid speed arguments. Now they're validated against both the
- general accepted set, and the system supported set of valid speeds.
+ invalid speed arguments, or give false warnings for valid speeds.
+ Now they're validated against both the general accepted set,
+ and the system supported set of valid speeds.
[This bug was present in "the beginning".]
** Changes in behavior
diff --git a/src/stty.c b/src/stty.c
index def09f09d..e93d92ffa 100644
--- a/src/stty.c
+++ b/src/stty.c
@@ -443,6 +443,7 @@ static bool recover_mode (char const *arg, struct termios *mode);
static int screen_columns (void);
static bool set_mode (struct mode_info const *info, bool reversed,
struct termios *mode);
+static bool eq_mode (struct termios *mode1, struct termios *mode2);
static unsigned long int integer_arg (char const *s, unsigned long int max);
static speed_t string_to_baud (char const *arg);
static tcflag_t *mode_type_flag (enum mode_type type, struct termios *mode);
@@ -1087,16 +1088,14 @@ settings, CHAR is taken literally, or coded as in ^c, 0x37, 0177 or\n\
}
-/* Apply specified settings to MODE, and update
- SPEED_WAS_SET and REQUIRE_SET_ATTR as required.
+/* Apply specified settings to MODE and REQUIRE_SET_ATTR as required.
If CHECKING is true, this function doesn't interact
with a device, and only validates specified settings. */
static void
apply_settings (bool checking, char const *device_name,
char * const *settings, int n_settings,
- struct termios *mode, bool *speed_was_set,
- bool *require_set_attr)
+ struct termios *mode, bool *require_set_attr)
{
#define check_argument(arg) \
do \
@@ -1178,7 +1177,6 @@ apply_settings (bool checking, char const *device_name,
if (checking)
continue;
set_speed (input_speed, settings[k], mode);
- *speed_was_set = true;
*require_set_attr = true;
}
else if (STREQ (arg, "ospeed"))
@@ -1193,7 +1191,6 @@ apply_settings (bool checking, char const *device_name,
if (checking)
continue;
set_speed (output_speed, settings[k], mode);
- *speed_was_set = true;
*require_set_attr = true;
}
#ifdef TIOCEXT
@@ -1267,7 +1264,6 @@ apply_settings (bool checking, char const *device_name,
if (checking)
continue;
set_speed (both_speeds, arg, mode);
- *speed_was_set = true;
*require_set_attr = true;
}
else
@@ -1286,16 +1282,13 @@ apply_settings (bool checking, char const *device_name,
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;
+ struct termios mode;
enum output_type output_type;
int optc;
int argi = 0;
int opti = 1;
bool require_set_attr;
- MAYBE_UNUSED bool speed_was_set;
bool verbose_output;
bool recoverable_output;
bool noargs = true;
@@ -1394,7 +1387,7 @@ main (int argc, char **argv)
{
static struct termios check_mode;
apply_settings (/* checking= */ true, device_name, argv, argc,
- &check_mode, &speed_was_set, &require_set_attr);
+ &check_mode, &require_set_attr);
}
if (file_name)
@@ -1419,16 +1412,13 @@ main (int argc, char **argv)
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);
+ &mode, &require_set_attr);
if (require_set_attr)
{
- /* 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 new_mode;
+ struct termios new_mode;
if (tcsetattr (STDIN_FILENO, tcsetattr_options, &mode))
die (EXIT_FAILURE, errno, "%s", quotef (device_name));
@@ -1443,51 +1433,46 @@ main (int argc, char **argv)
if (tcgetattr (STDIN_FILENO, &new_mode))
die (EXIT_FAILURE, errno, "%s", quotef (device_name));
- /* Normally, one shouldn't use memcmp to compare structures that
- may have 'holes' containing uninitialized data, but we have been
- careful to initialize the storage of these two variables to all
- zeroes. One might think it more efficient simply to compare the
- modified fields, but that would require enumerating those fields --
- and not all systems have the same fields in this structure. */
-
- if (memcmp (&mode, &new_mode, sizeof (mode)) != 0)
+ if (! eq_mode (&mode, &new_mode))
{
-#ifdef CIBAUD
- /* SunOS 4.1.3 (at least) has the problem that after this sequence,
- tcgetattr (&m1); tcsetattr (&m1); tcgetattr (&m2);
- sometimes (m1 != m2). The only difference is in the four bits
- of the c_cflag field corresponding to the baud rate. To save
- Sun users a little confusion, don't report an error if this
- happens. But suppress the error only if we haven't tried to
- set the baud rate explicitly -- otherwise we'd never give an
- error for a true failure to set the baud rate. */
-
- new_mode.c_cflag &= (~CIBAUD);
- if (speed_was_set || memcmp (&mode, &new_mode, sizeof (mode)) != 0)
-#endif
+ if (dev_debug)
{
- if (dev_debug)
+ error (0, 0, _("indx: mode: actual mode"));
+ for (unsigned int i = 0; i < sizeof (new_mode); i++)
{
- error (0, 0, _("indx: mode: actual mode"));
- for (unsigned int i = 0; i < sizeof (new_mode); i++)
- {
- unsigned int newc = *(((unsigned char *) &new_mode) + i);
- unsigned int oldc = *(((unsigned char *) &mode) + i);
- error (0, 0, "0x%02x, 0x%02x: 0x%02x%s", i, oldc, newc,
- newc == oldc ? "" : " *");
- }
+ unsigned int newc = *(((unsigned char *) &new_mode) + i);
+ unsigned int oldc = *(((unsigned char *) &mode) + i);
+ error (0, 0, "0x%02x, 0x%02x: 0x%02x%s", i, oldc, newc,
+ newc == oldc ? "" : " *");
}
-
- die (EXIT_FAILURE, 0,
- _("%s: unable to perform all requested operations"),
- quotef (device_name));
}
+
+ die (EXIT_FAILURE, 0,
+ _("%s: unable to perform all requested operations"),
+ quotef (device_name));
}
}
return EXIT_SUCCESS;
}
+/* Return true if modes are equivalent. */
+
+static bool
+eq_mode (struct termios *mode1, struct termios *mode2)
+{
+ return mode1->c_iflag == mode2->c_iflag
+ && mode1->c_oflag == mode2->c_oflag
+ && mode1->c_cflag == mode2->c_cflag
+ && mode1->c_lflag == mode2->c_lflag
+#ifdef HAVE_C_LINE
+ && mode1->c_line == mode2->c_line
+#endif
+ && memcmp (mode1->c_cc, mode2->c_cc, sizeof (mode1->c_cc)) == 0
+ && cfgetispeed (mode1) == cfgetispeed (mode2)
+ && cfgetospeed (mode1) == cfgetospeed (mode2);
+}
+
/* Return false if not applied because not reversible; otherwise
return true. */
--
2.26.2
