Date: Sat, 19 Jul 2014 09:39:46 -0500 From: Pedro Giffuni <pfg@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r268867 - head/lib/libc/net Message-ID: <53CA8332.2060507@freebsd.org> In-Reply-To: <20140719210009.O1782@besplex.bde.org> References: <201407190153.s6J1rqBn027367@svn.freebsd.org> <20140719171552.W874@besplex.bde.org> <A66005FE-1A28-417A-8268-9690BC68EFD7@freebsd.org> <20140719210009.O1782@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 07/19/14 06:29, Bruce Evans wrote:
> 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: 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.
>
Ugh.. wish they taught these type of things in books ...
Hopefully I got it right this time ;).
Thank you.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53CA8332.2060507>