Re: [Bug 269] Many compiler warnings with gcc 4.0

[Alan DeKok]

[hidden email] wrote:
> ------- Additional Comments From [hidden email]  2005-08-24 09:58 -------
> Many thanks for second patch! I've added it to the CVS branch 1.0,
> so your fixes will be in release 1.0.5.
>
> There're still problems of incompatible pointer types, but I need
> Alan's opinion about bringing so many casts in the
> source. (personally I don't like the idea)

  Neither do I.  They make the code harder to read & maintain.

  Alan DeKok.
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Paul "TBBle" Hampson]

On Thu, Aug 25, 2005 at 05:24:05PM -0400, Alan DeKok wrote:
> [hidden email] wrote:
>> ------- Additional Comments From [hidden email]  2005-08-24 09:58 -------
>> Many thanks for second patch! I've added it to the CVS branch 1.0,
>> so your fixes will be in release 1.0.5.
>>
>> There're still problems of incompatible pointer types, but I need
>> Alan's opinion about bringing so many casts in the
>> source. (personally I don't like the idea)

>   Neither do I.  They make the code harder to read & maintain.

>   Alan DeKok.

A brief glance through the patch in the bugreport indicates a lot of the casts
were for value_pair's strvalue member from uint8_t* to char*...

What might solve this better is if we can somehow get char to default to
unsigned when building, and all these will go away.

Also, hidden amongst the signedness errors are occasional "comparison always
<blah> due to limited range". One such (that I've looked at before and given up
on) is src/lib/radius.c:1439. This comes about because the definition of
TAG_VALID_ZERO contains two tests, one of which is >=0 which is called against
a uint8_t* here.

The other two uses of TAG_VALID_ZERO are in valuepair.c, and are made with an
explicitly signed char coming out of strtol (which is defined here as returning
a long int...! Is this a big nasty bug????) and which could maybe be replaced
with strtoul, since a return <0 is discarded. However, strtoul appears to
discard any negatiion, rather than truncating at 0, so this last may not be
much use.

Possibly large swathes of the server could be cleaned up to use uint8_t instead
of char, and such warnings could be eliminated, but we will still get
signedness problems from the libc calls which are defined with char parameters,
and char is considered signed.

I agree that the cast is ugly, and in fact a few prototypes (such as
rad_pwdecode) could be changed to take a uint8_t instead of a char to remove
some similar casts.  (Speculation based on the block directly above
src/lib/radius.c:1439)

And I then suggest that the above is more like HEAD work than RELEASE_1_0 work.
^_^

--
-----------------------------------------------------------
Paul "TBBle" Hampson, MCSE
8th year CompSci/Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
[hidden email]

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.1/au/
-----------------------------------------------------------

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Alan DeKok]

Paul TBBle Hampson <[hidden email]> wrote:
> A brief glance through the patch in the bugreport indicates a lot of
> the casts were for value_pair's strvalue member from uint8_t* to
> char*...
>
> What might solve this better is if we can somehow get char to default to
> unsigned when building, and all these will go away.

  Sure.

> Also, hidden amongst the signedness errors are occasional
> "comparison always <blah> due to limited range". One such (that I've
> looked at before and given up on) is src/lib/radius.c:1439. This
> comes about because the definition of TAG_VALID_ZERO contains two
> tests, one of which is >=3D0 which is called against a uint8_t*
> here.

  Yeah, it's annoying.  I'm looking at that code now, and will see if
I can fix it.

  I don't think it has any bad side-effects, but it should be fixed.

> Possibly large swathes of the server could be cleaned up to use
> uint8_t instead of char, and such warnings could be eliminated,
> but we will still get signedness problems from the libc calls which
> are defined with char parameters, and char is considered signed.

  Yes.

> I agree that the cast is ugly, and in fact a few prototypes (such as
> rad_pwdecode) could be changed to take a uint8_t instead of a char to remove
> some similar casts.  (Speculation based on the block directly above
> src/lib/radius.c:1439)

  Yes.

> And I then suggest that the above is more like HEAD work than
> RELEASE_1_0 work.

  Yes.

  Alan DeKok.
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Steven Simon]

I tend to think this is the wrong approach. By convention, C strings  
are signed and Pascal strings (I know, nobody uses them anymore) are  
unsigned. If char defaults to unsigned, it could cause more problems  
than it solves. We want the compiler to tell us if we're mixing C-
strings and data buffers. I agree with the other list members who  
don't like the casts. My question is, why is the strvalue field  
unsigned? Does it ever contain data that's not a C-string?

