svn commit: r268867 - head/lib/libc/net

[Pedro Giffuni]

Hello;

Il giorno 19/lug/2014, alle ore 02:36, Bruce Evans <brde at optusnet.com.au> ha scritto:

> On Sat, 19 Jul 2014, Pedro F. Giffuni wrote:
> 
>> Log:
>> Use unsigned optlen in getsourcefilter()
>> 
>> Sizes can not be negative and the functions that use it
>> expect an unsigned value anyways.
>> 
>> Obtained from:	Apple (Libc-997.90.3)
>> MFC after:	1 week
> 
> Most uses of unsigned types are bugs.  This one is an exception, but the
> change is still wrong.  The critical use of the type needs it to be
> socklen_t, not int or unsigned int.  socklen_t happens to have type
> uint32_t (unsigned due to old bugs).  It only accidentally has the
> same size as unsigned int.  It has a different type to plain int so
> compilers should warn about the type mismatch with certain warning
> flags.
> 

Ah, yes, we have had this discussion before … in relation with ext2fs ;).
The compiler doesn’t have a way to tell if socklen_t is of a certain type "by accident”.
I will use socklen_t in this case instead of unsigned int, but I will not MFC the changes
as the original change has no functional (end-user) effect.
 

>> Modified:
>> head/lib/libc/net/sourcefilter.c
>> 
>> Modified: head/lib/libc/net/sourcefilter.c
>> ==============================================================================
>> --- head/lib/libc/net/sourcefilter.c	Sat Jul 19 01:15:01 2014	(r268866)
>> +++ head/lib/libc/net/sourcefilter.c	Sat Jul 19 01:53:52 2014	(r268867)
>> @@ -337,7 +337,8 @@ getsourcefilter(int s, uint32_t interfac
>> {
>> 	struct __msfilterreq	 msfr;
>> 	sockunion_t		*psu;
>> -	int			 err, level, nsrcs, optlen, optname;
>> +	int			 err, level, nsrcs, optname;
>> +	unsigned int		 optlen;
> 
> This has mounds of style bugs.  2 more now (unsorting, and not using
> u_int, except u_int is not just a style bug)
> 

While aesthetically it may be better to keep the sorting, it doesn’t seem correct to preserve it at the expense of setting the incorrect type. Would you really want me to set optname in another line?

> Other uses of this variable:
> 
> % 	optlen = sizeof(struct __msfilterreq);
> 
> The rvalue has type size_t.  Unsigned is actually correct for size_t.
> The lvalue has a smaller type in many cases and we need to know that
> the result fits.  We know that it is small, so if fits just as well
> in an int as in an u_int.  It is less clear that it fits in a socklen_t
> since that is supposed to be opaque.
> 
> % 	memset(&msfr, 0, optlen);
> 
> memset() takes a size_t for its 3rd arg, but any type that can represent
> the value works provided memset()'s prototype is in scope.
> 
> % 	err = _getsockopt(s, level, optname, &msfr, &optlen);
> 
> This use needs the correct type since the reference is indirect so the
> prototype can't adjust the type.  The arg had type "int *" in 4.4BSD
> but has suffered from typedef poisoning so it is now "socklen_t *"
> 4.4BSD also doesn't have socklen_t.  socklen_t is specified by POSIX
> as being an integer type with width at least 32 bits.  It is not
> required to be unsigned, and there are portability problems from this.
> POSIX recommends that applications not store values larger than 2**31-1
> in socklen_t.  2**31 would only work if it is unsigned.
> 

While locally the variable doesn’t need to be unsigned, conceptually it will never be less than zero.
In this case, for the compiler, setting it to usigned int or to socklen_t makes no difference. In practice changing it from signed to unsigned has no effect either as the value will not be big enough to use the extra bit.

It’s all just a waste of time I guess, so as I said, I won’t MFC the change.

Pedro.