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

[Bruce Evans]

On Sat, 19 Jul 2014, Pedro Giffuni wrote:

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

Yes, it caused bugs in practice there.

> The compiler doesn’t have a way to tell if socklen_t is of a certain type "by accident”.

Yes, this is a fundamental problem with C typedefs -- they are too similar
to their underlying type.

Any size error would be detected more reliably than the sign error in
furture when int is expanded by socklen_t stays 32 bits.  Except the
assumptions that int is 32 bits is even more established now than it
was on vaxes, so no one would expand int.

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

You already had to change the sorting to split the declaration.  When
changing, it is better to put things in their correct place directly.
u_int is sorted before int because it is a more complex type with a
higher rank.  socklen_t is sorted before int because it is a more
complex type with an unknown (opaque) relative rank.

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

It is a common error to use unsigned types just because their value can
never be less than zero.  Most systems of arithmetic (floating point,
bignums(?), ...) don't even have unsigned numbers.

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

It avoids warnings like:

z.c:7: warning: pointer targets in passing arg 1 of `strlen' differ in signedness

This warning is under -pedantic for gcc, so it is rarely seen.  Other
compilers may differ.  With this warning and -Werror, you can't even
pass a "u_char *" pointer to strlen().  Strings consist of plain chars
so passing a "u_char *" to strlen() is a type mismatch.  IIRC, the C
standard requires a diagnostic for this, but gcc is not a C compiler
even with -pedantic so it doesn't print the diagnostic without
-pedantic.  This sign mismatch may be a serious error even for the
char type, but usually isn't.

Bruce