bug#11675: stty bad C semantics

[Jim Meyering]

Edward Schwartz wrote:
> Hi,
>
> I think there is a bug in main() of stty in coreutils 8.17.  The gist
> of the problem is that two structures are initialized:
>
>    struct termios mode = { 0, };
>
> and
>
>   struct termios new_mode = { 0, };
>
> They are then both modified, and then compared with memcmp.  The
> problem is that the structs contain padding bytes.  The C99 standard
> says "The value of padding bytes when storing values in structures or
> unions (6.2.6.1)." is unspecified, so the padding bytes may not be set
> to zero.
>
> I don't have any problem compiling with gcc.  On my machine, gcc
> initializes the entire struct memory with a loop that writes 0.
>
> I came across the bug when compiling coreutils under CIL, which
> rewrites many C language constructs to make them easier to analyze.
> CIL writes 0 to each struct field, leaving padding bytes untouched.
> Both are correct, under my interpretation of the C99 standard.
> However, CIL's behavior violates the assumptions of stty's memcmp,
> which assumes padding bytes are set to zero.
>
> The problem is easily fixed by using memset, instead of implied
> initializations.  I am attaching a patch that does this.  While it
> won't affect most coreutils users, it might save some time for someone
> using a non-standard compiler or analysis platform.
>
> Thanks,
> Ed
>
> Index: stty.c
> ===================================================================
> --- stty.c    (revision 11019)
> +++ stty.c    (working copy)
> @@ -729,7 +729,8 @@
>  {
>    /* Initialize to all zeroes so there is no risk memcmp will report a
>       spurious difference in an uninitialized portion of the structure.  */
> -  struct termios mode = { 0, };
> +  struct termios mode;
> +  memset(&mode, 0, sizeof(mode));
>
>    enum output_type output_type;
>    int optc;
> @@ -1002,8 +1003,9 @@
>      {
>        /* Initialize to all zeroes so there is no risk memcmp will report a
>           spurious difference in an uninitialized portion of the structure.  
> */
> -      struct termios new_mode = { 0, };
> -
> +      struct termios new_mode;
> +      memset(&new_mode, 0, sizeof(new_mode));
> +
>        if (tcsetattr (STDIN_FILENO, TCSADRAIN, &mode))
>          error (EXIT_FAILURE, errno, "%s", device_name);

Hi Ed,

Thank you for the report and the patch.
That has prompted a nicely animated debate ;-)

Here's a way to solve the problem that doesn't require restoring
the memset calls.  It feels slightly hackish, but there's already
a comment in each case, so it seems ok.

>From 5c2181c870f4bc1abaee8ffd0b088ab05f87a61c Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 12 Jun 2012 16:13:43 +0200
Subject: [PATCH] stty: portability: accommodate CIL

* src/stty.c (main): Declare locals "mode" and "new_mode" to be static
to ensure that each is initialized to zero, *including* all padding.
While gcc clears padding of a local automatic initialized to "{ 0, }",
CIL does not, and the C99 standard is not clear on this issue.
Reported by Edward Schwartz.  See http://bugs.gnu.org/11675 for details.
---
 THANKS.in  | 1 +
 src/stty.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/THANKS.in b/THANKS.in
index b9a6c64..51b2c7d 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -172,6 +172,7 @@ Doug Coleman                        address@hidden
 Doug McLaren                        address@hidden
 Dragos Harabor                      address@hidden
 Duncan Roe                          address@hidden
+Edward Schwartz                     address@hidden
 Edward Welbourne                    address@hidden
 Edzer Pebesma                       address@hidden
 Egmont Koblinger                    address@hidden
diff --git a/src/stty.c b/src/stty.c
index a3fc3dd..83b502c 100644
--- a/src/stty.c
+++ b/src/stty.c
@@ -730,7 +730,7 @@ 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.  */
-  struct termios mode = { 0, };
+  static struct termios mode;

   enum output_type output_type;
   int optc;
@@ -1003,7 +1003,7 @@ 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.  */
-      struct termios new_mode = { 0, };
+      static struct termios new_mode;

       if (tcsetattr (STDIN_FILENO, TCSADRAIN, &mode))
         error (EXIT_FAILURE, errno, "%s", device_name);
--
1.7.11.rc2.5.g68f532f