GNU bug report logs - #11675
stty bad C semantics

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 11675 in the body.
You can then email your comments to 11675 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Edward Schwartz <edmcman <at> cmu.edu>
To: bug-coreutils <at> gnu.org
Subject: stty bad C semantics
Date: Mon, 11 Jun 2012 15:16:39 -0400

[Message part 1 (text/plain, inline)]

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

[stty.patch (application/octet-stream, attachment)]

Message #8 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Edward Schwartz <edmcman <at> cmu.edu>
Cc: 11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Mon, 11 Jun 2012 13:50:06 -0600

[Message part 1 (text/plain, inline)]

On 06/11/2012 01:16 PM, 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 disagree with your interpretation of C99, and claim instead that your
CIL compiler is buggy.

C99 6.7.8 para. 21:

"If there are fewer initializers in a brace-enclosed list than there are
elements or members of an aggregate, or fewer characters in a string
literal used to initialize an array of known size than there are
elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage
duration."

And while there is no way to zero out padding bits by direct assignment,
I think we _are_ guaranteed that padding bits are zero when doing the
same initialization as static storage duration if we did not call out
all the named members.

> 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.

If I understand correctly, gcc is also able to optimize out the memset,
so that your patch has no net impact and would allow us to work around
your buggy compiler; but I will leave it up to Jim whether it is worth
applying.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #11 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Edward Schwartz <edmcman <at> cmu.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: 11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Mon, 11 Jun 2012 16:08:55 -0400

On Mon, Jun 11, 2012 at 3:50 PM, Eric Blake <eblake <at> redhat.com> wrote:
> On 06/11/2012 01:16 PM, Edward Schwartz wrote:
>
> I disagree with your interpretation of C99, and claim instead that your
> CIL compiler is buggy.
>
> C99 6.7.8 para. 21:
>
> "If there are fewer initializers in a brace-enclosed list than there are
> elements or members of an aggregate, or fewer characters in a string
> literal used to initialize an array of known size than there are
> elements in the array, the remainder of the aggregate shall be
> initialized implicitly the same as objects that have static storage
> duration."
>
> And while there is no way to zero out padding bits by direct assignment,
> I think we _are_ guaranteed that padding bits are zero when doing the
> same initialization as static storage duration if we did not call out
> all the named members.

Why do you think that?  The standard is pretty vague here, but I
interpret the "remainder of the aggregate" as the remaining fields,
since the initializer list is a list of fields.

I just want to make sure the bug is filed in the proper place.  If
it's a CIL bug, I'll file it with them.

>
>> 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.
>
> If I understand correctly, gcc is also able to optimize out the memset,
> so that your patch has no net impact and would allow us to work around
> your buggy compiler; but I will leave it up to Jim whether it is worth
> applying.

Thanks,
Ed

Message #14 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Eric Blake <eblake <at> redhat.com>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, 11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Mon, 11 Jun 2012 22:28:49 +0200

Eric Blake <eblake <at> redhat.com> writes:

> And while there is no way to zero out padding bits by direct assignment,
> I think we _are_ guaranteed that padding bits are zero when doing the
> same initialization as static storage duration if we did not call out
> all the named members.