Steve


On Aug 30, 2005, at 12:56 PM, Alan DeKok wrote:

>> What might solve this better is if we can somehow get char to  
>> default to
>> unsigned when building, and all these will go away.
>
>   Sure.

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Alan DeKok]

Steven Simon <[hidden email]> wrote:
> My question is, why is the strvalue field unsigned? Does it ever
> contain data that's not a C-string?

  Yes.  Lots.

  Alan DeKok.
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Paul "TBBle" Hampson]

On Tue, Aug 30, 2005 at 01:15:45PM -0700, Steven Simon wrote:
> I tend to think this is the wrong approach. By convention, C strings  are
> signed and Pascal strings (I know, nobody uses them anymore) are  unsigned.
> If char defaults to unsigned, it could cause more problems  than it solves.
> We want the compiler to tell us if we're mixing C- strings and data buffers.
> I agree with the other list members who  don't like the casts. My question
> is, why is the strvalue field unsigned? Does it ever contain data that's not
> a C-string?

Chars are unsigned on Linux PowerPC. I don't believe there's anything that
depends on chars being signed that doesn't explicitly declare such, as I ran
FreeRADIUS on LinuxPPC without issues for a few years. I think all
signed-char assumptions were therefore shaken out a fair while ago.

So we could add -funsigned-char to CFLAGS and see what happens... ^_^

I thought gcc4 was moving to unsigned-char by default, but I guess not...

(C-strings are null-terminated. Pascal strings start with their length. ^_^)

What's actually happening here is not that we're confusing data buffers and
strings, but that we're using string functions (which actually operate on
null-terminated data buffers) on null-terminated data buffers. So if anyone's
confusing the two, it's glibc. ^_^

--
-----------------------------------------------------------
Paul "TBBle" Hampson, MCSE
8th year CompSci/Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
[hidden email]

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.1/au/
-----------------------------------------------------------

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Frank Cusack]

On August 31, 2005 4:47:16 PM +1000 Paul TBBle Hampson <[hidden email]> wrote:
> On Tue, Aug 30, 2005 at 01:15:45PM -0700, Steven Simon wrote:
>> I tend to think this is the wrong approach. By convention, C strings  are
>> signed and Pascal strings (I know, nobody uses them anymore) are  unsigned.
>> If char defaults to unsigned, it could cause more problems  than it solves.
>> We want the compiler to tell us if we're mixing C- strings and data buffers.

Absolutely, but this only comes into play when promoting char to int,
as is done with varargs functions, e.g. sprintf().  (There is no such
thing as a char argument to a varargs function; all chars are promoted
to int, and unsigned chars to unsigned ints.)

One common place this happens is for debug output, when printing the
contents of buffers.

<http://www.freshsources.com/1994017B.HTM> is a good page describing
the difference between signed and unsigned char.

> Chars are unsigned on Linux PowerPC. I don't believe there's anything that
> depends on chars being signed that doesn't explicitly declare such, as I ran
> FreeRADIUS on LinuxPPC without issues for a few years. I think all
> signed-char assumptions were therefore shaken out a fair while ago.
...

To say that chars are unsigned on Linux PPC is meaningless.  Whether chars
are signed or unsigned is up to the compiler.  Now it may be the case that
gcc on Linux PPC does treat char as unsigned, but I highly doubt it.

Linux PPC itself may have lots of code that explicitly has unsigned char
data, but that doesn't affect application code at all.  Even whether or
not glibc uses explicit unsigned char internally doesn't affect applications
since behavior must still conform to the various standards.  Wherever
char *'s are used, promotion isn't done so signed vs. unsigned doesn't
matter.  (And current standards call for void * instead of char * almost
everywhere.)

My point is that FR running successfully on Linux PPC, which may use
unsigned char everywhere internally (I'll take your word on it) does
not imply anything about how char might be treated in application code.

Forcing unsigned char would be a mistake, IMHO.  I personally think casts
are good.  One man's ugly notation is another man's inline documentation.

-frank
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Alan DeKok]

Frank Cusack <[hidden email]> wrote:
> Forcing unsigned char would be a mistake, IMHO.  I personally think casts
> are good.  One man's ugly notation is another man's inline documentation.

  The casts hilight API incompatibilities, and potential security
issues.

  Maybe since we're doing massive re-writes anyhow, we could have:

VALUE_PAIR {
         union {
               char strvalue[];
               uint8_t octets[];
               ...
         } data;
}

  And then use the "right" data member to access the data.  "uint8_t"
