stty.c wrongly parse first argument as c_iflags

[malon123]

On Android 9, if I type
stty -F /dev/ttyS0 -echo
the new.c_iflag would become 0xffffff14
then I type
stty -F /dev/ttyS0
the new.c_iflag became normal as it should be.

after digging a while with my colleague
he found that in stty_main(),
else if (sscanf(arg, "%x:%x:%x:%x:%n", &new.c_iflag, &new.c_oflag,
&new.c_cflag, &new.c_lflag, &n) == 4)
should be responsible for this behavior.

no matter what argument is entered, new_c_iflag will be changed if the first letters in the argument is hexadecimal characters and/or the negative sign.

i understand this line of code is for convenience but it would break the functionality of stty.

[enh-google]

i'm not certain exactly what you're trying to report (it's always helpful to include "what i did", "what i expected to see", and "what i actually saw" in a bug report), but i think you're talking about the bug fixed by 9c72f31 ?

[E5ten]

@enh-google Actually I think they're talking about a different bug, that was worsened by that commit (because the sscanf happens earlier), because sscanf would modify new.c_iflag if the first character matches "%x", and then if the rest didn't match, the condition would fail and it would move on, but with a now incorrect new.c_iflag. I think the fix here would be using temporary variables for the sscanf, and if it succeeds entirely, setting the relevant members of new to them.

[enh-google]

ah, right, so the example was meant to be stty 300 or somesuch.

yeah, that's broken as they describe (with c_iflags getting mangled to 0x300):

~/toybox-stty$ strace -v -e ioctl ./toybox stty -F /dev/tty 300 ; stty sane
ioctl(3, TCGETS, {c_iflags=0x7fffa516, c_oflags=0x5, c_cflags=0xbf, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, TCGETS, {c_iflags=0x7fffa516, c_oflags=0x5, c_cflags=0xbf, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, SNDCTL_TMR_CONTINUE or TCSETSF, {c_iflags=0x300, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, TCGETS, {c_iflags=0x300, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, TCGETS, {c_iflags=0x300, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
+++ exited with 0 +++
~/toybox-stty$ make

this patch (which i'll send to the list momentarily) corresponds to your suggestion:

diff --git a/toys/pending/stty.c b/toys/pending/stty.c
index 4168b9ba..a997f49d 100644
--- a/toys/pending/stty.c
+++ b/toys/pending/stty.c
@@ -330,7 +330,7 @@ void stty_main(void)
   xtcgetattr(&old);
 
   if (*toys.optargs) {
-    struct termios new = old;
+    struct termios new = old, tmp;
 
     for (i=0; toys.optargs[i]; i++) {
       char *arg = toys.optargs[i];
@@ -340,11 +340,15 @@ void stty_main(void)
       else if (!strcmp(arg, "line")) new.c_line = get_arg(&i, NR_LDISCS);
       else if (!strcmp(arg, "min")) new.c_cc[VMIN] = get_arg(&i, 255);
       else if (!strcmp(arg, "time")) new.c_cc[VTIME] = get_arg(&i, 255);
-      else if (sscanf(arg, "%x:%x:%x:%x:%n", &new.c_iflag, &new.c_oflag,
-                        &new.c_cflag, &new.c_lflag, &n) == 4)
+      else if (sscanf(arg, "%x:%x:%x:%x:%n", &tmp.c_iflag, &tmp.c_oflag,
+                        &tmp.c_cflag, &tmp.c_lflag, &n) == 4)
       {
         int value;
 
+        new.c_iflag = tmp.c_iflag;
+        new.c_oflag = tmp.c_oflag;
+        new.c_cflag = tmp.c_cflag;
+        new.c_lflag = tmp.c_lflag;
         arg += n;
         for (j=0;j<NCCS;j++) {
           if (sscanf(arg, "%x%n", &value, &n) != 1) error_exit("bad -g string");

and works for me:

~/toybox-stty$ strace -v -e ioctl ./toybox stty -F /dev/tty 300 ; stty sane
ioctl(3, TCGETS, {c_iflags=0x2102, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, TCGETS, {c_iflags=0x2102, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, SNDCTL_TMR_CONTINUE or TCSETSF, {c_iflags=0x2102, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, TCGETS, {c_iflags=0x2102, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
ioctl(3, TCGETS, {c_iflags=0x2102, c_oflags=0x5, c_cflags=0xb7, c_lflags=0x8a3b, c_line=0, c_cc="\x03\x1c\x7f\x15\x04\x00\x01\x00\x11\x13\x1a\x00\x12\x0f\x17\x16\x00\x00\x00"}) = 0
+++ exited with 0 +++

[E5ten]

Looks good

[malon123]

First of all, I would apologize for the badly written bug report. This is my first bug report.
The bug is certainly what @E5ten described, once the beginning characters match %x format(signed hexadecimals) then the new.c_iflags will corrupt even the condition fails.
And thanks @enh-google, the patch works.
Another situation is whether stty 0x300 or stty 300 render new.c_iflags to 0x300 but this is also fixed by the patch.

[enh-google]

cool, thanks for confirming that the suggested fix solves your problem!

[landley]

I look forward to someday getting a chance to clean up the rest of the stuff in pending, but between work and toysh my brain's been full enough it's taken multiple attempts to switch dirtree_path() to a non-recursive implementation. :P