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

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--===============3071300122797330699==
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Sat, 19 Jul 2014, Pedro Giffuni wrote:

> Il giorno 19/lug/2014, alle ore 02:36, Bruce Evans <brde@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:=09Apple (Libc-997.90.3)
>>> MFC after:=091 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 =85 in relation with ext2fs ;=
).

Yes, it caused bugs in practice there.

> The compiler doesn=92t have a way to tell if socklen_t is of a certain ty=
pe "by accident=94.

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
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>> --- head/lib/libc/net/sourcefilter.c=09Sat Jul 19 01:15:01 2014=09(r268=
866)
>>> +++ head/lib/libc/net/sourcefilter.c=09Sat Jul 19 01:53:52 2014=09(r268=
867)
>>> @@ -337,7 +337,8 @@ getsourcefilter(int s, uint32_t interfac
>>> {
>>> =09struct __msfilterreq=09 msfr;
>>> =09sockunion_t=09=09*psu;
>>> -=09int=09=09=09 err, level, nsrcs, optlen, optname;
>>> +=09int=09=09=09 err, level, nsrcs, optname;
>>> +=09unsigned int=09=09 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=92t se=
em correct to preserve it at the expense of setting the incorrect type. Wou=
ld 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.

>> % =09err =3D _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=92t 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 n=
o effect either as the value will not be big enough to use the extra bit.
>
> It=92s all just a waste of time I guess, so as I said, I won=92t MFC the =
change.

It avoids warnings like:

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

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
--===============3071300122797330699==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
--===============3071300122797330699==--