shouldn't be passed to "strcpy", especially if we're using "uint8_t"
as "opaque data".

  Alan DeKok.

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Steven Simon]

The casts are a mixed bag. They can be distracting, but they document  
expectations, as Frank pointed out.
I'll present another option, also a bit ugly, and see how it goes.
We could use a union that has accessors for each value type. The code  
would have to use the appropriate one for the attribute.
union {
char str[MAX_STRING_LEN]; // or sstr for signed
uint8_t data[MAX_STRING_LEN]; // or ustr for unsigned
} value;

Steve



On Aug 31, 2005, at 12:38 PM, Frank Cusack wrote:

> On August 31, 2005 4:47:16 PM +1000 Paul TBBle Hampson  
> <[hidden email]> wrote:
>> On Tue, Aug 30, 2005 at 01:15:45PM -0700, Steven Simon wrote:
>>> I tend to think this is the wrong approach. By convention, C  
>>> strings  are
>>> signed and Pascal strings (I know, nobody uses them anymore) are  
>>> unsigned.
>>> If char defaults to unsigned, it could cause more problems  than  
>>> it solves.
>>> We want the compiler to tell us if we're mixing C- strings and  
>>> data buffers.
>
> Absolutely, but this only comes into play when promoting char to int,
> as is done with varargs functions, e.g. sprintf().  (There is no such
> thing as a char argument to a varargs function; all chars are promoted
> to int, and unsigned chars to unsigned ints.)
>
> One common place this happens is for debug output, when printing the
> contents of buffers.
>
> <http://www.freshsources.com/1994017B.HTM> is a good page describing
> the difference between signed and unsigned char.
>
>> Chars are unsigned on Linux PowerPC. I don't believe there's  
>> anything that
>> depends on chars being signed that doesn't explicitly declare  
>> such, as I ran
>> FreeRADIUS on LinuxPPC without issues for a few years. I think all
>> signed-char assumptions were therefore shaken out a fair while ago.
> ...
>
> To say that chars are unsigned on Linux PPC is meaningless.  
> Whether chars
> are signed or unsigned is up to the compiler.  Now it may be the  
> case that
> gcc on Linux PPC does treat char as unsigned, but I highly doubt it.
>
> Linux PPC itself may have lots of code that explicitly has unsigned  
> char
> data, but that doesn't affect application code at all.  Even  
> whether or
> not glibc uses explicit unsigned char internally doesn't affect  
> applications
> since behavior must still conform to the various standards.  Wherever
> char *'s are used, promotion isn't done so signed vs. unsigned doesn't
> matter.  (And current standards call for void * instead of char *  
> almost
> everywhere.)
>
> My point is that FR running successfully on Linux PPC, which may use
> unsigned char everywhere internally (I'll take your word on it) does
> not imply anything about how char might be treated in application  
> code.
>
> Forcing unsigned char would be a mistake, IMHO.  I personally think  
> casts
> are good.  One man's ugly notation is another man's inline  
> documentation.
>
> -frank
> - List info/subscribe/unsubscribe? See http://www.freeradius.org/ 
> list/devel.html

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Steven Simon]

looks like you beat me to it ;)

Steve

On Aug 31, 2005, at 1:09 PM, Alan DeKok wrote:

> Frank Cusack <[hidden email]> wrote:
>> Forcing unsigned char would be a mistake, IMHO.  I personally  
>> think casts
>> are good.  One man's ugly notation is another man's inline  
>> documentation.
>
>   The casts hilight API incompatibilities, and potential security
> issues.
>
>   Maybe since we're doing massive re-writes anyhow, we could have:
>
> VALUE_PAIR {
> union {
>       char strvalue[];
>       uint8_t octets[];
>       ...
> } data;
> }
>
>   And then use the "right" data member to access the data.  "uint8_t"
> shouldn't be passed to "strcpy", especially if we're using "uint8_t"
> as "opaque data".
>
>   Alan DeKok.
>
> -
> List info/subscribe/unsubscribe? See http://www.freeradius.org/list/ 
> devel.html

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Frank Cusack]

On August 31, 2005 4:09:49 PM -0400 Alan DeKok <[hidden email]> wrote:

