Re: z/OS enum size pitfall

[Ben Pfaff]

On Tue, Aug 22, 2017 at 10:09:22PM -0400, Daniel Richard G. wrote:
> On Tue, 2017 Aug 22 15:01-0700, Ben Pfaff wrote:
> > 
> > I don't know what XLC conforms to.
> >
> > C11 has the same text in 6.7.2.2p4.  The specification for enums has
> > not changed significantly since C89.
> >
> > Paul Eggert already explained the distinction between enumeration
> > constants and enumeration types, so I won't repeat it.
> 
> All the commentary here is greatly appreciated; please excuse my delay
> in replying.
> 
> Here is what IBM said relating to standards:
> 
> The compiler behavior is correct. The C standard says that the
> enumerated type can be char, or any signed or unsigned integer type
> (6.7.2.2#4).
> 
> The test case uses a default enumsize which is small and for the
> range of values in the test case it reserves an unsigned int to hold
> the enumeration values (this is documented in the User's Guide).
> 
> The conversion rules make the signed into unsigned int (6.3.1.8#1)
> meaning that expression becomes (unsigned int)x/BILLION because
> BILLION is an unsigned int;
> 
> The conversion of -9 produces 3294967297 (UINT_MAX +1
> -9) which when divided by 10 produces 3 as the
> result.
> 
> There was no further qualification of "C standard" in the first
> paragraph. However, the test program still returns 3 when XLC is
> in C99 mode:
> 
> $ xlc -qlanglvl=stdc99 -o enumsize enumsize.c
> $ ./enumsize 
> BILLION = 10
> x = -9
> x / BILLION = 3
> 
> Now, from what I'm hearing here, it sounds like IBM may be in the wrong---
> and this would be quite convenient, because while they may not be too
> bothered when their interpretation of POSIX et al. is different from the
> rest of the world's, they _will_ care about a red-letter violation of a
> spec they claim to support.
> 
> I can't standards-lawyer my way out of a paper bag, but if someone here
> could provide a line of argument that IBM's enum shenanigans are
> categorically unsupported by the standard, I'll gladly send it on in the
> hope that it'll get the beast to move.

The C99 rationale is even clearer than the text, in section 6.4.4.3
"Enumeration constants":

Whereas an enumeration variable may have any integer type that
correctly represents all its values when widened to int, an
enumeration constant is only usable as the value of an
expression. Hence its type is simply int.

Re: z/OS enum size pitfall

[Daniel Richard G.]

On Tue, 2017 Aug 22 15:01-0700, Ben Pfaff wrote:
> 
> I don't know what XLC conforms to.
>
> C11 has the same text in 6.7.2.2p4.  The specification for enums has
> not changed significantly since C89.
>
> Paul Eggert already explained the distinction between enumeration
> constants and enumeration types, so I won't repeat it.

All the commentary here is greatly appreciated; please excuse my delay
in replying.

Here is what IBM said relating to standards:

The compiler behavior is correct. The C standard says that the
enumerated type can be char, or any signed or unsigned integer type
(6.7.2.2#4).

The test case uses a default enumsize which is small and for the
range of values in the test case it reserves an unsigned int to hold
the enumeration values (this is documented in the User's Guide).

The conversion rules make the signed into unsigned int (6.3.1.8#1)
meaning that expression becomes (unsigned int)x/BILLION because
BILLION is an unsigned int;

The conversion of -9 produces 3294967297 (UINT_MAX +1
-9) which when divided by 10 produces 3 as the
result.

There was no further qualification of "C standard" in the first
paragraph. However, the test program still returns 3 when XLC is
in C99 mode:

$ xlc -qlanglvl=stdc99 -o enumsize enumsize.c
$ ./enumsize 
BILLION = 10
x = -9
x / BILLION = 3

Now, from what I'm hearing here, it sounds like IBM may be in the wrong---
and this would be quite convenient, because while they may not be too
bothered when their interpretation of POSIX et al. is different from the
rest of the world's, they _will_ care about a red-letter violation of a
spec they claim to support.

I can't standards-lawyer my way out of a paper bag, but if someone here
could provide a line of argument that IBM's enum shenanigans are
categorically unsupported by the standard, I'll gladly send it on in the
hope that it'll get the beast to move.

One minor addendum: In reviewing the support ticket, I noticed a
suggestion to use

#pragma enum(int)

as a workaround, which would probably be more convenient than a compiler
flag (this could go right into config.h). But even though it is supposed
to be equivalent to the ENUMSIZE option, it does not affect the output
of the test program. Seems like another PMR is in order...


--Daniel


-- 
Daniel Richard G. || sk...@iskunk.org
My ASCII-art .sig got a bad case of Times New Roman.

Re: z/OS enum size pitfall

[Ben Pfaff]

On Tue, Aug 22, 2017 at 10:43:07PM +0200, Tim Rühsen wrote:
> On Dienstag, 22. August 2017 13:25:55 CEST Ben Pfaff wrote:
> > On Tue, Aug 22, 2017 at 04:13:54PM -0400, Daniel Richard G. wrote:
> > > What happens is that BILLION is implicitly typed as an unsigned int,
> > > rather than an int. If you edit the code above to use BILLION2 instead
> > > of BILLION, you'll see the same result on GNU/Linux.
> > 
> > It's odd that they claim that this conforms to the C standard.  C11
> > says, in section 6.4.4.3 "Enumeration constants":
> > 
> > An identifier declared as an enumeration constant has type int.
> > 
> > It also says in section 6.7.2.2 "Enumeration specifiers":
> > 
> > The identifiers in an enumerator list are declared as constants that
> > have type int and may appear wherever such are permitted.
> > 
> > This seems pretty clear to me, so I wonder how this interpretation
> > arises.
> 
> Do you know to which C standard the XLC compiler complies to ?
> 
> C99, 6.7.2.2p4 says
> 
> Each enumerated type shall be compatible with char, a signed integer 
> type, 
> or an unsigned integer type. The choice of type is 
> implementation-defined,108) 
> but shall be capable of representing the values of all the members of the 
> enumeration.

I don't know what XLC conforms to.

C11 has the same text in 6.7.2.2p4.  The specification for enums has not
changed significantly since C89.

Paul Eggert already explained the distinction between enumeration
constants and enumeration types, so I won't repeat it.

Re: z/OS enum size pitfall

[Paul Eggert]

On 08/22/2017 01:13 PM, Daniel Richard G. wrote:

I have been in contact with IBM about this, originally reporting the
issue as a compiler bug. However, they responded that the compiler
behavior is conformant to the C standard and that they are less
concerned with matching the behavior of other systems than keeping
things as-is for the benefit of existing customer application code.


There seems to be some miscommunication here. The enum type might be 
either signed or unsigned, and I expect this is what IBM is talking 
about. However, the enum constants that are declared all must be of type 
'int'. This requirement has been in the standard for ages.  For example, 
given:


enum { a, b, c } v = a;

The expression "a < -1" must return 0, because a is zero and is of type 
int. However, the expression "v < -1" might return 0 (if v is signed) 
and it might return 1 (if v is unsigned). This is the case even though v 
is zero, just as a is. Since the code in question is using the enum 
constants, not the enum type, it must treat the values as signed 
integers in any conforming compiler.

Re: clang's Undefined Sanitizer

[Paul Eggert]

On 08/22/2017 10:39 AM, Adhemerval Zanella wrote:

In fact I decided to *not* sync flexmember because with
following patch I intend to send (which are in the original thread)
make flexmember unnecessary.


I see that you sent these proposed patches to glibc glob in the thread 
starting here:


https://sourceware.org/ml/libc-alpha/2017-08/msg01079.html

and I am looking into merging that into Gnulib glob. However, I don't 
see why the patch makes flexmember unnecessary. Even with that patch, 
there is still a datatype 'struct globnames' that has a fixed-size 
member array 'names', and the code still indexes the 'names' component 
past its bounds. That is, the recently-fixed problem is not 
out-of-bounds access into a local variable; it is out-of-bounds access 
into either malloc- or alloca-allocated storage, via a pointer to a type 
that has fixed-size bounds; the C standard does not allow this. So as 
far as I can see, a fix is still necessary even with your patch.


I'll try to resolve this and come up with a patch to Gnulib, and also 
with a patch to follow on to your proposed glibc patch. There are 
several other details that need to be looked at.

Re: z/OS enum size pitfall

[Ben Pfaff]

On Tue, Aug 22, 2017 at 04:13:54PM -0400, Daniel Richard G. wrote:
> Hello list,
> 
> I'm writing in to report a bizarre issue with the IBM z/OS XLC compiler
> that is currently causing one gnulib test to fail (test-timespec), and
> may present an issue for application code simply because no other
> compiler does things this way. My hope is to have gnulib integrate a
> workaround so that this won't bite anyone else.
> 
> I have been in contact with IBM about this, originally reporting the
> issue as a compiler bug. However, they responded that the compiler
> behavior is conformant to the C standard and that they are less
> concerned with matching the behavior of other systems than keeping
> things as-is for the benefit of existing customer application code.
> 
> The problem has to do with the implicit integer type that is used for
> enum symbols. Here is a sample program that illustrates the issue:
> 
> 8<
> #include 
> 
> enum { BILLION = 10 };
> 
> static const unsigned int BILLION2 = 10;
> 
> int main(void) {
> int x = -9;
> printf("BILLION = %d\n", (int)BILLION);
> printf("x = %d\n", x);
> printf("x / BILLION = %d\n", (int)(x / BILLION));
> return 0;
> }
> >8
> 
> On GNU/Linux and AIX, with a minimal compiler invocation, this
> program prints
> 
> BILLION = 10
> x = -9
> x / BILLION = 0
> 
> However, on z/OS, it prints
> 
> BILLION = 10
> x = -9
> x / BILLION = 3
> 
> What happens is that BILLION is implicitly typed as an unsigned int,
> rather than an int. If you edit the code above to use BILLION2 instead
> of BILLION, you'll see the same result on GNU/Linux.

It's odd that they claim that this conforms to the C standard.  C11
says, in section 6.4.4.3 "Enumeration constants":

An identifier declared as an enumeration constant has type int.

It also says in section 6.7.2.2 "Enumeration specifiers":

The identifiers in an enumerator list are declared as constants that
have type int and may appear wherever such are permitted.

This seems pretty clear to me, so I wonder how this interpretation
arises.

z/OS enum size pitfall

[Daniel Richard G.]

Hello list,

I'm writing in to report a bizarre issue with the IBM z/OS XLC compiler
that is currently causing one gnulib test to fail (test-timespec), and
may present an issue for application code simply because no other
compiler does things this way. My hope is to have gnulib integrate a
workaround so that this won't bite anyone else.

I have been in contact with IBM about this, originally reporting the
issue as a compiler bug. However, they responded that the compiler
behavior is conformant to the C standard and that they are less
concerned with matching the behavior of other systems than keeping
things as-is for the benefit of existing customer application code.

The problem has to do with the implicit integer type that is used for
enum symbols. Here is a sample program that illustrates the issue:

8<
#include 

enum { BILLION = 10 };

static const unsigned int BILLION2 = 10;

int main(void) {
int x = -9;
printf("BILLION = %d\n", (int)BILLION);
printf("x = %d\n", x);
printf("x / BILLION = %d\n", (int)(x / BILLION));
return 0;
}
>8

On GNU/Linux and AIX, with a minimal compiler invocation, this
program prints

BILLION = 10
x = -9
x / BILLION = 0

However, on z/OS, it prints

BILLION = 10
x = -9
x / BILLION = 3

What happens is that BILLION is implicitly typed as an unsigned int,
rather than an int. If you edit the code above to use BILLION2 instead
of BILLION, you'll see the same result on GNU/Linux.

test-timespec fails not because of the time function being tested, but
because of how TIMESPEC_RESOLUTION is defined in timespec.h.

IBM, in their response, suggested specifying the flag -qenumsize=4 .
With this flag, the output on z/OS matches that of other systems,
test-timespec passes, and the rest of the gnulib test suite is
unaffected. I think it may be worth considering having gnulib add this
flag by default on z/OS, to get the expected behavior.


--Daniel


-- 
Daniel Richard G. || sk...@iskunk.org
My ASCII-art .sig got a bad case of Times New Roman.

Re: clang's Undefined Sanitizer

[Adhemerval Zanella]

On 22/08/2017 14:29, Paul Eggert wrote:
> On 08/22/2017 09:49 AM, Bruno Haible wrote:
>> It obviously does not handle the 'struct globnames' allocated with
>> the FLEXSIZEOF macro (lines 1719..1732).
> 
> Yes, and I installed the attached patch into Gnulib to try to fix this. I 
> hope it is enough to pacify clang's Undefined Sanitizer. If not, Tim, please 
> let us know, and thanks for reporting the problem.
> 
> I'll CC: this to Adhemerval Zanella, who's working on porting Gnulib glob.c 
> back to glibc, as I worry that this patch (or something like it) should also 
> be ported back.
> 

Thanks for heads up Paul, I intend to send the sync patch for gnulib
glob today.  In fact I decided to *not* sync flexmember because with
following patch I intend to send (which are in the original thread)
make flexmember unnecessary. 

Re: clang's Undefined Sanitizer

[Paul Eggert]

On 08/22/2017 09:49 AM, Bruno Haible wrote:

It obviously does not handle the 'struct globnames' allocated with
the FLEXSIZEOF macro (lines 1719..1732).


Yes, and I installed the attached patch into Gnulib to try to fix this. 
I hope it is enough to pacify clang's Undefined Sanitizer. If not, Tim, 
please let us know, and thanks for reporting the problem.


I'll CC: this to Adhemerval Zanella, who's working on porting Gnulib 
glob.c back to glibc, as I worry that this patch (or something like it) 
should also be ported back.


>From 9211cb3147a1dac2a38a66fb02f24757b7350991 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Tue, 22 Aug 2017 10:13:50 -0700
Subject: [PATCH] glob: port to clang's Undefined Sanitizer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Tim Rühsen in:
http://lists.gnu.org/archive/html/bug-gnulib/2017-08/msg00144.html
* lib/glob.c (FLEXIBLE_ARRAY_MEMBER) [_LIBC]: Define to empty.
(glob_in_dir): Do not rely on undefined behavior in accessing
struct members beyond their bounds.  Use a flexible array member
instead.
---
 ChangeLog  | 10 ++
 lib/glob.c | 29 +++--
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 98643535f..3753cb2a5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2017-08-22  Paul Eggert  
+
+	glob: port to clang's Undefined Sanitizer
+	Problem reported by Tim Rühsen in:
+	http://lists.gnu.org/archive/html/bug-gnulib/2017-08/msg00144.html
+	* lib/glob.c (FLEXIBLE_ARRAY_MEMBER) [_LIBC]: Define to empty.
+	(glob_in_dir): Do not rely on undefined behavior in accessing
+	struct members beyond their bounds.  Use a flexible array member
+	instead.
+
 2017-08-21  Paul Eggert  
 
 	vc-list-files: port to Solaris 10
diff --git a/lib/glob.c b/lib/glob.c
index d06101799..2a7cc8705 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -75,6 +75,7 @@
 #  define __stat64(fname, buf) __xstat64 (_STAT_VER, fname, buf)
 # endif
 # define struct_stat64  struct stat64
+# define FLEXIBLE_ARRAY_MEMBER
 #else /* !_LIBC */
 # define __getlogin_r(buf, len) getlogin_r (buf, len)
 # define __stat64(fname, buf)   stat (fname, buf)
@@ -1590,25 +1591,25 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 {
   size_t dirlen = strlen (directory);
   void *stream = NULL;
-  struct globnames
-{
-  struct globnames *next;
-  size_t count;
-  char *name[64];
-};
-  struct globnames init_names;
-  struct globnames *names = _names;
-  struct globnames *names_alloca = _names;
+# define GLOBNAMES_MEMBERS(nnames) \
+struct globnames *next; size_t count; char *name[nnames];
+  struct globnames { GLOBNAMES_MEMBERS (FLEXIBLE_ARRAY_MEMBER) };
+  struct { GLOBNAMES_MEMBERS (64) } init_names_buf;
+  struct globnames *init_names = (struct globnames *) _names_buf;
+  struct globnames *names = init_names;
+  struct globnames *names_alloca = init_names;
   size_t nfound = 0;
   size_t cur = 0;
   int meta;
   int save;
   int result;
 
-  alloca_used += sizeof (init_names);
+  alloca_used += sizeof init_names_buf;
 
-  init_names.next = NULL;
-  init_names.count = sizeof init_names.name / sizeof init_names.name[0];
+  init_names->next = NULL;
+  init_names->count = ((sizeof init_names_buf
+- offsetof (struct globnames, name))
+   / sizeof init_names->name[0]);
 
   meta = __glob_pattern_type (pattern, !(flags & GLOB_NOESCAPE));
   if (meta == 0 && (flags & (GLOB_NOCHECK|GLOB_NOMAGIC)))
@@ -1789,7 +1790,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
  and this is the block assigned to OLD here.  */
   if (names == NULL)
 {
-  assert (old == _names);
+  assert (old == init_names);
   break;
 }
   cur = names->count;
@@ -1816,7 +1817,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
  and this is the block assigned to OLD here.  */
   if (names == NULL)
 {
-  assert (old == _names);
+  assert (old == init_names);
   break;
 }
   cur = names->count;
-- 
2.13.5

Re: Valgrind is complaining unitialized values in freea (malloca.c:135)

[Bruno Haible]

Hi,

Marc Nieper-Wißkirchen wrote:
> In freea in malloca.c, a possibly uninitialized indicator word is used for
> a comparison so that Valgrind reports: "Conditional jump or move depends on
> uninitialised value(s)".
> 
> Valgrind is not smart enough to understand the logic in freea.
> 
> It would be nice if the warning could be silenced

It can be silenced: The Gnulib 'malloca' module comes with a file
'malloca.valgrind'. Use a valgrind command-line option such as

  --suppressions=$(srcdir)/gnulib-lib/malloca.valgrind

You need to adjust the path to the file, to match the directory structure of
your package.

Bruno

Re: clang's Undefined Sanitizer

[Bruno Haible]

[Changing the subject, as this is an unrelated topic.]

Hi Tim,

> I also see several false positives from clang's Undefined Sanitizer due to
> alloca 'magic' (reallocations on stack space ?). This might not be directly
> related, but I think there is a common coding pattern.
> 
> glob.c:1738:23: runtime error: index 64 out of bounds for type 'char *[64]'
> #0 0x557545 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1738:40
> #1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
> 
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1738:23 in
> glob.c:1739:27: runtime error: index 64 out of bounds for type 'char *[64]'
> #0 0x5575d4 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1739:27
> #1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
> 
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1739:27 in
> glob.c:1811:21: runtime error: index 64 out of bounds for type 'char *[64]'
> #0 0x55845e in glob_in_dir /home/tim/src/wget2/lib/glob.c:1811:21
> #1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

It obviously does not handle the 'struct globnames' allocated with
the FLEXSIZEOF macro (lines 1719..1732).

Bruno

Re: Valgrind is complaining unitialized values in freea (malloca.c:135)

[Marc Nieper-Wißkirchen]

I just noticed the file lib/malloca.valgrind, which can be used with the
Valgrind option suppressions.

Marc

Am 22.08.2017 5:52 nachm. schrieb "Tim Rühsen" :

> On Dienstag, 22. August 2017 06:11:41 CEST Marc Nieper-Wißkirchen wrote:
> > In freea in malloca.c, a possibly uninitialized indicator word is used
> for
> > a comparison so that Valgrind reports: "Conditional jump or move depends
> on
> > uninitialised value(s)".
> >
> > Valgrind is not smart enough to understand the logic in freea.
> >
> > It would be nice if the warning could be silenced, either by amending
> freea
> > slightly (it seems that a similar thing has already been done for Clang
> > warnings) or by reporting the issue to the Valgrind developers so that
> they
> > can special-case gnulib's freea.
>
> I also see several false positives from clang's Undefined Sanitizer due to
> alloca 'magic' (reallocations on stack space ?). This might not be directly
> related, but I think there is a common coding pattern.
>
> glob.c:1738:23: runtime error: index 64 out of bounds for type 'char *[64]'
> #0 0x557545 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1738:40
> #1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
>
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1738:23 in
> glob.c:1739:27: runtime error: index 64 out of bounds for type 'char *[64]'
> #0 0x5575d4 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1739:27
> #1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
>
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1739:27 in
> glob.c:1811:21: runtime error: index 64 out of bounds for type 'char *[64]'
> #0 0x55845e in glob_in_dir /home/tim/src/wget2/lib/glob.c:1811:21
> #1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
>
> Regards, Tim
>

Re: Valgrind is complaining unitialized values in freea (malloca.c:135)

[Tim Rühsen]

On Dienstag, 22. August 2017 06:11:41 CEST Marc Nieper-Wißkirchen wrote:
> In freea in malloca.c, a possibly uninitialized indicator word is used for
> a comparison so that Valgrind reports: "Conditional jump or move depends on
> uninitialised value(s)".
> 
> Valgrind is not smart enough to understand the logic in freea.
> 
> It would be nice if the warning could be silenced, either by amending freea
> slightly (it seems that a similar thing has already been done for Clang
> warnings) or by reporting the issue to the Valgrind developers so that they
> can special-case gnulib's freea.

I also see several false positives from clang's Undefined Sanitizer due to
alloca 'magic' (reallocations on stack space ?). This might not be directly
related, but I think there is a common coding pattern.

glob.c:1738:23: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x557545 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1738:40
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1738:23 in
glob.c:1739:27: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x5575d4 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1739:27
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1739:27 in
glob.c:1811:21: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x55845e in glob_in_dir /home/tim/src/wget2/lib/glob.c:1811:21
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

Regards, Tim


signature.asc
Description: This is a digitally signed message part.

Valgrind is complaining unitialized values in freea (malloca.c:135)

[Marc Nieper-Wißkirchen]

In freea in malloca.c, a possibly uninitialized indicator word is used for
a comparison so that Valgrind reports: "Conditional jump or move depends on
uninitialised value(s)".

Valgrind is not smart enough to understand the logic in freea.

It would be nice if the warning could be silenced, either by amending freea
slightly (it seems that a similar thing has already been done for Clang
warnings) or by reporting the issue to the Valgrind developers so that they
can special-case gnulib's freea.

--

Marc