I think the standard is very clear about the state of padding bytes
[6.2.6.1#6]:

    When a value is stored in an object of structure or union type,
    including in a member object, the bytes of the object representation
    that correspond to any padding bytes take unspecified values.

(6.7.8#10 says nothing about the initial value of any padding bytes.)

Andreas.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Message #17 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, 11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Mon, 11 Jun 2012 13:29:08 -0700

This area is a bit of a mess in the standard, but I think
Edward Schwartz is right that initializers of auto variables
are not guaranteed to clear padding.

As I understand it under Eric Blake's interpretation, a
complete initializer for an auto variable is not guaranteed
to clear the padding, but an incomplete initializer is
guaranteed to do so.  This would be pretty strange.
I think Edward's interpretation of the incomplete-initializer
wording is more likely to be the correct one: namely, that
missing initializers are treated as if they were zero.

Even if the standard was intended to clear the padding,
clearly in practice we have an implementation that doesn't
do so, and it no doubt performs better that way, so I'd say
something like Edward's patch is advisable.

Message #20 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, Eric Blake <eblake <at> redhat.com>,
	11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 00:47:57 +0100

On 06/11/2012 09:29 PM, Paul Eggert wrote:
> This area is a bit of a mess in the standard, but I think
> Edward Schwartz is right that initializers of auto variables
> are not guaranteed to clear padding.
> 
> As I understand it under Eric Blake's interpretation, a
> complete initializer for an auto variable is not guaranteed
> to clear the padding, but an incomplete initializer is
> guaranteed to do so.  This would be pretty strange.

I agree with Eric's interpretation.

There are ambiguous statements in the standard,
but IMHO the _init_ requirement (C99 6.7.8 para. 21)
trumps the _assignment_ requirement (6.2.6.1#6).

I see it as a language standard way to init auto vars,
so that they match static vars of the same type,
and so can be compared.

http://www.pixelbeat.org/programming/gcc/auto_init.html

> I think Edward's interpretation of the incomplete-initializer
> wording is more likely to be the correct one: namely, that
> missing initializers are treated as if they were zero.
> 
> Even if the standard was intended to clear the padding,
> clearly in practice we have an implementation that doesn't
> do so, and it no doubt performs better that way, so I'd say
> something like Edward's patch is advisable.

Well we've not had an issue in nearly 6 years with the existing form:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=e245a66

If this was an isolated statement then I'd change for pragmatic reasons,
but since it touches multiple places in the code I'd suggest
patching coreutils locally (with essentially the reverse of the above),
until CIL is fixed to conform to the standard.

cheers,
Pádraig.

Message #23 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, Eric Blake <eblake <at> redhat.com>,
	11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Mon, 11 Jun 2012 17:13:51 -0700

On 06/11/2012 04:47 PM, Pádraig Brady wrote:
> it touches multiple places in the code

Which other places?  We do use = { FOO }
to initialize in many places in the code, but
I don't recall any other places that rely on
initializing padding to zero.

Message #26 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, Eric Blake <eblake <at> redhat.com>,
	11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 01:42:13 +0100

On 06/12/2012 01:13 AM, Paul Eggert wrote:
> On 06/11/2012 04:47 PM, Pádraig Brady wrote:
>> it touches multiple places in the code
> 
> Which other places?  We do use = { FOO }
> to initialize in many places in the code, but
> I don't recall any other places that rely on
> initializing padding to zero.

Oh right, sorry.
So if this is the only instance then I
guess it's pragmatic to memset here
(with an appropriate comment).

It's still a bug I think in CIL,
so it would be great to report there too.

cheers,
Pádraig.

Message #29 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Edward Schwartz <edmcman <at> cmu.edu>
Cc: 11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 16:33:47 +0200

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 <meyering <at> redhat.com>
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                        coleman <at> iarc1.ece.utexas.edu
 Doug McLaren                        dougmc <at> comco.com
 Dragos Harabor                      dharabor <at> us.oracle.com
 Duncan Roe                          duncanr <at> optimation.com.au
+Edward Schwartz                     edmcman <at> cmu.edu
 Edward Welbourne                    eddy <at> opera.com
 Edzer Pebesma                       Edzer.Pebesma <at> rivm.nl
 Egmont Koblinger                    egmont <at> uhulinux.hu
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

Message #32 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: "Voelker, Bernhard" <bernhard.voelker <at> siemens-enterprise.com>
To: Jim Meyering <jim <at> meyering.net>, Edward Schwartz <edmcman <at> cmu.edu>
Cc: "11675 <at> debbugs.gnu.org" <11675 <at> debbugs.gnu.org>
Subject: RE: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 14:52:04 +0000

Jim Meyering wrote:

> That has prompted a nicely animated debate ;-)

... and it goes on ;-)

What about these?

find . -name '*.c' | xargs grep -F '0, };'
./src/ls.c:                    mbstate_t mbstate = { 0, };
./src/shred.c:  struct Options flags = { 0, };
./src/tr.c:  bool in_set[N_CHARS] = { 0, };
./src/wc.c:      mbstate_t state = { 0, };
./src/pathchk.c:      mbstate_t mbstate = { 0, };

Have a nice day,
Berny

Message #35 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: "Voelker, Bernhard" <bernhard.voelker <at> siemens-enterprise.com>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, Jim Meyering <jim <at> meyering.net>,
	"11675 <at> debbugs.gnu.org" <11675 <at> debbugs.gnu.org>
Subject: Re: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 08:58:13 -0600

[Message part 1 (text/plain, inline)]

On 06/12/2012 08:52 AM, Voelker, Bernhard wrote:
> Jim Meyering wrote:
> 
>> That has prompted a nicely animated debate ;-)
> 
> ... and it goes on ;-)
> 
> What about these?
> 
> find . -name '*.c' | xargs grep -F '0, };'
> ./src/ls.c:                    mbstate_t mbstate = { 0, };
> ./src/shred.c:  struct Options flags = { 0, };
> ./src/tr.c:  bool in_set[N_CHARS] = { 0, };
> ./src/wc.c:      mbstate_t state = { 0, };
> ./src/pathchk.c:      mbstate_t mbstate = { 0, };

What about them?  None of them are using memcmp(), so none of them care
what the padding bytes (if any) got set to.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #38 received at 11675 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, 11675 <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 09:33:45 -0700

On 06/12/2012 07:33 AM, Jim Meyering wrote:
> Here's a way to solve the problem that doesn't require restoring
> the memset calls.  It feels slightly hackish

But it's hackish in a good way!  It's a bit faster
and smaller and more portable than the existing code,
and it's faster and smaller than all the other proposed
fixes.  It's clearly the best idea yet.  Thanks for
cutting the Gordian knot.

Message #43 received at 11675-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Edward Schwartz <edmcman <at> cmu.edu>, 11675-done <at> debbugs.gnu.org
Subject: Re: bug#11675: stty bad C semantics
Date: Tue, 12 Jun 2012 18:41:21 +0200

Paul Eggert wrote:
> On 06/12/2012 07:33 AM, Jim Meyering wrote:
>> Here's a way to solve the problem that doesn't require restoring
>> the memset calls.  It feels slightly hackish
>
> But it's hackish in a good way!  It's a bit faster
> and smaller and more portable than the existing code,
> and it's faster and smaller than all the other proposed
> fixes.  It's clearly the best idea yet.  Thanks for
> cutting the Gordian knot.

Thanks.  With that, I've pushed it and marked this as done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.