avoid compiler warning

avoid compiler warning

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

ccache gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h -DBSD=1 -DSHELL
-DIFS_BROKEN  -Wall -gdwarf-2 -Wall -Werror -MT mystring.o -MD -MP -MF
.deps/mystring.Tpo -c -o mystring.o mystring.c
miscbltin.c: In function `umaskcmd':
miscbltin.c:201: warning: subscript has type `char'

isdigit is only defined over EOF and unsigned char values, so without this
patch, you can trigger undefined behavior.

Or you can pull from
$ git pull git://repo.or.cz/dash/ericb.git master

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

[-- Attachment #2: dash.patch1 --]
[-- Type: text/plain, Size: 1005 bytes --]

From 4e9dc98029647880acc1992f36d12331872a818d Mon Sep 17 00:00:00 2001
From: Eric Blake <ebb9@byu.net>
Date: Thu, 9 Jul 2009 06:52:15 -0600
Subject: [PATCH] [BUILD] Avoid compiler warning

Pass correct type to ctype macro.

Signed-off-by: Eric Blake <ebb9@byu.net>
---
 ChangeLog       |    4 ++++
 src/miscbltin.c |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e6a1d26..5731f79 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2009-07-09  Eric Blake  <ebb9@byu.net>
+
+	* Avoid compiler warnings.
+
 2009-06-27  Herbert Xu <herbert@gondor.apana.org.au>

 	* Fix quoted pattern patch breakage.
diff --git a/src/miscbltin.c b/src/miscbltin.c
index 3f91bc3..65ff46d 100644
--- a/src/miscbltin.c
+++ b/src/miscbltin.c
@@ -198,7 +198,7 @@ umaskcmd(int argc, char **argv)
 	} else {
 		int new_mask;

-		if (isdigit(*ap)) {
+		if (isdigit((unsigned char) *ap)) {
 			new_mask = 0;
 			do {
 				if (*ap >= '8' || *ap < '0')
-- 
1.6.3.3.334.g916e1

Re: avoid compiler warning

[Herbert Xu]

On Thu, Jul 09, 2009 at 12:55:25PM +0000, Eric Blake wrote:
> ccache gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h -DBSD=1 -DSHELL
> -DIFS_BROKEN  -Wall -gdwarf-2 -Wall -Werror -MT mystring.o -MD -MP -MF
> .deps/mystring.Tpo -c -o mystring.o mystring.c
> miscbltin.c: In function `umaskcmd':
> miscbltin.c:201: warning: subscript has type `char'
> 
> isdigit is only defined over EOF and unsigned char values, so without this
> patch, you can trigger undefined behavior.

What compiler and what libc was this? isdigit is supposed to
be a function that takes an int argument according to POSIX.
If libc implements it as a macro then it's up to it to cast
the parameter to (int).

So I think you should fix this in your libc instead.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: avoid compiler warning

[H. Peter Anvin]

On 08/10/2009 09:03 PM, Herbert Xu wrote:
> On Thu, Jul 09, 2009 at 12:55:25PM +0000, Eric Blake wrote:
>> ccache gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h -DBSD=1 -DSHELL
>> -DIFS_BROKEN  -Wall -gdwarf-2 -Wall -Werror -MT mystring.o -MD -MP -MF
>> .deps/mystring.Tpo -c -o mystring.o mystring.c
>> miscbltin.c: In function `umaskcmd':
>> miscbltin.c:201: warning: subscript has type `char'
>>
>> isdigit is only defined over EOF and unsigned char values, so without this
>> patch, you can trigger undefined behavior.
> 
> What compiler and what libc was this? isdigit is supposed to
> be a function that takes an int argument according to POSIX.
> If libc implements it as a macro then it's up to it to cast
> the parameter to (int).
> 
> So I think you should fix this in your libc instead.
> 

Herbert... the *type* is int, but the *value* has to be in the range
[-1,UCHAR_MAX] or the behavior is undefined in both the C and POSIX
standards.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: avoid compiler warning

[Herbert Xu]

On Tue, Aug 11, 2009 at 09:33:43AM -0700, H. Peter Anvin wrote:
> 
> Herbert... the *type* is int, but the *value* has to be in the range
> [-1,UCHAR_MAX] or the behavior is undefined in both the C and POSIX
> standards.

Good point.  I'll apply the patch.  I'd be very surprised though
if this was the only instance in which we pass a char along.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: avoid compiler warning

[H. Peter Anvin]

On 08/11/2009 02:56 PM, Herbert Xu wrote:
> On Tue, Aug 11, 2009 at 09:33:43AM -0700, H. Peter Anvin wrote:
>> Herbert... the *type* is int, but the *value* has to be in the range
>> [-1,UCHAR_MAX] or the behavior is undefined in both the C and POSIX
>> standards.
> 
> Good point.  I'll apply the patch.  I'd be very surprised though
> if this was the only instance in which we pass a char along.
> 

Personally I have a habit of wrapping most ctype things in macros or
inlines which cast the argument to (unsigned char), then it's up to me
to make sure EOF doesn't slink in.

	-hpa

Re: avoid compiler warning

[Eric Blake]

According to Herbert Xu on 8/11/2009 3:56 PM:
> On Tue, Aug 11, 2009 at 09:33:43AM -0700, H. Peter Anvin wrote:
>> Herbert... the *type* is int, but the *value* has to be in the range
>> [-1,UCHAR_MAX] or the behavior is undefined in both the C and POSIX
>> standards.
> 
> Good point.  I'll apply the patch.  I'd be very surprised though
> if this was the only instance in which we pass a char along.

I would be surprised, too, but this was the only instance that triggered a
-Wall warning from gcc for my libc (more details in another mail, as
requested), so all other use of ctype is either through unsigned char or
int, rather than straight char.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

Re: avoid compiler warning

[Eric Blake]

According to Herbert Xu on 8/11/2009 3:56 PM:
> On Tue, Aug 11, 2009 at 09:33:43AM -0700, H. Peter Anvin wrote:
>> Herbert... the *type* is int, but the *value* has to be in the range
>> [-1,UCHAR_MAX] or the behavior is undefined in both the C and POSIX
>> standards.
> 
> Good point.  I'll apply the patch.  I'd be very surprised though
> if this was the only instance in which we pass a char along.

Ping.  Or do we want to go with an alternate patch of defining our own
version of ISDIGIT that handles the entire range of int and avoids
checking the current locale, since POSIX guarantees that isdigit can only
return true for the ten bytes '0' through '9'?

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

Re: avoid compiler warning

[Herbert Xu]

On Mon, Aug 31, 2009 at 05:30:18AM -0600, Eric Blake wrote:
> 
> Ping.  Or do we want to go with an alternate patch of defining our own
> version of ISDIGIT that handles the entire range of int and avoids
> checking the current locale, since POSIX guarantees that isdigit can only
> return true for the ten bytes '0' through '9'?

It's still in my queue.  I'll get to it next.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: avoid compiler warning

[Eric Blake]

According to H. Peter Anvin on 8/11/2009 10:33 AM:
> Herbert... the *type* is int, but the *value* has to be in the range
> [-1,UCHAR_MAX] or the behavior is undefined in both the C and POSIX
> standards.

Actually, the *value* has to be either EOF (which is negative, but not
necessarily required to be -1) or [0,UCHAR_MAX].  But you are correct that
for most (all?) platforms, EOF is -1 even though it is not mandated as such.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

Re: avoid compiler warning

[Eric Blake]

According to Herbert Xu on 8/10/2009 10:03 PM:
> On Thu, Jul 09, 2009 at 12:55:25PM +0000, Eric Blake wrote:
>> ccache gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h -DBSD=1 -DSHELL
>> -DIFS_BROKEN  -Wall -gdwarf-2 -Wall -Werror -MT mystring.o -MD -MP -MF
>> .deps/mystring.Tpo -c -o mystring.o mystring.c
>> miscbltin.c: In function `umaskcmd':
>> miscbltin.c:201: warning: subscript has type `char'
>>
>> isdigit is only defined over EOF and unsigned char values, so without this
>> patch, you can trigger undefined behavior.
> 
> What compiler and what libc was this? isdigit is supposed to
> be a function that takes an int argument according to POSIX.
> If libc implements it as a macro then it's up to it to cast
> the parameter to (int).

This is with recent newlib (the warning was intentionally added exactly to
catch the sort of bugs that my patch fixes), coupled with any version of
gcc 3.4 or newer.  Additionally, there is a pending bug report against
glibc requesting that glibc add the same QoI warning to flag potentially
buggy code, since it is quite easy to flag the use of raw char as a bug:
http://sources.redhat.com/bugzilla/show_bug.cgi?id=10296

In a particular demonstration of the bug, there are some locales where
isspace('\xff') is false but isspace((unsigned char)'\xff') [aka
isspace(0xff)] is true, when char is a signed type.  And although both
glibc and newlib cater to most instances of this bug (as a QoI
enhancement, these libraries guarantee that isspace('\xfe') [aka
isspace(-2)] returns the same result as isspace(0xfe)), not all platforms
have this QoI support, and can actually end up dereferencing outside of
array bounds.

Note that isdigit is a bit unique among the ctype macros: C89 and C99
state that it is locale dependent (with a range still limited to EOF or
[0-UCHAR_MAX]), but POSIX adds the additional restriction that it only
return true for the ten contiguous characters '0' through '9', meaning
that any POSIX-compliant version of isdigit(x) can be as simple as
((unsigned)(x)-'0'<=9) for all x, without regards to locale or
out-of-range arguments.  But not all locales comply with POSIX, so it is
not generally portable to rely on isdigit being this simple or fast.  On
the other hand, there are a number of packages that #define ISDIGIT,
rather than use ctype's isdigit, exactly to get the speedup guaranteed by
POSIX; this may be something you want to do in dash.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net