> Frank Cusack <[hidden email]> wrote:
>> Forcing unsigned char would be a mistake, IMHO.  I personally think casts
>> are good.  One man's ugly notation is another man's inline documentation.
>
>   The casts hilight API incompatibilities, and potential security
> issues.
>
>   Maybe since we're doing massive re-writes anyhow, we could have:
>
> VALUE_PAIR {
> union {
>       char strvalue[];
>       uint8_t octets[];
>       ...
> } data;
> }
>
>   And then use the "right" data member to access the data.  "uint8_t"
> shouldn't be passed to "strcpy", especially if we're using "uint8_t"
> as "opaque data".

I personally dislike that, however I have seen other pieces of code that
did similar things to avoid casts.  Myself, I find it quite hard to read.
Not sure why you'd use uint8_t instead of unsigned char though.

-frank
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Alan DeKok]

Frank Cusack <[hidden email]> wrote:
> I personally dislike that, however I have seen other pieces of code that
> did similar things to avoid casts.  Myself, I find it quite hard to read.

  I agree.  That's why som eheader files do:

#define my_foo bar.baz.y.z.foo

  It breaks some things, but it's easy to read.

> Not sure why you'd use uint8_t instead of unsigned char though.

  Less typing?

  Personally, I don't like "unsigned char", as that says to me
"unsigned ASCII character".  Instead "uint8_t" says "unsigned 8 bits
of data".

  Alan DeKok.
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Paul "TBBle" Hampson]

On Wed, Aug 31, 2005 at 12:38:03PM -0700, Frank Cusack wrote:
> On August 31, 2005 4:47:16 PM +1000 Paul TBBle Hampson <[hidden email]> wrote:
> >Chars are unsigned on Linux PowerPC. I don't believe there's anything that
> >depends on chars being signed that doesn't explicitly declare such, as I ran
> >FreeRADIUS on LinuxPPC without issues for a few years. I think all
> >signed-char assumptions were therefore shaken out a fair while ago.
> ...

> To say that chars are unsigned on Linux PPC is meaningless.  Whether chars
> are signed or unsigned is up to the compiler.  Now it may be the case that
> gcc on Linux PPC does treat char as unsigned, but I highly doubt it.

OK, let me rephrase. gcc and glibc on Linux PPC (specifically tested on Debian)
have unsigned chars by default.

This affects applications that do not explicitly state their signedness
requirements, which luckily FreeRADIUS does, as far as I can tell.

(The first thing I did when I started running FreeRADIUS on PowerPC is look for
"comparison is always <blah> due to limited range of type" warnings from
compiler output.)

--
-----------------------------------------------------------
Paul "TBBle" Hampson, MCSE
8th year CompSci/Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
[hidden email]

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.1/au/
-----------------------------------------------------------

-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Paul "TBBle" Hampson]

On Wed, Aug 31, 2005 at 04:09:49PM -0400, Alan DeKok wrote:
>   Maybe since we're doing massive re-writes anyhow, we could have:

> VALUE_PAIR {
> union {
>       char strvalue[];
>       uint8_t octets[];
>       ...
         }} data;


Can we throw a couple of #defines in there too?
#define vp_strval data.strvalue
#define vp_octets data.octets
?

That way vp->vp_strval is a direct substitute for vp->strvalue. It carries a
risk of namespace pollution though.

(I'm not too fussed, I'm as happy as not with unions, but I know
some people dislike the extra component.)

Otherwise, an anonymous union might do it, if I recall correctly?

--
-----------------------------------------------------------
Paul "TBBle" Hampson, MCSE
8th year CompSci/Asian Studies student, ANU
The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361)
[hidden email]

Of course Pacman didn't influence us as kids. If it did,
we'd be running around in darkened rooms, popping pills and
listening to repetitive music.
 -- Kristian Wilson, Nintendo, Inc, 1989

License: http://creativecommons.org/licenses/by/2.1/au/
-----------------------------------------------------------
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

[Alan DeKok]

Paul TBBle Hampson <[hidden email]> wrote:
> Can we throw a couple of #defines in there too?
> #define vp_strval data.strvalue
> #define vp_octets data.octets
> ?

  Yes.

> That way vp->vp_strval is a direct substitute for vp->strvalue. It carries a
> risk of namespace pollution though.

  Try using "st_mtime" in your code.  The headers for the stat()
routine do the same thing.

> (I'm not too fussed, I'm as happy as not with unions, but I know
> some people dislike the extra component.)
>
> Otherwise, an anonymous union might do it, if I recall correctly?

  I'm not sure how portable that is.

  In any case vp->vp_strvalue looks fine to me.  It also lets us do
vp->vp_ipv6addr and other things, which would be very good.

  Alan DeKok.
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html